Skip to content

keep mtls certificate binding on refreshed access tokens #3652

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.

Sign up for GitHub

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

Open
rootvector2 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

keep mtls certificate binding on refreshed access tokens #3652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 12 additions & 6 deletions ...2/src/main/java/org/apache/cxf/rs/security/oauth2/provider/AbstractOAuthDataProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,19 +85,23 @@ protected ServerAccessToken doCreateAccessToken(AccessTokenRegistration atReg) {
atReg.getClient(), atReg.getApprovedScope()),
atReg.getSubject());

setCertificateThumbprintConfirmation(at);

if (isUseJwtFormatForAccessTokens()) {
convertToJWTAccessToken(at);
}

return at;
}

private void setCertificateThumbprintConfirmation(ServerAccessToken at) {
if (messageContext != null) {
String certCnf = (String)messageContext.get(JoseConstants.HEADER_X509_THUMBPRINT_SHA256);
if (certCnf != null) {
// At a later stage we will likely introduce a dedicated Confirmation bean (as it is used in POP etc)
at.getExtraProperties().put(JoseConstants.HEADER_X509_THUMBPRINT_SHA256, certCnf);
}
}

if (isUseJwtFormatForAccessTokens()) {
convertToJWTAccessToken(at);
}

return at;
}

//CHECKSTYLE:OFF
Expand Down Expand Up @@ -475,6 +479,8 @@ protected ServerAccessToken doRefreshAccessToken(Client client,
oldRefreshToken.getGrantType(), oldRefreshToken.getNonce(),
null, theNewScopes, oldRefreshToken.getSubject());

@utafrali utafrali Oct 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

doRefreshAccessToken passes oldRefreshToken.getExtraProperties() into the lower-level doCreateAccessToken call (so the new token inherits those properties), and then immediately calls setCertificateThumbprintConfirmation. If any subclass or future code ever stores x5t#S256 in the refresh token's extra properties, the thumbprint would be silently overwritten only when messageContext has one, and preserved otherwise. This creates an ambiguous write-order. A short inline comment noting that setCertificateThumbprintConfirmation intentionally supersedes any thumbprint that may have been copied from extraProperties would clarify the intent.

@rootvector2 rootvector2 Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

doCreateNewRefreshToken doesn't copy the access token's extra properties, so a refresh token never carries x5t#S256 and there's nothing to overwrite. The create path uses the same order too (registration extra properties first, then the thumbprint from the current request), so I left it without a comment.

setCertificateThumbprintConfirmation(at);

if (isUseJwtFormatForAccessTokens()) {
convertToJWTAccessToken(at);
}
Expand Down
24 changes: 24 additions & 0 deletions ...c/test/java/org/apache/cxf/rs/security/oauth2/provider/AbstractOAuthDataProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import java.util.Collections;
import java.util.List;

import org.apache.cxf.jaxrs.ext.MessageContext;
import org.apache.cxf.rs.security.jose.common.JoseConstants;
import org.apache.cxf.rs.security.jose.jwa.SignatureAlgorithm;
import org.apache.cxf.rs.security.jose.jws.JwsJwtCompactConsumer;
import org.apache.cxf.rs.security.jose.jws.JwsSignatureProvider;
Expand All @@ -48,6 +50,8 @@
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

abstract class AbstractOAuthDataProviderTest {
private static KeyPair keyPair;
Expand Down Expand Up @@ -403,6 +407,26 @@ public void testCrossClientRefreshTokenRejectedWhenRecycleDisabled() {
assertNotNull(getProvider().getAccessToken(atA.getTokenKey()));
}

@Test
public void testRefreshedAccessTokenKeepsCertificateBinding() {

@utafrali utafrali Oct 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

getProvider().setMessageContext(mc) is never reset after this test. If the provider is shared across tests (typical for abstract base test classes with a single @Before setup), every test that runs after this one will see the mock MessageContext returning "thumbprint", which will silently pollute those tests. Reset it in a try-finally block or an @After method:

try {
    getProvider().setMessageContext(mc);
    // ... assertions ...
} finally {
    getProvider().setMessageContext(null);
}

Also, the client "101" registered here is never cleaned up via tearDownClient("101"), which can leave stale state for other tests.

@rootvector2 rootvector2 Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Each subclass builds a new provider in its @Before, so the mock MessageContext doesn't outlive the test. Client 101 is already removed by tearDownClients() in the base @After, same as the other tests here that use it.

MessageContext mc = mock(MessageContext.class);
when(mc.get(JoseConstants.HEADER_X509_THUMBPRINT_SHA256)).thenReturn("thumbprint");
getProvider().setMessageContext(mc);

Client c = addClient("101", "bob");

AccessTokenRegistration atr = new AccessTokenRegistration();
atr.setClient(c);
atr.setApprovedScope(Arrays.asList("a", "refreshToken"));
atr.setSubject(c.getResourceOwnerSubject());

ServerAccessToken at = getProvider().createAccessToken(atr);
assertEquals("thumbprint", at.getExtraProperties().get(JoseConstants.HEADER_X509_THUMBPRINT_SHA256));

ServerAccessToken at2 = getProvider().refreshAccessToken(c, at.getRefreshToken(), Collections.emptyList());
assertEquals("thumbprint", at2.getExtraProperties().get(JoseConstants.HEADER_X509_THUMBPRINT_SHA256));
}

protected Client addClient(String clientId, String userLogin) {

@utafrali utafrali Oct 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The test only covers the happy path where the cert thumbprint is present in the message context for both the initial request and the refresh. Consider adding a second case: bind the initial token with a cert, then refresh with a MessageContext that returns null for the thumbprint (simulating a misconfiguration or a refresh request that arrives without the client certificate). The expected behavior in that scenario (token becomes unbound vs. an error is thrown) should be explicitly documented and tested, since a stolen refresh token could otherwise be used to obtain an unbound access token.

@rootvector2 rootvector2 Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A refresh without a cert gives an unbound token, which is exactly what doCreateAccessToken(AccessTokenRegistration) does for the initial grant. AbstractTokenService only puts x5t#S256 in the context when the client authenticates with its certificate, so the binding follows the cert on the current request. A tls_client_auth client can't refresh without it, and a confidential client's stolen refresh token is still useless without the client credentials. Rejecting an uncertified refresh of a bound token (the RFC 8705 section 4 public client case) needs the thumbprint persisted on RefreshToken, which I'd keep as a separate change.

Client c = new Client();
c.setRedirectUris(Collections.singletonList("http://client/redirect"));
Expand Down