-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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 Kerberos Realm #31761
[Kerberos] Add Kerberos Realm #31761
Conversation
This commit adds authentication realm for handling Kerberos authentication by spnego mechanism. The class `KerberosRealm` authenticates user for given kerberos ticket after validating the ticket using `KerberosTicketValidator`. It uses native role mapping store to find user details and then creates an authenticated `User`. On successful authentication, it will return populated `User` object with roles. On failure to authenticate, it will terminate authentication process with a failure message. The failure could be due to gss context negotiation failure requiring further negotiation and it might return outToken to be communicated with peer as value for header `WWW-Authenticate` in the form 'Negotiate oYH1MIHyoAMK...'. There could be other failures like JAAS login exception or GSS Exception which will terminate the authentication process. As KerberosRealm can terminate authentication process during context negotiation with some outToken, the header value for `WWW-Authenticate` needs to be preserved. Earlier the behavior was to overwrite all the headers as defined in authentication failure handler in my last commit. Negotiate does maintain kind of state over HTTP and so we have to handle this in a special way. For this, I have added a special check for if exception has header 'WWW-Authenticate' with 'Negotiate ' scheme and token, it will not be overwritten. We want Kerberos to be a platinum feature, so it is not included as part of standard types similar to SAML. TODO: Support for user lookup from other realms like AD/LDAP. Authorizing realms feature is work in progress, once completed I will add the support to KerberosRealm. I have a TODO note in source code.
Pinging @elastic/es-security |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't read through the tests yet.
@@ -271,7 +270,7 @@ private void consumeToken(AuthenticationToken token) { | |||
if (result.getStatus() == AuthenticationResult.Status.TERMINATE) { | |||
logger.info("Authentication of [{}] was terminated by realm [{}] - {}", | |||
authenticationToken.principal(), realm.name(), result.getMessage()); | |||
userListener.onFailure(Exceptions.authenticationError(result.getMessage(), result.getException())); | |||
userListener.onFailure(result.getException()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't seem right - result.getException
can be null, and is for the reserved realm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you are right. The reason I made this change was to avoid wrapping of ElasticsearchSecurityException
which was thrown from KerberosRealm. It would not be a problem for other realms but for KerberosRealm there is a case where we do not want to do that. Like when it terminates authentication process with a token to be sent to peer for further gss negotiation. The header 'WWW-Authenticate', in this case, is present in thrown exception but if we wrap it then the header will be lost.
I did not think about exception being null
, so now modified it to create authentication error if the exception is null. Thank you.
private static final Set<String> TYPES_DEPEND_ON_THIRD_PARTY_SOURCES = new HashSet<>(); | ||
static { | ||
Collections.addAll(TYPES_DEPEND_ON_THIRD_PARTY_SOURCES, SamlRealmSettings.TYPE, KerberosRealmSettings.TYPE); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use a static block to initialise this?
Sets.newHashSet
can turn this into a 1 liner, and then you can wrap it in unmodifiableSet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, still getting used to these additional helpers. I had a look at CollectionUtils
but then there is also a Sets
class.
* @param args the arguments for the message | ||
* @return instance of {@link ElasticsearchSecurityException} | ||
*/ | ||
static ElasticsearchSecurityException unauthorized(final String message, final Throwable cause, final String outToken, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried that this method has too many arguments to also have a varargs args
.
It will be easy to accidentally pass things in the wrong order.
Maybe it should be split up so that it the authenticate-negotiate handling takes an ElasticsearchSecurityException
directly rather than the args to create one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, wanted to split but was being lazy. I have split this into two as suggested. Thanks.
|
||
@Override | ||
public void authenticate(final AuthenticationToken token, final ActionListener<AuthenticationResult> listener) { | ||
if (token instanceof KerberosAuthenticationToken) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be an assert
instead. You should never get passed a token you don't support
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was referring SamlRealm
during implementation and went with that but I do see support checks in AuthenticationService
so added an assert
. Thank you.
if (token instanceof KerberosAuthenticationToken) { | ||
final KerberosAuthenticationToken kerbAuthnToken = (KerberosAuthenticationToken) token; | ||
final Path keytabPath = | ||
config.env().configFile().resolve(KerberosRealmSettings.HTTP_SERVICE_KEYTAB_PATH.get(config.settings())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to resolve this on every call to authenticate? The setting will never change, so we can resolve & store the Path
in the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, earlier I was trying to make debug flag dynamic but then Jay's suggestion was to not make it unless required. So yes this can be set once in the constructor, changes done. Thank you.
} | ||
} | ||
|
||
private void buildUser(final Tuple<String, String> userPrincipalNameOutToken, final ActionListener<AuthenticationResult> listener) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer that you take 2 separate arguments rather than a Tuple
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, done.
} else { | ||
/** | ||
* Ongoing context establishment, terminate with UNAUTHORIZED and outToken | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment isn't very informative. Can you include an explanation about why you're choosing to terminate
instead of just calling unsuccessful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the comment with information why terminate than unsuccessful. Thank you.
listener.onResponse(AuthenticationResult.terminate(errorMessage, | ||
unauthorized(errorMessage, null, userPrincipalNameOutToken.v2()))); | ||
} | ||
}, (e) -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit brackets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed, Thanks.
|
||
private void handleException(Exception e, final ActionListener<AuthenticationResult> listener) { | ||
if (e instanceof LoginException) { | ||
logger.error("failed to authenticate user, service login failure", e); |
There was a problem hiding this comment.
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 want to log an error
everytime a login fails do we?
It seems quite verbose, especially since the AuthenticationService
will log this as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I see we are logging it in AuthenticationService. Removed these log statements.
} | ||
throw ExceptionsHelper.convertToRuntime(pve.getException()); | ||
actionListener.onFailure(ExceptionsHelper.convertToRuntime(pve.getException())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to convert it to runtime now that it's not being thrown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need, thank you.
Also refactored to break KerberosRealm Tests to avoid big test class.
Hi, @tvernum I have addressed your review comments. I have also split one big test file for KerberosRealm into multiple based on test scenario's. Please take a look at the changes when you get some time. Appreciate your feedback. Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
*/ | ||
containsNegotiateWithToken = | ||
ese.getHeader("WWW-Authenticate").stream() | ||
.anyMatch((s) -> s != null && s.toLowerCase(Locale.ENGLISH).contains("negotiate ")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We typically use Locale.ROOT
rather than ENGLISH
for case conversion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and nit on the unnecessary brackets :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, did the change and now configured IDE :)
listener.onResponse(AuthenticationResult.success(user)); | ||
} else { | ||
/** | ||
* TODO: bizybot AD/LDAP user lookup if lookup realm configured |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick comment, not a review issue: This TODO is in the wrong spot - you'll want to handle lookup-realms regardless of the cache status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
There was a problem hiding this 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 small comments. Other than those this LGTM
*/ | ||
containsNegotiateWithToken = | ||
ese.getHeader("WWW-Authenticate").stream() | ||
.anyMatch(s -> s != null && s.toLowerCase(Locale.ROOT).contains("negotiate ")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than use .toLowerCase shouldn't we use regionMatches?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, addressed this.
* The list of all realm types, which are those that have extensive interaction | ||
* with third party sources | ||
*/ | ||
private static final Set<String> TYPES_DEPEND_ON_THIRD_PARTY_SOURCES = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just remove this and change standard types to explicitly list the types. Collections.unmodifiableSet(NativeRealmSettings.TYPE, FileRealmSettings.TYPE, LdapRealmSettings.AD_TYPE, LdapRealmSettings.LDAP_TYPE, PkiRealmSettings.TYPE);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, yes it wasn't used anywhere modified it as suggested.
This commit adds authentication realm for handling Kerberos
authentication by spnego mechanism.
The class
KerberosRealm
authenticates user for given kerberosticket after validating the ticket using
KerberosTicketValidator
.It uses native role mapping store to find user details and
then creates an authenticated
User
.On successful authentication, it will return populated
User
object with roles. On failure to authenticate, it will terminate
authentication process with a failure message. The failure could be
due to gss context negotiation failure requiring further
negotiation and it might return outToken to be communicated with
peer as value for header
WWW-Authenticate
in the form'Negotiate oYH1MIHyoAMK...'. There could be other failures like
JAAS login exception or GSS Exception which will terminate the
authentication process.
As KerberosRealm can terminate authentication process during
context negotiation with some outToken, the header value for
WWW-Authenticate
needs to be preserved. Earlier the behaviorwas to overwrite all the headers as defined in authentication
failure handler in my last commit. Negotiate does maintain kind
of state over HTTP and so we have to handle this in a special way.
For this, I have added a special check for if exception has header
'WWW-Authenticate' with 'Negotiate ' scheme and token, it will
not be overwritten.
We want Kerberos to be a platinum feature, so it is not
included as part of standard types similar to SAML.
TODO: Support for user lookup from other realms like AD/LDAP.
Authorizing realms feature is work in progress, once completed
I will add the support to KerberosRealm. I have a TODO note in
the source code.