kmsv2: KDF based nonce extension #118828
kmsv2: KDF based nonce extension #118828
Conversation
|
/uncc alculquicondor caesarxuchao |
|
/cc @nilekhc |
|
Hey, I'm Anhelina from the Bug Triage Release team 1.28 👋. I wanted to let you know that Code Freeze is coming up on 01:00 UTC Wednesday 19th July 2023 / 18:00 PDT Tuesday 18th July 2023, Please make sure that this PR is submitted before the deadline. We are currently in Week 7, and the Code Freeze will be in effect by Week 9. |
3b9fcbc
to
19f6d89
Compare
19f6d89
to
9259498
Compare
d97b074
to
f8cdfbc
Compare
|
/retest |
f8cdfbc
to
6c1ee9a
Compare
| // generateKey generates a random key using system randomness. | ||
| func generateKey(length int) (key []byte, err error) { | ||
| // GenerateKey generates a random key using system randomness. | ||
| func GenerateKey(length int) (key []byte, err error) { |
There was a problem hiding this comment.
It is almost certainly fine to panic if rand.Read returns an error. On systems with getrandom (linux after 2014), it won't error after system entropy is initialized:
Also maybe GetRandomBytes would be a better name.
There was a problem hiding this comment.
Can do in a follow up - internally this sets metrics right now so would need to split this up to have a GetRandomBytes func.
| // Because this mode requires a generated IV and IV reuse is a known weakness of AES-GCM, keys | ||
| // must be rotated before a birthday attack becomes feasible. NIST SP 800-38D | ||
| // (http://csrc.nist.gov/publications/nistpubs/800-38D/SP-800-38D.pdf) recommends using the same | ||
| // key with random 96-bit nonces (the default nonce length) no more than 2^32 times, and |
There was a problem hiding this comment.
Just for fun, here's the calc. Birthday bound approximation is:
N^2 (2^n)^2 2^(2n)
p(N;M) ~= ─── == ─────── == ───────
2*M 2*2^m 2^(m+1)
M is size of set, m=log2(M) (i.e. m is bit size of nonce)
N is number of items chosen uniform randomly from the set, n=log2(N)
p(N;M) is probability of a collision given N draws from set of size M
SP 800-38D requires that “the probability that the authenticated encryption function ever will be invoked with the same IV and the same key on two (or more) distinct sets of input data shall be no greater than 2-32”.
2^(2n) 1
─────── <= ────
2^(m+1) 2^32
2^(2n)*2^32 <= 2^(m+1)
2^(2n+32) <= 2^(m+1)
2n+32 <= m+1
m-31
n <= ────
2
Then for AES-GCM with nonce size of 96 bits
n <= (96-31)/2
n <= 32.5
Which is where 2^32 comes from. Also, limiting probability of collision 2^-32 is a very high standard.
| // In regard to KMSv2, organization standards or compliance policies around rotation may require | ||
| // that the seed be rotated at some interval. This can be implemented externally by rotating | ||
| // the key encryption key via a key ID change. | ||
| func NewKDFExtendedNonceGCMTransformerFromSeed(seed []byte) (value.Transformer, error) { |
There was a problem hiding this comment.
| func NewKDFExtendedNonceGCMTransformerFromSeed(seed []byte) (value.Transformer, error) { | |
| func NewHKDFExtendedNonceGCMTransformer(key []byte) (value.Transformer, error) { |
two nits:
- Add H to KDF so the scheme is called HKDFExtendendNonceGCM.
- The seed is the key for this scheme. While it happens to seed a KDF as part of the internal key schedule, that's an implemenation detail. It may be less confusing to just call it key (i.e. you better keep it secret!).
| info := make([]byte, infoSizeExtendedNonceGCM) | ||
| if err := randomNonce(info); err != nil { | ||
| return nil, fmt.Errorf("failed to generate info for KDF: %w", err) | ||
| } |
There was a problem hiding this comment.
not: just call GetKey(infoSizeExtendedNonceGCM) especially if you rename it to GetRandomBytes
staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/kmsv2/envelope.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/kmsv2/v2/api.proto
Outdated
Show resolved
Hide resolved
| // NewKDFExtendedNonceGCMTransformerFromSeed is the same as NewGCMTransformer but trades storage, | ||
| // memory and CPU to work around the limitations of AES-GCM's 12 byte nonce size. The input seed | ||
| // is assumed to be a cryptographically strong slice of MinSeedSizeExtendedNonceGCM+ random bytes. | ||
| // Unlike NewGCMTransformer, this function is immune to the birthday attack because a new key is generated |
There was a problem hiding this comment.
Just found some relevant info on this https://words.filippo.io/dispatches/xaes-256-gcm-11/ (From Fillipo published a couple days ago), and linked from that: https://soatok.blog/2022/12/21/extending-the-aes-gcm-nonce-without-nightmare-fuel/
staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/kmsv2/v2/api.proto
Outdated
Show resolved
Hide resolved
2b0ce20
to
256ef44
Compare
|
lgtm/approve for the proto API update (@mikedanese can weigh in on enum names, or we can tweak the text names in the future without changing the wire serialization) I didn't review the implementation, will leave that review to mike |
|
/retest |
| EncryptedDEKSourceType encryptedDEKSourceType = 5; | ||
| } | ||
|
|
||
| enum EncryptedDEKSourceType { |
There was a problem hiding this comment.
Minor: I might drop the "Encrypted" prefix. This describes a property of the unencyrpted DEKSource, it shows up in code around the unencrypted key, etc... encryptedDEKSourceType sounds fine as the field name, I'd just drop it from the enum type name.
| block, err := aes.NewCipher(key) | ||
| if err != nil { | ||
| return nil, err | ||
| func (t *envelopeTransformer) addTransformerForDecryption(cacheKey []byte, key []byte, useSeed bool) (value.Read, error) { |
There was a problem hiding this comment.
We should pass in the EncryptedDEKSourceType and switch vs useSeed. That would be the factoring e.g. once we support three types.
| transformer, newKey, err := aestransformer.NewGCMTransformerWithUniqueKeyUnsafe() | ||
| // GenerateTransformer generates a new transformer and encrypts the DEK/seed using the envelope service. | ||
| // It returns the transformer, the encrypted DEK/seed, cache key and error. | ||
| func GenerateTransformer(ctx context.Context, uid string, envelopeService kmsservice.Service, useSeed bool) (value.Transformer, *kmstypes.EncryptedObject, []byte, error) { |
|
/lgtm |
|
LGTM label has been added. Git tree hash: 76ef461ead1e01091ddece09448388dad9551bfe
|
Signed-off-by: Monis Khan <[email protected]>
256ef44
to
bf49c72
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enj, liggitt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@enj: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Changelog suggestion: Changed KMS API encryption can generate data encryption keys.
When you enable the `KMSv2KDF` feature gate (off by default), KMS v2 uses a key derivation function to generate single use data encryption keys from a secret seed combined with some random data. This eliminates the need for a counter based nonce while avoiding nonce collision concerns associated with AES-GCM's 12 byte nonce.BTW, is there a docs PR to cover this change? |
Fixes #118831
/kind feature
/kind api-change
/sig auth
/triage accepted
/milestone v1.28