sfackler / rust-native-tls

Apache License 2.0
468 stars 197 forks source link

rust-native-tls is not able to receive peer certificate #262

Open jarlah opened 1 year ago

jarlah commented 1 year ago

I have made a fully reproducible bug in this repository:

https://github.com/jarlah/tls-peer-certificate-test

The gist of it is that both in hyper and and direct usage of rust-native-tls, im not able to receive the peer certificate.

I want to know if its a bug in this package, or if its a complete misunderstanding of the api method peer_certificate()?

🙏

sfackler commented 1 year ago

The TlsAcceptor would need to be configured to request a client certificate, which is not currently exposed.

jarlah commented 1 year ago

The TlsAcceptor would need to be configured to request a client certificate, which is not currently exposed.

Does that mean I have to code up something for the TlsAcceptor, or does it mean that TlsAcceptor needs to be modified to include such behaviour ?

sfackler commented 1 year ago

TlsAcceptor needs to be modified to include such behavior.

jarlah commented 1 year ago

I can help to code it up if you give me some pointers. Is it hard? I see this is not solved properly for any tls library, so I assume it's hard. Or, it's not hard but no one wants to use it. It's only in the few cases where you need to validate and authorise/authenticate the connecting client based off its client certificate, which is basically not that often right. But it would open up for so many possible server solutions in rust if we solve it here :)

jarlah commented 1 year ago

Some observations. Test peer_certificate in test.rs doesn't really test peer certificate. It's just calling it and checking if it returns None. That it doesn't crash. Coverage yeah, but it doesn't really show that it works when we would expect a peer certificate to be present in the stream.

I see this project uses test-cert-gen which doesn't generate cert and key for the client. Because "it's generally enough for unit and integration tests" to not include it. To be able to test peer_certificate() that returns Some() we would need to update this test-cert-gen crate to generate private key and cert for the client as well.

I see however in the test that it does assert for the client side that peer_certificate() returns the server certificate.. So it does at least work at some level. But not for the client certificate on the server end.

sfackler commented 1 year ago

Test peer_certificate in test.rs doesn't really test peer certificate.

That test both checks the case when the peer does not send a certificate and the case when the peer does send a certificate.

I have no reason to believe that peer_certificate would not work on the server side if the server actually requested that the client send a certificate.

If you wanted to implement this you'd need to make implementations for all of the underlying TLS implementations, OpenSSL, Security.framework, and schannel. It looks like the schannel library currently doesn't expose that configuration either so you'd need to start by adding it there.

jarlah commented 1 year ago

we disagree only on small nitpick details ;)

When you say that the Windows schannel implementation doesnt expose the configuration, can you point me to the configuration in OpenSSL or security framework impl for the same thing? Because you implied that these expose "the configuration"? Ill admit that im not tls or even native-tls expert. But i might be able to read documentation for Windows api, install a vm and test stuff. I suppose it would be possible to make this work in general without the schannel impl completely implemented? Lets say we didnt have schannel impl, would we be able to make this work now ?

sfackler commented 1 year ago

https://docs.rs/openssl/latest/openssl/ssl/struct.SslContextBuilder.html#method.set_verify https://docs.rs/security-framework/latest/security_framework/secure_transport/struct.SslContext.html#method.set_client_side_authenticate

Lets say we didnt have schannel impl, would we be able to make this work now ?

No

jarlah commented 1 year ago

Currently looking into it. Its a mess 😂 i understand why this is not yet implemented. 😶

jarlah commented 1 year ago

looked at it. Asked chatgpt. Gave up. For now