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

feat(core): New cryptoProvider config #939

Merged
merged 6 commits into from
Jun 12, 2024
Merged

feat(core): New cryptoProvider config #939

merged 6 commits into from
Jun 12, 2024

Conversation

dmihalcik-virtru
Copy link
Member

@dmihalcik-virtru dmihalcik-virtru commented Jun 6, 2024

  • define new cryptoProvider config structs to support rotation with different keys
  • this means the algorithm must be present
  • I'm using some heuristics to maintain backward compatibility of standard crypto configs, but hsm configs must be updated
  • Adds kid in response to kas_public_key
  • Adds kid in key access objects produced by SDK
  • Still no support in nanotdf, as we have not agreed on how/where to store the kid value in the header. See ADR: NanoTDF KAS resource locator path and key identifier #900

New Config:

server:
  cryptoProvider:
    type: standard
    standard:
      keys:
        - kid: r1
          alg: rsa:2048
          private: kas-private.pem
          cert: kas-cert.pem
        - kid: r0
          alg: rsa:2048
          private: kas-private-old.pem
          cert: kas-cert-old.pem
        - kid: e1
          alg: ec:secp256r1
          private: kas-ec-private.pem
          cert: kas-ec-cert.pem
services:
  kas:
    enabled: true
    keyring:
      - alg: rsa:2048
        kid: r1
      - alg: rsa:2048
        kid: r0
        legacy: true
      - alg: ec:secp256r1
        kid: e1

some notes:

  • kid values should be unique, preferably for the lifetime of the kas host domain name
  • kid values should short strings (I'd suggest maxing out at 44 characters)
  • private and cert indicate the location of private key and a certificate, if available
  • For hsm keys, these should be label values
  • For standard keys, these should be paths to PEM files relative to the current working directory
  • I've deprecated the eccertid for a new keyring parameter which describes how KAS will interpret the key. So we have two sections: server.cryptoProvider describes what keys are available, while service.kas.keyring describes how KAS uses those keys.
  • We don't have a 'rotate' script yet. To do this manually, update the init-temp-keys script to use a new name/label and rerun and add the new keys to the list, updating the certid fields to point to the new values

To come:

  1. nanoTDF support

@dmihalcik-virtru dmihalcik-virtru changed the title feature(core): Adds key identifier support to KAS WIP feat(core): Adds key identifier support to KAS WIP Jun 6, 2024
@dmihalcik-virtru dmihalcik-virtru force-pushed the feature/kid branch 4 times, most recently from 7f915f4 to 630e55a Compare June 7, 2024 17:21
@dmihalcik-virtru dmihalcik-virtru changed the title feat(core): Adds key identifier support to KAS WIP feat(core): New cryptoProvider config Jun 7, 2024
@dmihalcik-virtru dmihalcik-virtru marked this pull request as ready for review June 7, 2024 17:56
@dmihalcik-virtru dmihalcik-virtru requested review from a team as code owners June 7, 2024 17:56
@dmihalcik-virtru
Copy link
Member Author

@pflynn-virtru Can you review this?

Some ideas for improvement to discuss:

  • How to clean up how the default kid (rsacertid/eccertid is how we used to do it; we need at least a map from algorithm->kid somehow, either as a list or directly as a map, and we should avoid names that don't directly map to an algorithm)
  • How to load KAOs (and nanotdf headers) that don't have a kid. Right now it only tries the current default so it will fail on all older TDFs (especially given they won't have keys). Previously my plan was to either try them in reverse order from oldest to newest, so we'd have to specify an order or add some other sortable field to the KeyPairInfo object, or just use the ordering from the file.

@strantalis
Copy link
Member

@dmihalcik-virtru I opened up this issue earlier today. I think @ttschampel ran into similar issues. But it's not clear what eccertid is needed for or what it does.

Also what are your thoughts on moving the crypto provider. It has felt kind of odd living under the server block when kas is the only service that leverages it.

#954

@biscoe916 @jrschumacher Would we use this same crypto for signing attributes or other resources in the future?

- define new config structs to support rotation with different keys
- this means the algorithm must be present
- I'm using some heuristics to maintain backward compatibility of standard crypto configs, but hsm configs must be updated
- Adds `kid` in response to kas_public_key

sdk part
This allow searching multiple kas keys for ones that decrypt KAO values with no KID
Reverts changes only required for day2 key rotation tests
@dmihalcik-virtru
Copy link
Member Author

@strantalis We can probably move all of the config into the keyring section with little disruption to KAS. The code is written at the moment to allow all services to use a shared crypto provider, which is really an abstraction layer that allows us to switch between a pkcs#11 backend and a golang crypto backend - which could be something other services desire but in practice so far we haven't needed it elsewhere

@dmihalcik-virtru dmihalcik-virtru added this pull request to the merge queue Jun 12, 2024
Merged via the queue into main with commit 8150623 Jun 12, 2024
15 checks passed
@dmihalcik-virtru dmihalcik-virtru deleted the feature/kid branch June 12, 2024 14:34
github-merge-queue bot pushed a commit that referenced this pull request Jun 20, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.2.4](protocol/go/v0.2.3...protocol/go/v0.2.4)
(2024-06-18)


