Skip to content

Commit

Permalink
Done (#100919)
Browse files Browse the repository at this point in the history
It appears that some freshly generated tokens fail authn under
concurrency conditions. This change increases verbosity of the
TokenService logging in order to track down how exactly is the token not
good for authn.

Related: #85697
  • Loading branch information
albertzaharovits authored Oct 16, 2023
1 parent 1846414 commit 7df7776
Showing 1 changed file with 9 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasEntry;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.notNullValue;

Expand All @@ -75,6 +74,8 @@ public Settings nodeSettings(int nodeOrdinal, Settings otherSettings) {
.put(TokenService.DELETE_INTERVAL.getKey(), TimeValue.timeValueMillis(200L))
.put(TokenService.DELETE_TIMEOUT.getKey(), TimeValue.timeValueSeconds(5L))
.put(XPackSettings.TOKEN_SERVICE_ENABLED_SETTING.getKey(), true)
// used to diagnose Token authn failures, see: https://github.com/elastic/elasticsearch/issues/85697
.put("logger.org.elasticsearch.xpack.security.authc.TokenService", "TRACE")
.build();
}

Expand Down Expand Up @@ -593,7 +594,6 @@ public void testRefreshingMultipleTimesFails() throws Exception {
public void testRefreshingMultipleTimesWithinWindowSucceeds() throws Exception {
final Clock clock = Clock.systemUTC();
final List<String> tokens = Collections.synchronizedList(new ArrayList<>());
final List<RestStatus> authStatuses = Collections.synchronizedList(new ArrayList<>());
OAuth2Token createTokenResponse = createToken(TEST_USER_NAME, SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING);
assertNotNull(createTokenResponse.getRefreshToken());
final int numberOfProcessors = Runtime.getRuntime().availableProcessors();
Expand Down Expand Up @@ -625,8 +625,14 @@ public void testRefreshingMultipleTimesWithinWindowSucceeds() throws Exception {
result.getRefreshToken()
);
} else {
authStatuses.add(getAuthenticationResponseCode(result.accessToken()));
tokens.add(result.accessToken() + result.getRefreshToken());
// Assert that all requests from all threads could authenticate at the time they received the access token
// see: https://github.com/elastic/elasticsearch/issues/54289
try {
getSecurityClient(result.accessToken()).authenticate();
} catch (ResponseException esse) {
fail(esse);
}
}
logger.info("received access token [{}] and refresh token [{}]", result.accessToken(), result.getRefreshToken());
completedLatch.countDown();
Expand All @@ -651,12 +657,6 @@ public void testRefreshingMultipleTimesWithinWindowSucceeds() throws Exception {
synchronized (tokens) {
assertThat((int) tokens.stream().distinct().count(), equalTo(1));
}
// Assert that all requests from all threads could authenticate at the time they received the access token
// see: https://github.com/elastic/elasticsearch/issues/54289
synchronized (authStatuses) {
assertThat((int) authStatuses.stream().distinct().count(), equalTo(1));
assertThat(authStatuses, hasItem(RestStatus.OK));
}
}

public void testRefreshAsDifferentUser() throws IOException {
Expand Down Expand Up @@ -902,13 +902,4 @@ private void assertUnauthorizedToken(String accessToken) {
private TestSecurityClient getSecurityClient(String accessToken) {
return getSecurityClient(RequestOptions.DEFAULT.toBuilder().addHeader("Authorization", "Bearer " + accessToken).build());
}

private RestStatus getAuthenticationResponseCode(String accessToken) throws IOException {
try {
getSecurityClient(accessToken).authenticate();
return RestStatus.OK;
} catch (ResponseException esse) {
return RestStatus.fromCode(esse.getResponse().getStatusLine().getStatusCode());
}
}
}

0 comments on commit 7df7776

Please sign in to comment.