Skip to content
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.

Sign up for GitHub

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

Jump to bottom

JetStream TPM driven encryption #5273

Merged
merged 6 commits into from
Apr 10, 2024
Merged

JetStream TPM driven encryption #5273

merged 6 commits into from
Apr 10, 2024
+744 −7

Conversation

ColinSullivan1
Copy link
Member

JetStream TPM based Encryption

This PR applies to Windows Platforms. uses the seal/unseal functionality of the TPM to encrypt and decrypt a JetStream encryption key, similar to how disk encryption works on windows platforms.

How it works:
Create an instance of the Storage Root Key (SRK) in the TPM.
If existing JS Encryption keys do not exist on disk:

  1. Create a JetStream encryption key (js key) and seal it to the SRK using a provided js encryption key password.
  2. Save the public and private blobs to a file on disk.
  3. Write the new js encryption keys to disk.

Otherwise (keys exist on disk)

  1. Read the keys blobs from disk
  2. Load them into the TPM
  3. Unseal the js key using the TPM and the provided js encryption keys password.

Configuration (full):

jetstream {
    tpm {
        keys_file: "jskeys.json"
        encryption_password: $JSEKTPM_PASSWORD
        srk_password: $TPM_SRK_PASSWORD
        pcr: 19
        cipher: AES
    }

Configuration (Usual/Minimal):

jetstream {
    tpm {
        keys_file: "jskeys.json"
        encryption_password: $JSEKTPM_PASSWORD
    }

The configured defaults should work on factory settings of windows TPMs.

  • pcr (Platform Configuration Register): 22
  • cipher (used to encrypt JS Blocks): CHACAPOLY
  • srk_password (: (empty)

The created JS Key is simply an NKEY, but can be anything truly random serialized to a string.

Applies to windows builds although could be extended to linux in the future.

Note: Before merging another maintainer should test this on a windows 10+ laptop with a TPM version 2.0.

Hat tip to @philpennock for assistance in reviewing the approach and initial test code.

Signed-off-by: Colin Sullivan colin@luxantsolutions.com

@ColinSullivan1
JetStream TPM driven encryption
55ff514 
Signed-off-by: Colin Sullivan <colin@luxantsolutions.com>
@ColinSullivan1 ColinSullivan1 added the feature New feature request label Apr 3, 2024
@ColinSullivan1 ColinSullivan1 requested a review from a team as a code owner April 3, 2024 19:53
@ColinSullivan1
gofmt
eeaf6c1 
Signed-off-by: Colin Sullivan <colin@luxantsolutions.com>
@ColinSullivan1 ColinSullivan1 changed the title JetStream TPM driven encryption JetStream TPM driven encryption - WIP Apr 3, 2024
@ColinSullivan1
add tpm option to reload
7a9191b 
Signed-off-by: Colin Sullivan <colin@luxantsolutions.com>
@ColinSullivan1
windows only
017634a 
Signed-off-by: Colin Sullivan <colin@luxantsolutions.com>
@ColinSullivan1 ColinSullivan1 changed the title JetStream TPM driven encryption - WIP JetStream TPM driven encryption Apr 3, 2024
neilalexander
neilalexander reviewed Apr 4, 2024
View reviewed changes
Copy link
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't ran any tests on a real machine yet but a few first-pass comments.

server/tpm/js_ek_tpm_windows.go Outdated
return fmt.Errorf("unable to marshal keys to JSON: %v", err)
}
// Write the JSON to a file
if err := os.WriteFile(filename, keysJSON, 0644); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another world-readable as above.

server/tpm/js_ek_tpm_windows.go Outdated
// Reads the private and public blobs from a single file. If the file does not exist,
// or the file cannot be read and the keys decoded, an error is returned.
func readTPMKeysFromFile(filename string) ([]byte, []byte, error) {
if _, err := os.Stat(filename); os.IsNotExist(err) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if this is necessary or whether os.ReadFile(...) would return an os.IsNotExist(err) too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that's what I started with and ended up here but will double check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be wrong of course 😄 Curious about what you find.

server/tpm/js_ek_tpm_windows.go Outdated
// used to decrypt the key in future sessions will be saved to disk in the file provided.
// The key will be unsealed and returned only with the correct password and PCR value.
func LoadJetStreamEncryptionKeyFromTPM(srkPassword, jsKeyFile, jsKeyPassword string, pcr int) (string, error) {
var err error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably unnecessary as it is overshadowed by the following line.

@ColinSullivan1
Copy link
Member Author

Thank you for the review @neilalexander, much appreciated! I aim to have these addressed by the end of the day tomorrow.

@ColinSullivan1 ColinSullivan1 self-assigned this Apr 8, 2024
@ColinSullivan1
updates based on comments
fcf46c9 
Signed-off-by: Colin Sullivan <colin@luxantsolutions.com>
@ColinSullivan1
Copy link
Member Author

I believe I've addressed the requested changes - thanks for the suggestions.

Certainly let me know if there are additional changes. It'd be great if someone could independently run the tests.

@ColinSullivan1
cleanup tests
b0fcb13 
Signed-off-by: Colin Sullivan <colin@luxantsolutions.com>
@derekcollison
Copy link
Member

Where are we on this in terms of getting it merged?

neilalexander
neilalexander approved these changes Apr 10, 2024
View reviewed changes
Copy link
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, although I don't have appropriate hardware to test it with.

mtmk
mtmk approved these changes Apr 10, 2024
View reviewed changes
Copy link
Contributor

@mtmk mtmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using hyper-v Windows 11 dev environment image:

  • Ran with above config successfully; data was encrypted, password and keys json worked as expected.
  • Also ran the TPM tests successfully.

LGTM

@derekcollison derekcollison merged commit 11613f5 into main Apr 10, 2024
4 checks passed
@derekcollison derekcollison deleted the tpm-js-encrypt branch April 10, 2024 16:00
@friedrichsenm
Copy link

Is there a way to disable the PCR part of the TPM policy with this feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neilalexander neilalexander neilalexander approved these changes

@mtmk mtmk mtmk approved these changes

@philpennock philpennock Awaiting requested review from philpennock

@escidmore escidmore Awaiting requested review from escidmore

Assignees

@ColinSullivan1 ColinSullivan1

Labels
feature New feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ColinSullivan1 @derekcollison @friedrichsenm @neilalexander @mtmk