-
Notifications
You must be signed in to change notification settings - Fork 27
Flag to save tee key to local file #286
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
base: main
Are you sure you want to change the base?
Conversation
let signer = generate_signer(); | ||
|
||
// Save key to file with 0600 permissions | ||
let key_hex = hex::encode(signer.secret.secret_bytes()); |
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.
I think there could be security implications
Is this secure way to generate key? Will the key be stored in RAM?
I see that this attack vector is unlikely, but if we could make it right easily - let's do this
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.
not sure what you mean? The image measurements will guarantee the file will be saved to persistent file storage on the TEE and not a malicious external storage source. When a new deploy is made a new VM is deployed which wipes the storage.
The security model assumes that SSH access is disabled so no one can access the machine and extract the key.
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.
Note the /run
directory is indeed in ram. Saving to disk is a bit tricky, since disks can always be tampered with (harder to do with memory).
// Try to load existing key | ||
if path.exists() { | ||
info!("Loading TEE key from {}", key_path); | ||
match fs::read_to_string(path) { |
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.
Can you use inspect_err
or unwrap_
functions to clean up the error handling?
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.
I don't want errors related to reading the key to fail the flashtestations bootstrap. I believe inspect_err
and unwrap_
will return the result
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.
It's difficult to follow the logic when you have nested statements. I attempted to extract this into a separate function like this
fn try_load_key_from_path(key_path: &str) -> Option<Signer> {
let path = Path::new(key_path);
if !path.exists() {
return None;
}
info!("Loading TEE key from {}", key_path);
let key_hex = fs::read_to_string(path)
.inspect_err(|e| warn!("failed to read key file: {:?}", e))
.ok()?
.trim();
let secret_bytes = B256::try_from(
hex::decode(key_hex)
.wrap_err("failed to decode tee key")?
.as_slice(),
)
.inspect_err(|e| warn!("failed to parse stored key: {:?}", e))
.ok()?;
Signer::try_from_secret(secret_bytes)
.inspect_err(|e| warn!("failed to create signer from stored key: {:?}", e))
.ok()
}
Now it's quicker to connect each error message with the corresponding function.
Why do you return an error when you fail to decode the tee key but create a new one if you fail to parse it? I feel like you should have the same behavior in these scenarios
|
||
/// Path to save ephemeral TEE key between restarts | ||
#[arg( | ||
long = "flashtestations.key-path", |
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.
I key-file
would be a better name for this, maybe even tee-key-file
📝 Summary
Resolves #284
💡 Motivation and Context
Adding arg to save the key in local fs between restarts. File is guaranteed to never leave the TEE by the image measurements that includes all the config of the builder
✅ I have completed the following steps:
make lint
make test