Add ec::EcKey::private_key_from_der_for_group #2371
Conversation
openssl/src/ec.rs
Outdated
| &mut cbs as *mut ffi::CBS, | ||
| group.as_ptr(), | ||
| )) | ||
| .map(|p| EcKey::from_ptr(p)) |
There was a problem hiding this comment.
Without commenting on whether this makes sense in this project or not, FYI this patch is wrong. It doesn't account for trailing data by checking the CBS is empty after parsing.
There was a problem hiding this comment.
(There also is a way to do this in OpenSSL but it's suuuuuuuper confusing and hard to use. d2i_ECPrivateKey peeks at the state of the input EC_KEY in the object reuse mode. You also have to be very careful when using the object reuse mode because the function might destroy the object and put a different one in its place.)
There was a problem hiding this comment.
Without commenting on whether this makes sense in this project or not, FYI this patch is wrong. It doesn't account for trailing data by checking the CBS is empty after parsing.
Chiming in here since I'm helping Francois with this effort - what would the correct handling be? Generate an error if the CBS is non-empty after parsing?
There was a problem hiding this comment.
Yup. Both CBS and d2i functions parse a prefix of the input and tell you how far they got, so that you can parse a series of things. I'm... not sure this was the least errorprone API design in hindsight but here we are. This means that if you are implementing an API that just parses it, rather than parses and tells you what was left, you need to check the input was fully consumed.
There was a problem hiding this comment.
Gotcha; does the new version I uploaded look good? (Assuming we figure out a reasonable error to report)
Upstreaming https://android-review.googlesource.com/c/platform/external/rust/crates/openssl/+/2191953 In the `ECPrivateKey` schema (RFC 5915 s3) the parameters field which holds the NamedCurve is optional. If an EC key was originally enclosed in a PKCS#8 (RFC 5208 s5) wrapper, the curve may be identified in the wrapper (in the `AlgorithmIdentifier.parameters` field) rather than in the `ECPrivateKey`. In this case, the existing `ec::EcKey::private_key_from_der()` method (which corresponds to `d2i_ECPrivateKey()`) cannot determine the curve and so cannot import such a key. Add a new method that includes explicit specification of the curve to cope with this situation, passing through to the (BoringSSL-specific) `EC_KEY_parse_private_key` function.
joshlf
force-pushed
the
private_key_from_der_for_group
branch
from
b996561 to
5198075
Compare
| group.as_ptr(), | ||
| ))?; | ||
| if ffi::CBS_len(p) != 0 { | ||
| todo!() |
There was a problem hiding this comment.
Can one of the maintainers let me know what you'd suggest I do here to generate a new error? Thanks!
|
I'm not wild about APIs that are unique to BoringSSL. Given that BoringSSL also provides a native set of Rust bindings, it seems to me the role of BoringSSL support in rust-openssl is primarily to provide compatibility to projects already using rust-openssl which want to also support BoringSSL. (That's how I've personally used the BoringSSL support thus far.) Since, AFAIK, Android is the primary other consumer of this support, I'd appreciate understanding your thoughts/vision? |
My intention was to avoid having to carry the same downstream patch in both Android and Fuchsia. I can understand not wanting something BoringSSL-specific upstream and it seems that providing an OpenSSL equivalent might be a fair bit of work. Maybe an alternative we can explore is to try to lift this from a downstream rust-openssl patch to something in KeyMint itself and then Fuchsia will get it when rolling KeyMint – from the look of it, this feels like a constructor more so than accessing private fields so it might be possible? |
|
It's certainly possible to carry this as a function out of tree, yes.
…On Fri, Feb 21, 2025 at 7:58 PM frncs-rss ***@***.***> wrote:
I'm not wild about APIs that are unique to BoringSSL. Given that BoringSSL
also provides a native set of Rust bindings, it seems to me the role of
BoringSSL support in rust-openssl is primarily to provide compatibility to
projects already using rust-openssl which want to also support BoringSSL.
(That's how I've personally used the BoringSSL support thus far.)
Since, AFAIK, Android is the primary other consumer of this support, I'd
appreciate understanding your thoughts/vision?
My intention was to avoid having to carry the same downstream patch in
both Android and Fuchsia.
I can understand not wanting something BoringSSL-specific upstream and it
seems that providing an OpenSSL equivalent might be a fair bit of work.
Maybe an alternative we can explore is to try to lift this from a
downstream rust-openssl patch to something in KeyMint itself and then
Fuchsia will get it when rolling KeyMint – from the look of it, this feels
like a constructor more so than accessing private fields so it might be
possible?
—
Reply to this email directly, view it on GitHub
<#2371 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAGBEXC2BOUF7BA4645LD2Q7DU7AVCNFSM6AAAAABXO4LJOWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNZVHA4TMNRSG4>
.
You are receiving this because you commented.Message ID:
***@***.***>
[image: frncs-rss]*frncs-rss* left a comment (rust-openssl/rust-openssl#2371)
<#2371 (comment)>
I'm not wild about APIs that are unique to BoringSSL. Given that BoringSSL
also provides a native set of Rust bindings, it seems to me the role of
BoringSSL support in rust-openssl is primarily to provide compatibility to
projects already using rust-openssl which want to also support BoringSSL.
(That's how I've personally used the BoringSSL support thus far.)
Since, AFAIK, Android is the primary other consumer of this support, I'd
appreciate understanding your thoughts/vision?
My intention was to avoid having to carry the same downstream patch in
both Android and Fuchsia.
I can understand not wanting something BoringSSL-specific upstream and it
seems that providing an OpenSSL equivalent might be a fair bit of work.
Maybe an alternative we can explore is to try to lift this from a
downstream rust-openssl patch to something in KeyMint itself and then
Fuchsia will get it when rolling KeyMint – from the look of it, this feels
like a constructor more so than accessing private fields so it might be
possible?
—
Reply to this email directly, view it on GitHub
<#2371 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAGBEXC2BOUF7BA4645LD2Q7DU7AVCNFSM6AAAAABXO4LJOWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNZVHA4TMNRSG4>
.
You are receiving this because you commented.Message ID:
***@***.***>
--
All that is necessary for evil to succeed is for good people to do nothing.
|
we temporarily need 2 patches: 1. encrypt.rs: rust-openssl/rust-openssl#2372 already upstream waiting for the next quarterly release 2. ec.rs: rust-openssl/rust-openssl#2371 rejected upstream waiting to be inlined into KeyMint tracked by https://fxbug.dev/398863191 Fixed: 395970649 Change-Id: I12a795f645b74297948eddb6baaa8ca44df7404d Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1219109 Reviewed-by: Erick Tryzelaar <[email protected]> Commit-Queue: Francois Rousseau <[email protected]>
4 participants
Upstreaming https://android-review.googlesource.com/c/platform/external/rust/crates/openssl/+/2191953
In the
ECPrivateKeyschema (RFC 5915 s3) the parameters field which holds the NamedCurve is optional.If an EC key was originally enclosed in a PKCS#8 (RFC 5208 s5) wrapper, the curve may be identified in the wrapper (in the
AlgorithmIdentifier.parametersfield) rather than in theECPrivateKey.In this case, the existing
ec::EcKey::private_key_from_der()method (which corresponds tod2i_ECPrivateKey()) cannot determine the curve and so cannot import such a key.Add a new method that includes explicit specification of the curve to cope with this situation, passing through to the (BoringSSL-specific)
EC_KEY_parse_private_keyfunction.