-
Notifications
You must be signed in to change notification settings - Fork 17
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
Encrypt pod logs in bootstrap runner and decrypt in Tentacle #1047
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to Kubernetes/Crypto and updated with new interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now have 2 places where we need to retrieve the machine encryption key from the secret, so splitting this code into an interface
//we use the machine encryption key as the password and the script ticket is the salt | ||
pdb.Init(PbeParametersGenerator.Pkcs5PasswordToBytes(Encoding.ASCII.GetChars(machineEncryptionKey)), Encoding.UTF8.GetBytes(scriptTicket.TaskId), 1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where we use the machine encryption key as the "password" and the script ticket as the salt to generate a known 32 byte key
//If we can't load the encryption key from the filesystem | ||
if (fileContents == null) | ||
{ | ||
//regenerate the encryption key, write to the filesystem and return the key | ||
return await GenerateAndWriteEncryptionKeyfileToWorkspace(scriptTicket, workspace, cancellationToken); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When tentacle gets the encryption key, if it can't load it from memory or the script workspace, we regenerate the same key and store it in memory and the disk. This should only happen after a tentacle & NFS pod restart.
//if we can't read the pod log encryption key for a while | ||
var message = $"Failed to read pod log encryption key. No new pod logs will be read."; | ||
tentacleScriptLog.Verbose(message); | ||
Log.Warn(ex, message); | ||
|
||
//If we somehow come across weird/missing line numbers, try load the whole Pod logs to see if that helps | ||
return await GetPodLogsWithSinceTime(null); | ||
return (new List<ProcessOutput>(), lastLogSequence, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we can't read the encryption key, don't read the pod logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nits, but not enough to block. Thanks for the hard work!
this.log = log; | ||
} | ||
|
||
public async Task WriteEncryptionKeyfileToWorkspace(ScriptTicket scriptTicket, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could do with a rename (follow up PR?)
var pdb = new Pkcs5S2ParametersGenerator(new Sha256Digest()); | ||
|
||
//we use the machine encryption key as the password and the script ticket is the salt | ||
pdb.Init(PbeParametersGenerator.Pkcs5PasswordToBytes(Encoding.ASCII.GetChars(machineEncryptionKey)), Encoding.UTF8.GetBytes(scriptTicket.TaskId), 1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Consider fewer conversion passes and simply pass through the machineEncryptionKey bytearray
{ | ||
var message = $"Unexpected Pod log line numbers found with sinceTime='{sinceTime}', loading all logs"; | ||
//if we can't read the pod log encryption key for a while | ||
var message = $"Failed to read pod log encryption key. No new pod logs will be read."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a minimum, we still get whether the script exited successfully from K8s, so this is a reasonable trade-off to protect against transient decryption problems.
{ | ||
var message = $"Unexpected Pod log line numbers found with sinceTime='{sinceTime}', loading all logs"; | ||
//if we can't read the pod log encryption key for a while | ||
var message = $"Failed to read pod log encryption key. No new pod logs will be read."; | ||
tentacleScriptLog.Verbose(message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: change this to a warning
if (messagePart.StartsWith(EndOfStreamMarkerPrefix)) | ||
|
||
//the log messages are being returned from the pods encrypted, decrypt them here | ||
var decryptedMessagePath = encryptionProvider.Decrypt(encryptedMessagePart); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this might be better named decryptedMessagePart (or decryptedMessage)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For safety we should check this version of tentacle works with a full build of octopus server.
@LukeButters Yep, this was done in https://github.com/OctopusDeploy/OctopusDeploy/pull/29373 and was fully green! |
ADR for change here: https://github.com/OctopusDeploy/adr/pull/68 |
Shortcut story: [sc-98480] |
Background
To avoid leaking sensitive values in pod logs, we are now encrypting the logs in the script pods and decrypting them in Tentacle
Results
The process is now:
keyfile
in the script workspace. This key is a PBKDF2-derived hash of the tentacle machine encryption key salted with the script ticket id. This is done so that if the NFS is lost and tentacle restarted by the watchdog (ala automatic upgrade), that the key can be regenerated again so running pod logs can be decrypted.The nonce, generated on a per-log message bases in the bootstrap runner, is prepended to the encrypted message and split in the tentacle decryption.
I had to do a bit of refactoring to the KubernetesMachineKeyEncryptor so that we could now retrieve the machine key in multiple places.
After
Example pod log output
The output in Server
How to review this PR
Quality ✔️
Pre-requisites