[Kerberos] Add support for list of auth challenge #31594

bizybot · 2018-06-27T01:09:50Z

Till now we had support for 'Basic', 'Bearer' auth schemes and
this was sufficient for us to reply WWW-Authenticate header
with one value either for Basic or Bearer for unauthorized
access.
After introducing Kerberos we will be supporting Negotiate scheme.
As per RFC7235,
we may respond with the list of challenges. This list is of auth
schemes supported by the server. We can also have custom Realms
defining their own response header value for 'WWW-Authenticate'
header. This commit introduces a getWWWAuthenticateHeaderValue
in Realm to identify the scheme which it wants to use. By default,
it uses 'Basic' auth scheme. This can be overridden
by realms like KerberosRealm to specify 'Negotiate' scheme or OAuth
to specify 'Bearer' or custom realms added by security extensions to
specify their own scheme.
SAML specifications do not specify anything related to the header but
unofficially many have used 'SAML' as auth scheme or used 'Bearer'
auth scheme for passing SAML tokens.
But most of the realms would use the existing schemes
like 'Basic', 'Digest', 'Bearer', 'Negotiate' etc.
At the startup, Security#createComponents will take care of
creating DefaultAuthenticationFailureHandler with default
response header values for 'WWW-Authenticate' as a list of configured
and enabled auth schemes.

