Skip to content
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

feature(*): Add missing implementation to support Client Certificate Authorization #135

Merged
merged 20 commits into from
Feb 29, 2024

Conversation

MathiasKoch
Copy link
Contributor

@MathiasKoch MathiasKoch commented Feb 6, 2024

This pr does two major changes:

1. Introduce a new CryptoProvider trait

The new trait collects all the user-provided stuff for PKI, such as a source of random number, a verifier if server certificates are to be verified, a signer if client certificate authentication is used, as well as information around which CipherSuite is to be used, and which curve is to be used by the signer.

The trait looks as:

pub trait CryptoProvider {
    type CipherSuite: TlsCipherSuite;
    type SecureRandom: CryptoRngCore;
    type SignatureCurve: CurveArithmetic + DigestPrimitive;

    fn rng(&mut self) -> &mut Self::SecureRandom;

    fn verifier(&mut self) -> Result<&mut impl TlsVerifier<Self::CipherSuite>, crate::TlsError> {
        Err::<&mut NoVerify, _>(crate::TlsError::Unimplemented)
    }

    /// Decode and validate a private signing key from `key_der`.
    fn signer(
        &mut self,
        _key_der: &[u8],
    ) -> Result<Signer<Self::SignatureCurve, Self::SecureRandom>, crate::TlsError> {
        Err::<Signer<Self::SignatureCurve, Self::SecureRandom>, _>(crate::TlsError::Unimplemented)
    }
}

This trait replaces the previous generics of CipherSuite, RNG and Verifier, plus the new Signer that would have been otherwise added and collects them in a single generic, and the tradeoff of forcing users to implement the trait on a struct they provide to TlsContext. To make this a bit less painful, a SimpleProvider is provided by this PR as an easy-to-use way of providing only RNG and a CipherSuite.

2. Implement the remaining logic for Client Certificate Authorization

It implements the missing state ClientCertVerify, as well a providing the means for adding the required signature to prove ownership of the client certificate using the corresponding private key.

It also adds a test_client_certificate_auth integration test, that tests client certificate auth against a rustls server.

