-
Notifications
You must be signed in to change notification settings - Fork 23
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
feature(*): Add missing implementation to support Client Certificate Authorization #135
Conversation
…scribed in TLS1.3 RFC8446 section 4.4.3
@lulf Only thing missing is the actual signer (https://github.com/MathiasKoch/embedded-tls/blob/b95e3e7b9d11438ee078c05acbfbdd84f7d9977f/src/connection.rs#L569-L571) 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 |
@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
The client certificate authentication test is currently failing with 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. |
ClientCertVerify
required for Client Certificate Authorization
Hmm.. Darn, trying to fix the examples not compiling reveals that there are some lifetime issues around the |
@lulf 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? |
Hmm.. Damn, This is still an issue, and the last one remaining.
|
It looks great, well done! And great that you added tests 👍
Not at all, I think this PR looks pretty good as is.
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
Just slightly changed the CryptoProvider trait to drop the associated SecureRandom in favor of a return type bound of
I think I would like to see the I can try to come up with something when I find some time, if this is something you agree with? |
Sounds good to me! 🚀 |
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 you add an entry to the change log as well?
…as an owned heapless String
@lulf I ended up removing the lifetime from TlsVerifier, and in exchange changed the hostname of webpki to an owned heapless String. |
This pr does two major changes:
1. Introduce a new
CryptoProvider
traitThe 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:
This trait replaces the previous generics of
CipherSuite
,RNG
andVerifier
, plus the newSigner
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 toTlsContext
. To make this a bit less painful, aSimpleProvider
is provided by this PR as an easy-to-use way of providing only RNG and aCipherSuite
.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