🚧 Let DPoP proposed standard supplant client signing key

[dmihalcik-virtru]

MINOR Update: Explicitly supporting IETF DPoP Draft Proposal

• This proposal aligns our Proof of Possession of the access token with the current IETF DPoP draft proposal. This includes:
  • Deprecating storing the entire key in the tdf_claims.client_public_signing_key subclaim
  • Store a SHA-256 hash of the key in the cnf.jkt field, per the current draft proposal
  • Submit a DPoP in the DPoP header, which will include the complete key and request identifying information
• Not included:
  • To get complete support for the missing functionality we may desire to add a body hash to the DPoP claims list, which will provides an additional check for partial message integrity.

A reference implementation of this change is now included with opentdf/backend and opentdf/client-web. If accepted we will port this to the remaining opentdf/client-*.

Reference Implementation Details

Client Implementation

For the client, we use an existing DPoP implementation to generate proofs. To use, we added a parameter (dpopEnabled) to the clients, and if present make this required.

Implementation: opentdf/web-sdk#118

Identity Provider Implementation (Access Token Generation)

Our reference IdP, Keycloak, provides a java plugin environment for adding claims to access tokens. Our current reference implementation extended our existing plugin, which adds the tdf_claims claim, to include the cnf.jkt claim when there is a DPoP header in the code exchange.

Reference: https://github.com/opentdf/backend/blob/9603538287e23240c6d40ea596cf42f8b26bfcc4/containers/keycloak-protocol-mapper/custom-mapper/src/main/java/com/virtru/keycloak/TdfClaimsMapper.java#L292

Access Point Validation (KAS)

KAS currently uses application level validation of the OIDC header. We extended this to include the DPoP check iff the access token includes the jkt claim.

https://github.com/opentdf/backend/blob/9603538287e23240c6d40ea596cf42f8b26bfcc4/containers/kas/kas_core/tdf3_kas_core/dpop.py#L55

Checklist

• A clear description of the change has been included in this PR.
• A clear description of whether this change is a Major, Minor, Patch or cosmetic change as per the Versioning Guidelines has been included in this PR.
• All schema validation tests have been updated appropriately and are passing.
• MAJOR/MINOR VERSION CHANGES ONLY: This PR should be made in branches prefixed with draft-<change>
• MAJOR/MINOR VERSION CHANGES ONLY: A link to a reference implementation (PR or set of PRs) of the change has been included in this PR.
• MAJOR/MINOR VERSION CHANGES ONLY: A writeup has been included discussing the motivation and impact of this change.
• MAJOR/MINOR VERSION CHANGES ONLY: The minimum wait time has elapsed.
• DRAFT MERGE ONLY: Draft Semver has been updated in the VERSION file (optional)
• DRAFT MERGE ONLY: Tagged this branch with new semver version and an annotation describing the change (ex: git tag -s 4.1.0 -m "Spec version 4.1.0 - did a thing")
• DRAFT MERGE ONLY: Version numbers have been updated as per the Versioning Guidelines.
• This change otherwise adheres to the project Contribution Guidelines.

[bleggett]

Suggested change

* From Wikipedia: "Keycloak is an open source software product to allow single sign-on with Identity and Access Management aimed at modern applications and services. As of March 2018 this JBoss community project is under the stewardship of Red Hat. Keycloak is licensed under Apache 2.0."

(Predates your change but) I don't think this line adds much, so feel like it should be nuked - there's no point to quoting the first sentence of a wikipedia article anybody can google.

Would rather simply link to Keycloak.org as suggested above.

[dmihalcik-virtru]

I'll try moving to a footnote and see if that makes sense

[dmihalcik-virtru]

@bleggett should I close this PR and start a new one in a branch named draft-dpop?

[bleggett]

@dmihalcik-virtru can you update the PR comment itself (or link to a doc if necessary) with a concise explanation of what specific attacks the DPoP model here prevents against, that are not prevented by the existing "body signature PoP" in the current spec?

I think that will be important/useful.

@bleggett should I close this PR and start a new one in a branch named draft-dpop?

Meh, at this point that's probably not productive. I'll add a #draft PR label instead.

[bleggett]

Having looked at the linked PR/draft implementation example (thx @dmihalcik-virtru - having a Real PR to back up a draft proposal is extremely helpful), this looks good to me.

[dmihalcik-virtru]

@bleggett Added details about the current reference implementation to the PR description. PTAL

[bleggett]

@dmihalcik-virtru looks good to me, and hits all the stuff in CONTRIBUTING.md - you want to merge?

After merge, I can cut a new minor version specbump tag.

[dmihalcik-virtru]

@bleggett Sure, I'll merge after a restamp for the last commit.