This is an automated email from the ASF dual-hosted git repository. smolnar82 pushed a commit to branch knox_idf in repository https://gitbox.apache.org/repos/asf/knox.git
commit 5926b85af210a7efb2f681924e8f611fb19388c2 Author: Sandor Molnar <[email protected]> AuthorDate: Tue Aug 11 23:18:02 2026 +0200 KNOX-3414: atomically consume the refresh token before rotation (review finding M1) handleRefreshToken revoked the presented refresh token with revokeToken() only after validation, then minted the replacement. On DefaultTokenStateService revokeToken is check-then-act, so two concurrent redemptions of the same refresh token could both pass validation and both mint a new access/refresh pair. (The JDBC/Derby path is already atomic via a PK DELETE; this is the in-memory gap.) Replace revokeToken with the atomic consumeToken as a single-use claim performed BEFORE issuance: exactly one concurrent caller wins and rotates, the losers get invalid_grant and mint nothing. Mirrors the consume-before-issue guard already on the authorization_code grant (finding 2.4). Covered by TokenResourceRefreshTokenRotationTest (a lost consume yields invalid_grant with no issuance). Co-Authored-By: Claude Opus 4.8 <[email protected]> --- .../gateway/service/knoxidf/TokenResource.java | 21 +++- .../TokenResourceRefreshTokenRotationTest.java | 134 +++++++++++++++++++++ 2 files changed, 150 insertions(+), 5 deletions(-) diff --git a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/TokenResource.java b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/TokenResource.java index 8c463ab1f..c0c71a452 100644 --- a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/TokenResource.java +++ b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/TokenResource.java @@ -204,21 +204,32 @@ public class TokenResource extends PasscodeTokenResourceBase { return responseMap; } - private Response handleRefreshToken() { + // Package-private for testability (the single-use rotation guard is exercised by + // TokenResourceRefreshTokenRotationTest); not part of the public resource API. + Response handleRefreshToken() { try { final String refreshTokenParam = getRequestParam(REFRESH_TOKEN); final String refreshTokenId = TokenUtils.getTokenId(refreshTokenParam); final TokenMetadata refreshTokenMetadata = tokenStateService.getTokenMetadata(refreshTokenId); validateRefreshTokenGrant(refreshTokenParam, refreshTokenId, refreshTokenMetadata); - // Valid refresh token -> issue new access token and new refresh token (rotation) + + // Rotation is single-use: atomically consume (revoke) the presented refresh token BEFORE + // issuing its replacement. consumeToken is an atomic claim -- exactly one of N concurrent + // redemptions wins -- so two concurrent refreshes cannot both mint a new token pair from + // the same refresh token. (DefaultTokenStateService otherwise has a check-then-act race in + // revokeToken; the JDBC path is already atomic via a PK DELETE.) A lost claim means another + // request already redeemed/rotated this token, so reject it as invalid_grant. This mirrors + // the consume-before-issue guard on the authorization_code grant (see handleAuthorizationCodeFlow). + if (!tokenStateService.consumeToken(refreshTokenId)) { + return error("invalid_grant", "Refresh token has already been redeemed"); + } + + // Valid, freshly-consumed refresh token -> issue new access token and new refresh token (rotation) final String userName = refreshTokenMetadata.getUserName(); final String scope = refreshTokenMetadata.getMetadata(SCOPE); final Map<String, Object> userParams = userParamsProvider.getParamsFor(userName, scope); userParams.put(SCOPE, scope); - // Revoke old refresh token (rotation) - tokenStateService.revokeToken(refreshTokenId); - // Build new tokens final UserContext userContext = new UserContext(userName, null, userParams); final TokenResponseContext resp = getTokenResponse(userContext); diff --git a/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/TokenResourceRefreshTokenRotationTest.java b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/TokenResourceRefreshTokenRotationTest.java new file mode 100644 index 000000000..90b7346f2 --- /dev/null +++ b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/TokenResourceRefreshTokenRotationTest.java @@ -0,0 +1,134 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package org.apache.knox.gateway.service.knoxidf; + +import static org.apache.knox.gateway.security.CommonTokenConstants.CLIENT_SECRET; +import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.CLIENT_ID; +import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.REFRESH_TOKEN; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import javax.servlet.http.HttpServletRequest; +import javax.ws.rs.core.Response; + +import org.apache.knox.gateway.services.security.token.TokenMetadata; +import org.apache.knox.gateway.services.security.token.TokenMetadataType; +import org.apache.knox.gateway.services.security.token.TokenStateService; +import org.apache.knox.gateway.services.security.token.impl.TokenMAC; +import org.easymock.EasyMock; +import org.junit.Before; +import org.junit.Test; + +/** + * Verifies single-use enforcement of the {@code refresh_token} grant (review finding M1). The + * presented refresh token must be atomically consumed BEFORE its replacement is issued: exactly one + * of N concurrent redemptions wins the consume and rotates; the losers are rejected with + * {@code invalid_grant} and no new token pair is minted. This closes the check-then-act window that + * existed when rotation used {@code revokeToken} on the in-memory backend. Mirrors the + * authorization_code single-use guard (see {@link TokenResourceAuthCodeReplayTest}). + */ +public class TokenResourceRefreshTokenRotationTest { + + // A UUID so TokenUtils.getTokenId returns it verbatim (no JWT parsing needed). + private static final String REFRESH_TOKEN_ID = "11111111-2222-3333-4444-555555555555"; + private static final String CLIENT = "client-abc"; + private static final String USER_NAME = "alice"; + private static final long ISSUE_TIME = 1_700_000_000_000L; + private static final String RAW_PASSCODE = "0f1e2d3c-4b5a-6978-8796-a5b4c3d2e1f0"; + + private TokenStateService tokenStateService; + private TestableTokenResource resource; + private final AtomicInteger issuedCount = new AtomicInteger(); + + /** Field injection plus a stub for the token-mint step so the rotation guard is tested in isolation. */ + final class TestableTokenResource extends TokenResource { + void inject(final TokenStateService tss, final TokenMAC mac, final HttpServletRequest req) { + this.tokenStateService = tss; + this.tokenMAC = mac; + this.request = req; + } + + @Override + protected TokenResponseContext getTokenResponse(final UserContext context) { + issuedCount.incrementAndGet(); + return new TokenResponseContext(null, "issued", Response.ok()); + } + } + + private static String wireSecret(final String tokenId, final String rawPasscode) { + final String inner = Base64.getEncoder().encodeToString(tokenId.getBytes(StandardCharsets.UTF_8)) + + "::" + Base64.getEncoder().encodeToString(rawPasscode.getBytes(StandardCharsets.UTF_8)); + return Base64.getEncoder().encodeToString(inner.getBytes(StandardCharsets.UTF_8)); + } + + @Before + public void setUp() throws Exception { + final TokenMAC tokenMAC = new TokenMAC("HmacSHA256", "0123456789abcdef0123456789abcdef".toCharArray()); + final String storedPasscodeHash = tokenMAC.hash(CLIENT, ISSUE_TIME, USER_NAME, RAW_PASSCODE); + + final TokenMetadata refreshTokenMetadata = EasyMock.createNiceMock(TokenMetadata.class); + EasyMock.expect(refreshTokenMetadata.getType()).andReturn(TokenMetadataType.REFRESH_TOKEN.name()).anyTimes(); + EasyMock.expect(refreshTokenMetadata.isEnabled()).andReturn(true).anyTimes(); + EasyMock.expect(refreshTokenMetadata.getMetadata(CLIENT_ID)).andReturn(CLIENT).anyTimes(); + EasyMock.expect(refreshTokenMetadata.getUserName()).andReturn(USER_NAME).anyTimes(); + EasyMock.replay(refreshTokenMetadata); + + final TokenMetadata clientMetadata = EasyMock.createNiceMock(TokenMetadata.class); + EasyMock.expect(clientMetadata.getUserName()).andReturn(USER_NAME).anyTimes(); + EasyMock.expect(clientMetadata.getPasscode()).andReturn(storedPasscodeHash).anyTimes(); + EasyMock.replay(clientMetadata); + + tokenStateService = EasyMock.createNiceMock(TokenStateService.class); + EasyMock.expect(tokenStateService.getTokenMetadata(REFRESH_TOKEN_ID)).andReturn(refreshTokenMetadata).anyTimes(); + EasyMock.expect(tokenStateService.getTokenExpiration(REFRESH_TOKEN_ID)) + .andReturn(System.currentTimeMillis() + TimeUnit.MINUTES.toMillis(30)).anyTimes(); + EasyMock.expect(tokenStateService.getTokenMetadata(CLIENT)).andReturn(clientMetadata).anyTimes(); + EasyMock.expect(tokenStateService.getTokenIssueTime(CLIENT)).andReturn(ISSUE_TIME).anyTimes(); + // consumeToken behaviour is set per-test (win vs. lose) before replay(). + + final HttpServletRequest req = EasyMock.createNiceMock(HttpServletRequest.class); + EasyMock.expect(req.getParameter(REFRESH_TOKEN)).andReturn(REFRESH_TOKEN_ID).anyTimes(); + EasyMock.expect(req.getParameter(CLIENT_ID)).andReturn(CLIENT).anyTimes(); + EasyMock.expect(req.getParameter(CLIENT_SECRET)).andReturn(wireSecret(CLIENT, RAW_PASSCODE)).anyTimes(); + EasyMock.replay(req); + + resource = new TestableTokenResource(); + resource.inject(tokenStateService, tokenMAC, req); + } + + @Test + public void testAlreadyRedeemedRefreshTokenIsRejectedWithoutIssuing() { + // A concurrent redemption already consumed the token: this consume loses. + EasyMock.expect(tokenStateService.consumeToken(REFRESH_TOKEN_ID)).andReturn(false).once(); + EasyMock.replay(tokenStateService); + + final Response response = resource.handleRefreshToken(); + + assertEquals("A refresh token already consumed by a concurrent rotation must be rejected.", + Response.Status.BAD_REQUEST.getStatusCode(), response.getStatus()); + assertTrue("The error body should identify the invalid_grant condition.", + String.valueOf(response.getEntity()).contains("invalid_grant")); + assertEquals("A losing redemption must not mint any token.", 0, issuedCount.get()); + // Proves rotation now goes through the atomic consume path rather than revokeToken. + EasyMock.verify(tokenStateService); + } +}
