Add support for secondary authentication #52093

tvernum · 2020-02-08T06:03:57Z

This change makes it possible to send secondary authentication
credentials to select endpoints that need to perform a single action
in the context of two users.

Typically this need arises when a server process needs to call an
endpoint that users should not (or might not) have direct access to,
but some part of that action must be performed using the logged-in
user's identity.

elasticmachine · 2020-02-08T06:03:59Z

Pinging @elastic/es-security (:Security/Authentication)

tvernum · 2020-02-08T06:04:41Z

Draft/WIP because it builds on #52032 which is not merged yet

jkakavas · 2020-02-13T09:48:58Z

@tvernum is this good to be reviewed now that #52032 is merged or do you want to make adjustments first ?

tvernum · 2020-02-13T09:58:48Z

Not yet, I need to rebase first

…

On Thu, 13 Feb 2020, 8:49 pm Ioannis Kakavas, ***@***.***> wrote: @tvernum <https://github.com/tvernum> is this good to be reviewed now that #52032 <#52032> is merged or do you want to make adjustments first ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#52093?email_source=notifications&email_token=AARD6KMG7KHVCATTKBRJWATRCUJQZA5CNFSM4KRYQXM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELUCZJY#issuecomment-585641127>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARD6KKEODAFAWNK3I5R2ETRCUJQZANCNFSM4KRYQXMQ> .

This change makes it possible to send secondary authentication credentials to select endpoints that need to perform a single action in the context of two users. Typically this need arises when a server process needs to call an endpoint that users should not (or might not) have direct access to, but some part of that action must be performed using the logged-in user's identity.

tvernum · 2020-02-14T01:41:09Z

