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

UserInfoFetcher: ActiveDirectory backend #622

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Conversation

nightkr
Copy link
Member

@nightkr nightkr commented Aug 14, 2024

Description

Fixes #524

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes

Author

Reviewer

Acceptance

@fhennig
Copy link
Member

fhennig commented Aug 19, 2024

I saw this in the decision RfC column, anything particular you are looking for comments on?

@nightkr
Copy link
Member Author

nightkr commented Aug 19, 2024

Primarily the CRD changes configuring the new backend.

@nightkr
Copy link
Member Author

nightkr commented Aug 21, 2024

Moving CRD review to voting, please vote on this comment.


[#backend-keycloak]
Copy link
Member

Choose a reason for hiding this comment

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

I know it was this way before, but IMHO it makes sense to move the Keycloak example above below the Keycloak section :)

Copy link
Member Author

Choose a reason for hiding this comment

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

The API docs are fairly independent from the backend in use, mentioned that the example shows some keycloakisms in db44296

#[serde(rename_all = "camelCase")]
pub struct ActiveDirectoryBackend {
/// Hostname of the domain controller, e.g. `ad-ds-1.contoso.com`.
pub ldap_hostname: String,
Copy link
Member

Choose a reason for hiding this comment

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

In the AuthenticationClass we also have an ldapPort field: https://docs.stackable.tech/home/stable/concepts/authentication#ldap. It might make sense to have that for consistency reasons as well.

If not please test a non-default port and update the CRD docs, e.g.

Hostname of the domain controller, e.g. `ad-ds-1.contoso.com` or `ad-ds-1.contoso.com:1234`.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's fair, the inspiration here was secret-op's LDAP support, which also doesn't support custom LDAP ports at the moment. As far as I know, there's no way to customize AD's LDAP port.

Copy link
Member Author

Choose a reason for hiding this comment

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

In fairness, there are a few things we could port over from secret-op here:

  • rename to ldapServer
  • explicitly forbid port numbers (though it should break the SASL bind anyway)

Copy link
Member

Choose a reason for hiding this comment

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

As far as I know, there's no way to customize AD's LDAP port.

Oh that's a bit unexpected :D Happy to rename it to ldapServer and close this discussion

Copy link
Member Author

Choose a reason for hiding this comment

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

@nightkr
Copy link
Member Author

nightkr commented Aug 30, 2024

Closing CRD vote as accepted

@nightkr nightkr requested a review from sbernauer August 30, 2024 12:32
@nightkr nightkr marked this pull request as ready for review August 30, 2024 12:32
@nightkr
Copy link
Member Author

nightkr commented Aug 30, 2024

Proper testing is blocked on stackabletech/issues#629

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.

Support ActiveDirectory backend for user-info-fetcher
3 participants