-
Notifications
You must be signed in to change notification settings - Fork 143
Add signature support to sealed secrets #1272
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
108bc7a to
7b2a4ee
Compare
| } | ||
|
|
||
| if self.skip_sealed_secret_verification { | ||
| env::set_var("SKIP_SEALED_SECRET_VERIFICATION", "true"); |
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.
set_var is a bit iffy on posix systems, do we need to set a global variable like this? I understand we did that elsewhere because some value had to propagated to child processes that we didn't want to modify, but if it's merely in other crates and we had to adjust them to respect that env, could this just be explicitly handled application state in Rust code? (idk "config::sealed_secret_verification()" or something)
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 could extend this part to have another parameter
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've never been a big fan of the env var approach, but this is sort of orthogonal to this PR. Maybe we can do a follow-up were we rework the configuration logic.
It is easy to extend the parameter one level up the call tree, but that's where things start to get a bit complicated. secret::unseal_secret is called in a few far-flung places. Instead, I kept the flag at a lower level (so we can use if for testing and the CLI), but had the unseal_secret method implement the config logic so the caller doesn't need to know about the configuration or change at all.
Xynnn007
left a comment
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.
Overall lgtm except for the skip option. I leave some nits. Thanks!
| credentials: Vec::new(), | ||
| socket: DEFAULT_CDH_SOCKET_ADDR.into(), | ||
| image: ImageConfig::from_kernel_cmdline(), | ||
| skip_sealed_secret_verification: false, |
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.
If we by default set this to false, there would be some breaks for current existing sealed secrets. We should either claim this severe change, or set this to true.
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.
Let's ask at the meeting on thursday. I would prefer to have a more secure default, but obviously it is a breaking change.
| } | ||
|
|
||
| if self.skip_sealed_secret_verification { | ||
| env::set_var("SKIP_SEALED_SECRET_VERIFICATION", "true"); |
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 could extend this part to have another parameter
| return Ok(verification_key); | ||
| } | ||
|
|
||
| // Get key as local credential. |
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.
btw this behavior is somehow not that safe. It can allow a malicious host to do read operation upon any place in the guest. Just points out this as I did not think of an successful attaking routine.
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.
Yeah this is a very interesting point. I was thinking through this a bit. There are a few potential issues.
First, is there any way for an attacker to craft a signing key and payload that would leak information about the private key? Typically the public key and the payload are both public anyway, so I don't think this is a problem.
Second, can an attacker leak information about some other guest data by setting the kid to a different path. The code does block directory traversal, but you could still snoop on another credential. Maybe if you craft a certain payload you can leak the secret. In practice I think this would be very hard, but it's worth thinking about.
Probably we have a very similar concern about other code that uses credentials. Personally, I have never liked storing the credentials in the filesystem. I think we might consider re-working that as a follow-up.
| println!("{}", kid_cred_path); | ||
|
|
||
| std::fs::create_dir_all(SIGNING_CREDENTIALS_PATH).unwrap(); | ||
| std::fs::write(kid_cred_path, serde_json::to_string(&jwk).unwrap()).unwrap(); |
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.
here is a writing operation. is this 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.
Yes, we have to provide the signing key for the test to work. This is not ideal, but I couldn't think of another way to do it without a lot of mock code.
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.
ok.
The files in the global path /run have been modified. Is it possible to implement this functionality using the tempfile crate, while minimizing the impact of testing on the environment?
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.
The only issue there is that the filename must match what is in the secret header. There are some possible workarounds, but not sure if it is worth the complexity.
11707f1 to
18b03d7
Compare
Xynnn007
left a comment
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 these are the last comments I would have. Nice work
| /// The kid of the signing key. This will be used when unsealing | ||
| /// to find the key. | ||
| #[arg(short, long)] | ||
| signing_kid: String, |
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 this required? I mean, if the sealed secret is not to be signed, could we still generate the old life-style sealed secret?
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.
In this PR we only support unsigned secrets in unsealing. If you are sealing a secret, you must provide the signing info. We could relax this, but I think it's probably good to force people to use signatures when making any new secrets.
| /// A path to a file containing a JWK (with a private component) | ||
| /// for signing the sealed secret. | ||
| #[arg(short, long)] | ||
| signing_jwk_path: String, |
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.
The same question. Btw I must have missed the final conclusion about the compability of sealed secret. Do people agree on totally switch to signed sealed secret?
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 talked about it on the Trustee call and people said to go with the more secure option even if it breaks compatibility. I am planning to ask on this Thursday as well.
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.
Yep move on to more secure option.
| "d": "7baJLiTk68NcLgYPv05jJvElqoAhbzi1ZWx4OyILUtA", | ||
| "use": "sig", | ||
| "crv": "P-256", | ||
| "kid": "test-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.
Could we have a kid that uses KBS URI for example?
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.
Unsealing that will require having some kind of KBS we can connect to. Do we have any tooling for a fake KMS backend?
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.
offline-fs-kbc can work, but needs some change upon /etc/ files, thus probably not good in unit test.
btw, looks like this file is only used for serde/de-serde test, so we might not need to care about the unsealing?
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.
Ok let me see about adding another case.
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.
Ok, this test actually does both the serialization to/from json and to/from string. When we serialize to string we trigger the signature code. We could split this into two tests, but imo the serialization to/from json isn't particularly interesting anyway.
|
|
||
| let header = Protected { | ||
| oth: Unprotected { | ||
| alg: Some(Signing::Ps256), |
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.
Should this be Es256 for elliptic curve P-256?
https://docs.rs/jose-jwa/0.1.2/jose_jwa/enum.Signing.html#variant.Es256
https://docs.rs/jose-jwa/0.1.2/jose_jwa/enum.Signing.html#variant.Ps256
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.
Oh yeah switched this around halfway through. Names are fixed up now.
| Ok(verification_key) | ||
| } | ||
|
|
||
| fn validate_ps256(secret: String, verification_key: Vec<u8>, signature: Vec<u8>) -> 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.
Assuming you want elliptic curve and not RSA
| fn validate_ps256(secret: String, verification_key: Vec<u8>, signature: Vec<u8>) -> Result<()> { | |
| fn validate_es256(secret: String, verification_key: Vec<u8>, signature: Vec<u8>) -> Result<()> { |
| // Try validating the secret with the key using whatever | ||
| // algorithm is specified in the JWS header. | ||
| match alg { | ||
| Signing::Ps256 => Secret::validate_ps256(secret, verification_key, sig)?, |
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.
Do you want Es256?
|
Negative test cases would be great. |
18b03d7 to
df82785
Compare
Added a negative test. |
Previously, we didn't support secret signing. Instead we created secrets with a fake header and a fake signature. There might still be users out there with these secrets. How can we avoid breaking things for them while also ensuring that sealed secrets are a secure feature? One option would be to allow secrets with our particular fake header and signature to be unwrapped. Of course the problem with this is that a user with a real sealed secret that guarantees integrity could have their secret swapped for one with a fake signature. We could warn users in this case, but realistically nobody is going to see that. Instead, let's block these non-secure secrets by default and introduce a flag in the config file to re-enable them. To do so, create a new config field and turn that config into an env var. The env var configuration pattern might seem a little weird at first, but in cases where the configuration affects multiple parts of the CDH (in different crates), it is understandable. Since we interact with sealed secrets in a few unrelated places (such as the storage code), this seems like the best approach. Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>
5d760db to
a4edf0c
Compare
dcmiddle
left a comment
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.
Very thorough!
I think this
closes #597
Add mechanism to serialize and deserialize secrets with signatures. When deserializing, use the KID in the JWS header to find the signing key. The signing key will either be retrieved from the KBS (if it is a resource URI) or it will be treated as a local credential. The second approach is used for testing (although sadly the tests do require sudo or some fiddling). The key should be an EC PS256 JWK. There are hooks to support more key types in the future. When serializing, a signature will be created. The JWK and KID must be provided. Note that the JWK has a KID field of its own, but we don't care about this. The KID in the JWS is what matters. Also update the secret_cli. Note that the CLI will no longer let you create a secret without a signature. Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>
With secret signatures, it's non-trivial to generate a secret with bash. Update the docs to show how the secret-cli can be used. Include example command and an example (fake) JWK. Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>
a4edf0c to
23c850b
Compare
|
Updated docs. @Xynnn007 is on vacation, but let's give him a couple days to take another look at this. |
See individual commit messages for information such as how we deal existing sealed secrets with fake signatures.
Will add docs to the PR next week. We will also need to update the website.