### Features

* **core:** New cryptoProvider config
([#939](#939))
([8150623](8150623))
* **policy:** add unsafe service protos and unsafe service proto Go
gencode ([#1003](#1003))
([55cc045](55cc045))


### Bug Fixes

* **core:** policy resource-mappings fix doc drift in proto comments
([#980](#980))
([09ab763](09ab763))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
github-merge-queue bot pushed a commit that referenced this pull request Jun 24, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.2.8](sdk/v0.2.7...sdk/v0.2.8)
(2024-06-24)


### Features

* Audit GetDecisions
([#976](#976))
([55bdfeb](55bdfeb))
* **core:** New cryptoProvider config
([#939](#939))
([8150623](8150623))


### Bug Fixes

* **core:** Update to lib/fixtures 0.2.7
([#1017](#1017))
([dbae6ff](dbae6ff))
* **core:** Updates to protos 0.2.4
([#1014](#1014))
([43e11a3](43e11a3))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
github-merge-queue bot pushed a commit that referenced this pull request Jun 24, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.4.7](service/v0.4.6...service/v0.4.7)
(2024-06-24)


### Features

* add dev_mode flag
([#985](#985))
([8da2436](8da2436))
* adds new trace log level
([#989](#989))
([25f699e](25f699e))
* Audit GetDecisions
([#976](#976))
([55bdfeb](55bdfeb))
* **authz:** Use flattened entity representations in subject mapping
evaluation ([#1007](#1007))
([b80443f](b80443f))
* **core:** add doublestar for public routes
([#998](#998))
([1c70c16](1c70c16))
* **core:** New cryptoProvider config
([#939](#939))
([8150623](8150623))
* **policy:** add unsafe service protos and unsafe service proto Go
gencode ([#1003](#1003))
([55cc045](55cc045))
* **policy:** policy unsafe namespace RPCs wired up to database
([#1018](#1018))
([239d9fa](239d9fa))
* **policy:** service stubs and registration for unsafe service
([#1009](#1009))
([9145491](9145491))


### Bug Fixes

* config loaded debug statement logs secrets
([#1010](#1010))
([6f6a603](6f6a603))
* **core:** Autobump service
([#1025](#1025))
([588827c](588827c))
* **core:** Fixes issue failing to find keys for kid-free kaos
([#982](#982))
([f27d484](f27d484))
* **core:** policy resource-mappings fix doc drift in proto comments
([#980](#980))
([09ab763](09ab763))
* **core:** Update to lib/fixtures 0.2.7
([#1017](#1017))
([dbae6ff](dbae6ff))
* **core:** Updates to protos 0.2.4
([#1014](#1014))
([43e11a3](43e11a3))
* **kas:** remove old logs
([#992](#992))
([192ff6d](192ff6d))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
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.

5 participants