Remove deprecated Authentication#getLookedUpBy by ywangd · Pull Request #91126 · elastic/elasticsearch

ywangd · 2022-10-26T03:19:47Z

This PR removes the deprecated Authentication#getLookedUpBy method. Its usages are replaced by either isRunAs or
getEffectiveSubject#getRealm or their combinations depending on the actual context.

Relates: #88494

This PR removes the deprecated Authentication#getLookedUpBy method. Its usages are replaced by either isRunAs or getEffectiveSubject#getRealm or their combinations depending on the actual context. Relates: elastic#88494

elasticsearchmachine · 2022-10-26T03:20:10Z

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

ywangd · 2022-10-26T03:21:46Z

...test/java/org/elasticsearch/xpack/security/action/user/TransportAuthenticateActionTests.java

-            assertThat(auth.getAuthenticatingSubject().getRealm(), sameInstance(auth.getAuthenticatingSubject().getRealm()));
-            assertThat(auth.getLookedUpBy(), sameInstance(auth.getLookedUpBy()));
-            assertThat(auth.getEffectiveSubject().getVersion(), sameInstance(auth.getEffectiveSubject().getVersion()));
-            assertThat(auth.getAuthenticationType(), sameInstance(auth.getAuthenticationType()));
-            assertThat(auth.getAuthenticatingSubject().getMetadata(), sameInstance(auth.getAuthenticatingSubject().getMetadata()));
+            assertThat(auth.getAuthenticatingSubject().getRealm(), sameInstance(authentication.getAuthenticatingSubject().getRealm()));
+            assertThat(auth.getEffectiveSubject().getRealm(), sameInstance(authentication.getEffectiveSubject().getRealm()));
+            assertThat(auth.getEffectiveSubject().getVersion(), sameInstance(authentication.getEffectiveSubject().getVersion()));
+            assertThat(auth.getAuthenticationType(), sameInstance(authentication.getAuthenticationType()));
+            assertThat(
+                auth.getAuthenticatingSubject().getMetadata(),
+                sameInstance(authentication.getAuthenticatingSubject().getMetadata())
+            );


There are existing bugs in this code block. Some assertions were checking same auth object against itself.

Very strong assertions 😅

ywangd · 2022-10-26T03:22:10Z

...test/java/org/elasticsearch/xpack/security/action/user/TransportAuthenticateActionTests.java

-        if (anonymousUser.enabled()) {
-            final Authentication auth = responseRef.get().authentication();
-            final User authUser = auth.getEffectiveSubject().getUser();
-            assertThat(authUser.roles(), emptyArray());
-            assertThat(auth.getAuthenticatingSubject().getRealm(), sameInstance(auth.getAuthenticatingSubject().getRealm()));
-            assertThat(auth.getLookedUpBy(), sameInstance(auth.getLookedUpBy()));
-            assertThat(auth.getEffectiveSubject().getVersion(), sameInstance(auth.getEffectiveSubject().getVersion()));
-            assertThat(auth.getAuthenticationType(), sameInstance(auth.getAuthenticationType()));
-            assertThat(auth.getAuthenticatingSubject().getMetadata(), sameInstance(auth.getAuthenticatingSubject().getMetadata()));
-        } else {
-            assertThat(responseRef.get().authentication(), sameInstance(authentication));
-        }
+        // Anonymous roles should not be added which means there is no change to the authentication at all
+        assertThat(responseRef.get().authentication(), sameInstance(authentication));


This can be simplified to just ensure the authentication object does not change at all.

ywangd · 2022-10-26T03:23:38Z

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Authentication.java

-    public RealmRef getLookedUpBy() {
-        if (isRunAs()) {
-            return effectiveSubject.getRealm();
-        } else {
-            // retain the behaviour of returning null for lookup realm for if the authentication is not run-as
-            return null;
-        }


Because we use null to indicate lookup failure, it means the method can return null even when isRunAs is true. I think this behaviour is itself a tech debt. But I am leaving it for another day.

n1v0lg

LGTM. Sanity check on one of the changes (not blocking).

n1v0lg · 2022-10-26T09:57:35Z

...security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java

                final Authentication.RealmRef authenticatedBy = authentication.getAuthenticatingSubject().getRealm();
                if (authentication.isRunAs()) {
-                    final Authentication.RealmRef lookedUpBy = authentication.getLookedUpBy();
+                    final Authentication.RealmRef lookedUpBy = authentication.getEffectiveSubject().getRealm();


I know we're not introducing this behavior in this PR, but it's a little suspect that we handle nullability of "lookedUpBy" in some places but not in others. Just jotting this down as an observation, I don't think we should change existing behavior in this refactor.

This is a great observation! This turned out to be a bug! When run-as fails (because user does not exist) and authentication_success event is enabled for audit logging, the server throws a NPE

Cannot invoke "org.elasticsearch.xpack.core.security.authc.Authentication$RealmRef.getName()" because "lookedUpBy" is null

Since this is an existing bug, I'll have a separate PR to address this. Thanks for raising it.

Glad I jotted it down! Thanks for following up and figuring out the NPE. Separate PR makes sense 👍

n1v0lg · 2022-10-26T10:12:28Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java

        final Authentication authentication = requestInfo.getAuthentication();
-        if (authentication.getLookedUpBy() == null) {
+        assert authentication.isRunAs() : "authentication does not have a run-as";
+        if (authentication.getEffectiveSubject().getRealm() == null) {


This is a slight change in business logic: if runAs is false, the condition here would be true regardless of the value of authentication.getEffectiveSubject().getRealm() before the change, whereas now this is not the case anymore (in production, where assertions are disabled). Just double-checking that this is intentional.

The enclosing method is authorizeRunAs which is called inside maybeAuthorizeRunAs when the authentication is indeed a run-as. So the new assertion is basically making the existing invariant explicit and does not change existing logic.

That said, I will change the assertion message to something like

authentication must have run-as for run-as to be authorized

Hope it makes the intention clearer.

Alternatively, we can inline this method. But since it is private. I don't feel strong for it. What do you think?

I missed the fact that isRunAs is necessarily true here due to maybeAuthorizeRunAs. I had only looked at it in isolation. I'm good to leave as is -- it's a private method and to anyone looking at the code as a whole (as opposed to just the diff in the PR), things will make sense. I don't think we should inline.

n1v0lg · 2022-10-26T10:14:59Z

...test/java/org/elasticsearch/xpack/security/action/user/TransportAuthenticateActionTests.java

-            assertThat(auth.getAuthenticatingSubject().getRealm(), sameInstance(auth.getAuthenticatingSubject().getRealm()));
-            assertThat(auth.getLookedUpBy(), sameInstance(auth.getLookedUpBy()));
-            assertThat(auth.getEffectiveSubject().getVersion(), sameInstance(auth.getEffectiveSubject().getVersion()));
-            assertThat(auth.getAuthenticationType(), sameInstance(auth.getAuthenticationType()));
-            assertThat(auth.getAuthenticatingSubject().getMetadata(), sameInstance(auth.getAuthenticatingSubject().getMetadata()));
+            assertThat(auth.getAuthenticatingSubject().getRealm(), sameInstance(authentication.getAuthenticatingSubject().getRealm()));
+            assertThat(auth.getEffectiveSubject().getRealm(), sameInstance(authentication.getEffectiveSubject().getRealm()));
+            assertThat(auth.getEffectiveSubject().getVersion(), sameInstance(authentication.getEffectiveSubject().getVersion()));
+            assertThat(auth.getAuthenticationType(), sameInstance(authentication.getAuthenticationType()));
+            assertThat(
+                auth.getAuthenticatingSubject().getMetadata(),
+                sameInstance(authentication.getAuthenticatingSubject().getMetadata())
+            );


Very strong assertions 😅

n1v0lg · 2022-10-26T10:15:15Z

...test/java/org/elasticsearch/xpack/security/action/user/TransportAuthenticateActionTests.java

        }
        final User user = authentication.getEffectiveSubject().getUser();
+        // API key or service account have no named roles
+        assertThat(user.roles(), emptyArray());


…xist When run-as fails because the target user does not exist, the authentication is created with a null lookup realm. It is then rejected at authorization time. But for authentication, it is treated as success. This can lead to NPE when auditing the authenticationSuccess event. This PR fixes the NPE by checking whether lookup realm is null before using it. Relates: elastic#91126 (comment)

#91171) When run-as fails because the target user does not exist, the authentication is created with a null lookup realm. It is then rejected at authorization time. But for authentication, it is treated as success. This can lead to NPE when auditing the authenticationSuccess event. This PR fixes the NPE by checking whether lookup realm is null before using it. Relates: #91126 (comment)

elastic#91171) When run-as fails because the target user does not exist, the authentication is created with a null lookup realm. It is then rejected at authorization time. But for authentication, it is treated as success. This can lead to NPE when auditing the authenticationSuccess event. This PR fixes the NPE by checking whether lookup realm is null before using it. Relates: elastic#91126 (comment)

#91171) (#91240) When run-as fails because the target user does not exist, the authentication is created with a null lookup realm. It is then rejected at authorization time. But for authentication, it is treated as success. This can lead to NPE when auditing the authenticationSuccess event. This PR fixes the NPE by checking whether lookup realm is null before using it. Relates: #91126 (comment)

Remove deprecated Authentication#getLookedUpBy

18986bc

This PR removes the deprecated Authentication#getLookedUpBy method. Its usages are replaced by either isRunAs or getEffectiveSubject#getRealm or their combinations depending on the actual context. Relates: elastic#88494

ywangd added >refactoring :Security/Security Security issues without another label v8.6.0 labels Oct 26, 2022

ywangd requested a review from n1v0lg October 26, 2022 03:19

elasticsearchmachine added the Team:Security Meta label for security team label Oct 26, 2022

ywangd commented Oct 26, 2022

View reviewed changes

n1v0lg approved these changes Oct 26, 2022

View reviewed changes

address feedback

cb62c00

ywangd merged commit bb44f7f into elastic:main Oct 26, 2022

ywangd mentioned this pull request Oct 28, 2022

Fix NPE in auditing authenticationSuccess for non-existing run-as user #91171

Merged

ywangd mentioned this pull request Nov 2, 2022

Fix NPE in auditing authenticationSuccess for non-existing run-as user (#91171) #91240

Merged

Conversation

ywangd commented Oct 26, 2022

Uh oh!

elasticsearchmachine commented Oct 26, 2022

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!

n1v0lg left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

3 participants

n1v0lg left a comment •

edited

Loading