-
-
Notifications
You must be signed in to change notification settings - Fork 201
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
WIP: Client auth tests and Custom acceptor CA #23
Conversation
@@ -20,3 +20,11 @@ openssl = "0.9" | |||
|
|||
[dev-dependencies] | |||
openssl = { version = "0.9", features = ["v102", "v110"] } | |||
|
|||
[replace] | |||
"openssl:0.9.7" = { git = "https://github.com/sfackler/rust-openssl.git", features = ["v102", "v110"] } |
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.
Why is this necessary?
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.
It's temporary, I don't plan on that being merged. It's necessary until the next release of openssl b/c of my use of the pkcs12 builder.
@sfackler since you're looking at this: I've got a few problems in getting this to pass:
|
There's a test for client auth in the security-framework repo: https://github.com/sfackler/rust-security-framework/blob/master/security-framework/src/os/macos/secure_transport.rs#L462 |
Hm, can you explain: let stream = p!(listener.accept()).0;
match ctx.handshake(stream) {
Ok(_) => panic!("unexpected success"),
Err(HandshakeError::Failure(_)) => {}
Err(err) => panic!("unexpected error {:?}", err),
} Shouldn't that |
Hmm, looking back at that test that does seem strange... I'm not really sure why that would be expected to fail. |
The error I'm getting on the server side test on macOS is
So I was going to start investigating the server side registration of the cert to see if there's an issue there, as the same cert construction works in all other contexts. |
I wonder if the configured anchor certs aren't taken into account? It may require breaking on peer certs and doing it manually? |
I think we might be missing based on this doc: https://developer.apple.com/reference/security/1400161-sslhandshake?language=objc I'll see if I can find that code. edit: n/m this error code |
@sfackler , see recent change, this doesn't validate the cert chain... we'll need to figure that out, but it does properly pause on the client and then resume to complete the connection. |
What do you think of providing a single function abstraction over the three different implementations to handle this case? It would be something like: fn handle_client_auth(cert: X509Type) -> Result<()>; The X509Type would need to be a wrapper type of the specific implementation across the underlying libraries. The complexity here, is that we'll need to establish custom certificate chains for validating the client. I think the cert chain and how it is validated will be somewhat domain specific, but it's probably hairy enough that there should be some standard interfaces for certificate validation. Do you have any thoughts in this area? I imagine these would be custom SslContexts... |
@anowell this is the PR I mentioned at RustConf I was working on a while back for some mTLS stuff. Do you think it's out of date/unnecessary at this point? |
Thanks, @bluejekyll. I haven't had a chance to dig deep enough into this change yet, but before I crash for the night I thought I'd point you at how I'm currently reading client key/cert from a kubernetes config and then turning that into a Pkcs12 that I plumb through [a fork of |
This has some client auth tests. It also adds custom association for CAs on the server side.