Skip to content
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.

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

✨ Adds key identifier field to KAO #21

Merged
merged 5 commits into from
Nov 14, 2023

Conversation

dmihalcik-virtru
Copy link
Member

@dmihalcik-virtru dmihalcik-virtru commented Aug 21, 2023

Proposed Changes

MINOR enhancement

  • Adds a SHOULD field to the KAO, kid, which identifies which KAS key was used to wrap the data encryption key.
  • We also allow the kas_public_key endpoint to allow return the kid. Unfortunately this is a major update for that field, so to maintain backward compatibility I'm adding it as an additional request parameter to the endpoint

Intended use:

  • Allow KAS key rotation. If a kas sees a rewrap request with an 'old' key identifier, it can still implement a rewrap for it.

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.

- This will simplify key rotation on the KAS, while maintaining support for 'legacy' or 'past' keys.
- Adds v2 format, with `kid` in response`
- Allows returning the value in JWK format
pflynn-virtru
pflynn-virtru previously approved these changes Aug 21, 2023
@dmihalcik-virtru dmihalcik-virtru marked this pull request as draft August 22, 2023 14:53
@dmihalcik-virtru
Copy link
Member Author

Converting to draft while I implement a proof of concept in opentdf/backend and opentdf/client-web

@dmihalcik-virtru
Copy link
Member Author

Backend Proof of concept: https://github.com/opentdf/backend/pull/521
Client Proof of Concept: opentdf/web-sdk#243

@dmihalcik-virtru dmihalcik-virtru marked this pull request as ready for review October 11, 2023 18:22
@@ -25,6 +26,7 @@ A Key Access Object stores not only a wrapped (encrypted) key used to encrypt th
|`keyAccess`|Object|KeyAccess object stores all information about how an object key OR key split is stored, and if / how it has been encrypted (e.g., with KEK or pub wrapping key).|Yes|
|`type`|String|Specifies how the key is stored.<p>Possible Values: <dl><dt>remote</dt><dd>The wrapped key (see below) is stored using Virtru infrastructure and is thus not part of the final TDF manifest.</dd><dt>wrapped</dt><dd>Default for TDF 3.x and newer, the wrapped key is stored as part of the manifest.</dd><dt>remoteWrapped</dt><dd>Allows management of customer hosted keys, such as with a *Customer Key Server*. This feature is available as an upgrade path.</dd></dl>|Yes|
|`url`|String|A url pointing to the desired KAS deployment|Yes|
|`kid`|String|Identifier for the KAS public key.|Recommended|
Copy link
Member

Choose a reason for hiding this comment

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

Can we be more descriptive about how the KID is generated?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added some detail here about what we currently use in our reference implementation and other alternatives, and also added a note in the swagger for the relevant endpoint

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