Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds LDAP auth engine #45

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vasyl-purchel
Copy link

@vasyl-purchel vasyl-purchel commented Sep 25, 2022

Adds new auth engine: LDAP.

Also attempt at adding a test for said engine but while manual test seems to work just fine (example added in a comment) but automated test fails to bind as a user (probably missing some knowledge on how docker_servertest localstack works)

@jmgilman
Copy link
Owner

Thanks for the PR! So which tests are failing?

@vasyl-purchel
Copy link
Author

LDAP tests - currently it's commented out inside the login method (to check that return is OK result)

@vasyl-purchel
Copy link
Author

I've pushed a commit to uncomment the test so that you can see how it fails. Hope you can help with getting it fixed :)

@jmgilman
Copy link
Owner

Thanks! I have limited bandwidth at the moment, but I'll see if I can take a deeper look later today or early this week. If we can't get it figured out, we can just mark tests as TODO and merge.

@vasyl-purchel
Copy link
Author

Cool thanks, I'll also continue looking into fixing it, there is no rush in merging it so I'd prefer tests fixed first. :)

@@ -30,7 +30,7 @@ use self::sys::responses::WrappingLookupResponse;
/// are automatically logged accordingly.
#[derive(Deserialize, Debug)]
pub struct EndpointResult<T> {
pub data: Option<T>,
pub data: Option<EitherData<T>>,
Copy link
Author

Choose a reason for hiding this comment

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

Also worth pointing out explicitly why this was changed:

as it seems that for currently implemented auth methods there is no "data" field returned for login api but for ldap/login it returns "data": {} for some reason and that breaks normal deserialisation. This "either data" structure ensures that if unexpectedly data field was returned it still can parse json object.

@patryk-s
Copy link

any updates on why this isn't merged? Would be nice to have.

Copy link
Collaborator

@Haennetz Haennetz left a comment

Choose a reason for hiding this comment

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

Thanks again for your pull request, and sorry for the delay.
I added some suggestions mainly some naming stuff to be closer to the vault documentation.

Comment on lines +41 to +42
pub username_as_alias: Option<bool>,
pub token_ttl: Option<String>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to the documentation Vault add some parameters to the auth method.

Suggested change
pub username_as_alias: Option<bool>,
pub token_ttl: Option<String>,
pub username_as_alias: Option<bool>,
pub dereference_aliases: Option<String>,
pub max_page_size: Option<u32>,
pub use_token_groups: Option<bool>,
pub token_ttl: Option<String>,

Comment on lines +73 to +92
/// ## Read LDAP Group
/// Reads the policies associated with a LDAP group.
///
/// * Path: /auth/{self.mount}/groups/{self.groupname}
/// * Method: GET
/// * Response: [ReadLDAPGroupResponse]
/// * Reference: https://www.vaultproject.io/api-docs/auth/ldap#read-ldap-group
#[derive(Builder, Debug, Default, Endpoint)]
#[endpoint(
path = "/auth/{self.mount}/groups/{self.groupname}",
response = "ReadLDAPGroupResponse",
builder = "true"
)]
#[builder(setter(into, strip_option), default)]
pub struct ReadLDAPGroupRequest {
#[endpoint(skip)]
pub mount: String,
#[endpoint(skip)]
pub groupname: String,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to the official documentation we should change it from groupname to name.

Suggested change
/// ## Read LDAP Group
/// Reads the policies associated with a LDAP group.
///
/// * Path: /auth/{self.mount}/groups/{self.groupname}
/// * Method: GET
/// * Response: [ReadLDAPGroupResponse]
/// * Reference: https://www.vaultproject.io/api-docs/auth/ldap#read-ldap-group
#[derive(Builder, Debug, Default, Endpoint)]
#[endpoint(
path = "/auth/{self.mount}/groups/{self.groupname}",
response = "ReadLDAPGroupResponse",
builder = "true"
)]
#[builder(setter(into, strip_option), default)]
pub struct ReadLDAPGroupRequest {
#[endpoint(skip)]
pub mount: String,
#[endpoint(skip)]
pub groupname: String,
}
/// ## Read LDAP Group
/// Reads the policies associated with a LDAP group.
///
/// * Path: /auth/{self.mount}/groups/{self.name}
/// * Method: GET
/// * Response: [ReadLDAPGroupResponse]
/// * Reference: https://www.vaultproject.io/api-docs/auth/ldap#read-ldap-group
#[derive(Builder, Debug, Default, Endpoint)]
#[endpoint(
path = "/auth/{self.mount}/groups/{self.name}",
response = "ReadLDAPGroupResponse",
builder = "true"
)]
#[builder(setter(into, strip_option), default)]
pub struct ReadLDAPGroupRequest {
#[endpoint(skip)]
pub mount: String,
#[endpoint(skip)]
pub name: String,
}

/// ## Create/Update LDAP Group
/// Creates or updates LDAP group policies.
///
/// * Path: /auth/{self.mount}/users/{self.groupname}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// * Path: /auth/{self.mount}/users/{self.groupname}
/// * Path: /auth/{self.mount}/users/{self.name}

/// * Reference: https://www.vaultproject.io/api-docs/auth/ldap#create-update-ldap-group
#[derive(Builder, Debug, Default, Endpoint)]
#[endpoint(
path = "/auth/{self.mount}/groups/{self.groupname}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
path = "/auth/{self.mount}/groups/{self.groupname}",
path = "/auth/{self.mount}/groups/{self.name}",

#[endpoint(skip)]
pub mount: String,
#[endpoint(skip)]
pub groupname: String,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pub groupname: String,
pub name: String,

api::exec_with_result(client, endpoint).await
}

/// Crates or updates a new user.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Crates or updates a new user.
/// Crates or updates a new group.

Comment on lines +234 to +235
.groupname(groupname)
.policies(policies)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.groupname(groupname)
.policies(policies)
.name(name)

Comment on lines +226 to +227
groupname: &str,
policies: &str,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Policies should also be optional and are included in opts.

Suggested change
groupname: &str,
policies: &str,
name: &str,

Comment on lines +99 to +100
policies: &str,
groups: &str,
Copy link
Collaborator

Choose a reason for hiding this comment

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

policies and groups should also be optional.

Suggested change
policies: &str,
groups: &str,

Comment on lines +108 to +109
.policies(policies)
.groups(groups)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.policies(policies)
.groups(groups)

@vasyl-purchel
Copy link
Author

hi, sorry, currently busy with other things, I'll try to take a look at fixing the comments later

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

Successfully merging this pull request may close these issues.

4 participants