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 dcd1e01779b95bfc3d39214d820ba35b8d950b44 Author: Sandor Molnar <[email protected]> AuthorDate: Mon Aug 10 21:10:11 2026 +0200 KNOX-3414: harden secrets-at-rest, auth-code single-use, and persistence (1.5, 2.4, 2.11) Three KnoxIDF security/correctness hardening findings from the OIDC-provider review, landed together. 1.5 - Secrets at rest - Federated OP client_secret is now resolvable via AliasService. New optional topology param federated.op.<name>.clientSecret.alias (FederatedOpConfiguration). AuthorizeResource.resolveClientSecret prefers the alias (getPasswordFromAliasForCluster, cluster from GATEWAY_CLUSTER_ATTRIBUTE with NO_CLUSTER_NAME fallback); fails closed if a configured alias is unresolvable rather than leaking the plaintext param. Plaintext clientSecret retained only as a fallback when no alias is set. - Federated OP access token is no longer persisted at rest: decorateAuthCodeToken stores only the FEDERATED_IDENTITY_ID pointer. Removed the now-dead split/join/chunk token helpers and FEDERATED_*_TOKEN_PREFIX constants. 2.4 - Single-use authorization codes (close the replay window) - SPI TokenStateService: new default consumeToken(String) - atomic single-use consume; exactly one concurrent caller receives true, an absent token is false (never throws). - DefaultTokenStateService: overrides consumeToken with a true atomic claim (tokenExpirations.remove(id) != null), evicting the remaining per-token state. - JDBCTokenStateService: overrides consumeToken using the primary-key DELETE as the atomic arbiter, evicts the in-memory cache, and fails closed on SQLException (the inherited removeToken swallows it and would falsely report a win). Derby inherits this. - idf TokenResource: validateAuthCode now returns the metadata it already reads; handleAuthorizationCodeFlow validates, then atomically consumes the code BEFORE issuing any token, stashing the metadata in a per-request attribute consumed by the issuance steps (getAuthCodeMetadata). The revoke-in-finally is removed. A code that fails validation is deliberately NOT consumed, so replaying with bad params cannot burn a victim's still-valid code. 2.11 - Persistence hardening - Consent key reshaped to fit KNOX_TOKEN_METADATA.md_name VARCHAR(32): AuthorizeResource.consentMetadataKey(subject) = "consent_" + first-20-hex(SHA-256(subject)) (28 chars), used by both hasConsent and markConsentAccepted so read and write agree. No schema change to md_name. - Derby DDL parity: added the missing NOT NULL constraints to the federated identity and attribute tables (the UNIQUE index was already present). - TOCTOU: JdbcFederatedIdentityService.addFederatedIdentity now inserts and catches instead of check-then-insert; a unique-constraint violation (SQLIntegrityConstraintViolationException or SQLState class 23, walked up the cause chain) is treated as a benign already-exists. The unique index is the arbiter. - Transaction boundary: FederatedIdentityDatabase.addFederatedIdentity writes the core row and attribute rows on a single connection with autocommit off, then commits (rollback on failure) so an identity is never persisted without its attributes. - Double-checked locking: added the missing inner recheck in JdbcFederatedIdentityService.init; corrected its misleading exception message. - SELECT * replaced with the explicit column list in the by-provider/issuer/subject query; KnoxDatabase resolves DDL via getClass().getClassLoader() so each subclass loads its own create*.sql. Tests - DefaultTokenStateServiceTest: +3 consumeToken tests (single-use, state removed, unknown id). - New TokenResourceAuthCodeReplayTest: a losing (already-consumed) redemption yields invalid_grant with no issuance; a winning redemption issues exactly once. - New ConsentMetadataKeyTest: key fits VARCHAR(32) for realistic subjects, deterministic, distinct subjects -> distinct keys. - FederatedOpConfigurationTest: +2 (clientSecret alias read / absent by default). Co-Authored-By: Claude Opus 4.8 <[email protected]> --- .../apache/knox/gateway/database/KnoxDatabase.java | 7 +- .../federation/FederatedIdentityDatabase.java | 56 +++++--- .../FederatedIdentityServiceMessages.java | 3 + .../federation/JdbcFederatedIdentityService.java | 54 ++++++-- .../token/impl/DefaultTokenStateService.java | 16 +++ .../services/token/impl/JDBCTokenStateService.java | 19 +++ ...noxIDFFederatedIdentityAttributesTableDerby.sql | 4 +- .../createKnoxIDFFederatedIdentityTableDerby.sql | 10 +- .../token/impl/DefaultTokenStateServiceTest.java | 36 +++++ .../gateway/service/knoxidf/AuthorizeResource.java | 69 +++++++++- .../gateway/service/knoxidf/TokenResource.java | 62 +++++++-- .../service/knoxidf/ConsentMetadataKeyTest.java | 65 +++++++++ .../knoxidf/TokenResourceAuthCodeReplayTest.java | 153 +++++++++++++++++++++ .../services/security/token/TokenStateService.java | 26 ++++ .../util/knoxidf/FederatedOpConfiguration.java | 14 ++ .../gateway/util/knoxidf/KnoxIDFConstants.java | 2 - .../knox/gateway/util/knoxidf/KnoxIDFUtils.java | 28 ---- .../util/knoxidf/FederatedOpConfigurationTest.java | 25 ++++ 18 files changed, 558 insertions(+), 91 deletions(-) diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/database/KnoxDatabase.java b/gateway-server/src/main/java/org/apache/knox/gateway/database/KnoxDatabase.java index 261b36899..dd6ee68e8 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/database/KnoxDatabase.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/database/KnoxDatabase.java @@ -16,8 +16,6 @@ */ package org.apache.knox.gateway.database; -import org.apache.knox.gateway.services.token.impl.TokenStateDatabase; - import javax.sql.DataSource; public class KnoxDatabase { @@ -30,7 +28,10 @@ public class KnoxDatabase { protected void createTableIfNotExists(String tableName, String createSqlFileName) throws Exception { if (!JDBCUtils.tableExists(tableName, dataSource)) { - JDBCUtils.createTableFromSQL(createSqlFileName, dataSource, TokenStateDatabase.class.getClassLoader()); + // Resolve the DDL resource via the actual subclass's classloader so each KnoxDatabase + // subclass (TokenStateDatabase, FederatedIdentityDatabase) loads its own create*.sql + // rather than being coupled to one hardcoded sibling class's classloader. + JDBCUtils.createTableFromSQL(createSqlFileName, dataSource, getClass().getClassLoader()); } } } diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/federation/FederatedIdentityDatabase.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/federation/FederatedIdentityDatabase.java index 649bf3038..0fc2790a4 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/federation/FederatedIdentityDatabase.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/federation/FederatedIdentityDatabase.java @@ -35,8 +35,8 @@ class FederatedIdentityDatabase extends KnoxDatabase { + " (id, user_id, provider, external_subject, external_issuer, created_at) VALUES (?, ?, ?, ?, ?, ?)"; private static final String ADD_FEDERATED_IDENTITY_ATTR_SQL = "INSERT INTO " + FEDERATED_IDENTITY_ATTRIBUTES_TABLE_NAME + " (identity_id, attr_key, attr_value) VALUES (?, ?, ?)"; - private static final String FETCH_FEDERATED_IDENTITY_BY_PROV_ISS_SUB_SQL = "SELECT * FROM " + FEDERATED_IDENTITY_TABLE_NAME + - " WHERE provider = ? AND external_issuer = ? AND external_subject = ?"; + private static final String FETCH_FEDERATED_IDENTITY_BY_PROV_ISS_SUB_SQL = "SELECT id, user_id, provider, external_subject, external_issuer, created_at FROM " + + FEDERATED_IDENTITY_TABLE_NAME + " WHERE provider = ? AND external_issuer = ? AND external_subject = ?"; private static final String FETCH_FEDERATED_IDENTITY_SQL_BY_ID = "SELECT id, user_id, provider, external_subject, external_issuer, created_at FROM " + FEDERATED_IDENTITY_TABLE_NAME + " WHERE id = ?"; private static final String FETCH_FEDERATED_IDENTITY_ATTR_SQL = "SELECT attr_key, attr_value FROM " + FEDERATED_IDENTITY_ATTRIBUTES_TABLE_NAME + " WHERE identity_id = ?"; @@ -49,26 +49,42 @@ class FederatedIdentityDatabase extends KnoxDatabase { } void addFederatedIdentity(FederatedIdentity identity) throws SQLException { - // save core metadata first - try (Connection connection = dataSource.getConnection(); PreparedStatement addFederatedIdentityStatement = connection.prepareStatement(ADD_FEDERATED_IDENTITY_SQL)) { - addFederatedIdentityStatement.setString(1, identity.getId()); - addFederatedIdentityStatement.setString(2, identity.getUserId()); - addFederatedIdentityStatement.setString(3, identity.getProvider()); - addFederatedIdentityStatement.setString(4, identity.getExternalSubject()); - addFederatedIdentityStatement.setString(5, identity.getExternalIssuer()); - addFederatedIdentityStatement.setTimestamp(6, Timestamp.from(identity.getCreatedAt())); - addFederatedIdentityStatement.executeUpdate(); - } + // Persist the core identity row and its attribute rows atomically on a single connection + // with autocommit off: either the identity and all its attributes commit together, or the + // whole write rolls back. Previously each INSERT ran on its own auto-committed connection, + // so a failure between them could leave an identity persisted without its attributes. + try (Connection connection = dataSource.getConnection()) { + final boolean previousAutoCommit = connection.getAutoCommit(); + connection.setAutoCommit(false); + try { + // save core metadata first + try (PreparedStatement addFederatedIdentityStatement = connection.prepareStatement(ADD_FEDERATED_IDENTITY_SQL)) { + addFederatedIdentityStatement.setString(1, identity.getId()); + addFederatedIdentityStatement.setString(2, identity.getUserId()); + addFederatedIdentityStatement.setString(3, identity.getProvider()); + addFederatedIdentityStatement.setString(4, identity.getExternalSubject()); + addFederatedIdentityStatement.setString(5, identity.getExternalIssuer()); + addFederatedIdentityStatement.setTimestamp(6, Timestamp.from(identity.getCreatedAt())); + addFederatedIdentityStatement.executeUpdate(); + } - // save attributes - try (Connection connection = dataSource.getConnection(); PreparedStatement addFederatedIdentityAttrStatement = connection.prepareStatement(ADD_FEDERATED_IDENTITY_ATTR_SQL)) { - for (var attribute : identity.getAttributes().entrySet()) { - addFederatedIdentityAttrStatement.setString(1, identity.getId()); - addFederatedIdentityAttrStatement.setString(2, attribute.getKey()); - addFederatedIdentityAttrStatement.setString(3, attribute.getValue()); - addFederatedIdentityAttrStatement.addBatch(); + // save attributes + try (PreparedStatement addFederatedIdentityAttrStatement = connection.prepareStatement(ADD_FEDERATED_IDENTITY_ATTR_SQL)) { + for (var attribute : identity.getAttributes().entrySet()) { + addFederatedIdentityAttrStatement.setString(1, identity.getId()); + addFederatedIdentityAttrStatement.setString(2, attribute.getKey()); + addFederatedIdentityAttrStatement.setString(3, attribute.getValue()); + addFederatedIdentityAttrStatement.addBatch(); + } + addFederatedIdentityAttrStatement.executeBatch(); + } + connection.commit(); + } catch (SQLException e) { + connection.rollback(); + throw e; + } finally { + connection.setAutoCommit(previousAutoCommit); } - addFederatedIdentityAttrStatement.executeBatch(); } } diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/federation/FederatedIdentityServiceMessages.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/federation/FederatedIdentityServiceMessages.java index d6f820a1d..1241e54e7 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/federation/FederatedIdentityServiceMessages.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/federation/FederatedIdentityServiceMessages.java @@ -32,4 +32,7 @@ public interface FederatedIdentityServiceMessages { @Message(level = MessageLevel.ERROR, text = "An error occurred while fetching federated identity ({0}) from the database : {1}") void errorFetchingFederatedIdentityFromDatabase(String id, String errorMessage, @StackTrace(level = MessageLevel.DEBUG) Exception e); + + @Message(level = MessageLevel.DEBUG, text = "Federated identity ({0} / {1} / {2}) already exists; skipping insert") + void federatedIdentityAlreadyExists(String provider, String issuer, String subject); } diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/federation/JdbcFederatedIdentityService.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/federation/JdbcFederatedIdentityService.java index 8e26bdc1d..546536e78 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/federation/JdbcFederatedIdentityService.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/federation/JdbcFederatedIdentityService.java @@ -23,6 +23,7 @@ import org.apache.knox.gateway.services.ServiceLifecycleException; import org.apache.knox.gateway.services.security.AliasService; import java.sql.SQLException; +import java.sql.SQLIntegrityConstraintViolationException; import java.util.Map; import java.util.Optional; import java.util.concurrent.atomic.AtomicBoolean; @@ -42,14 +43,18 @@ public class JdbcFederatedIdentityService implements FederatedIdentityService { if (!initialized.get()) { initLock.lock(); try { - if (aliasService == null) { - throw new ServiceLifecycleException("The required AliasService reference has not been set."); - } - try { - this.federatedIdentityDatabase = new FederatedIdentityDatabase(DataSourceProvider.getDataSource(config, aliasService), config.getDatabaseType()); - initialized.set(true); - } catch (Exception e) { - throw new ServiceLifecycleException("Error while initiating JDBCTokenStateService: " + e, e); + // Double-checked locking: re-test under the lock so a thread that blocked while + // another was initialising does not re-initialise the database a second time. + if (!initialized.get()) { + if (aliasService == null) { + throw new ServiceLifecycleException("The required AliasService reference has not been set."); + } + try { + this.federatedIdentityDatabase = new FederatedIdentityDatabase(DataSourceProvider.getDataSource(config, aliasService), config.getDatabaseType()); + initialized.set(true); + } catch (Exception e) { + throw new ServiceLifecycleException("Error while initiating JdbcFederatedIdentityService: " + e, e); + } } } finally { initLock.unlock(); @@ -75,16 +80,43 @@ public class JdbcFederatedIdentityService implements FederatedIdentityService { @Override public void addFederatedIdentity(FederatedIdentity identity) { + // Insert-and-catch rather than check-then-insert: the UNIQUE(provider, external_issuer, + // external_subject) index is the atomic arbiter, so two concurrent requests for the same + // external identity cannot both insert. A unique-constraint violation means the row already + // exists, which is exactly the desired end state, so it is treated as benign rather than + // surfaced as an error (closing the prior TOCTOU race between the pre-check and the insert). try { - if (findByProviderAndSubject(identity.getProvider(), identity.getExternalIssuer(), identity.getExternalSubject()).isEmpty()) { - federatedIdentityDatabase.addFederatedIdentity(identity); - } + federatedIdentityDatabase.addFederatedIdentity(identity); } catch (SQLException e) { + if (isUniqueConstraintViolation(e)) { + LOG.federatedIdentityAlreadyExists(identity.getProvider(), identity.getExternalIssuer(), identity.getExternalSubject()); + return; + } LOG.errorSavingFederatedIdentityInDatabase(identity.getId(), e.getMessage(), e); throw new FederatedIdentityServiceException("An error occurred while saving Federated Identity " + identity.getId() + " in the database", e); } } + /** + * Recognises a unique/primary-key constraint violation across dialects: either a + * {@link SQLIntegrityConstraintViolationException} or any {@link SQLException} in the cause + * chain whose SQLState is in the {@code 23} (integrity constraint violation) class. + */ + private static boolean isUniqueConstraintViolation(SQLException e) { + for (Throwable t = e; t != null; t = t.getCause()) { + if (t instanceof SQLIntegrityConstraintViolationException) { + return true; + } + if (t instanceof SQLException) { + final String sqlState = ((SQLException) t).getSQLState(); + if (sqlState != null && sqlState.startsWith("23")) { + return true; + } + } + } + return false; + } + @Override public Optional<FederatedIdentity> findByProviderAndSubject(String provider, String issuer, String subject) { try { diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java index afca435bd..95d0b8eab 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java @@ -265,6 +265,22 @@ public class DefaultTokenStateService implements TokenStateService { log.revokedToken(Tokens.getTokenIDDisplayText(tokenId)); } + @Override + public boolean consumeToken(final String tokenId) { + validateTokenIdentifier(tokenId); + // Atomic single-use claim: ConcurrentHashMap#remove returns the prior value to exactly one + // caller, so concurrent redemptions of the same token can never both observe it as present. + final boolean claimed = tokenExpirations.remove(tokenId) != null; + if (claimed) { + // Evict the remaining per-token state (idempotent for a token we won the race for). + tokenIssueTimes.remove(tokenId); + maxTokenLifetimes.remove(tokenId); + metadataMap.remove(tokenId); + log.revokedToken(Tokens.getTokenIDDisplayText(tokenId)); + } + return claimed; + } + @Override public boolean isExpired(final JWTToken token) throws UnknownTokenException { return getTokenExpiration(token) <= System.currentTimeMillis(); diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/JDBCTokenStateService.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/JDBCTokenStateService.java index 4dfc46d29..5d60fbe33 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/JDBCTokenStateService.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/JDBCTokenStateService.java @@ -240,6 +240,25 @@ public class JDBCTokenStateService extends AbstractPersistentTokenStateService i } } + @Override + public boolean consumeToken(String tokenId) { + // The single-row primary-key DELETE is the atomic arbiter: only the caller whose statement + // actually removed the row observes rowsAffected == 1, so exactly one concurrent redemption + // wins. Fail closed on a SQL error (report "not consumed by us") rather than the inherited + // removeToken() behaviour of swallowing the exception, which would falsely signal a win. + try { + final boolean removed = tokenDatabase.removeToken(tokenId); + if (removed) { + super.removeTokens(Collections.singleton(tokenId)); // evict the in-memory cache copy + log.removedTokenFromDatabase(Tokens.getTokenIDDisplayText(tokenId)); + } + return removed; + } catch (SQLException e) { + log.errorRemovingTokenFromDatabase(Tokens.getTokenIDDisplayText(tokenId), e.getMessage(), e); + return false; + } + } + @Override protected void evictExpiredTokens() { try { diff --git a/gateway-server/src/main/resources/createKnoxIDFFederatedIdentityAttributesTableDerby.sql b/gateway-server/src/main/resources/createKnoxIDFFederatedIdentityAttributesTableDerby.sql index 13ae9ab11..90c70ff0f 100644 --- a/gateway-server/src/main/resources/createKnoxIDFFederatedIdentityAttributesTableDerby.sql +++ b/gateway-server/src/main/resources/createKnoxIDFFederatedIdentityAttributesTableDerby.sql @@ -14,8 +14,8 @@ -- the License. CREATE TABLE FEDERATED_IDENTITY_ATTR ( - identity_id VARCHAR(36), - attr_key VARCHAR(128), + identity_id VARCHAR(36) NOT NULL, + attr_key VARCHAR(128) NOT NULL, attr_value CLOB, PRIMARY KEY (identity_id, attr_key), CONSTRAINT fk_fed_attr FOREIGN KEY (identity_id) REFERENCES FEDERATED_IDENTITY(id) ON DELETE CASCADE diff --git a/gateway-server/src/main/resources/createKnoxIDFFederatedIdentityTableDerby.sql b/gateway-server/src/main/resources/createKnoxIDFFederatedIdentityTableDerby.sql index 7152d4c71..acaf1c340 100644 --- a/gateway-server/src/main/resources/createKnoxIDFFederatedIdentityTableDerby.sql +++ b/gateway-server/src/main/resources/createKnoxIDFFederatedIdentityTableDerby.sql @@ -15,11 +15,11 @@ CREATE TABLE FEDERATED_IDENTITY ( id VARCHAR(36) PRIMARY KEY, - user_id VARCHAR(36), - provider VARCHAR(64), - external_subject VARCHAR(255), - external_issuer VARCHAR(255), - created_at TIMESTAMP + user_id VARCHAR(36) NOT NULL, + provider VARCHAR(64) NOT NULL, + external_subject VARCHAR(255) NOT NULL, + external_issuer VARCHAR(255) NOT NULL, + created_at TIMESTAMP NOT NULL ); CREATE UNIQUE INDEX UX_FED_IDENTITY ON FEDERATED_IDENTITY (provider, external_issuer, external_subject); \ No newline at end of file diff --git a/gateway-server/src/test/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateServiceTest.java b/gateway-server/src/test/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateServiceTest.java index 419d077c6..3cb0b2738 100644 --- a/gateway-server/src/test/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateServiceTest.java +++ b/gateway-server/src/test/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateServiceTest.java @@ -162,6 +162,42 @@ public class DefaultTokenStateServiceTest { tss.isExpired(token); } + @Test + public void testConsumeToken_SingleUse() throws Exception { + // Single-use semantics for auth codes: the first consume wins, a second consume of the same + // id loses (the token is already gone), and the token state is actually removed. + final JWTToken token = createMockToken(System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(60)); + final TokenStateService tss = createTokenStateService(); + final String tokenId = TokenUtils.getTokenId(token); + + addToken(tss, token, System.currentTimeMillis()); + + assertTrue("First consume should win.", tss.consumeToken(tokenId)); + assertFalse("Second consume of the same token must lose.", tss.consumeToken(tokenId)); + } + + @Test(expected = UnknownTokenException.class) + public void testConsumeToken_RemovesState() throws Exception { + final JWTToken token = createMockToken(System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(60)); + final TokenStateService tss = createTokenStateService(); + final String tokenId = TokenUtils.getTokenId(token); + + addToken(tss, token, System.currentTimeMillis()); + assertTrue(tss.consumeToken(tokenId)); + + // The token must no longer be known after being consumed. + tss.getTokenExpiration(tokenId); + } + + @Test + public void testConsumeToken_UnknownToken() throws Exception { + // An id that was never stored cannot be "won" - consume reports false rather than throwing. + final JWTToken token = createMockToken(System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(60)); + final TokenStateService tss = createTokenStateService(); + + assertFalse(tss.consumeToken(TokenUtils.getTokenId(token))); + } + @Test public void testRenewal() throws Exception { final JWTToken token = createMockToken(System.currentTimeMillis() - TimeUnit.SECONDS.toMillis(60)); diff --git a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/AuthorizeResource.java b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/AuthorizeResource.java index 4fd703c7a..d778aa7e1 100644 --- a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/AuthorizeResource.java +++ b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/AuthorizeResource.java @@ -37,6 +37,7 @@ import org.apache.knox.gateway.services.ServiceType; import org.apache.http.ssl.SSLContexts; import org.apache.knox.gateway.services.knoxidf.federation.FederatedIdentity; import org.apache.knox.gateway.services.knoxidf.federation.FederatedIdentityService; +import org.apache.knox.gateway.services.security.AliasService; import org.apache.knox.gateway.services.security.AliasServiceException; import org.apache.knox.gateway.services.security.KeystoreService; import org.apache.knox.gateway.services.security.token.JWTokenAuthority; @@ -51,7 +52,6 @@ import org.apache.knox.gateway.util.knoxidf.AuthorizeRequestMetadata; import org.apache.knox.gateway.util.knoxidf.AuthorizeRequestMetadataStore; import org.apache.knox.gateway.util.knoxidf.FederatedOpConfiguration; import org.apache.knox.gateway.util.knoxidf.FederatedOpConfigurationStore; -import org.apache.knox.gateway.util.knoxidf.KnoxIDFUtils; import javax.annotation.PostConstruct; import javax.servlet.ServletContext; @@ -66,6 +66,8 @@ import javax.net.ssl.SSLContext; import java.io.UnsupportedEncodingException; import java.net.URI; import java.security.KeyStore; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; import java.net.URISyntaxException; import java.net.URLEncoder; import java.nio.charset.StandardCharsets; @@ -189,7 +191,7 @@ public class AuthorizeResource extends PasscodeTokenResourceBase { private boolean hasConsent(final AuthorizeRequestMetadata authorizeRequestMetadata) { try { final TokenMetadata tokenMetadata = tokenStateService.getTokenMetadata(authorizeRequestMetadata.getClientId()); - final String consentKey = "consentAccepted_" + authorizeRequestMetadata.getSubject(); + final String consentKey = consentMetadataKey(authorizeRequestMetadata.getSubject()); final String storedScopes = tokenMetadata.getMetadataMap().get(consentKey); if (storedScopes == null || storedScopes.isEmpty()) { return false; @@ -204,10 +206,36 @@ public class AuthorizeResource extends PasscodeTokenResourceBase { private void markConsentAccepted(AuthorizeRequestMetadata authorizeRequestMetadata) { final TokenMetadata consentAcceptedMetadata = new TokenMetadata(); - consentAcceptedMetadata.add("consentAccepted_" + authorizeRequestMetadata.getSubject(), authorizeRequestMetadata.getJoinedRequestedScopes()); + consentAcceptedMetadata.add(consentMetadataKey(authorizeRequestMetadata.getSubject()), authorizeRequestMetadata.getJoinedRequestedScopes()); tokenStateService.addMetadata(authorizeRequestMetadata.getClientId(), consentAcceptedMetadata); } + /** + * Derives the metadata key under which a subject's granted consent scopes are stored. Consent is + * persisted in {@code KNOX_TOKEN_METADATA.md_name}, which is {@code VARCHAR(32)}; the previous + * {@code "consentAccepted_" + subject} key overflowed that for realistic subjects (federated + * UUID subjects, long usernames), silently truncating or failing the write on strict dialects. + * This derives a fixed-width key {@code "consent_" + first-20-hex-chars(SHA-256(subject))} = 28 + * chars, comfortably within the column. ~80 bits of hash is collision-safe for any realistic + * user population, and the derivation is uniform for plain usernames and UUID subjects alike. + * {@link #hasConsent} and {@link #markConsentAccepted} both route through here so read and write + * always agree on the key. + */ + static String consentMetadataKey(final String subject) { + try { + final MessageDigest digest = MessageDigest.getInstance("SHA-256"); + final byte[] hash = digest.digest((subject == null ? "" : subject).getBytes(StandardCharsets.UTF_8)); + final StringBuilder hex = new StringBuilder("consent_"); + for (int i = 0; i < 10; i++) { // 10 bytes -> 20 hex chars + hex.append(String.format(Locale.US, "%02x", hash[i])); + } + return hex.toString(); + } catch (NoSuchAlgorithmException e) { + // SHA-256 is a required algorithm on every JRE; its absence is unrecoverable. + throw new IllegalStateException("SHA-256 is required but unavailable", e); + } + } + private Response getAuthCodeFromKnox(final AuthorizeRequestMetadata authorizeRequestMetadata, final Pair<String, String> federatedTokens) { final Response tokenResponse = getAuthenticationToken(); if (tokenResponse.getStatus() == Response.Status.OK.getStatusCode()) { @@ -307,8 +335,11 @@ public class AuthorizeResource extends PasscodeTokenResourceBase { authCodeTokenMap.put(CODE_CHALLENGE_METHOD, authorizeRequestMetadata.getCodeChallengeMethod()); } if (federatedTokens != null) { + // Persist only the pointer to the (separately stored) federated identity. The OP's + // access token (federatedTokens.getRight()) is deliberately NOT persisted: nothing reads + // it back, and storing an OP bearer secret in plaintext token metadata is a secret-at-rest + // exposure. If a future feature needs it, store it encrypted, not in the clear. authCodeTokenMap.put(FEDERATED_IDENTITY_ID, federatedTokens.getLeft()); - authCodeTokenMap.putAll(KnoxIDFUtils.splitFederatedToken(federatedTokens.getRight(), false)); } tokenStateService.addMetadata(tokenId, new TokenMetadata(authCodeTokenMap)); } @@ -413,13 +444,41 @@ public class AuthorizeResource extends PasscodeTokenResourceBase { } } + /** + * Resolves the federated OP's client secret for the back-channel token request. An + * {@code AliasService} credential alias ({@code federated.op.<name>.clientSecret.alias}) is the + * preferred, secure source and takes precedence: when it resolves to a value, that value is + * used and the plaintext {@code clientSecret} topology param is never consulted. The plaintext + * param remains supported as a fallback only when no alias is configured, so existing + * deployments keep working. If an alias is configured but cannot be resolved we fail closed + * (return {@code null}) rather than silently leaking through to the plaintext param, so a + * misconfigured alias surfaces as an auth failure instead of masking the intended secure source. + */ + private String resolveClientSecret(final FederatedOpConfiguration opConfig) { + final String alias = opConfig.getClientSecretAlias(); + if (StringUtils.isBlank(alias)) { + return opConfig.getClientSecret(); + } + try { + final AliasService aliasService = getGatewayServices().getService(ServiceType.ALIAS_SERVICE); + String clusterName = (String) servletContext.getAttribute(GatewayServices.GATEWAY_CLUSTER_ATTRIBUTE); + if (StringUtils.isBlank(clusterName)) { + clusterName = AliasService.NO_CLUSTER_NAME; + } + final char[] secret = aliasService.getPasswordFromAliasForCluster(clusterName, alias, false); + return secret == null ? null : new String(secret); + } catch (AliasServiceException e) { + return null; + } + } + private Response fetchFederatedTokens(final String code, FederatedOpConfiguration opConfig) { final List<NameValuePair> params = new ArrayList<>(); params.add(new BasicNameValuePair(CODE, code)); params.add(new BasicNameValuePair(REDIRECT_URI, opConfig.getAuthorizeCallback())); params.add(new BasicNameValuePair(GRANT_TYPE, "authorization_code")); params.add(new BasicNameValuePair(CLIENT_ID, opConfig.getClientId())); - params.add(new BasicNameValuePair(CLIENT_SECRET, opConfig.getClientSecret())); + params.add(new BasicNameValuePair(CLIENT_SECRET, resolveClientSecret(opConfig))); try (CloseableHttpClient httpClient = createFederatedHttpClient()) { HttpPost post = new HttpPost(opConfig.getTokenEndpoint()); 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 97e24378c..540df06f6 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 @@ -81,10 +81,18 @@ import static org.apache.knox.gateway.util.knoxidf.KnoxIDFUtils.error; @Produces(MediaType.APPLICATION_JSON) public class TokenResource extends PasscodeTokenResourceBase { static final String RESOURCE_PATH = BASE_RESORCE_PATH + "/token"; + + // Per-request stash for the auth-code TokenMetadata read during validation. The code is + // atomically consumed (deleted) BEFORE token issuance to close the replay window, so the + // issuance steps (buildUserContext/addArbitraryTokenMetadata/buildResponseMap) can no longer + // re-read it from the store; they read this request attribute instead. This resource is a + // singleton, but the @Context request is a per-request proxy, so the attribute is request-scoped. + private static final String AUTH_CODE_METADATA_ATTR = "knoxidf.authCode.metadata"; + private UserParamsProvider userParamsProvider; @Context - private HttpServletRequest request; + HttpServletRequest request; // package-private for test injection; @Context injection is reflective @Context private ServletContext servletContext; @@ -140,8 +148,7 @@ public class TokenResource extends PasscodeTokenResourceBase { @Override protected UserContext buildUserContext(HttpServletRequest request) { try { - final String code = getRequestParam(CODE); - final TokenMetadata tokenMetadata = tokenStateService.getTokenMetadata(code); + final TokenMetadata tokenMetadata = getAuthCodeMetadata(); final String scope = tokenMetadata.getMetadata(SCOPE); final Map<String, Object> userParams = userParamsProvider.getParamsFor(tokenMetadata.getUserName(), scope); userParams.put(SCOPE, scope); @@ -158,7 +165,7 @@ public class TokenResource extends PasscodeTokenResourceBase { super.addArbitraryTokenMetadata(tokenMetadata); final String code = getRequestParam(CODE); if (StringUtils.isNotBlank(code)) { - final TokenMetadata authCodeTokenMetadata = tokenStateService.getTokenMetadata(code); + final TokenMetadata authCodeTokenMetadata = getAuthCodeMetadata(); //if the auth code token was a result of a federated OIDC call, we need to save the associated //federated identity ID in the JWT too (so that it can be looked up while fetching user info) @@ -181,7 +188,7 @@ public class TokenResource extends PasscodeTokenResourceBase { TokenMetadata authCodeTokenMetadata = null; if (StringUtils.isNotBlank(code)) { try { - authCodeTokenMetadata = tokenStateService.getTokenMetadata(code); + authCodeTokenMetadata = getAuthCodeMetadata(); } catch (UnknownTokenException e) { //NOP } @@ -251,25 +258,49 @@ public class TokenResource extends PasscodeTokenResourceBase { } } - private Response handleAuthorizationCodeFlow() { + // Package-private for testability (single-use replay guard is exercised by + // TokenResourceAuthCodeReplayTest); not part of the public resource API. + Response handleAuthorizationCodeFlow() { final String code = getRequestParam(CODE); final String redirectUri = getRequestParam(REDIRECT_URI); + final TokenMetadata authCodeMetadata; try { - validateAuthCode(code, redirectUri); - return getAuthenticationToken(); + authCodeMetadata = validateAuthCode(code, redirectUri); } catch (AuthTokenValidationError e) { return error("Auth code validation error", e.getMessage()); - } finally { - try { - tokenStateService.revokeToken(code); - } catch (UnknownTokenException e) { - //NOP: this should have been handled by the above UnknownTokenException already - } } + + // Enforce single-use: atomically consume the code BEFORE issuing any token. Of N concurrent + // redemptions of the same code, exactly one wins the consume and proceeds; the losers get + // invalid_grant. This closes the replay window that existed when the code was only revoked + // in a finally block AFTER issuance. A code that fails validation above is deliberately NOT + // consumed here, so replaying with bad params cannot burn a victim's still-valid code. + if (!tokenStateService.consumeToken(code)) { + return error("invalid_grant", "Authorization code has already been redeemed"); + } + + // The code is now gone from the store; hand the already-validated metadata to the issuance + // path via a request attribute (see getAuthCodeMetadata) so it need not re-read the code. + request.setAttribute(AUTH_CODE_METADATA_ATTR, authCodeMetadata); + return getAuthenticationToken(); + } + + /** + * Returns the auth-code {@link TokenMetadata} captured at validation time and stashed in a + * request attribute by {@link #handleAuthorizationCodeFlow()}. Because the code is consumed + * (deleted) before token issuance, the issuance steps can no longer re-read it from the store; + * this serves the cached copy, falling back to a store read only if the attribute is absent. + */ + private TokenMetadata getAuthCodeMetadata() throws UnknownTokenException { + final Object cached = request.getAttribute(AUTH_CODE_METADATA_ATTR); + if (cached instanceof TokenMetadata) { + return (TokenMetadata) cached; + } + return tokenStateService.getTokenMetadata(getRequestParam(CODE)); } - private void validateAuthCode(String code, String redirectUri) throws AuthTokenValidationError { + private TokenMetadata validateAuthCode(String code, String redirectUri) throws AuthTokenValidationError { try { if (code == null || code.isEmpty()) { throw new AuthTokenValidationError("Invalid request: missing code"); @@ -316,6 +347,7 @@ public class TokenResource extends PasscodeTokenResourceBase { } else if (!isValidClientSecret(clientId, getRequestParam(CLIENT_SECRET))) { throw new AuthTokenValidationError("Invalid client authentication"); } + return authCodeTokenMetadata; } catch (UnknownTokenException e) { throw new AuthTokenValidationError("Unknown auth_code"); } diff --git a/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/ConsentMetadataKeyTest.java b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/ConsentMetadataKeyTest.java new file mode 100644 index 000000000..e4feee4d6 --- /dev/null +++ b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/ConsentMetadataKeyTest.java @@ -0,0 +1,65 @@ +/* + * 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.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; + +import org.junit.Test; + +/** + * Verifies the consent metadata key derivation (finding 2.11). Consent is stored in + * {@code KNOX_TOKEN_METADATA.md_name VARCHAR(32)}; the key must therefore stay within 32 chars for + * every subject, be deterministic (so a later read finds an earlier write), and separate distinct + * subjects. + */ +public class ConsentMetadataKeyTest { + + /** The backing column is VARCHAR(32). */ + private static final int MD_NAME_MAX = 32; + + @Test + public void testKeyFitsColumnForRealisticSubjects() { + final String[] subjects = { + "alice", + "[email protected]", + // a federated UUID subject - the case that overflowed the old "consentAccepted_" + subject + "b9f8e7d6-c5a4-4321-9876-0123456789abcdef-very-long-external-subject-identifier", + "", + }; + for (final String subject : subjects) { + final String key = AuthorizeResource.consentMetadataKey(subject); + assertTrue("Key '" + key + "' (" + key.length() + " chars) must fit VARCHAR(" + MD_NAME_MAX + ")", + key.length() <= MD_NAME_MAX); + assertTrue("Key should carry the consent_ prefix", key.startsWith("consent_")); + } + } + + @Test + public void testKeyIsDeterministic() { + final String subject = "b9f8e7d6-c5a4-4321-9876-0123456789ab"; + assertEquals("Same subject must always derive the same key (read must match write).", + AuthorizeResource.consentMetadataKey(subject), AuthorizeResource.consentMetadataKey(subject)); + } + + @Test + public void testDistinctSubjectsDeriveDistinctKeys() { + assertNotEquals(AuthorizeResource.consentMetadataKey("alice"), + AuthorizeResource.consentMetadataKey("bob")); + } +} diff --git a/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/TokenResourceAuthCodeReplayTest.java b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/TokenResourceAuthCodeReplayTest.java new file mode 100644 index 000000000..ac0fb6cab --- /dev/null +++ b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/TokenResourceAuthCodeReplayTest.java @@ -0,0 +1,153 @@ +/* + * 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.CODE; +import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.CODE_CHALLENGE; +import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.REDIRECT_URI; +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.TokenStateService; +import org.apache.knox.gateway.services.security.token.impl.TokenMAC; +import org.easymock.EasyMock; +import org.junit.Before; +import org.junit.Test; + +/** + * Verifies the single-use enforcement of the authorization_code grant (finding 2.4). The code must + * be atomically consumed BEFORE any token is issued: exactly one of N concurrent redemptions wins + * the consume and proceeds to issuance; the losers are rejected with {@code invalid_grant} and no + * token is minted. This closes the replay window that existed when the code was only revoked in a + * {@code finally} block after issuance. + */ +public class TokenResourceAuthCodeReplayTest { + + private static final String AUTH_CODE = "auth-code-xyz"; + private static final String CLIENT = "client-abc"; + private static final String REDIRECT = "https://app.example/cb";; + 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(); + + /** + * Exposes field injection and stubs out the heavy token-issuance path so the replay guard can be + * exercised in isolation. {@code getAuthenticationToken} is the step that mints tokens; here it + * only records that issuance was reached and returns a sentinel. + */ + 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 + public Response getAuthenticationToken() { + issuedCount.incrementAndGet(); + return Response.ok("issued").build(); + } + } + + 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); + + // Metadata for the authorization code being redeemed (confidential client, no PKCE challenge). + final TokenMetadata authCodeMetadata = EasyMock.createNiceMock(TokenMetadata.class); + EasyMock.expect(authCodeMetadata.isAuthCode()).andReturn(true).anyTimes(); + EasyMock.expect(authCodeMetadata.getMetadata(REDIRECT_URI)).andReturn(REDIRECT).anyTimes(); + EasyMock.expect(authCodeMetadata.getMetadata(CLIENT_ID)).andReturn(CLIENT).anyTimes(); + EasyMock.expect(authCodeMetadata.getMetadata(CODE_CHALLENGE)).andReturn(null).anyTimes(); + EasyMock.replay(authCodeMetadata); + + // Metadata for the confidential client, used to authenticate the client_secret. + 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(AUTH_CODE)).andReturn(authCodeMetadata).anyTimes(); + EasyMock.expect(tokenStateService.getTokenExpiration(AUTH_CODE)) + .andReturn(System.currentTimeMillis() + TimeUnit.MINUTES.toMillis(5)).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(CODE)).andReturn(AUTH_CODE).anyTimes(); + EasyMock.expect(req.getParameter(REDIRECT_URI)).andReturn(REDIRECT).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 testFirstRedemptionConsumesThenIssues() { + EasyMock.expect(tokenStateService.consumeToken(AUTH_CODE)).andReturn(true).once(); + EasyMock.replay(tokenStateService); + + final Response response = resource.handleAuthorizationCodeFlow(); + + assertEquals("A validated, freshly-consumed code should issue a token.", + Response.Status.OK.getStatusCode(), response.getStatus()); + assertEquals("Exactly one issuance for a single winning redemption.", 1, issuedCount.get()); + EasyMock.verify(tokenStateService); + } + + @Test + public void testReplayedCodeIsRejectedWithoutIssuing() { + // Simulate a concurrent redemption having already consumed the code: consumeToken loses. + EasyMock.expect(tokenStateService.consumeToken(AUTH_CODE)).andReturn(false).once(); + EasyMock.replay(tokenStateService); + + final Response response = resource.handleAuthorizationCodeFlow(); + + assertEquals("A code already consumed by a concurrent redemption must be rejected.", + Response.Status.UNAUTHORIZED.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()); + EasyMock.verify(tokenStateService); + } +} diff --git a/gateway-spi/src/main/java/org/apache/knox/gateway/services/security/token/TokenStateService.java b/gateway-spi/src/main/java/org/apache/knox/gateway/services/security/token/TokenStateService.java index 2d3ea1b20..82d3331ee 100644 --- a/gateway-spi/src/main/java/org/apache/knox/gateway/services/security/token/TokenStateService.java +++ b/gateway-spi/src/main/java/org/apache/knox/gateway/services/security/token/TokenStateService.java @@ -102,6 +102,32 @@ public interface TokenStateService extends Service { */ void revokeToken(String tokenId) throws UnknownTokenException; + /** + * Atomically consume (revoke) the specified token, reporting whether <em>this</em> caller + * performed the removal. This enforces single-use semantics (e.g. OAuth authorization codes) + * under concurrent redemption: of N callers racing to consume the same token, exactly one + * receives {@code true} and all others receive {@code false} because the token was already gone. + * Unlike {@link #revokeToken(String)}, an absent token is reported as {@code false} rather than + * raising {@link UnknownTokenException}. + * <p> + * The default implementation delegates to {@link #revokeToken(String)} and is only as atomic as + * that method; implementations backed by a store that can remove-and-report atomically (a + * concurrent-map removal or a primary-key {@code DELETE}) should override this to provide a true + * single-winner guarantee. + * + * @param tokenId The token unique identifier. + * @return {@code true} iff this invocation removed a present token; {@code false} if it was + * already absent (never existed, or consumed by a concurrent caller). + */ + default boolean consumeToken(String tokenId) { + try { + revokeToken(tokenId); + return true; + } catch (UnknownTokenException e) { + return false; + } + } + /** * Extend the lifetime of the specified token by the default amount of time. * diff --git a/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/FederatedOpConfiguration.java b/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/FederatedOpConfiguration.java index 8ebdea5b1..b82d34441 100644 --- a/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/FederatedOpConfiguration.java +++ b/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/FederatedOpConfiguration.java @@ -23,6 +23,7 @@ public class FederatedOpConfiguration { private final String name; private final String clientId; private final String clientSecret; + private final String clientSecretAlias; private final String tokenEndpoint; private final String authorizeEndpoint; private final String userInfoEndpoint; @@ -41,6 +42,10 @@ public class FederatedOpConfiguration { this.enabled = Boolean.parseBoolean(servletContext.getInitParameter(prefix + "enabled")); this.clientId = servletContext.getInitParameter(prefix + "clientId"); this.clientSecret = servletContext.getInitParameter(prefix + "clientSecret"); + // Preferred, secure source for the OP client secret: an AliasService credential alias. + // Resolved at point of use (AuthorizeResource) since this holder has no access to services. + // When set it takes precedence over the plaintext clientSecret param above. + this.clientSecretAlias = servletContext.getInitParameter(prefix + "clientSecret.alias"); this.tokenEndpoint = servletContext.getInitParameter(prefix + "token.endpoint"); this.authorizeEndpoint = servletContext.getInitParameter(prefix + "authorize.endpoint"); this.authorizeCallback = servletContext.getInitParameter(prefix + "authorize.callback"); @@ -70,6 +75,15 @@ public class FederatedOpConfiguration { return clientSecret; } + /** + * @return the name of the AliasService credential alias holding this OP's client secret, or + * {@code null}/blank when the deployment supplies the secret via the plaintext + * {@code clientSecret} param instead. When set, the alias is authoritative. + */ + public String getClientSecretAlias() { + return clientSecretAlias; + } + String getAuthorizeEndpoint() { return authorizeEndpoint; } diff --git a/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFConstants.java b/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFConstants.java index aabb85684..57e8b3caf 100644 --- a/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFConstants.java +++ b/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFConstants.java @@ -52,8 +52,6 @@ public interface KnoxIDFConstants { String SCOPE_ATTRIBUTE = "X-Token-Scope"; String FEDERATED_IDENTITY_ID = "federated_identity_id"; - String FEDERATED_ID_TOKEN_PREFIX = "fed_id_"; - String FEDERATED_ACCESS_TOKEN_PREFIX = "fed_access_"; String FEDERATED_OP_CONFIG_PREFIX = "federated.op."; String FEDERATED_OP_CONFIG_NAMES = FEDERATED_OP_CONFIG_PREFIX + "names"; diff --git a/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFUtils.java b/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFUtils.java index 989a17e61..98aeb8520 100644 --- a/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFUtils.java +++ b/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFUtils.java @@ -25,42 +25,14 @@ import javax.servlet.http.HttpServletRequest; import javax.ws.rs.core.Response; import java.util.Arrays; import java.util.Collections; -import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; -import java.util.LinkedHashMap; import java.util.Map; import java.util.Set; -import java.util.stream.Collectors; public class KnoxIDFUtils { - private static final int CHUNK_SIZE = 255; - - public static Map<String, String> splitFederatedToken(String token, boolean idToken) { - final String prefix = idToken ? KnoxIDFConstants.FEDERATED_ID_TOKEN_PREFIX : KnoxIDFConstants.FEDERATED_ACCESS_TOKEN_PREFIX; - final Map<String, String> parts = new LinkedHashMap<>(); - int i = 0, part = 1; - while (i < token.length()) { - int end = Math.min(i + CHUNK_SIZE, token.length()); - parts.put(prefix + part++, token.substring(i, end)); - i = end; - } - return parts; - } - - public static String joinFederatedToken(Map<String, String> tokenMetadataMap, boolean idToken) { - final String prefix = idToken ? KnoxIDFConstants.FEDERATED_ID_TOKEN_PREFIX : KnoxIDFConstants.FEDERATED_ACCESS_TOKEN_PREFIX; - return tokenMetadataMap.entrySet().stream() - .filter(e -> e.getKey().startsWith(prefix)) - .sorted(Map.Entry.comparingByKey( - Comparator.comparingInt(k -> Integer.parseInt(k.replace(prefix, ""))) - )) - .map(Map.Entry::getValue) - .collect(Collectors.joining()); - } - public static Response error(String error, String description) { final Map<String, String> errorMap = new HashMap<>(); errorMap.put("error", error); diff --git a/gateway-util-common/src/test/java/org/apache/knox/gateway/util/knoxidf/FederatedOpConfigurationTest.java b/gateway-util-common/src/test/java/org/apache/knox/gateway/util/knoxidf/FederatedOpConfigurationTest.java index cfe74d8d9..8d1b87e54 100644 --- a/gateway-util-common/src/test/java/org/apache/knox/gateway/util/knoxidf/FederatedOpConfigurationTest.java +++ b/gateway-util-common/src/test/java/org/apache/knox/gateway/util/knoxidf/FederatedOpConfigurationTest.java @@ -59,6 +59,31 @@ public class FederatedOpConfigurationTest { assertEquals("RS256", config.getSignatureAlgorithm()); } + @Test + public void testClientSecretAliasIsRead() { + final ServletContext context = EasyMock.createNiceMock(ServletContext.class); + EasyMock.expect(context.getInitParameter(PREFIX + "clientSecret")).andReturn("plaintext-secret").anyTimes(); + EasyMock.expect(context.getInitParameter(PREFIX + "clientSecret.alias")).andReturn("keycloak-op-secret").anyTimes(); + EasyMock.replay(context); + + final FederatedOpConfiguration config = new FederatedOpConfiguration(context, OP); + + // Both are exposed; AuthorizeResource#resolveClientSecret decides precedence (alias wins). + assertEquals("plaintext-secret", config.getClientSecret()); + assertEquals("keycloak-op-secret", config.getClientSecretAlias()); + } + + @Test + public void testClientSecretAliasAbsentByDefault() { + final ServletContext context = EasyMock.createNiceMock(ServletContext.class); + EasyMock.replay(context); + + final FederatedOpConfiguration config = new FederatedOpConfiguration(context, OP); + + // No alias configured -> resolveClientSecret falls back to the plaintext clientSecret param. + assertNull(config.getClientSecretAlias()); + } + @Test public void testVerificationParamsAbsentByDefault() { final ServletContext context = EasyMock.createNiceMock(ServletContext.class);
