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

[Kerberos] Add authorization realms support to Kerberos realm #32392

Merged

Conversation

bizybot
Copy link
Contributor

@bizybot bizybot commented Jul 26, 2018

This commit allows Kerberos realm to delegate User resolution
to configured authorization realms. If Kerberos realm is not configured
with any authorization realms then it uses the native role mapper to
resolve User.

This commit allows Kerberos realm to delegate `User` creation
to configured authorization realms.
If no authorization realms are configured, then Kerberos realm
uses native role mapper to resolve User.
@bizybot bizybot added review :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC labels Jul 26, 2018
@bizybot bizybot requested review from tvernum and jaymode July 26, 2018 09:27
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

You don't seem to add the new setting in KerberosRealmSettings

This commit adds missing authorization settings in
Kerberos realm settings.
@bizybot
Copy link
Contributor Author

bizybot commented Jul 30, 2018

Thanks, @tvernum. I have addressed your review comment. Please review when you get some time.

Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

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

I left a couple of comments. Otherwise LGTM

if (delegatedRealms.hasDelegation()) {
delegatedRealms.resolve(username, ActionListener.wrap(result -> {
if (result.isAuthenticated() && userPrincipalNameToUserCache != null) {
userPrincipalNameToUserCache.put(username, result.getUser());
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to cache here. The delegated realm resolved the user and it should be caching it. The kerberos realm will never use the cache entry that we add

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I too did not want to add caching here but thought it would be consistent with others. I will remove it. Thank you.

@@ -101,13 +107,20 @@ protected void assertSuccessAuthenticationResult(final User expectedUser, final
is(equalTo(KerberosAuthenticationToken.NEGOTIATE_AUTH_HEADER_PREFIX + outToken)));
}


Copy link
Member

Choose a reason for hiding this comment

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

nit: drop this extra line

@bizybot
Copy link
Contributor Author

bizybot commented Jul 31, 2018

@elasticmachine, test this please.

Copy link
Contributor

@albertzaharovits albertzaharovits left a comment

Choose a reason for hiding this comment

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

LGTM too Yogesh!

@bizybot bizybot merged commit 4e67689 into elastic:security-lookup-realms Aug 2, 2018
@bizybot bizybot deleted the kerberos-lookup-realm-support branch August 2, 2018 11:20
@colings86 colings86 added >enhancement and removed :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Oct 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants