Adds recording encryption manager #55078
Conversation
eriktate
force-pushed
the
eriktate/encrypted-recording-manager
branch
from
7c97b4f to
1a198d9
Compare
eriktate
force-pushed
the
eriktate/encrypted-recording-resources
branch
from
c900a70 to
cee99e5
Compare
eriktate
force-pushed
the
eriktate/encrypted-recording-manager
branch
from
1a198d9 to
7713063
Compare
eriktate
force-pushed
the
eriktate/encrypted-recording-resources
branch
from
cee99e5 to
7afa9b6
Compare
eriktate
force-pushed
the
eriktate/encrypted-recording-manager
branch
3 times, most recently
from
e944d7d to
848b135
Compare
eriktate
force-pushed
the
eriktate/encrypted-recording-resources
branch
from
7afa9b6 to
6fa728b
Compare
eriktate
force-pushed
the
eriktate/encrypted-recording-manager
branch
3 times, most recently
from
37bcbfe to
06206eb
Compare
eriktate
force-pushed
the
eriktate/encrypted-recording-resources
branch
2 times, most recently
from
653f866 to
23d65c6
Compare
eriktate
force-pushed
the
eriktate/encrypted-recording-manager
branch
from
4950191 to
54c711f
Compare
eriktate
added
the
no-changelog
eriktate
force-pushed
the
eriktate/encrypted-recording-manager
branch
from
54c711f to
df2ca7c
Compare
eriktate
force-pushed
the
eriktate/encrypted-recording-manager
branch
4 times, most recently
from
7f1c265 to
5f9781b
Compare
eriktate
force-pushed
the
eriktate/encrypted-recording-manager
branch
3 times, most recently
from
2aff1c8 to
cff5c92
Compare
eriktate
force-pushed
the
eriktate/encrypted-recording-manager
branch
3 times, most recently
from
bc89945 to
3860f34
Compare
eriktate
force-pushed
the
eriktate/encrypted-recording-manager
branch
2 times, most recently
from
d3760ee to
78d0a9c
Compare
| // recording encryption key pairs) will be fulfilled using their public key encryption keys. | ||
| // | ||
| // If there are no unfulfilled keys present, then nothing should be done. | ||
| func (m *Manager) ResolveRecordingEncryption(ctx context.Context, postProcessFn func(context.Context, *recordingencryptionv1.RecordingEncryption) error) (encryption *recordingencryptionv1.RecordingEncryption, err error) { |
There was a problem hiding this comment.
Is the postProcessFn necessary? If this function returns the resolved recordingencryptionv1.RecordingEncryption can't callers mutate the returned encryption directly instead of via the callback?
There was a problem hiding this comment.
The postProcessFn really only exists to update the public encryption keys in the session_recording_config in the same lock as resolving the recording_encryption resource. I had originally avoided acquiring the lock within ResolveRecordingEncryption so it wasn't necessary, but I think it makes more sense to make sure it's impossible to call concurrently.
Maybe postProcessFn is a bit of a misnomer and it should be something like onResolvedFn instead?
There was a problem hiding this comment.
I'm wondering if there is any benefit to separating the Manager and the ClusterConfigService. The decoupling is what led to this callback in the first place, but I'm not sure it adds much given how simple the ClusterConfigService wrapper is. What do you think?
eriktate
force-pushed
the
eriktate/encrypted-recording-manager
branch
from
6f616df to
c9cb026
Compare
…ops more complex than CRUD
eriktate
force-pushed
the
eriktate/encrypted-recording-manager
branch
4 times, most recently
from
7126b84 to
5feae17
Compare
…gurationInternal a interface for interacting with the session_recording_config within the same lock that recording encryption keys are resolved
eriktate
force-pushed
the
eriktate/encrypted-recording-manager
branch
from
5feae17 to
f956674
Compare
| // If there are no unfulfilled keys present, then nothing should be done. | ||
| func (m *Manager) ResolveRecordingEncryption(ctx context.Context) (encryption *recordingencryptionv1.RecordingEncryption, err error) { | ||
| err = backend.RunWhileLocked(ctx, m.lockConfig, func(ctx context.Context) error { | ||
| if m.sessionRecordingConfig == nil { |
There was a problem hiding this comment.
It feels slightly wrong that we seem to be relying on the backend lock to perform as a synchronization mechanism for sessionRecordingConfig. Who now has the responsibility of calling ResolveRecordingEncryption? Is it still the the ClusterConfigService implemented in https://github.com/gravitational/teleport/pull/54901/files?
There was a problem hiding this comment.
It was the ClusterConfigService and the RecordingEncryption watcher, but I'm increasingly having to jump through hoops in order to make that implementation work. At this point I think having a second watcher on the SessionRecordingConfig is simpler and probably better performing since the Manager can just cache it 99% of the time. The latest commit I pushed implements that and #54901 should no longer have ClusterConfigService, which means the only caller of ResolveRecordingEncryption is the watcher.
As far as synchronization goes, I'm not sure of another way to approach it 🤔 There is the possibility of a data race right now because manual edits to the session_recording_config don't happen within a lock. That could be fixed by using the same lock in the ClusterConfigurationService, but that's just using more of the same sync mechanism. Maybe there's something I could do with an AtomicWrite instead?
eriktate
force-pushed
the
eriktate/encrypted-recording-manager
branch
from
d02c350 to
2ba2ca1
Compare
This PR adds a
Managerstruct responsible for higher levelRecordingEncryptionoperations such as resolving state in order to facilitate cooperation between auth servers. It also includes the implementation for a watcher that will automatically attempt to resolveRecordingEncryptionstate whenever the resource is modified.