Adding manual encryption key management for encrypted recordings#56920
Merged
Adding manual encryption key management for encrypted recordings#56920
Conversation
eriktate
commented
Jul 18, 2025
Contributor
Author
There was a problem hiding this comment.
I added these helpers because I noticed that most of the enum types with custom unmarshalers were doing essentially the same thing. For now these are only used by PrivateKeyType since that's the only one I'm modifying for this PR, but I might do a separate PR that updates the other enum types to use this unless I get feedback against implementing this generically.
eriktate
force-pushed
the
eriktate/rsa-recording-encryption
branch
from
65d37b1 to
4c6f773
Compare
eriktate
force-pushed
the
eriktate/manual-encryption-keys
branch
2 times, most recently
from
72bfb2a to
e6cde62
Compare
eriktate
commented
Jul 21, 2025
Comment on lines
+229
to
+232
| shouldUpdate := len(addedKeys) > 0 && (keysChanged || len(existingKeys) != len(addedKeys)) | ||
| if !shouldUpdate { |
Contributor
Author
There was a problem hiding this comment.
This was previously failing to remove keys after rotation
eriktate
force-pushed
the
eriktate/manual-encryption-keys
branch
from
e6cde62 to
03952e5
Compare
eriktate
force-pushed
the
eriktate/rsa-recording-encryption
branch
from
4c6f773 to
cd87b3e
Compare
eriktate
force-pushed
the
eriktate/manual-encryption-keys
branch
from
03952e5 to
11aec2a
Compare
eriktate
force-pushed
the
eriktate/rsa-recording-encryption
branch
from
cd87b3e to
6044e3f
Compare
eriktate
force-pushed
the
eriktate/manual-encryption-keys
branch
from
24ea8a4 to
cd09a43
Compare
Contributor
|
Amplify deployment status
|
eriktate
force-pushed
the
eriktate/manual-encryption-keys
branch
from
29da268 to
3f8ea79
Compare
eriktate
force-pushed
the
eriktate/rsa-recording-encryption
branch
from
17e0aab to
ced2f23
Compare
eriktate
force-pushed
the
eriktate/manual-encryption-keys
branch
from
3f8ea79 to
6c7edeb
Compare
eriktate
force-pushed
the
eriktate/manual-encryption-keys
branch
from
6c7edeb to
174da72
Compare
eriktate
added
the
no-changelog
eriktate
force-pushed
the
eriktate/manual-encryption-keys
branch
from
174da72 to
836f806
Compare
rosstimothy
reviewed
Jul 28, 2025
|
|
||
| func (s *kmsKey) Decrypt(rand io.Reader, ciphertext []byte, opts crypto.DecrypterOpts) (plaintext []byte, err error) { | ||
| resp, err := doGCPRequest(s.ctx, s.g, s.g.kmsClient.AsymmetricDecrypt, &kmspb.AsymmetricDecryptRequest{ | ||
| resp, err := doGCPRequest(context.Background(), s.g, s.g.kmsClient.AsymmetricDecrypt, &kmspb.AsymmetricDecryptRequest{ |
Contributor
There was a problem hiding this comment.
How come this was changed to a background context?
Contributor
Author
There was a problem hiding this comment.
I meant to come back around and fix this. The embedded context in kmsKey was causing cache issues because it only had a request lifetime when the keys were fetched during asession_recording_config update. I added a context to the Manager struct to be used for this so any cached key context has at least the same lifetime as the manager itself.
eriktate
force-pushed
the
eriktate/manual-encryption-keys
branch
from
72c8635 to
72afacd
Compare
eriktate
force-pushed
the
eriktate/manual-encryption-keys
branch
from
7739cfa to
6f668f3
Compare
Contributor
Author
I do and you can find it here.
Most definitely, should be in the latest commit 👍 |
eriktate
force-pushed
the
eriktate/manual-encryption-keys
branch
from
6f668f3 to
27e5b9c
Compare
rosstimothy
reviewed
Jul 30, 2025
| return nil, trace.AccessDenied("this request can be only executed by an auth server") | ||
| } | ||
|
|
||
| if err := cfg.CheckAndSetDefaults(); err != nil { |
Contributor
There was a problem hiding this comment.
What do you think about adding a services.ValidateSessionRecordingConfig function similar to services.ValidateAuthPreference which can also be home to the cloud and fips checks from your future PR instead of performing a CheckAndSetDefaults in the gRPC layer?
Contributor
Author
There was a problem hiding this comment.
Yeah I wasn't totally sure about using CheckAndSetDefaults for this. Adding a new validation func to services makes sense to me 👍
eriktate
force-pushed
the
eriktate/manual-encryption-keys
branch
from
d114b67 to
3d01dd2
Compare
Contributor
Author
|
@nklaassen @rudream friendly ping! |
nklaassen
approved these changes
Jul 30, 2025
rosstimothy
approved these changes
Jul 31, 2025
| ) | ||
|
|
||
| // ValidateSessionRecordingConfig checks that the state of a [SessionRecordingConfig] meets constraints. | ||
| func ValidateSessionRecordingConfig(cfg types.SessionRecordingConfig) error { |
Contributor
There was a problem hiding this comment.
Suggestion: add some test coverage for this
public-teleport-github-review-bot
Bot
removed the request for review
from rudream
eriktate
force-pushed
the
eriktate/manual-encryption-keys
branch
from
3d01dd2 to
0372b0a
Compare
eriktate
force-pushed
the
eriktate/manual-encryption-keys
branch
from
0372b0a to
2c98b5d
Compare
eriktate
added a commit
that referenced
this pull request
Aug 15, 2025
eriktate
added a commit
that referenced
this pull request
Aug 27, 2025
eriktate
added a commit
that referenced
this pull request
Sep 2, 2025
eriktate
added a commit
that referenced
this pull request
Sep 2, 2025
eriktate
added a commit
that referenced
this pull request
Sep 2, 2025
eriktate
added a commit
that referenced
this pull request
Sep 3, 2025
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Sep 3, 2025
* adding support for encryption/decryption keys to keystore manager (#54428, #55652) * adds new protos for resources related to encrypted session recordings and updates the existing SessionRecordingConfig protos to include a Status (#54780) * adding local service implementation for recording encryption resources (#54816) * adding Manager for RecordingEncryption resources that handles shared ops more complex than CRUD (#55078) * Adding session recording plugin for `age` (#55120) * adding Manager for RecordingEncryption resources that handles shared ops more complex than CRUD * adding age plugin wrapping default X25519 Identity/Recipient implementation with hooks to more efficiently lookup private keys given their respective public key * Adding recording encryption and playback for `sync` modes (#54901) * adding cache for RecordingEncryption (#55857) * adding recording_encryption service protos (#55121) * adding async recording encryption with gRPC multipart uploader (#55859) * adding file configuration for encrypted session recording (#56200) * Switching recording encryption to unwrap keys using direct keystore RSA decryption (#56776) * adding manual key management config (#56920) * updating protos for recording encryption (#57055) * Add missing handling for recording encryption configs and keys (#57279) * updating protos for recording encryption * changing labels for encryption keys to prevent automatic cleanup, adjusting pkcs11 host UUID check to allow for key sharing of encryption keys, preventing cloud tenants from enabling manual key management, preventing use of recording encryption in FIPS mode * adding new protos for rotated keys and the local service for interacting (#57576) with them * Switching encryption keys from PEM to ASN.1 DER encoding (#58137) * using pregenerated RSA4096 key for keystore tests because generation is too slow (#58138) * extending precomputed RSA keys to support 4096-bit keys (#58251) * adding rotation process to Manager and exposing with new RPCs and (#57577) * adding rotation sub commands for recording encryption keys and fixing (#57780) broken session_recording_config when using fileconf * using more reliable method of validating key bit length
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds configurations for manual management of session recording encryption keys. This works by accepting an active and rotated list of
KeyLabels which designate which keystore a key can be found in and an identifying label. For HSM systems this is a label that that might identify multiple keys, but for KMS systems this will be an ID, ARN, or fully qualified version name of a specific key.Once an auth server knows how to find the keys, it will search for them in its configured keystore (configurable with
ca_key_params), save the public keys to thesession_recording_configresource to enable encryption, and cache the associatedcrypto.Decrypterfor future replay. When in manual management mode, Teleport will make no attempt at provisioning or rotating recording encryption keys.