Skip to content

Commit c23c057

Browse files
Fixing a problem with potential dirty read of a token document on token refresh (#64031)
* Fixing a problem with potential dirty read of a token document. Related to #59685 * Fixing a problem with potential dirty read of a token document. Adding CreateTokenResult to hold authentication object * Fixing a problem with potential dirty read of a token document. Adding CreateTokenResult to hold authentication object Co-authored-by: Elastic Machine <[email protected]>
1 parent 6093518 commit c23c057

12 files changed

+116
-80
lines changed

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportDelegatePkiAuthenticationAction.java

+3-2
Original file line numberDiff line numberDiff line change
@@ -82,9 +82,10 @@ protected void doExecute(Task task, DelegatePkiAuthenticationRequest request,
8282
ActionListener.wrap(authentication -> {
8383
assert authentication != null : "authentication should never be null at this point";
8484
tokenService.createOAuth2Tokens(authentication, delegateeAuthentication, Map.of(), false,
85-
ActionListener.wrap(tuple -> {
85+
ActionListener.wrap(tokenResult -> {
8686
final TimeValue expiresIn = tokenService.getExpirationDelay();
87-
listener.onResponse(new DelegatePkiAuthenticationResponse(tuple.v1(), expiresIn, authentication));
87+
listener.onResponse(new DelegatePkiAuthenticationResponse(tokenResult.getAccessToken(), expiresIn,
88+
authentication));
8889
}, listener::onFailure));
8990
}, e -> {
9091
logger.debug((Supplier<?>) () -> new ParameterizedMessage("Delegated x509Token [{}] could not be authenticated",

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/oidc/TransportOpenIdConnectAuthenticateAction.java

+3-2
Original file line numberDiff line numberDiff line change
@@ -72,9 +72,10 @@ protected void doExecute(Task task, OpenIdConnectAuthenticateRequest request,
7272
@SuppressWarnings("unchecked") final Map<String, Object> tokenMetadata = (Map<String, Object>) result.getMetadata()
7373
.get(OpenIdConnectRealm.CONTEXT_TOKEN_DATA);
7474
tokenService.createOAuth2Tokens(authentication, originatingAuthentication, tokenMetadata, true,
75-
ActionListener.wrap(tuple -> {
75+
ActionListener.wrap(tokenResult -> {
7676
final TimeValue expiresIn = tokenService.getExpirationDelay();
77-
listener.onResponse(new OpenIdConnectAuthenticateResponse(authentication, tuple.v1(), tuple.v2(), expiresIn));
77+
listener.onResponse(new OpenIdConnectAuthenticateResponse(authentication, tokenResult.getAccessToken(),
78+
tokenResult.getRefreshToken(), expiresIn));
7879
}, listener::onFailure));
7980
}, e -> {
8081
logger.debug(() -> new ParameterizedMessage("OpenIDConnectToken [{}] could not be authenticated", token), e);

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlAuthenticateAction.java

+3-2
Original file line numberDiff line numberDiff line change
@@ -65,10 +65,11 @@ protected void doExecute(Task task, SamlAuthenticateRequest request, ActionListe
6565
assert authentication != null : "authentication should never be null at this point";
6666
final Map<String, Object> tokenMeta = (Map<String, Object>) result.getMetadata().get(SamlRealm.CONTEXT_TOKEN_DATA);
6767
tokenService.createOAuth2Tokens(authentication, originatingAuthentication,
68-
tokenMeta, true, ActionListener.wrap(tuple -> {
68+
tokenMeta, true, ActionListener.wrap(tokenResult -> {
6969
final TimeValue expiresIn = tokenService.getExpirationDelay();
7070
listener.onResponse(
71-
new SamlAuthenticateResponse(authentication, tuple.v1(), tuple.v2(), expiresIn));
71+
new SamlAuthenticateResponse(authentication, tokenResult.getAccessToken(), tokenResult.getRefreshToken(),
72+
expiresIn));
7273
}, listener::onFailure));
7374
}, e -> {
7475
logger.debug(() -> new ParameterizedMessage("SamlToken [{}] could not be authenticated", saml), e);

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/token/TransportCreateTokenAction.java

+4-3
Original file line numberDiff line numberDiff line change
@@ -131,11 +131,12 @@ private void clearCredentialsFromRequest(GrantType grantType, CreateTokenRequest
131131
private void createToken(GrantType grantType, CreateTokenRequest request, Authentication authentication, Authentication originatingAuth,
132132
boolean includeRefreshToken, ActionListener<CreateTokenResponse> listener) {
133133
tokenService.createOAuth2Tokens(authentication, originatingAuth, Collections.emptyMap(), includeRefreshToken,
134-
ActionListener.wrap(tuple -> {
134+
ActionListener.wrap(tokenResult -> {
135135
final String scope = getResponseScopeValue(request.getScope());
136136
final String base64AuthenticateResponse = (grantType == GrantType.KERBEROS) ? extractOutToken() : null;
137-
final CreateTokenResponse response = new CreateTokenResponse(tuple.v1(), tokenService.getExpirationDelay(), scope,
138-
tuple.v2(), base64AuthenticateResponse, authentication);
137+
final CreateTokenResponse response = new CreateTokenResponse(tokenResult.getAccessToken(),
138+
tokenService.getExpirationDelay(), scope, tokenResult.getRefreshToken(), base64AuthenticateResponse,
139+
authentication);
139140
listener.onResponse(response);
140141
}, listener::onFailure));
141142
}

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/token/TransportRefreshTokenAction.java

+5-7
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
import org.elasticsearch.action.support.ActionFilters;
1010
import org.elasticsearch.action.support.HandledTransportAction;
1111
import org.elasticsearch.common.inject.Inject;
12-
import org.elasticsearch.common.settings.SecureString;
1312
import org.elasticsearch.tasks.Task;
1413
import org.elasticsearch.transport.TransportService;
1514
import org.elasticsearch.xpack.core.security.action.token.CreateTokenRequest;
@@ -31,13 +30,12 @@ public TransportRefreshTokenAction(TransportService transportService, ActionFilt
3130

3231
@Override
3332
protected void doExecute(Task task, CreateTokenRequest request, ActionListener<CreateTokenResponse> listener) {
34-
tokenService.refreshToken(request.getRefreshToken(), ActionListener.wrap(tuple -> {
33+
tokenService.refreshToken(request.getRefreshToken(), ActionListener.wrap(tokenResult -> {
3534
final String scope = getResponseScopeValue(request.getScope());
36-
tokenService.authenticateToken(new SecureString(tuple.v1()), ActionListener.wrap(authentication -> {
37-
listener.onResponse(new CreateTokenResponse(tuple.v1(), tokenService.getExpirationDelay(), scope, tuple.v2(), null,
38-
authentication));
39-
},
40-
listener::onFailure));
35+
final CreateTokenResponse response =
36+
new CreateTokenResponse(tokenResult.getAccessToken(), tokenService.getExpirationDelay(), scope,
37+
tokenResult.getRefreshToken(), null, tokenResult.getAuthentication());
38+
listener.onResponse(response);
4139
}, listener::onFailure));
4240
}
4341
}

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java

+50-17
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,7 @@ public TokenService(Settings settings, Clock clock, Client client, XPackLicenseS
254254
* {@link #VERSION_TOKENS_INDEX_INTRODUCED} and to a specific security tokens index for later versions.
255255
*/
256256
public void createOAuth2Tokens(Authentication authentication, Authentication originatingClientAuth, Map<String, Object> metadata,
257-
boolean includeRefreshToken, ActionListener<Tuple<String, String>> listener) {
257+
boolean includeRefreshToken, ActionListener<CreateTokenResult> listener) {
258258
// the created token is compatible with the oldest node version in the cluster
259259
final Version tokenVersion = getTokenVersionCompatibility();
260260
// tokens moved to a separate index in newer versions
@@ -273,7 +273,7 @@ public void createOAuth2Tokens(Authentication authentication, Authentication ori
273273
//public for testing
274274
public void createOAuth2Tokens(String accessToken, String refreshToken, Authentication authentication,
275275
Authentication originatingClientAuth,
276-
Map<String, Object> metadata, ActionListener<Tuple<String, String>> listener) {
276+
Map<String, Object> metadata, ActionListener<CreateTokenResult> listener) {
277277
// the created token is compatible with the oldest node version in the cluster
278278
final Version tokenVersion = getTokenVersionCompatibility();
279279
// tokens moved to a separate index in newer versions
@@ -306,12 +306,13 @@ public void createOAuth2Tokens(String accessToken, String refreshToken, Authenti
306306
* @param authentication The authentication object representing the user for which the tokens are created
307307
* @param originatingClientAuth The authentication object representing the client that called the related API
308308
* @param metadata A map with metadata to be stored in the token document
309-
* @param listener The listener to call upon completion with a {@link Tuple} containing the
310-
* serialized access token and serialized refresh token as these will be returned to the client
309+
* @param listener The listener to call upon completion with a {@link CreateTokenResult} containing the
310+
* serialized access token, serialized refresh token and authentication for which the token is created
311+
* as these will be returned to the client
311312
*/
312313
private void createOAuth2Tokens(String accessToken, String refreshToken, Version tokenVersion, SecurityIndexManager tokensIndex,
313314
Authentication authentication, Authentication originatingClientAuth, Map<String, Object> metadata,
314-
ActionListener<Tuple<String, String>> listener) {
315+
ActionListener<CreateTokenResult> listener) {
315316
assert accessToken.length() == TOKEN_LENGTH : "We assume token ids have a fixed length for nodes of a certain version."
316317
+ " When changing the token length, be careful that the inferences about its length still hold.";
317318
ensureEnabled();
@@ -351,12 +352,13 @@ private void createOAuth2Tokens(String accessToken, String refreshToken, Version
351352
final String versionedRefreshToken = refreshToken != null
352353
? prependVersionAndEncodeRefreshToken(tokenVersion, refreshToken)
353354
: null;
354-
listener.onResponse(new Tuple<>(versionedAccessToken, versionedRefreshToken));
355+
listener.onResponse(new CreateTokenResult(versionedAccessToken, versionedRefreshToken,
356+
authentication));
355357
} else {
356358
// prior versions of the refresh token are not version-prepended, as nodes on those
357359
// versions don't expect it.
358360
// Such nodes might exist in a mixed cluster during a rolling upgrade.
359-
listener.onResponse(new Tuple<>(versionedAccessToken, refreshToken));
361+
listener.onResponse(new CreateTokenResult(versionedAccessToken, refreshToken,authentication));
360362
}
361363
} else {
362364
listener.onFailure(traceLog("create token",
@@ -859,10 +861,11 @@ private void indexInvalidation(Collection<String> tokenIds, SecurityIndexManager
859861
* Called by the transport action in order to start the process of refreshing a token.
860862
*
861863
* @param refreshToken The refresh token as provided by the client
862-
* @param listener The listener to call upon completion with a {@link Tuple} containing the
863-
* serialized access token and serialized refresh token as these will be returned to the client
864+
* @param listener The listener to call upon completion with a {@link CreateTokenResult} containing the
865+
* serialized access token, serialized refresh token and authentication for which the token is created
866+
* as these will be returned to the client
864867
*/
865-
public void refreshToken(String refreshToken, ActionListener<Tuple<String, String>> listener) {
868+
public void refreshToken(String refreshToken, ActionListener<CreateTokenResult> listener) {
866869
ensureEnabled();
867870
final Instant refreshRequested = clock.instant();
868871
final Iterator<TimeValue> backoff = DEFAULT_BACKOFF.iterator();
@@ -995,7 +998,7 @@ private void findTokenFromRefreshToken(String refreshToken, SecurityIndexManager
995998
*/
996999
private void innerRefresh(String refreshToken, String tokenDocId, Map<String, Object> source, long seqNo, long primaryTerm,
9971000
Authentication clientAuth, Iterator<TimeValue> backoff, Instant refreshRequested,
998-
ActionListener<Tuple<String, String>> listener) {
1001+
ActionListener<CreateTokenResult> listener) {
9991002
logger.debug("Attempting to refresh token stored in token document [{}]", tokenDocId);
10001003
final Consumer<Exception> onFailure = ex -> listener.onFailure(traceLog("refresh token", tokenDocId, ex));
10011004
final Tuple<RefreshTokenStatus, Optional<ElasticsearchSecurityException>> checkRefreshResult;
@@ -1014,7 +1017,9 @@ private void innerRefresh(String refreshToken, String tokenDocId, Map<String, Ob
10141017
if (refreshTokenStatus.isRefreshed()) {
10151018
logger.debug("Token document [{}] was recently refreshed, when a new token document was generated. Reusing that result.",
10161019
tokenDocId);
1017-
decryptAndReturnSupersedingTokens(refreshToken, refreshTokenStatus, refreshedTokenIndex, listener);
1020+
final Tuple<UserToken, String> parsedTokens = parseTokensFromDocument(source, null);
1021+
Authentication authentication = parsedTokens.v1().getAuthentication();
1022+
decryptAndReturnSupersedingTokens(refreshToken, refreshTokenStatus, refreshedTokenIndex, authentication, listener);
10181023
} else {
10191024
final String newAccessTokenString = UUIDs.randomBase64UUID();
10201025
final String newRefreshTokenString = UUIDs.randomBase64UUID();
@@ -1126,11 +1131,13 @@ public void onFailure(Exception e) {
11261131
* @param refreshTokenStatus The {@link RefreshTokenStatus} containing information about the superseding tokens as retrieved from the
11271132
* index
11281133
* @param tokensIndex the manager for the index where the tokens are stored
1129-
* @param listener The listener to call upon completion with a {@link Tuple} containing the
1130-
* serialized access token and serialized refresh token as these will be returned to the client
1134+
* @param authentication The authentication object representing the user for which the tokens are created
1135+
* @param listener The listener to call upon completion with a {@link CreateTokenResult} containing the
1136+
* serialized access token, serialized refresh token and authentication for which the token is created
1137+
* as these will be returned to the client
11311138
*/
11321139
void decryptAndReturnSupersedingTokens(String refreshToken, RefreshTokenStatus refreshTokenStatus, SecurityIndexManager tokensIndex,
1133-
ActionListener<Tuple<String, String>> listener) {
1140+
Authentication authentication, ActionListener<CreateTokenResult> listener) {
11341141

11351142
final byte[] iv = Base64.getDecoder().decode(refreshTokenStatus.getIv());
11361143
final byte[] salt = Base64.getDecoder().decode(refreshTokenStatus.getSalt());
@@ -1166,8 +1173,10 @@ public void onResponse(GetResponse response) {
11661173
if (response.isExists()) {
11671174
try {
11681175
listener.onResponse(
1169-
new Tuple<>(prependVersionAndEncodeAccessToken(refreshTokenStatus.getVersion(), decryptedTokens[0]),
1170-
prependVersionAndEncodeRefreshToken(refreshTokenStatus.getVersion(), decryptedTokens[1])));
1176+
new CreateTokenResult(prependVersionAndEncodeAccessToken(refreshTokenStatus.getVersion(),
1177+
decryptedTokens[0]),
1178+
prependVersionAndEncodeRefreshToken(refreshTokenStatus.getVersion(), decryptedTokens[1]),
1179+
authentication));
11711180
} catch (GeneralSecurityException | IOException e) {
11721181
logger.warn("Could not format stored superseding token values", e);
11731182
onFailure.accept(invalidGrantException("could not refresh the requested token"));
@@ -1910,6 +1919,30 @@ boolean isExpirationInProgress() {
19101919
return expiredTokenRemover.isExpirationInProgress();
19111920
}
19121921

1922+
public static final class CreateTokenResult {
1923+
private final String accessToken;
1924+
private final String refreshToken;
1925+
private final Authentication authentication;
1926+
1927+
public CreateTokenResult(String accessToken, String refreshToken, Authentication authentication) {
1928+
this.accessToken = accessToken;
1929+
this.refreshToken = refreshToken;
1930+
this.authentication = authentication;
1931+
}
1932+
1933+
public String getAccessToken() {
1934+
return accessToken;
1935+
}
1936+
1937+
public String getRefreshToken() {
1938+
return refreshToken;
1939+
}
1940+
1941+
public Authentication getAuthentication() {
1942+
return authentication;
1943+
}
1944+
}
1945+
19131946
private class KeyComputingRunnable extends AbstractRunnable {
19141947

19151948
private final BytesKey decodedSalt;

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/oidc/TransportOpenIdConnectLogoutActionTests.java

+2-3
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
import org.elasticsearch.client.Client;
2727
import org.elasticsearch.cluster.service.ClusterService;
2828
import org.elasticsearch.common.UUIDs;
29-
import org.elasticsearch.common.collect.Tuple;
3029
import org.elasticsearch.common.settings.Settings;
3130
import org.elasticsearch.common.util.concurrent.ThreadContext;
3231
import org.elasticsearch.env.Environment;
@@ -203,11 +202,11 @@ public void testLogoutInvalidatesTokens() throws Exception {
203202
final Authentication authentication = new Authentication(user, realmRef, null, null, Authentication.AuthenticationType.REALM,
204203
tokenMetadata);
205204

206-
final PlainActionFuture<Tuple<String, String>> future = new PlainActionFuture<>();
205+
final PlainActionFuture<TokenService.CreateTokenResult> future = new PlainActionFuture<>();
207206
final String userTokenId = UUIDs.randomBase64UUID();
208207
final String refreshToken = UUIDs.randomBase64UUID();
209208
tokenService.createOAuth2Tokens(userTokenId, refreshToken, authentication, authentication, tokenMetadata, future);
210-
final String accessToken = future.actionGet().v1();
209+
final String accessToken = future.actionGet().getAccessToken();
211210
mockGetTokenFromId(tokenService, userTokenId, authentication, false, client);
212211

213212
final OpenIdConnectLogoutRequest request = new OpenIdConnectLogoutRequest();

0 commit comments

Comments
 (0)