(It's ready now, sorry, I forgot to ping)

tvernum · 2020-02-18T22:56:07Z

Ping @jkakavas

jkakavas

LGTM!

Sorry it took me a while, I wanted to go through and understand all changes and interactions. I left some nitpicking suggestions and a question

jkakavas · 2020-02-19T06:16:42Z

...c/main/java/org/elasticsearch/xpack/core/security/authc/support/SecondaryAuthentication.java

+ */
+public class SecondaryAuthentication {
+
+    private static final String THREAD_CTX_KEY = "_xpack_security_2nd_authc";


nit:

Suggested change

private static final String THREAD_CTX_KEY = "_xpack_security_2nd_authc";

private static final String THREAD_CTX_KEY = "_xpack_security_secondary_authc";

purely personal preference, that 2nd doesn't read well

jkakavas · 2020-02-19T10:24:09Z

...c/main/java/org/elasticsearch/xpack/core/security/authc/support/SecondaryAuthentication.java

+    }
+
+    @Nullable
+    public static SecondaryAuthentication restoreFromContext(SecurityContext securityContext) throws IOException {


Should we name this readFromContext in order to not imply that it does anything more?

jkakavas · 2020-02-19T10:47:29Z

...c/main/java/org/elasticsearch/xpack/core/security/authc/support/SecondaryAuthentication.java

+
+    private static final String THREAD_CTX_KEY = "_xpack_security_2nd_authc";
+
+    public interface Authenticator {


More of a legitimate question than feedback, what value does this interface add?

It existed so that x-pack-core had something usable even though the implementation was in security.
But actually that doesn't matter anymore because the only use the authentication from within security.

jkakavas · 2020-02-19T10:49:45Z

...ity/src/main/java/org/elasticsearch/xpack/security/authc/support/SecondaryAuthenticator.java

+        authenticate(authListener -> authenticationService.authenticate(action, request, false, authListener), listener);
+    }
+
+  /**


Suggested change

/**

/**

jkakavas · 2020-02-19T11:22:34Z

...ity/src/main/java/org/elasticsearch/xpack/security/authc/support/SecondaryAuthenticator.java

+            ));
+        try (ThreadContext.StoredContext ignore = threadContext.stashContext()) {
+            logger.trace("found secondary authentication credentials, placing them in the internal [{}] header for authentication",
+                UsernamePasswordToken.BASIC_AUTH_HEADER);


Unrelated to this change but we should move BASIC_AUTH_HEADER away from UsernamePasswordToken , it confused me for more time than I'd like to admit :/

jkakavas · 2020-02-19T11:32:41Z

...urity/src/main/java/org/elasticsearch/xpack/security/action/filter/SecurityActionFilter.java

@@ -150,6 +150,7 @@ the system itself (e.g. pings, update mappings, share relocation, etc...) and we
         it to the action without an associated user (not via REST or transport - this is taken care of by
         the {@link Rest} filter and the {@link ServerTransport} filter respectively), it's safe to assume a system user
         here if a request is not associated with any other user.
+         Because we want to fallback to the SystemUser, we don't allow anonymous (AnonymousUser) requests.


This comment found its way back here : #52094 (comment)

Splitting this branch up ended up being hard work.
It was worth it because each of the changes was substantial, but there was a lot of rebasing and merging.

jkakavas · 2020-02-19T11:35:24Z

...ity/src/main/java/org/elasticsearch/xpack/security/authc/support/SecondaryAuthenticator.java

+     */
+    @Override
+    public void authenticate(String action, TransportRequest request, ActionListener<SecondaryAuthentication> listener) {
+        authenticate(authListener -> authenticationService.authenticate(action, request, false, authListener), listener);


Would it be worth adding a comment or jdoc to mention that allowAnonymous is set to false for a reason?

jkakavas · 2020-02-19T11:45:05Z

...rc/test/java/org/elasticsearch/xpack/security/authc/support/SecondaryAuthenticatorTests.java

+            .put(buildEnvSettings(Settings.EMPTY))
+            .put("xpack.security.authc.realms.dummy.test_realm.order", 1)
+            .put("xpack.security.authc.token.enabled", true)
+            .put("xpack.security.authc.api_key.enabled", true)


We don't actually use ApiKeyService here so we could just mock it below and return false on isEnabled() ?

Yes. I originally expected to include a test for ApiKey authentication, but Bearer Token is sufficient to verify that we're not restricted to the realm chain.

Actually, this is not as easy as it sounds.
authenticateWithApiKeyIfPresent is package protected, so it's really hard to mock.

jkakavas · 2020-02-19T11:48:30Z

...in/security/src/test/java/org/elasticsearch/xpack/security/rest/SecurityRestFilterTests.java

@@ -48,7 +49,9 @@

 public class SecurityRestFilterTests extends ESTestCase {


Should we add a test here to verify we process secondary authentication failures correctly?

This change makes it possible to send secondary authentication credentials to select endpoints that need to perform a single action in the context of two users. Typically this need arises when a server process needs to call an endpoint that users should not (or might not) have direct access to, but some part of that action must be performed using the logged-in user's identity. Backport of: elastic#52093

This change makes it possible to send secondary authentication credentials to select endpoints that need to perform a single action in the context of two users. Typically this need arises when a server process needs to call an endpoint that users should not (or might not) have direct access to, but some part of that action must be performed using the logged-in user's identity. Backport of: #52093

tvernum added WIP :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 v7.7.0 labels Feb 8, 2020

tvernum added the >enhancement label Feb 8, 2020

tvernum force-pushed the multiple-auth-headers branch from e5f483c to de24408 Compare February 11, 2020 04:45

tvernum force-pushed the multiple-auth-headers branch from de24408 to 6fd9b62 Compare February 13, 2020 11:06

tvernum marked this pull request as ready for review February 13, 2020 11:06

tvernum removed the WIP label Feb 13, 2020

tvernum requested a review from jkakavas February 13, 2020 11:06

jkakavas approved these changes Feb 19, 2020

View reviewed changes

tvernum added 2 commits February 21, 2020 14:21

Merge branch 'master' into multiple-auth-headers

ac77b09

Address feedback

3dbfbd1

tvernum merged commit 2c6aa90 into elastic:master Feb 21, 2020

tvernum added the backport pending label Mar 11, 2020

tvernum mentioned this pull request Mar 13, 2020

[Backport] Add support for secondary authentication #53530

Merged

tvernum removed the backport pending label Mar 13, 2020

albertzaharovits mentioned this pull request Mar 17, 2020

[APM] Allow kibana to collect APM telemetry in background task #52917

Merged

droberts195 mentioned this pull request Mar 19, 2020

[ML] Prefer secondary credentials when storing security headers for background tasks #53801

Closed

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 2) elastic/elasticsearch-net#4533

Closed

lcawl mentioned this pull request Apr 17, 2020

[DOCS] Adds documentation for secondary authorization headers #55365

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for secondary authentication #52093

Add support for secondary authentication #52093

tvernum commented Feb 8, 2020

elasticmachine commented Feb 8, 2020

tvernum commented Feb 8, 2020

jkakavas commented Feb 13, 2020

tvernum commented Feb 13, 2020 via email

tvernum commented Feb 14, 2020

tvernum commented Feb 18, 2020

jkakavas left a comment

jkakavas Feb 19, 2020

jkakavas Feb 19, 2020

jkakavas Feb 19, 2020

tvernum Feb 21, 2020

jkakavas Feb 19, 2020

jkakavas Feb 19, 2020

jkakavas Feb 19, 2020

tvernum Feb 21, 2020

jkakavas Feb 19, 2020

jkakavas Feb 19, 2020

tvernum Feb 21, 2020

tvernum Feb 21, 2020

jkakavas Feb 19, 2020

	private static final String THREAD_CTX_KEY = "_xpack_security_2nd_authc";
	private static final String THREAD_CTX_KEY = "_xpack_security_secondary_authc";


		private static final String THREAD_CTX_KEY = "_xpack_security_2nd_authc";

		public interface Authenticator {

		@@ -48,7 +49,9 @@

		public class SecurityRestFilterTests extends ESTestCase {

Add support for secondary authentication #52093

Add support for secondary authentication #52093

Conversation

tvernum commented Feb 8, 2020

elasticmachine commented Feb 8, 2020

tvernum commented Feb 8, 2020

jkakavas commented Feb 13, 2020

tvernum commented Feb 13, 2020 via email

tvernum commented Feb 14, 2020

tvernum commented Feb 18, 2020

jkakavas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment