Decrypt keys JWEs with ring directly #95
Conversation
|
my comments are suggestions, I don't think they have to be addressed before merging this. |
| peer_pub_key.extend_from_slice(&y); | ||
| let peer_pub_key = Input::from(&peer_pub_key); | ||
| let secret = agreement::agree_ephemeral(self.private_key, &agreement::ECDH_P256, peer_pub_key, ring::error::Unspecified, |z| { | ||
| // ConcatKDF |
linuxwolf
Jul 6, 2018
I think it might help future us to have a comment that this is an optimization of ConcatKDF when keyLen <= hashLen.
I think it might help future us to have a comment that this is an optimization of ConcatKDF when keyLen <= hashLen.
| // ConcatKDF | ||
| let counter = 1; | ||
| let alg = protected_header["enc"].as_str().unwrap(); | ||
| let apu = ""; |
linuxwolf
Jul 6, 2018
To be a little more resilient, this ought look for protected_header["apu"] and use "" if it's missing/empty.
To be a little more resilient, this ought look for protected_header["apu"] and use "" if it's missing/empty.
| let counter = 1; | ||
| let alg = protected_header["enc"].as_str().unwrap(); | ||
| let apu = ""; | ||
| let apv = ""; |
linuxwolf
Jul 6, 2018
Same as above, but protected_header["apv"].
Same as above, but protected_header["apv"].
| let mut buf: Vec<u8> = vec![]; | ||
| buf.extend_from_slice(&to_32b_buf(counter)); | ||
| buf.extend_from_slice(&z); | ||
| buf.extend_from_slice(&to_32b_buf(alg.len() as u32)); |
linuxwolf
Jul 6, 2018
maybe include a comment this is ConcatKDF's "otherinfo" -- again just thinking about future us.
maybe include a comment this is ConcatKDF's "otherinfo" -- again just thinking about future us.
| pub fn generate_keys_jwk(&self) -> Result<String> { | ||
| let mut pub_key = vec![0u8; self.private_key.public_key_len()]; | ||
| &self.private_key.compute_public_key(&mut pub_key)?; | ||
| // First byte is 4, then 32 bytes for x, and 32 bytes for y. |
linuxwolf
Jul 6, 2018
This format (0x04 + public.x + public.y) is the "uncompressed representation" -- might be worth a note if there's ever a discrepancy in the future.
This format (0x04 + public.x + public.y) is the "uncompressed representation" -- might be worth a note if there's ever a discrepancy in the future.
|
This is great, nice work @eoger! I think my main suggestion would be to add more docs and tests - anytime you have what looks like hand-rolled crypto code, it's worth investing in re-assuring the reader that everything is in order.
|
| } | ||
|
|
||
| impl ScopedKeysFlow { | ||
| pub fn with_random_ecdh_key() -> Result<ScopedKeysFlow> { |
rfk
Jul 10, 2018
Member
nit: is the fact that it's "ecdh" important enough to expose in the name, or would something like "with_random_key" be sufficient?
nit: is the fact that it's "ecdh" important enough to expose in the name, or would something like "with_random_key" be sufficient?
| @@ -497,7 +499,7 @@ mod tests { | |||
| } | |||
|
|
|||
| pub struct OAuthFlow { | |||
| pub jwk: Option<JWK>, | |||
| pub scoped_keys_flow: Option<ScopedKeysFlow>, | |||
rfk
Jul 10, 2018
Member
I like the way this encapsulates the state FWIW.
I like the way this encapsulates the state FWIW.
| let mut pub_key = vec![0u8; self.private_key.public_key_len()]; | ||
| &self.private_key.compute_public_key(&mut pub_key)?; | ||
| // Uncompressed representation: | ||
| // First byte is 4, then 32 bytes for x, and 32 bytes for y. |
rfk
Jul 10, 2018
Member
It would be useful to link to external documentation on the format you're encoding into here, for reference.
It would be useful to link to external documentation on the format you're encoding into here, for reference.
| let x = base64::decode_config(&protected_header["epk"]["x"].as_str().unwrap(), base64::URL_SAFE_NO_PAD)?; | ||
| let y = base64::decode_config(&protected_header["epk"]["y"].as_str().unwrap(), base64::URL_SAFE_NO_PAD)?; | ||
| let mut peer_pub_key: Vec<u8> = vec![0x04]; | ||
| peer_pub_key.extend_from_slice(&x); |
rfk
Jul 10, 2018
Member
Do we know min/max length of x and y here? If so, it may be worth asserting them explicitly as an extra sanity-check.
Do we know min/max length of x and y here? If so, it may be worth asserting them explicitly as an extra sanity-check.
| peer_pub_key.extend_from_slice(&y); | ||
| let peer_pub_key = Input::from(&peer_pub_key); | ||
| let secret = agreement::agree_ephemeral(self.private_key, &agreement::ECDH_P256, peer_pub_key, ring::error::Unspecified, |z| { | ||
| // ConcatKDF (1 iteration since keyLen <= hashLen). |
rfk
Jul 10, 2018
Member
As above, it's probably worth linking to the relevant parts of the JWA spec, for reference of future readers.
As above, it's probably worth linking to the relevant parts of the JWA spec, for reference of future readers.
|
Amazing work!! My only wish , just like Ryan already mentioned, some kind of a test that will make it easier for us to maintain this JWE code. |
eoger
force-pushed the
manual-jwe-decrypt
branch
2 times, most recently
from
3086a79
to
fa6aec8
|
Thank you for the reviews everyone, I've added tests that rely on briansmith/ring#674, let's see how it goes on the other side. |
| #[fail(display = "JOSE error: {}", _0)] | ||
| JoseError(#[fail(cause)] SyncFailure<jose::error::Error>), | ||
| #[fail(display = "Unspecified ring error: {}", _0)] | ||
| UnspecifiedRingError(#[fail(cause)] ring::error::Unspecified), |
briansmith
Jul 10, 2018
I don't recommend that people map ring's Unspecified error to another Unspecified error. Instead I would create separate errors, e.g. KeyGenerationFailed, KeyAgreementFailed, and map Unspecified to these based on the context.
I don't recommend that people map ring's Unspecified error to another Unspecified error. Instead I would create separate errors, e.g. KeyGenerationFailed, KeyAgreementFailed, and map Unspecified to these based on the context.
|
Thanks everyone for your comments! |
Merge v0.7.2 to main
This allows us to get rid of the dependency on jansson and cjose, and unblocks us in pursuing a complete removal of the openssl dependency.
I don't mind dropping the 2nd commit if people feel the
ScopedKeysFlowabstraction is confusing.Many thanks to @linuxwolf for his assistance!