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

engineccl: Support JWKS format for encryption-at-rest keys #119767

Closed
bdarnell opened this issue Feb 29, 2024 · 0 comments · Fixed by #119913
Closed

engineccl: Support JWKS format for encryption-at-rest keys #119767

bdarnell opened this issue Feb 29, 2024 · 0 comments · Fixed by #119913
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@bdarnell
Copy link
Contributor

bdarnell commented Feb 29, 2024

Is your feature request related to a problem? Please describe.

The enterprise encryption flag contains support for key rotation, but not for migrating to new encryption algorithms. We need to introduce new configurability to select an encryption algorithm. This could be done as a part of the command line flag, but it would be even better to attach algorithm information to the key files themselves to take advantage of the ability to reload keys without restarting the server.

Describe the solution you'd like

Replace our current non-standard key file format with the JWKS format (specified in RFC 7517 and already used elsewhere in CRDB for SSO protocols). The key file must be a JWKS object that contains exactly one key, while the old-key file could optionally gain the ability to contain multiple keys (to facilitate frequent key rotations without waiting for data to be rewritten). (Is there a better or more JWKS-standard way to designate one active key out of a key set?)

Our current key files are a concatenation of a binary key id and the raw key data. The new key files would look like

{
    "keys": [
        {
            "kty": "oct",  // "octets", indicates that this is a single blob as opposed to e.g. a pub/priv pair
            "kid": "base64'd key id",
            "k": "base64'd key data",
            "alg": "cockroach-aes-ctr-v1",
        }
    ],
}

The point of the new format is the "alg" field, where we would define identifiers for our encryption-at-rest algorithms. Unfortunately we'd have to define our own algorithm IDs, since none of the standardized ones are suitable for our purpose (they are all designed for messages small enough to be decrypted all at once, and not for large files like our sstables). While cockroach-aes-ctr-v1 is something that should remain cockroach-specific (aes-ctr-v2 is a little better but still not AEAD), we could consider in the future advocating for standardization of a better seekable algorithm/format (i.e. one that divides the data stream into equal-sized chunks and applies AES-GCM on a per-chunk basis).

Concretely, this means the following changes:

  • cockroach gen encryption-key gains the option to generate the new format
  • The key loading code learns to recognize the new format. Unfortunately the old format doesn't contain any version marker, but since it contains purely random data it's safe to assume that any key file that parses as json is in the new format.
  • The alg field is used to select between encryption implementations, and is persisted in the per-file EncryptionSettings in the registry.

Describe alternatives you've considered

The --enterprise-encryption flag could gain a new alg argument and leave the key files unchanged. But this means that changing algorithms requires a node restart, and we'd probably have to build some more observability tools to track the progress of the change.

The JWKS format in theory makes it possible to use off-the-shelf tools instead of writing our own, but the need to use custom alg values limits the applicability of existing tools. If the JWKS format had notable drawbacks this might be a reason to design our own, but I don't see any reason to do so.

Other key file formats are in use in the public-key crypto world (PEM, etc), but as far as I can tell there's not much besides JWKS that is widely used for symmetric keys.

Integrating encryption into the pebble WAL and SST format at a higher level would be a better way to use AEAD algorithms. If we went that route the existing encryption at rest code could become obsolete.

Additional context

This is the replacement for #118824

Open question: Is the "alg" aes-ctr and it supports multiple key sizes, or are there separate algs for each key size? The standard JWKS algorithms include the key size in the alg name, and I have a slight preference for following that pattern going forward.

CCing some folks who have reviewed previous PRs in this series: @RaduBerinde @sumeerbhola @rsevinsky-cr

Jira issue: CRDB-36304

@bdarnell bdarnell added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Feb 29, 2024
bdarnell added a commit to bdarnell/cockroach that referenced this issue Mar 6, 2024
Fixes: cockroachdb#119767
Release note (enterprise change): `cockroach gen encryption-key` now
accepts a `--version=2` parameter. Version 2 keys activate a new
encryption implementation with improved performance. This is
expected to become the default in CockroachDB 24.2.
bdarnell added a commit to bdarnell/cockroach that referenced this issue Mar 7, 2024
Fixes: cockroachdb#119767
Release note (enterprise change): `cockroach gen encryption-key` now
accepts a `--version=2` parameter. Version 2 keys activate a new
encryption implementation with improved performance. This is
expected to become the default in CockroachDB 24.2.
craig bot pushed a commit that referenced this issue Mar 11, 2024
119913: engineccl: Support JWKS for encryption-at-rest keys and hook up v2 r=RaduBerinde a=bdarnell

The JWKS format for key files is now supported in addition to the legacy format (which was just raw key bytes). Keys in JWKS format can request the faster V2 encryption implementation, supporting graceful rotation into the new implementation.

Fixes #119767

Co-authored-by: Ben Darnell <ben@cockroachlabs.com>
@craig craig bot closed this as completed in 617bac7 Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant