Remove ca_file, require_cert, and truststore options to X509 middleware #67
Conversation
drcapulet
force-pushed
the
alexc-x509-verify
branch
from
102964a to
be5e85c
Compare
There was a problem hiding this comment.
Some comments about comments (meta!), but other than that LGTM.
At some point it would probably be useful to have a higher-level middleware that would capture and set the identity of the forwarded client (e.g. something like ForwardedIdentityMiddleware). Such middleware would not operate on certificates directly though, but client identities (like a slug or SPIFFE ID), and it would set it accordingly (e.g. credentials[:client]).
Either way, this seems like a good change for now. 👍
CHANGES.md
Outdated
| ### Unreleased | ||
|
|
||
| * [#67](https://github.com/square/rails-auth/pull/67) | ||
| Remove `ca_file`, `require_cert`, and `truststore` options to X509 middleware. |
There was a problem hiding this comment.
we should probably add here that the X509 middleware will no longer attempt to verify a certificate chain.
| # @param [Logger] logger place to log verification successes & failures | ||
| # @param [Boolean] require_cert causes middleware to raise if certs are unverified | ||
| # @param [OpenSSL::X509::Store] truststore (optional) provide your own truststore (for e.g. CRLs) | ||
| # @param [Object] app next app in the Rack middleware chain |
There was a problem hiding this comment.
I can't seem to add my review directly on line 9, but that comment is no longer true. It might be worth adding a bit of context that you've added to the PR description.
There was a problem hiding this comment.
Similarly, I believe CertificateVerifyFailed (line 7) is unused now.
drcapulet
force-pushed
the
alexc-x509-verify
branch
from
be5e85c to
cf59400
Compare
Since the middleware isn't the one managing the TLS connection, we already have to rely on whatever is managing the TLS connection to verify & pass it down into the Rack environment safely. Verifying it in the middleware simply proves you know the cert (which are more or less public) and doesn't prove ownership of the underlying private key.