Skip to content

fix: DPoP should only include the path #296

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

Closed
wants to merge 4 commits into from
Closed

fix: DPoP should only include the path #296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4e87cf1
fix: DPoP should only include the path
mkleene Sep 23, 2025
86d603d
Update sdk/src/test/java/io/opentdf/platform/sdk/TokenSourceTest.java
mkleene Sep 23, 2025
ad8760f
coverage
mkleene Sep 23, 2025
612cbbd
Merge remote-tracking branch 'origin/fix-dpop' into fix-dpop
mkleene Sep 23, 2025
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
29 changes: 18 additions & 11 deletions sdk/src/main/java/io/opentdf/platform/sdk/TokenSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
* timeouts and creating OIDC calls. It is thread-safe.
*/
class TokenSource {
private final DefaultDPoPProofFactory dpopProofFactory;
private Instant tokenExpiryTime;
private AccessToken token;
private final ClientAuthentication clientAuth;
Expand All @@ -51,6 +52,11 @@ public TokenSource(ClientAuthentication clientAuth, RSAKey rsaKey, URI tokenEndp
this.tokenEndpointURI = tokenEndpointURI;
this.sslFactory = sslFactory;
this.authzGrant = authzGrant;
try {
this.dpopProofFactory = new DefaultDPoPProofFactory(rsaKey, JWSAlgorithm.RS256);
} catch (JOSEException e) {
throw new SDKException("Error creating DPoP proof factory", e);
}
}

class AuthHeaders {
Expand All @@ -74,24 +80,25 @@ public String getDpopHeader() {
public AuthHeaders getAuthHeaders(URL url, String method) {
// Get the access token
AccessToken t = getToken();

// Build the DPoP proof for each request
String dpopProof;
try {
DPoPProofFactory dpopFactory = new DefaultDPoPProofFactory(rsaKey, JWSAlgorithm.RS256);
SignedJWT proof = dpopFactory.createDPoPJWT(method, url.toURI(), t);
dpopProof = proof.serialize();
} catch (URISyntaxException e) {
throw new SDKException("Invalid URI syntax for DPoP proof creation", e);
} catch (JOSEException e) {
throw new SDKException("Error creating DPoP proof", e);
}
String dpopProof = getDPoPProof(url, method, dpopProofFactory, t);

return new AuthHeaders(
"DPoP " + t.getValue(),
dpopProof);
}

// package-private for testing
static String getDPoPProof(URL url, String method, DPoPProofFactory dpopFactory, AccessToken t) {
try {
URI onlyPath = new URI(url.getPath());
Copy link
Contributor

Choose a reason for hiding this comment

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

high

Using just the path for the htu (HTTP URI) claim in the DPoP proof might not be compliant with RFC 9449. Section 4.2 of the RFC specifies that the htu claim MUST be an absolute URI for requests over TLS.

The htu value MUST be an absolute URI, as defined in Section 4.3 of [RFC3986], unless the HTTP request is to a resource at an origin that is not deployed with a TLS server certificate, in which case the htu value MUST be a relative URI...

This implementation unconditionally uses a relative URI (the path), which is only valid for non-TLS requests according to the standard. While this may be necessary for a specific server implementation, it could cause DPoP validation to fail with other servers that strictly adhere to the RFC when using TLS.

If this change is intentional to support a specific backend, it would be good to document this behavior. Otherwise, you might consider making this logic conditional on the URL's protocol (e.g., http vs. https).

Copy link
Member

Choose a reason for hiding this comment

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

Ditto this, add a comment that this describes why we diverge from the RFC here

SignedJWT proof = dpopFactory.createDPoPJWT(method, onlyPath, t);
return proof.serialize();
} catch (JOSEException | URISyntaxException e) {
throw new SDKException("Error creating DPoP proof", e);
}
}

/**
* Either fetches a new access token or returns the cached access token if it is still valid.
*
Expand Down
35 changes: 35 additions & 0 deletions sdk/src/test/java/io/opentdf/platform/sdk/TokenSourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package io.opentdf.platform.sdk;

import com.nimbusds.jose.JOSEException;
import com.nimbusds.jose.JWSAlgorithm;
import com.nimbusds.jose.JWSObject;
import com.nimbusds.jose.jwk.RSAKey;
import com.nimbusds.oauth2.sdk.dpop.DefaultDPoPProofFactory;
import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
import org.junit.jupiter.api.Test;

import java.net.MalformedURLException;
import java.net.URI;
import java.net.URISyntaxException;
import java.security.interfaces.RSAPublicKey;
import java.text.ParseException;

import static org.assertj.core.api.Assertions.assertThat;

class TokenSourceTest {

@Test
void getDPoPProof() throws URISyntaxException, JOSEException, MalformedURLException, ParseException {
var fakeToken = new BearerAccessToken("this is a fake token");
var keypair = CryptoUtils.generateRSAKeypair();
var dpopKey = new RSAKey.Builder((RSAPublicKey) keypair.getPublic()).privateKey(keypair.getPrivate()).build();
var dpopProofFactory = new DefaultDPoPProofFactory(dpopKey, JWSAlgorithm.RS256);

var dpop = TokenSource.getDPoPProof(new URI("http://example.org/path/to/the/resource").toURL(), "POST", dpopProofFactory, fakeToken);

var jws = JWSObject.parse(dpop);
assertThat(jws.getPayload().toJSONObject())
.extracting("htu")
.containsOnly("/path/to/the/resource");
}
}
Loading