Skip to content
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

Remove deprecated Authentication#getAuthenticatedBy #91104

Merged
merged 2 commits into from
Oct 25, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public SamlAuthenticateResponse(StreamInput in) throws IOException {

public SamlAuthenticateResponse(Authentication authentication, String tokenString, String refreshToken, TimeValue expiresIn) {
this.principal = authentication.getEffectiveSubject().getUser().principal();
this.realm = authentication.getAuthenticatedBy().getName();
this.realm = authentication.getEffectiveSubject().getRealm().getName();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This technically should be effectiveSubject. So I changed it be so. In practice, it does not matter because SAML realm authentication cannot have run-as. I add such assertion in other places (closer to where authentication is created).

this.tokenString = tokenString;
this.refreshToken = refreshToken;
this.expiresIn = expiresIn;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,14 +167,6 @@ public boolean isRunAs() {
return authenticatingSubject != effectiveSubject;
}

/**
* Use {@code getAuthenticatingSubject().getRealm()} instead.
*/
@Deprecated
public RealmRef getAuthenticatedBy() {
return authenticatingSubject.getRealm();
}

/**
* The use case for this method is largely trying to tell whether there is a run-as user
* and can be replaced by {@code isRunAs}
Expand Down Expand Up @@ -367,7 +359,7 @@ public boolean isAssignedToDomain() {
}

public boolean isAuthenticatedWithServiceAccount() {
return ServiceAccountSettings.REALM_TYPE.equals(getAuthenticatedBy().getType());
return ServiceAccountSettings.REALM_TYPE.equals(getAuthenticatingSubject().getRealm().getType());
}

/**
Expand Down Expand Up @@ -568,12 +560,12 @@ public void toXContentFragment(XContentBuilder builder) throws IOException {
builder.field(User.Fields.METADATA.getPreferredName(), user.metadata());
builder.field(User.Fields.ENABLED.getPreferredName(), user.enabled());
builder.startObject(User.Fields.AUTHENTICATION_REALM.getPreferredName());
builder.field(User.Fields.REALM_NAME.getPreferredName(), getAuthenticatedBy().getName());
builder.field(User.Fields.REALM_TYPE.getPreferredName(), getAuthenticatedBy().getType());
builder.field(User.Fields.REALM_NAME.getPreferredName(), getAuthenticatingSubject().getRealm().getName());
builder.field(User.Fields.REALM_TYPE.getPreferredName(), getAuthenticatingSubject().getRealm().getType());
// domain name is generally ambiguous, because it can change during the lifetime of the authentication,
// but it is good enough for display purposes (including auditing)
if (getAuthenticatedBy().getDomain() != null) {
builder.field(User.Fields.REALM_DOMAIN.getPreferredName(), getAuthenticatedBy().getDomain().name());
if (getAuthenticatingSubject().getRealm().getDomain() != null) {
builder.field(User.Fields.REALM_DOMAIN.getPreferredName(), getAuthenticatingSubject().getRealm().getDomain().name());
}
builder.endObject();
builder.startObject(User.Fields.LOOKUP_REALM.getPreferredName());
Expand All @@ -584,10 +576,10 @@ public void toXContentFragment(XContentBuilder builder) throws IOException {
builder.field(User.Fields.REALM_DOMAIN.getPreferredName(), getLookedUpBy().getDomain().name());
}
} else {
builder.field(User.Fields.REALM_NAME.getPreferredName(), getAuthenticatedBy().getName());
builder.field(User.Fields.REALM_TYPE.getPreferredName(), getAuthenticatedBy().getType());
if (getAuthenticatedBy().getDomain() != null) {
builder.field(User.Fields.REALM_DOMAIN.getPreferredName(), getAuthenticatedBy().getDomain().name());
builder.field(User.Fields.REALM_NAME.getPreferredName(), getAuthenticatingSubject().getRealm().getName());
builder.field(User.Fields.REALM_TYPE.getPreferredName(), getAuthenticatingSubject().getRealm().getType());
if (getAuthenticatingSubject().getRealm().getDomain() != null) {
builder.field(User.Fields.REALM_DOMAIN.getPreferredName(), getAuthenticatingSubject().getRealm().getDomain().name());
}
}
builder.endObject();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ public void testIsServiceAccount() {
authentication = AuthenticationTestHelper.builder().serviceAccount().build();
} else {
authentication = randomValueOtherThanMany(
authc -> "_service_account".equals(authc.getAuthenticatedBy().getName()),
authc -> "_service_account".equals(authc.getAuthenticatingSubject().getRealm().getName()),
() -> AuthenticationTestHelper.builder().build()
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -745,8 +745,8 @@ public void testOperationsOnReservedUsers() throws Exception {
Collections.singletonMap("Authorization", basicAuthHeaderValue(username, getReservedPassword()))
).execute(AuthenticateAction.INSTANCE, AuthenticateRequest.INSTANCE).get();
assertThat(authenticateResponse.authentication().getEffectiveSubject().getUser().principal(), is(username));
assertThat(authenticateResponse.authentication().getAuthenticatedBy().getName(), equalTo("reserved"));
assertThat(authenticateResponse.authentication().getAuthenticatedBy().getType(), equalTo("reserved"));
assertThat(authenticateResponse.authentication().getAuthenticatingSubject().getRealm().getName(), equalTo("reserved"));
assertThat(authenticateResponse.authentication().getAuthenticatingSubject().getRealm().getType(), equalTo("reserved"));
assertNull(authenticateResponse.authentication().getLookedUpBy());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ protected void doExecute(Task task, OpenIdConnectLogoutRequest request, ActionLi
final String token = request.getToken();
tokenService.getAuthenticationAndMetadata(token, ActionListener.wrap(tuple -> {
final Authentication authentication = tuple.v1();
assert false == authentication.isRunAs() : "oidc realm authentication cannot have run-as";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, oidc realm authentication cannot have run-as either.

final Map<String, Object> tokenMetadata = tuple.v2();
validateAuthenticationAndMetadata(authentication, tokenMetadata);
tokenService.invalidateAccessToken(token, ActionListener.wrap(result -> {
Expand All @@ -86,7 +87,7 @@ protected void doExecute(Task task, OpenIdConnectLogoutRequest request, ActionLi

private OpenIdConnectLogoutResponse buildResponse(Authentication authentication, Map<String, Object> tokenMetadata) {
final String idTokenHint = (String) getFromMetadata(tokenMetadata, "id_token_hint");
final Realm realm = this.realms.realm(authentication.getAuthenticatedBy().getName());
final Realm realm = this.realms.realm(authentication.getEffectiveSubject().getRealm().getName());
Comment on lines -89 to +90
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the other two places in this file are the same stroy as the saml realm authentication.

final JWT idToken;
try {
idToken = JWTParser.parse(idTokenHint);
Expand All @@ -108,11 +109,11 @@ private void validateAuthenticationAndMetadata(Authentication authentication, Ma
throw new ElasticsearchSecurityException("No active user");
}

final Authentication.RealmRef ref = authentication.getAuthenticatedBy();
final Authentication.RealmRef ref = authentication.getEffectiveSubject().getRealm();
if (ref == null || Strings.isNullOrEmpty(ref.getName())) {
throw new ElasticsearchSecurityException("Authentication {} has no authenticating realm", authentication);
}
final Realm realm = this.realms.realm(authentication.getAuthenticatedBy().getName());
final Realm realm = this.realms.realm(authentication.getEffectiveSubject().getRealm().getName());
if (realm == null) {
throw new ElasticsearchSecurityException("Authenticating realm {} does not exist", ref.getName());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ protected void doExecute(Task task, SamlAuthenticateRequest request, ActionListe
return;
}
assert authentication != null : "authentication should never be null at this point";
assert false == authentication.isRunAs() : "saml realm authentication cannot have run-as";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assertion for saml realm not having run-as.

@SuppressWarnings("unchecked")
final Map<String, Object> tokenMeta = (Map<String, Object>) result.getMetadata().get(SamlRealm.CONTEXT_TOKEN_DATA);
tokenService.createOAuth2Tokens(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ protected void doExecute(Task task, SamlLogoutRequest request, ActionListener<Sa
final String token = request.getToken();
tokenService.getAuthenticationAndMetadata(token, ActionListener.wrap(tuple -> {
Authentication authentication = tuple.v1();
assert false == authentication.isRunAs() : "saml realm authentication cannot have run-as";
final Map<String, Object> tokenMetadata = tuple.v2();
SamlLogoutResponse response = buildResponse(authentication, tokenMetadata);
tokenService.invalidateAccessToken(token, ActionListener.wrap(created -> {
Expand Down Expand Up @@ -134,9 +135,9 @@ private String getMetadataString(Map<String, Object> metadata, String key) {
}

private SamlRealm findRealm(Authentication authentication) {
final Authentication.RealmRef ref = authentication.getAuthenticatedBy();
final Authentication.RealmRef ref = authentication.getEffectiveSubject().getRealm();
if (ref == null || Strings.isNullOrEmpty(ref.getName())) {
throw SamlUtils.samlException("Authentication {} has no authenticating realm", authentication);
throw SamlUtils.samlException("Authentication {} has no effective realm", authentication);
Comment on lines -137 to +140
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly here as well.

}
final Realm realm = realms.realm(ref.getName());
if (realm == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,7 @@ public void authenticationSuccess(String requestId, Authentication authenticatio
)
) == false) {
// this is redundant information maintained for bwc purposes
final String authnRealm = authentication.getAuthenticatedBy().getName();
final String authnRealm = authentication.getAuthenticatingSubject().getRealm().getName();
new LogEntryBuilder().with(EVENT_TYPE_FIELD_NAME, REST_ORIGIN_FIELD_VALUE)
.with(EVENT_ACTION_FIELD_NAME, "authentication_success")
.with(REALM_FIELD_NAME, authnRealm)
Expand Down Expand Up @@ -1531,10 +1531,10 @@ LogEntryBuilder withRestUriAndMethod(RestRequest request) {

LogEntryBuilder withRunAsSubject(Authentication authentication) {
logEntry.with(PRINCIPAL_FIELD_NAME, authentication.getAuthenticatingSubject().getUser().principal())
.with(PRINCIPAL_REALM_FIELD_NAME, authentication.getAuthenticatedBy().getName())
.with(PRINCIPAL_REALM_FIELD_NAME, authentication.getAuthenticatingSubject().getRealm().getName())
.with(PRINCIPAL_RUN_AS_FIELD_NAME, authentication.getEffectiveSubject().getUser().principal());
if (authentication.getAuthenticatedBy().getDomain() != null) {
logEntry.with(PRINCIPAL_DOMAIN_FIELD_NAME, authentication.getAuthenticatedBy().getDomain().name());
if (authentication.getAuthenticatingSubject().getRealm().getDomain() != null) {
logEntry.with(PRINCIPAL_DOMAIN_FIELD_NAME, authentication.getAuthenticatingSubject().getRealm().getDomain().name());
}
if (authentication.getLookedUpBy() != null) {
logEntry.with(PRINCIPAL_RUN_AS_REALM_FIELD_NAME, authentication.getLookedUpBy().getName());
Expand Down Expand Up @@ -1625,7 +1625,7 @@ LogEntryBuilder withAuthentication(Authentication authentication) {
// No domain information is needed here since API key itself does not work across realms
}
} else {
final Authentication.RealmRef authenticatedBy = authentication.getAuthenticatedBy();
final Authentication.RealmRef authenticatedBy = authentication.getAuthenticatingSubject().getRealm();
if (authentication.isRunAs()) {
final Authentication.RealmRef lookedUpBy = authentication.getLookedUpBy();
logEntry.with(PRINCIPAL_REALM_FIELD_NAME, lookedUpBy.getName())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1513,17 +1513,20 @@ private static Optional<ElasticsearchSecurityException> checkClientCanRefresh(
clientAuthentication.getEffectiveSubject().getUser().principal()
);
return Optional.of(invalidGrantException("tokens must be refreshed by the creating client"));
} else if (clientAuthentication.getAuthenticatedBy().getName().equals(refreshToken.getAssociatedRealm()) == false) {
logger.warn(
"[{}] created the refresh token while authenticated by [{}] but is now authenticated by [{}]",
refreshToken.getAssociatedUser(),
refreshToken.getAssociatedRealm(),
clientAuthentication.getAuthenticatedBy().getName()
);
return Optional.of(invalidGrantException("tokens must be refreshed by the creating client"));
} else {
return Optional.empty();
}
} else if (clientAuthentication.getAuthenticatingSubject()
.getRealm()
.getName()
.equals(refreshToken.getAssociatedRealm()) == false) {
Comment on lines +1516 to +1519
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is inconsistent, i.e. it first checks effective user but then checks authenticating realm. But it is an existing bug (the token was created by capturing effective user + authenticating realm). So this is kept as is. Also it does not really matter much in practice because we now capture full authentication object for new tokens and these logics are not really used much.

logger.warn(
"[{}] created the refresh token while authenticated by [{}] but is now authenticated by [{}]",
refreshToken.getAssociatedUser(),
refreshToken.getAssociatedRealm(),
clientAuthentication.getAuthenticatingSubject().getRealm().getName()
);
return Optional.of(invalidGrantException("tokens must be refreshed by the creating client"));
} else {
return Optional.empty();
}
}
}

Expand Down Expand Up @@ -1795,9 +1798,9 @@ static BytesReference createTokenDocument(
builder.field("authentication", originatingClientAuth.maybeRewriteForOlderVersion(userToken.getVersion()).encode());
} else {
builder.field("user", originatingClientAuth.getEffectiveSubject().getUser().principal())
.field("realm", originatingClientAuth.getAuthenticatedBy().getName());
if (originatingClientAuth.getAuthenticatedBy().getDomain() != null) {
builder.field("realm_domain", originatingClientAuth.getAuthenticatedBy().getDomain());
.field("realm", originatingClientAuth.getAuthenticatingSubject().getRealm().getName());
if (originatingClientAuth.getAuthenticatingSubject().getRealm().getDomain() != null) {
builder.field("realm_domain", originatingClientAuth.getAuthenticatingSubject().getRealm().getDomain());
}
}
builder.endObject().endObject();
Expand Down Expand Up @@ -2546,7 +2549,7 @@ static final class RefreshTokenStatus {
this.invalidated = invalidated;
// not used, filled-in for consistency's sake
this.associatedUser = associatedAuthentication.getEffectiveSubject().getUser().principal();
this.associatedRealm = associatedAuthentication.getAuthenticatedBy().getName();
this.associatedRealm = associatedAuthentication.getAuthenticatingSubject().getRealm().getName();
this.associatedAuthentication = associatedAuthentication;
this.refreshed = refreshed;
this.refreshInstant = refreshInstant;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,8 @@ private void buildUser(X509AuthenticationToken token, String principal, ActionLi
"pki_delegated_by_user",
token.getDelegateeAuthentication().getEffectiveSubject().getUser().principal(),
"pki_delegated_by_realm",
token.getDelegateeAuthentication().getAuthenticatedBy().getName()
// TODO: this should be the realm of effective subject
token.getDelegateeAuthentication().getAuthenticatingSubject().getRealm().getName()
Comment on lines -217 to +218
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bug. I'll have a separate PR just for it so it is not blended in a pure refactoring.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Happy to review that one, too.

);
} else {
metadata = Map.of("pki_dn", token.dn());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -866,7 +866,7 @@ private static boolean checkChangePasswordAction(Authentication authentication)
if (isRunAs) {
realmType = authentication.getLookedUpBy().getType();
} else {
realmType = authentication.getAuthenticatedBy().getType();
realmType = authentication.getAuthenticatingSubject().getRealm().getType();
}

assert realmType != null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ public void testExecuteAfterRewritingAuthentication() throws IOException {
securityContext.executeAfterRewritingAuthentication(originalCtx -> {
Authentication authentication = securityContext.getAuthentication();
assertEquals(original.getEffectiveSubject().getUser(), authentication.getEffectiveSubject().getUser());
assertEquals(original.getAuthenticatedBy(), authentication.getAuthenticatedBy());
assertEquals(original.getAuthenticatingSubject().getRealm(), authentication.getAuthenticatingSubject().getRealm());
assertEquals(original.getLookedUpBy(), authentication.getLookedUpBy());
assertEquals(VersionUtils.getPreviousVersion(), authentication.getEffectiveSubject().getVersion());
assertEquals(original.getAuthenticationType(), securityContext.getAuthentication().getAuthenticationType());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ public void onFailure(Exception e) {
if (auth.isRunAs()) {
assertThat(auth.getAuthenticatingSubject().getUser(), sameInstance(authentication.getAuthenticatingSubject().getUser()));
}
assertThat(auth.getAuthenticatedBy(), sameInstance(auth.getAuthenticatedBy()));
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()));
Expand Down Expand Up @@ -196,7 +196,7 @@ public void onFailure(Exception e) {
final Authentication auth = responseRef.get().authentication();
final User authUser = auth.getEffectiveSubject().getUser();
assertThat(authUser.roles(), emptyArray());
assertThat(auth.getAuthenticatedBy(), sameInstance(auth.getAuthenticatedBy()));
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()));
Expand Down
Loading