ClientCertVerify is described in TLS1.3 RFC8446 section 4.4.3 (https://datatracker.ietf.org/doc/html/rfc8446#section-4.4.3)

Fixes #68
Fixes #14

@MathiasKoch MathiasKoch marked this pull request as draft February 6, 2024 11:11
src/handshake/certificate_request.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
@MathiasKoch
Copy link
Contributor Author

MathiasKoch commented Feb 7, 2024

@lulf
I think I have managed to come up with something I believe in 😄

Only thing missing is the actual signer (https://github.com/MathiasKoch/embedded-tls/blob/b95e3e7b9d11438ee078c05acbfbdd84f7d9977f/src/connection.rs#L569-L571)
Most likely something that implements https://docs.rs/signature/latest/signature/trait.Signer.html

Not quite sure how I should introduce that, with the least annoyance to everyone else? I don't think it fits into a trait called TlsVerifier?

@lulf
Copy link
Member

lulf commented Feb 8, 2024

@MathiasKoch Looking good, a separate signer trait makes more sense I guess.

…certificateVerify state. Also add a temporary rustls to rustls test to verify and compare client certificate authentication
@MathiasKoch
Copy link
Contributor Author

The client certificate authentication test is currently failing with Err(InvalidCertificate(BadSignature)).
I don't quite understand why. I have been trying to compare 1:1 with the implementation in rustls and https://github.com/RustCrypto/rustls-rustcrypto/blob/master/src/sign.rs

I have also set up an equal test using rustls client, with the same client certificates just to verify that the certificates and keys are good, and it passes.

@MathiasKoch
Copy link
Contributor Author

@lulf
Do you have any knowledge about this one? c8d634d

It seems to break my signature if I take end - 1, on the other hand all tests pass with just end? Any recollection on why it was minus 1 before?

src/asynch.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/connection.rs Outdated Show resolved Hide resolved
@MathiasKoch MathiasKoch marked this pull request as ready for review February 14, 2024 19:20
@MathiasKoch MathiasKoch changed the title feature(*): Add missing state ClientCertVerify required for Client Certificate Authorization feature(*): Add missing implementation to support Client Certificate Authorization Feb 14, 2024
src/blocking.rs Outdated Show resolved Hide resolved
@MathiasKoch
Copy link
Contributor Author

MathiasKoch commented Feb 15, 2024

Hmm.. Darn, trying to fix the examples not compiling reveals that there are some lifetime issues around the verifier function in CryptoProvider, that I need to solve for the webpki verifier. The lifetime of the hostname makes it hard..

@MathiasKoch
Copy link
Contributor Author

@lulf
Think I managed to fix the remaining points, and also got rid of the ugly trait bounds!
I am really happy with the result of this! It should work for any signatures schemes now.

All I have left on my to-do, is to try to implement this on top of my ATECC crypto chip, but I don't think that will be a blocker for this PR?
I can always open another PR with improvements for hardware crypto.

@MathiasKoch
Copy link
Contributor Author

Hmm.. Damn,

This is still an issue, and the last one remaining.

Hmm.. Darn, trying to fix the examples not compiling reveals that there are some lifetime issues around the verifier function in CryptoProvider, that I need to solve for the webpki verifier. The lifetime of the hostname makes it hard..

@lulf
Copy link
Member

lulf commented Feb 26, 2024

@lulf Think I managed to fix the remaining points, and also got rid of the ugly trait bounds! I am really happy with the result of this! It should work for any signatures schemes now.

It looks great, well done! And great that you added tests 👍

All I have left on my to-do, is to try to implement this on top of my ATECC crypto chip, but I don't think that will be a blocker for this PR? I can always open another PR with improvements for hardware crypto.

Not at all, I think this PR looks pretty good as is.

Hmm.. Damn,

This is still an issue, and the last one remaining.

Hmm.. Darn, trying to fix the examples not compiling reveals that there are some lifetime issues around the verifier function in CryptoProvider, that I need to solve for the webpki verifier. The lifetime of the hostname makes it hard..

I'm not particularly keen on keeping the webpki verifier if that helps, it doesn't really work on cortex-m so it's kinda useless in IMHO, and I don't know of any wasm users of embedded-tls.

…eRandom, but rather have a return type impl bound. Also add blanket impl of CryptoProvider for mutable references
@MathiasKoch
Copy link
Contributor Author

Just slightly changed the CryptoProvider trait to drop the associated SecureRandom in favor of a return type bound of impl CryptoRngCore. This makes it much easier to implement in practice.

I'm not particularly keen on keeping the webpki verifier if that helps, it doesn't really work on cortex-m so it's kinda useless in IMHO, and I don't know of any wasm users of embedded-tls.

I think I would like to see the TlsVerifier replaced by something similar to the new signer, where it is made up of RustCrypto traits instead, making it much more composable with stuff in the wild.

I can try to come up with something when I find some time, if this is something you agree with?

@lulf
Copy link
Member

lulf commented Feb 27, 2024

Just slightly changed the CryptoProvider trait to drop the associated SecureRandom in favor of a return type bound of impl CryptoRngCore. This makes it much easier to implement in practice.

I'm not particularly keen on keeping the webpki verifier if that helps, it doesn't really work on cortex-m so it's kinda useless in IMHO, and I don't know of any wasm users of embedded-tls.

I think I would like to see the TlsVerifier replaced by something similar to the new signer, where it is made up of RustCrypto traits instead, making it much more composable with stuff in the wild.

I can try to come up with something when I find some time, if this is something you agree with?

Sounds good to me! 🚀

lulf
lulf previously approved these changes Feb 27, 2024
Copy link
Member

@lulf lulf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add an entry to the change log as well?

@MathiasKoch
Copy link
Contributor Author

@lulf
I think this is ready to go now.

I ended up removing the lifetime from TlsVerifier, and in exchange changed the hostname of webpki to an owned heapless String.
Think this is a reasonable tradeoff for now, while I look into refactoring the TlsVerifier in a new PR.

@lulf lulf merged commit d7c9b93 into drogue-iot:main Feb 29, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for x.509 certification. Add support for client cert authentication
2 participants