Till now we had support for 'Basic', 'Bearer' auth schemes and this was sufficient for us to reply `WWW-Authenticate` header with one value either for `Basic` or `Bearer` for unauthorized access. After introducing Kerberos we will be supporting `Negotiate` scheme. As per [RFC7235](https://tools.ietf.org/html/rfc7235#section-4.1), we may respond with the list of challenges. This list is of auth schemes supported by the server. We can also have custom Realms defining their own response header value for 'WWW-Authenticate' header. This commit introduces a `getWWWAuthenticateHeaderValue` in `Realm` to identify the scheme which it wants to use. By default it uses 'Basic' auth scheme. This can be overriden by realms like KerberosRealm to specify 'Negotiate' scheme or OAuth to specify 'Bearer' or custom realms added by security extensions to specify their own scheme. SAML specifications do not specify anything related to the header but unofficially many have used 'SAML' as auth scheme or used 'Bearer' auth scheme for passing SAML tokens. But most of the realms would use the existing schemes like 'Basic', 'Digest', 'Bearer', 'Negotiate' etc. At the startup, `Security#createComponents` will take care of creating `DefaultAuthenticationFailureHandler` with default response header values for 'WWW-Authenticate' as a list of configured and enabled auth schemes.

elasticmachine · 2018-06-27T01:09:51Z

Pinging @elastic/es-security

bizybot · 2018-06-27T01:10:30Z

...in/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java

+     * @deprecated @see {@link #DefaultAuthenticationFailureHandler(List)}
+     */
+    @Deprecated
+    public DefaultAuthenticationFailureHandler() {


This deprecated default constructor is added to not break security extensions.

As Realms can return empty list due to license restriction adding default 'Basic' scheme to the authentication failure handler header value. Refactoring to create authentication failure handler in extracted method.

tvernum · 2018-06-27T05:45:07Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Realm.java

+     *
+     * @return value to be returned in 'WWW-Authenticate' response header.
+     */
+    public String getWWWAuthenticateHeaderValue() {


I know I normally lean towards "just build what you need", but this method feels way too specific to me.
Did you consider a getAuthenticationFailureHeaders() method instead?

Thank you for the suggestion yes, you right it's too specific and I also felt it should return a List of supported schemes. Usually, the authentication failure response headers will be populated in the ElasticsearchSecurityException and the method name you suggested might confuse. How about getSupportedAuthenticateChallenges instead? or any other suggestion. Thanks.

Did you consider a getAuthenticationFailureHeaders() method instead?

+1. I feel like this is the way to go. Ultimately this might allow us to remove the authentication failure handler (way off low priority future idea)

ok, are we thinking of letting the realms drive everything? Because I do see the value in it. Current flow makes us look at the code all over to see where the exception was populated whether it was populated with right headers, that too outside the realm if not then add those headers. I will make it a generic name as suggested. But would like to know more on what other headers do you foresee that needs to be sent and might be the realm driven?

I think it makes sense to let the realms handle this.
I don't know what other headers we could expect - but that's kind of the point, we can just support a relatively generic method and let custom realms do whatever they need. My guess is that anything other than WWW-Authenticate would be due to weird proprietary protocols, but if they exist then we can support them.

bizybot · 2018-06-29T07:04:47Z

Hi @jaymode, @tvernum I have addressed your review comments. Please take a look when you get some time. Thank you.

jaymode

I left some minor comments. Otherwise this change LGTM

jaymode · 2018-06-29T16:09:21Z

...in/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java

+     */
+    public DefaultAuthenticationFailureHandler(Map<String, List<String>> failureResponseHeaders) {
+        if (failureResponseHeaders == null || failureResponseHeaders.isEmpty()) {
+            failureResponseHeaders = new HashMap<>();


just use Collections.singletonMap

jaymode · 2018-06-29T16:11:50Z

...in/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java

+     * Creates an instance of {@link ElasticsearchSecurityException} with
+     * {@link RestStatus#UNAUTHORIZED} status.
+     * <p>
+     * Also adds response headers as configured


complete the thought, configured where?

jaymode · 2018-06-29T16:13:11Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Realm.java

+     */
+    public Map<String, List<String>> getAuthenticationFailureHeaders() {
+        final Map<String, List<String>> headers = new HashMap<>();
+        headers.put("WWW-Authenticate", Arrays.asList("Basic realm=\"" + XPackField.SECURITY + "\" charset=\"UTF-8\""));


Use Collections.singletonMap here and Collections.singletonList

]

bizybot added >feature :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Jun 27, 2018

bizybot requested review from jaymode and tvernum June 27, 2018 01:09

bizybot commented Jun 27, 2018

View reviewed changes

Yogesh Gaikwad added 2 commits June 27, 2018 13:30

[Kerberos] Fix failing integTest

efd58b4

As Realms can return empty list due to license restriction adding default 'Basic' scheme to the authentication failure handler header value. Refactoring to create authentication failure handler in extracted method.

[Kerberos] Fix javadoc

f0af4ce

tvernum reviewed Jun 27, 2018

View reviewed changes

Yogesh Gaikwad added 2 commits June 28, 2018 20:32

[Kerberos] Address review comments

f030693

[Kerberos] Correct javadocs and add check to avoid same header values.

435d19c

bizybot force-pushed the kerberos/authnchallenge branch from 0c24b1a to 435d19c Compare June 29, 2018 04:36

Yogesh Gaikwad added 3 commits June 29, 2018 14:41

Merge branch 'kerberos/sync' into kerberos/authnchallenge

2f91190

[Kerberos] Add null check and test modification

432bbf0

[Kerberos] Add headers to cause if it is ElasticsearchSecurityException

3d7b3fc

[Kerberos] Use HashMap instead of LinkedHashMap

16f8711

jaymode approved these changes Jun 29, 2018

View reviewed changes

Yogesh Gaikwad added 2 commits June 30, 2018 05:02

[Kerberos] Address review comments

d811123

]

[Kerberos] Address check style errors.

4bc7894

bizybot merged commit 52367f2 into elastic:feature/kerberos Jul 3, 2018

bizybot deleted the kerberos/authnchallenge branch July 3, 2018 00:55

[Kerberos] Add support for list of auth challenge #31594

[Kerberos] Add support for list of auth challenge #31594

Uh oh!

Conversation

bizybot commented Jun 27, 2018

Uh oh!

elasticmachine commented Jun 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bizybot commented Jun 29, 2018

Uh oh!

jaymode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants