Skip to content
This repository has been archived by the owner on Dec 21, 2022. It is now read-only.

support K8s service account for auth #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

anapsix
Copy link

@anapsix anapsix commented Apr 6, 2021

.. mostly stolen from Jack Newton's old PR #3

/cc @jnewton-avvo

  .. mostly stolen from Jack Newton's old PR
@jnewton-avvo jnewton-avvo mentioned this pull request Apr 6, 2021
// .is_err() {
// return Err(Error::);
// // return Err(String::from("Kubernetes authentication failed"));
// };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this error handling not needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure how to add it properly..
read_to_string? is not possible (perhaps, after Rust language changes)
and I could not find a usage Error class.. I guess, new one needs to be created

Copy link
Contributor

@matsadler matsadler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey 👋 this project is open source, and I apparently have nothing better to do with my time even though I don't work at Avvo anymore.

Comment on lines +148 to +150
let mut file = File::open("/var/run/secrets/kubernetes.io/serviceaccount/token").expect("Unable to open");
let mut jwt = String::new();
file.read_to_string(&mut jwt);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both File::open and File::read_to_string can return a std::io::Error, to be able to use the ? operator you need to be able to convert a std::io::Error to the Error type this function returns (defined up on line 87).

To be able to do that conversion you need to implement the std::convert::From trait. Currently the way Error is defined it holds a ClientError, but you've got a std::io::Error, so you probably want it to also be able to hold a ClientError or a std::io::Error. You would do this by changing it to an enum, like:

pub enum Error {
    ClientError(ClientError),
    IoError(std::io::Error),
}

You'd then need to update the various trait implementations for ClientError to now be able to cope with it being an enum, and finally implement the From trait. Check client_error.rs for examples.


Ignoring the error from File::read_to_string will mean that if it fails you'll end up with an empty jwt, and things will probably break later down the line in confusing ways.

Using expect(...) on the error from File::open will mean the program will immediately crash with an ugly error, instead of passing the error back up to any error handing that may exist in the caller.

Comment on lines 1 to +4
use std::{fmt, str::FromStr};

use std::fs::File;
use std::io::Read;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can combine these into a single statement like use std::{io::Read, fmt, fs::File, str::FromStr};

Comment on lines +120 to +121
let role: String = service.to_string();
vault.kubernetes_auth(role)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

service is already a String, what you want here is to either give kubernetes_auth a copy of service, like vault.kubernetes_auth(service.clone())?;, or lend it a &str reference to service like vault.kubernetes_auth(&service)?;.

The second option requires a small change to kubernetes_auth (explained in later comments), but makes more sense in Rust, and avoids a needless copy.

In Rust when you give a function a value it is moved into that function, and you can't get it back, but using & lets you lend a value to a function in a way that you do get it back.

@@ -135,6 +144,20 @@ impl Client {
Ok(())
}

pub fn kubernetes_auth(&mut self, role: String) -> Result<(), Error> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to take an owned copy of the role argument, it would work fine with just a reference, like pub fn kubernetes_auth(&mut self, role:&str) -> Result<(), Error> {. That will require a tweak to AuthKubernetesRequest, see the comment on AuthKubernetesRequest.

Comment on lines +71 to +74
pub struct AuthKubernetesRequest {
pub jwt: String,
pub role: String
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be able to just take string references like:

#[derive(Serialize)]
pub struct AuthKubernetesRequest<'a> {
    pub jwt: &'a str,
    pub role: &'a str,
}

You'll with that change probably need to create an instance like:

AuthKubernetesRequest { jwt: &jwt, role }

as you'll have an owned String, but it wants a &str reference (role will already be a &str reference if you follow my other comments).

// // return Err(String::from("Kubernetes authentication failed"));
// };
let request = AuthKubernetesRequest { jwt, role };
let response: AuthResponseWrapper = self.post(&format!("auth/kubernetes/login"), &request)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to use format here, as you're not adding anything to the string, just self.post("auth/kubernetes/login", &request)?; should do.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants