Better logging and internal user handling for operator privileges #79331

ywangd · 2021-10-18T01:59:12Z

This PR improves logging situation for operator privilegs. The loggings
are now more informational and also do not rely on the tracing level.
Internal user handling is also improved by skipping them more
consistently when marking and checking for operator users.

The existing code performs unnecessary checks for internal actions such
as leader/follower checks. It is technically a bug because threadContext
is prepared differently for internal actions and does not have the
operatorUser marking. However, the bug currently does not manifest
itself since internal actions are not protected by operator privileges.

This PR improves logging situation for operator privilegs. The loggings are now more informational and also do not rely on the tracing level. Internal user handling is also improved by skipping them more consistently when marking and checking for operator users. The existing code performs unnecessary checks for internal actions such as leader/follower checks. It is technically a bug because threadContext is prepared differently for internal actions and does not have the operatorUser marking. However, the bug currently does not manifest itself since internal actions are not protected by operator privileges.

elasticmachine · 2021-10-18T01:59:15Z

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

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

tvernum · 2021-10-18T02:08:11Z

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

        if (operatorException != null) {
-            throw denialException(authentication, action, originalRequest, operatorException);
+            throw denialException(authentication, action, originalRequest, "and operator privileges", operatorException);


This "context" doesn't seem very clear to me.
What exactly is it trying to say?

It didn't do what I expected. Sorry for the confusion. I was meant to add it at the end of the error message. So eorror message says something like "... this action is granted by .. and operator privileges". It should work now after the latest push. Let me know if you are OK with the approach.

Btw, I added it because I'd like the error with operator privileges to present in the main error message which is more prominent in logging and response.

tvernum · 2021-10-18T02:10:53Z

...security/src/main/java/org/elasticsearch/xpack/security/operator/FileOperatorUsersStore.java

+                    operatorUsersDescriptor.groups.size(),
+                    operatorUsersDescriptor.groups.stream().mapToLong(g -> g.usernames.size()).sum(),
+                    file.toAbsolutePath());
+                logger.trace("operator user descriptor: [{}]", operatorUsersDescriptor);


Should this be debug?

Yes I agree it is better to be debug. I was trying to avoid flooding logs on Cloud because it seems the file gets renewed on Cloud every few seconds (same for roles.yml etc. But now I think it is better and useful to log on the debug level.

I think we need to deal with the cloud file refresh problem separately. It's been an issue for years, and I think we need to put something into the resource watching infrastructure that can check if the file contents have really changed.

I'll work on something.

...gin/security/src/main/java/org/elasticsearch/xpack/security/operator/OperatorPrivileges.java

tvernum · 2021-10-18T02:21:04Z

...security/src/main/java/org/elasticsearch/xpack/security/operator/FileOperatorUsersStore.java

-            // Internal user are considered operator users
-            return true;
-        }
-
        // Other than realm name, other criteria must always be an exact match for the user to be an operator.
        // Realm name of a descriptor can be null. When it is null, it is ignored for comparison.
        // If not null, it will be compared exactly as well.


Given the intent of this PR, I think we should have some trace logging when checking for a matching group below.
It looks like there's a possibility that a user isn't being matched as an operator when it should be, and we probably want to be able to trace that.

Thanks, I added a trace log for when the result is false.

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

