-
Notifications
You must be signed in to change notification settings - Fork 148
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
Agent configuration encryption #398
Conversation
This pull request does not have a backport label. Could you fix it @aleksmaus? 🙏
NOTE: |
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.
nice
One request, could we add meta data to the keys? Specifically date created would be really nice if we have to do any kind of key rotation.
// Wrap into crypto writer, reusing already existing crypto writer, open to other suggestions | ||
w, err := crypto.NewWriterWithDefaults(fd, d.key) | ||
if err != nil { | ||
fd.Close() |
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.
tmp file not removed in this case, is that expected?
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.
fixed
if _, err = rand.Read(nonce); err != nil { | ||
return nil, err | ||
} | ||
ciphertext := aesGCM.Seal(nonce, nonce, data, nil) |
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.
is the first nonce
really supposed to be the dst?
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.
yes, this way the ciphertext is the concatenation of nonce + encrypted data, thats what is intent here
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.
makes sense. might be worth a comment, on first read it looks a little odd.
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.
commented
internal/pkg/agent/vault/seed.go
Outdated
|
||
func getSeed(path string) ([]byte, error) { | ||
fp := filepath.Join(path, seedFile) | ||
b, err := ioutil.ReadFile(fp) |
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.
might be a good idea to check permissions of seed file and complain if they are too permissive.
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.
added the root owner check. the agent will fail to run in .seed file is not root/root owned
$ sudo ./elastic-agent -e
Error: could not read overwrites: could not initialize config store: non-root file owner
For help, please see our troubleshooting guide at https://www.elastic.co/guide/en/fleet/8.3/fleet-troubleshooting.html
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.
@nimarezainia, do we have plans to allow users to run the agent as non-root?
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.
@nimarezainia, do we have plans to allow users to run the agent as non-root?
Not at the moment.
sure, can encode that into stored data. |
done now, the value is encoded as JSON
|
…tate.yml into fleet.enc and state.enc after the encryption
… sudo, thus don't have access to the system keychain
unnecessary conversion (unconvert)
…encrypted storage on any of the platforms
This reverts commit c3d3c4c.
Hi @jlind23
Further we have observed the files like:
We haven't observed Build details: NOTE: Please let us know if we are missing anything here. |
For Mac OS we use keychain, no vault folder is needed. |
Hi @aleksmaus Please let us know if we are missing anything here. |
Full regression testing of Elastic Agent would be necessary to make sure we didn't miss any code paths when transitioned to the encrypted storage. |
Hi @aleksmaus Thanks |
@amolnater-qasource have you tried standalone mode with this PR? |
Hi @michalpristas However we had revalidated this issue on latest 8.3 BC2 and found this fixed. Thanks |
Based on elastic/elastic-agent#398 and a discussion with @cmacknz . It's a somewhat common ask from synthetics users.
* [agent] Add documentation for Agent encryption at rest Based on elastic/elastic-agent#398 and a discussion with @cmacknz . It's a somewhat common ask from synthetics users. * Apply suggestions from code review Thanks for the copy edits Dede! Co-authored-by: DeDe Morton <[email protected]> Co-authored-by: DeDe Morton <[email protected]>
* [agent] Add documentation for Agent encryption at rest Based on elastic/elastic-agent#398 and a discussion with @cmacknz . It's a somewhat common ask from synthetics users. * Apply suggestions from code review Thanks for the copy edits Dede! Co-authored-by: DeDe Morton <[email protected]> Co-authored-by: DeDe Morton <[email protected]> (cherry picked from commit 7996f31)
* [agent] Add documentation for Agent encryption at rest Based on elastic/elastic-agent#398 and a discussion with @cmacknz . It's a somewhat common ask from synthetics users. * Apply suggestions from code review Thanks for the copy edits Dede! Co-authored-by: DeDe Morton <[email protected]> Co-authored-by: DeDe Morton <[email protected]> (cherry picked from commit 7996f31) Co-authored-by: Andrew Cholakian <[email protected]>
* [agent] Add documentation for Agent encryption at rest Based on elastic/elastic-agent#398 and a discussion with @cmacknz . It's a somewhat common ask from synthetics users. * Apply suggestions from code review Thanks for the copy edits Dede! Co-authored-by: DeDe Morton <[email protected]> Co-authored-by: DeDe Morton <[email protected]> (cherry picked from commit 7996f31)
* [agent] Add documentation for Agent encryption at rest Based on elastic/elastic-agent#398 and a discussion with @cmacknz . It's a somewhat common ask from synthetics users. * Apply suggestions from code review Thanks for the copy edits Dede! Co-authored-by: DeDe Morton <[email protected]> Co-authored-by: DeDe Morton <[email protected]> (cherry picked from commit 7996f31) Co-authored-by: Andrew Cholakian <[email protected]>
* [agent] Add documentation for Agent encryption at rest Based on elastic/elastic-agent#398 and a discussion with @cmacknz . It's a somewhat common ask from synthetics users. * Apply suggestions from code review Thanks for the copy edits Dede! Co-authored-by: DeDe Morton <[email protected]> Co-authored-by: DeDe Morton <[email protected]>
* [agent] Add documentation for Agent encryption at rest Based on elastic/elastic-agent#398 and a discussion with @cmacknz . It's a somewhat common ask from synthetics users. * Apply suggestions from code review Thanks for the copy edits Dede! Co-authored-by: DeDe Morton <[email protected]> Co-authored-by: DeDe Morton <[email protected]>
What does this PR do?
Implements the key material storage and the agent configuration and state encryption.
Providing the bulletproof key material storage across all the platforms is fairly difficult.
The open source nature of the project is making it even more easier to figure any obfuscation techniques that we could have used to make it difficult to reverse engineer the key protection algorithm.
Opening this PR draft to gather feedback. If we agree on the general approach there are still some TODO items to finalize this change
TODO
Implementation details
Here is the approach that was taken on the different platforms at the moment:
Darwin (Mac OS)
The key material is stored in the System keychain. The value is stored as is without any additional transformations.
Windows
The data is encrypted with DPAPI CryptProtectData with CRYPTPROTECT_LOCAL_MACHINE.
The additional entropy is derived from crypto/rand bytes stored in .seed file.
The data is stored as separate files where the name of the file is SHA256 hash of the key and the content of the file is encrypted with DPAPI data.
The security of entropy data relies on file system permissions. Only the Administrator should be able to access the file.
Linux
The encryption key is derived from crypto/rand bytes that stored in the .seed file after PBKDF2 transformation.
The data is stored as separate files where the name of the file is SHA256 hash of the key and the content of the file is AES256-GSM encrypted.
The security if the key material largely relies on the files system permissions.
The Agent modifications
Why is it important?
Address the configuration encryption at rest requirement.
https://github.com/elastic/obs-dc-team/issues/535
Checklist
How to test this PR locally
Full regression testing of the agent, including enrollment, upgrade etc.
Related issues