Skip to content

Conversation

@dmihalcik-virtru
Copy link
Member

  • expose legacy public key endpoint as public
  • DPoP htu should include origin part of url
  • clarify error messages for dpop

Copy link
Contributor

@mkleene mkleene left a comment

Choose a reason for hiding this comment

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

Do we gain anything by only allowing some hosts? This seems to make sure that the token is for us. I'd say that the aud parameter is what we want here and this is just another thing that we need to configure.

There might be something that I'm missing about what this buys us, though.

@sonarqubecloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@dmihalcik-virtru
Copy link
Member Author

Do we gain anything by only allowing some hosts? This seems to make sure that the token is for us. I'd say that the aud parameter is what we want here and this is just another thing that we need to configure.

RFC 9449 seems to indicate we should do a full match of the URI, after 'normalization' and removing any fragment or query. The code currently only does a match of the path. A compliant DPoP client will set the HTU to exactly this value, although we should still do normalization of it after receiving it.

https://www.rfc-editor.org/rfc/rfc9449.html#name-checking-dpop-proofs

Anyway I see two solutions:

  1. only look at the path. this seems to be a violation of the spec
  2. Use the origin header from the request, if present
  3. allow the URL or URL root to be passed in as a parameter, of which this is one solution.

I'll look into 2

- expose legacy public key endpoint as public
- DPoP `htu` should include origin part of url
- clarify error messages for dpop
removes allowedHosts parameter, isntead use origin header in request
@dmihalcik-virtru
Copy link
Member Author

Now it checks the htu field of the DPoP token matches the request Origin + Path

Copy link
Contributor

@mkleene mkleene left a comment

Choose a reason for hiding this comment

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

This looks great!

@dmihalcik-virtru dmihalcik-virtru added this pull request to the merge queue Apr 24, 2024
Merged via the queue into main with commit 10b81e5 Apr 24, 2024
@dmihalcik-virtru dmihalcik-virtru deleted the feature/key-fix branch April 24, 2024 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants