fixes #7822 - forbid HTTPS requests with no client SSL certificate #217
Conversation
domcleal
force-pushed
the
7822-ssl-verify-only
branch
2 times, most recently
from
15b27bf to
6fb7466
Compare
|
@witlessbird fixed the handling of different HTTPS values, thanks (domcleal@84f5b4c#commitcomment-8064426) |
| before do | ||
| if ['yes', 'on', '1'].include? request.env['HTTPS'].to_s | ||
| if request.env['SSL_CLIENT_CERT'].to_s.empty? | ||
| log_halt 403, "No client SSL certificate supplied" |
There was a problem hiding this comment.
Is there a plan to test the cert chain here?
There was a problem hiding this comment.
nm, we have OpenSSL::SSL::VERIFY_PEER in the https config.
|
Not a really excessive test, but I'm getting the expected responses now, according to my input.
|
|
tested, looks good. |
domcleal
force-pushed
the
7822-ssl-verify-only
branch
from
6fb7466 to
2cd292c
Compare
|
|
||
| ::Sinatra::Base.helpers ::Proxy::Helpers | ||
| ::Sinatra::Base.helpers ::Proxy::Log | ||
| ::Sinatra::Base.before do |
There was a problem hiding this comment.
looks good, although it might be a bit more readable if you used a proc here instead...
There was a problem hiding this comment.
Is that possible? I can't get it to work..
There was a problem hiding this comment.
::Sinatra::Base.before(&Proc.new { verify_client_cert })
def verify_client_cert
...
end
Having tried that though, I can say it doesn't make any difference in clarity whatsoever. My apologies.
|
[test] |
|
The test failure looks valid, let me look into it. gem update perhaps. |
|
tested with the updated filter, looks good! |
|
Test failure's addressed in #218 btw. |
domcleal
force-pushed
the
7822-ssl-verify-only
branch
from
2cd292c to
a49993b
Compare
|
[test] |
Please review as a priority, and test heavily. It seems that OpenSSL correctly refuses connections when the certificate presented isn't trusted by the CA that the server is using (note, we don't have separate client/server CAs), and the problem is only when no client cert is presented.
I can't see a way to check that relying on OpenSSL for bad verification is always solid. The closest I've found is that the socket that WEBrick sees has an OpenSSL verification result attached (http://ruby-doc.org/stdlib-1.9.3/libdoc/openssl/rdoc/OpenSSL/SSL/SSLSocket.html) but I can't see a way to get at that. In any case, it only seems to reach the smart proxy when there's either no cert or it validates and in both cases the verify_result was "OK".
Lastly, I'm looking at more tests in the 7822-ssl-verify branch which test Proxy::Launcher's https_app by spinning up the rack/WEBrick app and then make requests, but this will take more refactoring. I will probably follow up with this in another ticket, and only to develop (not 1.6 etc).
No CVE assigned yet - will add it to commit message before it's merged.