httpd: add MtlsAuthenticator - #91
Conversation
To make testing more realistic and to fix a regression with the authentication refactor this commit extracts a helper `build_authenticators()` that is used during the tests to build a more realisitic configuration for our tests.
This commit makes the mTLS client certificate authentication its own authenticator to be symmetric with the SshAuthenticator and the coming jwt/api-key authenticators. Thi fixes the bug that previous an empty sshauth would silently skip mTLS. Note that the mTLS is slightly different as it is the only auth that has "AND" semantic. I.e. when mTLS auth is configured it is required to even connect. So any (optional) additional http protocol level auth is not even tried when missing. This is mostly to ensure that any http level attack is rejected unless mTLS passes (when mTLS is configured).
| authenticate each request. In short: mTLS plus optional HTTP auth is | ||
| the recommended setup right now. |
There was a problem hiding this comment.
Not sure I get this. A few lines above it says that if mTLS is configured, it is required for any connection, and that it is enough to pass authentication. The new MtlsAuthenticator is pushed first to the list and will pass before any other/future auth mechanism is considered.
Why is the recommended setup to combine mTLS with optional auth, where the optional auth is never exercised?
| /// catch config-to-authenticator bugs; the empty etc root keeps host | ||
| /// SSH keys from leaking in. | ||
| fn production_authenticators(insecure: bool, has_mtls: bool) -> Vec<Box<dyn Authenticator>> { | ||
| let empty_etc_root = tempfile::tempdir().unwrap(); |
There was a problem hiding this comment.
claude:
TempDir is dropped at return, so the SshKeyAuthenticator retains a path under a deleted directory.
| fn verified_client_cert_subject(ssl: &openssl::ssl::SslRef) -> Option<String> { | ||
| // a peer cert is only present when the handshake verified it | ||
| ssl.peer_certificate() | ||
| .map(|cert| format_x509_subject(&cert)) |
There was a problem hiding this comment.
claude:
format_x509_subject is not injective. renders a DN as ", "-joined SHORTNAME=value with no RFC 4514 escaping. A cert with a single CN of admin, OU=ops renders identically to a cert with CN=admin plus OU=ops.
| } | ||
|
|
||
| fn verified_client_cert_subject(ssl: &openssl::ssl::SslRef) -> Option<String> { | ||
| // a peer cert is only present when the handshake verified it |
There was a problem hiding this comment.
Maybe still add
if ssl.verify_result() != openssl::x509::X509VerifyResult::OK {
return None;
}| } | ||
| } | ||
|
|
||
| fn verified_client_cert_subject(ssl: &openssl::ssl::SslRef) -> Option<String> { |
There was a problem hiding this comment.
What's the plan here? Right now we accept anything signed by --trust, we don't use the subject/SAN, no CRL or similar. Maybe we should add a TODO where future work is required.
2 participants
Add a MtlsAuthenticator to fix a bug in the client cert auth and also for better symmetry/logging.