Expose accessors for private/public key on elliptic curve keys

[justsmth]

A rebase of #234 onto the latest on main.

Description of changes:

• Rebased changes from Expose accessors for private/public key on elliptic curve keys #234
• Exposes a Ed25519Seed type.

mod signature {
    // Technically not exposed before (private type but visible via KeyPair impl on EcdsaKeyPair)
    pub struct EcdsaPublicKey { ... }
    pub struct EcdsaPrivateKey<'a> { ... }
    pub struct Ed25519Seed<'a> { ... }


    // These are buffer discriminants, not actual buffer types.
    pub struct EcPublicDer { ... }
    pub struct EcPrivateKeyBuffer { ... }
    pub struct EcPrivateKeyDer { ... }
    pub struct Ed25519SeedBuffer { ... }

    impl EcdsaKeyPair {
        fn generate(alg: &'static EcdsaSigningAlgorithm) -> Result<Self, Unspecified>;
        fn to_pkcs8v1(&self) -> Result<Document, Unspecified>;
        fn private_key(&self) -> EcdsaPrivateKey<'_>;
        fn from_private_key_der(alg: &'static EcdsaSigningAlgorithm, key: &[u8]) -> Result<Self, KeyRejected>;
    }
    impl EcdsaPrivateKey {
        fn to_buffer(&self) -> Result<Buffer<'static, EcPrivateKeyBuffer>, Unspecified>;
        fn to_der(&self) -> Result<Buffer<'static, EcPrivateKeyDer>, Unspecified>;
    }
    impl EcdsaPublicKey {
        /// Provides the public key as a DER-encoded SubjectPublicKeyInfo structure.
        fn as_der(&self) -> Buffer<'_, EcPublicDer>;
    }
    impl Ed25519Keypair {
        pub fn to_pkcs8v1(&self) -> Result<Document, Unspecified>;
        pub fn to_pkcs8v2(&self) -> Result<Document, Unspecified>;
        pub fn seed(&self) -> Result<Ed25519Seed, Unspecified>;
        impl Drop for Ed25519KeyPair { ... }
    }
    impl Ed25519Seed {
    	fn to_buffer(&self) -> Result<Buffer<'static, Ed25519SeedBuffer>, Unspecified>;
    }
}

mod buffer {
    pub struct Buffer<'a, T> { ... }
    impl<T> AsRef<[u8]> for Buffer<'_, T> { ... }
    impl<T> fmt::Debug for Buffer<'_, T> { ... }
}

Call-outs:

• There’s inconsistency currently around whether to have a function names like to_pkcs8v1 versus to_pkcs8. Perhaps we need both?

(From #234)

We should discuss & agree to the general shape of the EcdsaKeyPair / EcdsaPrivateKey / EcdsaPublicKey types, as well as the new PrivateBuffer, to decide whether that shape makes sense. It's likely that replicating it for the other keys (e.g., RSA) will make sense. It's worth noting that the openssl crate takes a different approach with a general PKey type with functions to do a variety of operations -- that approach definitely leads to a simpler API, but it's less ring-like.

The current API surface is already a bit awkward -- for example, the only way to generate a key is via EcdsaKeyPair::generate_pkcs8 which gives you a serialized form rather than the Rust type. This PR adds a dedicated generate function, for latter use via to_pkcs8v1() if desired, but doesn't deprecate or remove the old API. We're also asking for a full signature algorithm rather than just the key type.

Testing:

(From #234)

A few unit tests are added. I'd like to agree on the general API shape before extending these further and it would help to gain some understanding of how much we want to test "trivial" wrappers around underlying AWS-LC functions for serialization/deserialization.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

[Mark-Simulacrum]

Just a couple tiny nits.

[codecov-commenter]

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (95228a8) 95.42% compared to head (712f0f2) 95.47%.

Files	Patch %	Lines
aws-lc-rs/src/ec.rs	94.28%	4 Missing ⚠️
aws-lc-rs/src/ec/key_pair.rs	98.21%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #259      +/-   ##
==========================================
+ Coverage   95.42%   95.47%   +0.05%     
==========================================
  Files          51       52       +1     
  Lines        6733     6921     +188     
==========================================
+ Hits         6425     6608     +183     
- Misses        308      313       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[skmcgrail]

I'm really not to fond of how we are using the names as_der and EcPublicKeyDer to mean "write the output as DER following the X.509 ASN.1 SubjectPublicKeyInfo type definition". DER is an ASN.1 encoding scheme, SubjectPublicKeyInfo is the structure to describe a public key that could be encoded in various formats, PEM, DER, BER, etc. I feel like we should be more explicit for own benefit in case we need to serialize to different structure types in the future.

Case in point, you could have a function that just writes the raw key bytes as a DER encode OCTET STRING. That's valid DER, but not a SubjectPublicKeyInfo.

Thoughts?

[hansonchar]

Would as_x509 and X509PublicKey any better?

As a reference, in the BC-FIPS Java world, the import of the der bytes (after base64 decoding if any) is something like:

new X509EncodedKeySpec(bytes)

[skmcgrail]

Just wanted to highlight this as another example of ambiguity around meaning of to to_der. EcsdaPrivateKey encode to DER but follow PKCS#8 (v1 or v2) and is explicit with the functions. In this case though to_der is serializing only to the specific PrivateKey format, specifically from RFC 5915. This is the same structure that is wrapped by PKCS#8 structure! Now you could argue that KeyPair type makes sense to use PKCS#8 cause that supports both being present (V2). But it also supports serializing just the private key, so why does to_der here not use that? Basically I think the intent needs to be more clear for the serialization functions. I'm not saying we shouldn't support these types, but don't want to overload the term DER. ASN.1 is already confusing enough :)

[justsmth]

I agree with your concerns. I'm still thinking about what changes need to be made regarding them.

Here are a few thoughts I have about what's important regarding the serialization/deserialization methods we provide for our structs:

• Documentation should be unambiguous about the encoding(s) provided or required.
• Method names should be clear about their purpose, but don't need to indicate details of the encoding.
• Method names should be consistent/symmetric within a given struct. For example, the encoding serialized by a to_der function should be appropriate for a from_der to deserialize the same struct (if such a method exists).
• Structs/Types (e.g., EcPrivateKeyDer) should be unambiguous and guide users toward proper usage of serialized data.

[justsmth]

My latest commit adds/improves the documentation so that the encoding provided by serialization methods is more clear. I also moved the structs that differentiate buffer types to a signature::encoding module. We might also rename the (e.g.) to_der methods to more directly indicate the type of encoding provided, but I'm not yet sure that's needed.

This is a one-way-door API decision, so it should be carefully considered.

[skmcgrail]

Just some minor feedback and one question, otherwise this looks good.

[hansonchar]

Instead of always marshaling a public key eagerly into both octet string and x509 formats, would it make more sense to have an API that allows either format to be lazily retrieved instead?

I can imagine cases where only one format is needed but not the other.

[skmcgrail]

Hopefully my final comment :)

[samuel40791765]

Just a general question: We were doing this previously as well, so its not a blocker. We allocate the public key with a maximum length on the stack in marshal_public_key, does octets have to be a Box here?

[samuel40791765]

minor nit: Noticed this when checking if Buffer was a new public interface, should we move buffer down to list of other non-public modules below

[justsmth]

I have another PR ready after this. I'll update the order there.

[hansonchar]

👏