Add a class for handling secrets encoding/decoding.
Review Request #15182 — Created July 21, 2026 and updated
When managing stored secrets (such as auth-related state or other
sensitive data), the current best practice is to:
- Generate a one-time-use key (a DEK, or Data Encryption Key)
- Encrypt the plaintext
- Key-wrap (effectively encrypt) the DEK using a parent KEK (Key
Encryption Key), identified by a KID (key ID)- Store the resulting encrypted DEK, the KEK's KID, the encryption and
keywrap algorithms, and a version as an envelope header, with the
ciphertext appended.This change implements this through a
Secretclass, which manages a
plaintext and allows for encoding and decoding. It will generate a DEK
(unless one is available -- provided when decoding or for unit tests).Decoding takes a KEK resolver that returns the key matching the ID.
This keeps it separate from any key management mechanism (though once
that's built, this will cleanly integrate).
Secretis invalidatable, which means that it will automatically clear
its internal state when it falls out of scope or when used as a context
manager and the context closes. This helps avoid situations where the
resulting plaintext or DEK may be lingering somewhere accidentally.
Callers should decode aSecretusingwith Secret.from_encoded()
and run all operations that need access to the plaintext within that
context.
Unit tests pass.
| Summary | ID |
|---|---|
| 0a4fe505d9b1aeffc4a1b9a8ec6905299f33136c |
| Description | From | Last Updated |
|---|---|---|
|
This should probably be called _KEY_CLASSES. We should also type as Final[...] and set it to a tuple instead of … |
|
|
|
Even though this is typed as Final, it's assigned as a mutable set. Can we use frozenset here? |
|
|
|
If the secret has been invalidated, we'll silently encrypt nothing. Can we start this method with: if not self.is_valid(): raise … |
|
|
|
This will give a misleading error in the case that the version is present but malformed. Can we break this … |
|
|
|
This should probably raise SecretDecodeError(...) from None so that Python doesn't chain underlying library detail in the traceback/logs. Same below … |
|
|
|
This is read from the envelope and used verbatim to decrypt without doing any kind of authentication. This means if … |
|
|
|
This validates against every class, but the actual unwrap goes through kek.unwrap_key(), so it might pass validation here even if … |
|
|
|
Using this as a context manager clears its state on exit. This means that calling this method will destroy the … |
|
|
|
I think we should be a little more limited in what we catch so it's just what can come from … |
|
|
|
This should probably be formatting in message instead of e (like you do above in the handler after unwrap_key()) |
|
|
|
In from_encoded, you add in the version field (_RESERVED_FIELDS | {version_field}). We should do the same here so that extra_fields={'encv': … |
|
|
|
The value is quoted, but the key is not (and not validated). A ; character in a key could cause … |
|
|
|
This doesn't have "Version Added" |
|
|
|
We should probably add and self.dek.is_valid() so an invalid DEK doesn't cause a confusing failure later. |
|
|
|
This doesn't have "Version Added" |
|
|
|
This is repeated verbatim 12 times. Can we create a helper to make the resolver? |
|
|
|
This text has changed across python versions. Can we just assert on message.startswith('Corrupted ciphertext:')? |
|
-
-
This should probably be called
_KEY_CLASSES.We should also type as
Final[...]and set it to a tuple instead of a mutable list. -
-
If the secret has been invalidated, we'll silently encrypt nothing. Can we start this method with:
if not self.is_valid(): raise ValueError( 'This secret has been invalidated and cannot be encoded.')Would be good to have a test for calling
encodeafter invalidation. -
This will give a misleading error in the case that the version is present but malformed. Can we break this apart?
try: version_str = fields[version_field] except KeyError: raise SecretDecodeError('Envelope version field is missing.') from None` try: version = int(version_str) except ValueError: raise SecretDecodeError( f'Invalid envelope version {version_str!r}.' ) from None -
This should probably
raise SecretDecodeError(...) from Noneso that Python doesn't chain underlying library detail in the traceback/logs. Same below wherever we catch one exception and raise another. -
This is read from the envelope and used verbatim to decrypt without doing any kind of authentication. This means if it was rewritten from alg=AES-256-GCM to AES-256-CFB8, it would disable authentication on the decryption.
I think there are two things we should do to mitigate this:
- Bind the header as AAD (pass the envelope prefix into
encrypt/decryptas associated data). This would also make it soextra_fieldswould be authenticated (instead of freely rewritable). - Restrict
from_encodedto AEAD algorithms (perhaps allowing an escape hatch to CFB8 if we need it).
- Bind the header as AAD (pass the envelope prefix into
-
This validates against every class, but the actual unwrap goes through
kek.unwrap_key(), so it might pass validation here even if the underlying KEK can't do it. How about we just validate againstkekdirectly?kek = kek_resolver(kid) if not kek.supports_keywrap_alg(wrap_alg): raise SecretDecodeError(...) -
Using this as a context manager clears its state on exit. This means that calling this method will destroy the kek that gets passed in. For some cases this may not matter, but a KEK seems like it could be a long-lived object that a caller would expect to be able to hand out repeatedly:
kek = load_kek() with Secret.from_encoded(enc1, kek_resolver=lambda kid: kek) as s1: ... with Secret.from_encoded(enc2, kek_resolver=lambda kid: kek) as s2: # this failsTaking ownership of the passed-in KEK seems highly fragile, so I think we'd do better to just have the try/catch without the context manager use here.
Would be good to have a test for this situation too.
-
I think we should be a little more limited in what we catch so it's just what can come from the crypto later:
KeyUnwrapError,UnsupportedAlgorithmLayer,CryptozoologyError, andValueError. As-is this can swallowTypeErrororAttributeErrors that come from genuine bugs.Same with the
exceptfordek.decrypt()just below. -
This should probably be formatting in
messageinstead ofe(like you do above in the handler afterunwrap_key()) -
In
from_encoded, you add in the version field (_RESERVED_FIELDS | {version_field}). We should do the same here so thatextra_fields={'encv': '...'}doesn't silently create an unreadable secret.Might be worth a test that passes
version_fieldin the extra fields to verify. -
The value is quoted, but the key is not (and not validated). A
;character in a key could cause envelope field injection. Given thatextra_fieldsis meant to be a general-purpose mapping, we should make sure it's safe by default.Would need a corresponding change in
decodeWe might also want to call
quote()withsafe=''. Right now/characters aren't escaped, which shouldn't be a problem currently, but I'd prefer to not have that assumption baked in. -
-
We should probably add
and self.dek.is_valid()so an invalid DEK doesn't cause a confusing failure later. -
-
-
This text has changed across python versions. Can we just assert on
message.startswith('Corrupted ciphertext:')?