-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Resolve anonymous roles and deduplicate roles during authentication #53453
Changes from 6 commits
8dac757
09df4fb
e24072a
2cf8516
8c5531b
903b805
c171757
c43f8ba
857c9e4
1deaa58
fe6eae8
2fcbfad
72d2667
65faa2c
c686985
faa0471
384aa17
6fa37e4
6fd8249
cc7f259
9a8bd63
6af416c
2892218
c6a8aa1
c63c49f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,4 +34,4 @@ public void writeTo(StreamOutput out) throws IOException { | |
authentication.writeTo(out); | ||
} | ||
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -40,8 +40,11 @@ | |||
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.EmptyAuthorizationInfo; | ||||
import org.elasticsearch.xpack.core.security.support.Exceptions; | ||||
import org.elasticsearch.xpack.core.security.user.AnonymousUser; | ||||
import org.elasticsearch.xpack.core.security.user.AsyncSearchUser; | ||||
import org.elasticsearch.xpack.core.security.user.SystemUser; | ||||
import org.elasticsearch.xpack.core.security.user.User; | ||||
import org.elasticsearch.xpack.core.security.user.XPackSecurityUser; | ||||
import org.elasticsearch.xpack.core.security.user.XPackUser; | ||||
import org.elasticsearch.xpack.security.audit.AuditTrail; | ||||
import org.elasticsearch.xpack.security.audit.AuditTrailService; | ||||
import org.elasticsearch.xpack.security.audit.AuditUtil; | ||||
|
@@ -657,7 +660,8 @@ void finishAuthentication(User finalUser) { | |||
logger.debug("user [{}] is disabled. failing authentication", finalUser); | ||||
listener.onFailure(request.authenticationFailed(authenticationToken)); | ||||
} else { | ||||
final Authentication finalAuth = new Authentication(finalUser, authenticatedBy, lookedupBy); | ||||
final Authentication finalAuth = new Authentication( | ||||
maybeMergeAnonymousRolesForUser(finalUser), authenticatedBy, lookedupBy); | ||||
writeAuthToContext(finalAuth); | ||||
} | ||||
} | ||||
|
@@ -690,6 +694,45 @@ void writeAuthToContext(Authentication authentication) { | |||
private void authenticateToken(AuthenticationToken token) { | ||||
this.consumeToken(token); | ||||
} | ||||
|
||||
private User maybeMergeAnonymousRolesForUser(User user) { | ||||
if (SystemUser.is(user) || XPackUser.is(user) || XPackSecurityUser.is(user) || AsyncSearchUser.is(user)) { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm also a bit worried about duplicating the check here. Should we make Line 418 in 9467dbf
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes makes sense. Long logical expression is never good as an But maybe we could find a better home for this method. How about User#isInternal(User user)? The AsyncSearchUser is also missed in InternalUserSerializationHelper. The new Use#isInternal method can be leveraged in here as well. It won't be able to access AuthorizationService#isInternalUser due to circular dependency. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure thing! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did the refactoring. But decided to leave |
||||
return user; | ||||
} else if (isAnonymousUserEnabled && anonymousUser.equals(user) == false) { | ||||
if (anonymousUser.roles().length == 0) { | ||||
throw new IllegalStateException("anonymous is only enabled when the anonymous user has roles"); | ||||
} | ||||
User userWithMergedRoles = new User( | ||||
user.principal(), | ||||
mergeAnonymousRoles(user.roles()), | ||||
user.fullName(), | ||||
user.email(), | ||||
user.metadata(), | ||||
user.enabled() | ||||
); | ||||
if (user.isRunAs()) { | ||||
final User authenticatedUserWithMergedRoles = new User( | ||||
user.authenticatedUser().principal(), | ||||
mergeAnonymousRoles(user.authenticatedUser().roles()), | ||||
user.authenticatedUser().fullName(), | ||||
user.authenticatedUser().email(), | ||||
user.authenticatedUser().metadata(), | ||||
user.authenticatedUser().enabled() | ||||
); | ||||
userWithMergedRoles = new User(userWithMergedRoles, authenticatedUserWithMergedRoles); | ||||
} | ||||
return userWithMergedRoles; | ||||
} else { | ||||
return user; | ||||
} | ||||
} | ||||
|
||||
private String[] mergeAnonymousRoles(String[] existingRoles) { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this implies that we have some special handling when merging the anonymous roles, while we just merge the two String arrays. I get that this is cleaner than doing it twice above but maybe a more generic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good suggestion. It is cleaner that way. I updated. |
||||
String[] mergedRoles = new String[existingRoles.length + anonymousUser.roles().length]; | ||||
System.arraycopy(existingRoles, 0, mergedRoles, 0, existingRoles.length); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we create a Set here and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I intentially didn't do deduplication here. Deduplication, filtering out un-resolvable roles, preempting the superuser role are all done in CompositeRolesStore#getRoles, which comes after authentication. Even without the anonymous roles, it is still possible to create an user with duplicated roles, e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should entertain this existing behavior for no reason, and since we go in the trouble of merging the roles we should do de-duplication here . We could tackle this in a follow up where we for instance should not allow a user to be created with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Merging only happens when anonymous access is enabled. If we de-duplicate here, it must also be applied when anonymous access is not enabled. So I re-arranged the code. |
||||
System.arraycopy(anonymousUser.roles(), 0, mergedRoles, existingRoles.length, anonymousUser.roles().length); | ||||
return mergedRoles; | ||||
} | ||||
} | ||||
|
||||
abstract static class AuditableRequest { | ||||
|
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 we guarantee order ? if not maybe
containsInAnyOrder()
is better here ?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 order is preserved. But I don't think we need guarantee it.