tvernum · 2021-10-18T03:27:13Z

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

        if (operatorException != null) {
-            throw denialException(authentication, action, originalRequest, operatorException);
+            throw denialException(authentication, action, originalRequest, null, true, operatorException);


I don't hate what you've ended up with, but I think you could have achieved what you want by setting context to

"because it requires operator privileges"

No problem. I reverted the change and took your suggested wording for context. So it now reads something like

... is unauthorized for user [...] with roles [..] because it requires operator privileges, this action is granted by ...

tvernum · 2021-10-18T03:28:52Z

...security/src/main/java/org/elasticsearch/xpack/security/operator/FileOperatorUsersStore.java

+        if (false == userIsOperator) {
+            logger.trace("User [{}] is not an operator", authentication.getUser());
+        }
+        return userIsOperator;


For a trace, I was thinking in would be inside the anyMatchpredicate.

Ah OK. Tracing inside anyMatch is better. I removed mine.

...security/src/main/java/org/elasticsearch/xpack/security/operator/FileOperatorUsersStore.java

...gin/security/src/main/java/org/elasticsearch/xpack/security/operator/OperatorPrivileges.java

…ecurity/operator/FileOperatorUsersStore.java Co-authored-by: Tim Vernum <[email protected]>

…-loggings

tvernum

LGTM

ywangd · 2021-10-18T04:49:24Z

Test failures are not related. I raised 2 issues for them:

ywangd · 2021-10-18T04:49:42Z

@elasticmachine run elasticsearch-ci/part-1-fips

elasticsearchmachine · 2021-10-18T06:26:13Z

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts
❌	7.15	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 79331

…astic#79331) This PR improves logging situation for operator privilegs. The loggings are now more informational and also do not rely on the tracing level. Internal user handling is also improved by skipping them more consistently when marking and checking for operator users. The existing code performs unnecessary checks for internal actions such as leader/follower checks. It is technically a bug because threadContext is prepared differently for internal actions and does not have the operatorUser marking. However, the bug currently does not manifest itself since internal actions are not protected by operator privileges.

…9331) (#79337) This PR improves logging situation for operator privilegs. The loggings are now more informational and also do not rely on the tracing level. Internal user handling is also improved by skipping them more consistently when marking and checking for operator users. The existing code performs unnecessary checks for internal actions such as leader/follower checks. It is technically a bug because threadContext is prepared differently for internal actions and does not have the operatorUser marking. However, the bug currently does not manifest itself since internal actions are not protected by operator privileges.

…9331) (#79336) This PR improves logging situation for operator privilegs. The loggings are now more informational and also do not rely on the tracing level. Internal user handling is also improved by skipping them more consistently when marking and checking for operator users. The existing code performs unnecessary checks for internal actions such as leader/follower checks. It is technically a bug because threadContext is prepared differently for internal actions and does not have the operatorUser marking. However, the bug currently does not manifest itself since internal actions are not protected by operator privileges.

* upstream/master: Changing test keytab to use aes256-cts-hmac-sha1-96 instead of des3-cbc-sha1-kd (elastic#78703) Add support for configuring HNSW parameters (elastic#79193) Deprecate resolution loss on date field (elastic#78921) Add Optional to Configure bind user (elastic#78303) Adapt BWC after backporting elastic#78765 (elastic#79350) [DOCS] Add deprecation notice for reset password tool (elastic#78793) added test for flattened type in top_metrics.yml (elastic#78960) [DOCS] Fixes indentation issue in GET trained models API docs. (elastic#79347) Fix parsing of PBES2 encrypted PKCS#8 keys (elastic#78904) Mute testReindex (elastic#79343) Node level can match action (elastic#78765) Fix duplicate license header in source files (elastic#79236) AllowAll for indicesAccessControl (elastic#78498) Better logging and internal user handling for operator privileges (elastic#79331) # Conflicts: # server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java

ywangd added 2 commits October 18, 2021 12:39

checkstyle

376325b

ywangd added >enhancement :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v8.0.0 v7.16.0 v7.15.2 labels Oct 18, 2021

ywangd requested a review from tvernum October 18, 2021 01:59

elasticmachine added the Team:Security Meta label for security team label Oct 18, 2021

ywangd commented Oct 18, 2021

View reviewed changes

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java Outdated Show resolved Hide resolved

tvernum reviewed Oct 18, 2021

View reviewed changes

ywangd added 2 commits October 18, 2021 13:59

address feedback

a4c936c

spotless

abc1f1c

ywangd requested a review from tvernum October 18, 2021 03:19

oops

649ba6d

tvernum reviewed Oct 18, 2021

View reviewed changes

ywangd and others added 3 commits October 18, 2021 14:35

Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/s…

e94b5b7

…ecurity/operator/FileOperatorUsersStore.java Co-authored-by: Tim Vernum <[email protected]>

address feedback

0898932

more spotless

ae2ab3c

ywangd requested a review from tvernum October 18, 2021 03:49

Merge remote-tracking branch 'origin/master' into operator-privileges…

a1f1d0a

…-loggings

tvernum approved these changes Oct 18, 2021

View reviewed changes

ywangd added the auto-backport-and-merge label Oct 18, 2021

ywangd merged commit 778c569 into elastic:master Oct 18, 2021

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better logging and internal user handling for operator privileges #79331

Better logging and internal user handling for operator privileges #79331

ywangd commented Oct 18, 2021

elasticmachine commented Oct 18, 2021

tvernum Oct 18, 2021

ywangd Oct 18, 2021

tvernum Oct 18, 2021

ywangd Oct 18, 2021

tvernum Oct 18, 2021

tvernum Oct 18, 2021

ywangd Oct 18, 2021

tvernum Oct 18, 2021

ywangd Oct 18, 2021

tvernum Oct 18, 2021

ywangd Oct 18, 2021

tvernum left a comment

ywangd commented Oct 18, 2021

ywangd commented Oct 18, 2021

elasticsearchmachine commented Oct 18, 2021

Better logging and internal user handling for operator privileges #79331

Better logging and internal user handling for operator privileges #79331

Conversation

ywangd commented Oct 18, 2021

elasticmachine commented Oct 18, 2021

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

tvernum left a comment

Choose a reason for hiding this comment

ywangd commented Oct 18, 2021

ywangd commented Oct 18, 2021

elasticsearchmachine commented Oct 18, 2021

💔 Backport failed