basho / riak_api

Riak Client APIs
27 stars 45 forks source link

CRL handling is over-aggressive #65

Open macintux opened 10 years ago

macintux commented 10 years ago

Per http://security.stackexchange.com/questions/10158/ocsp-and-crls-specified-in-ca-or-client-certificate it doesn't appear that a CRL is mandatory for every item in a chain of certificates.

Yet https://github.com/basho/riak_api/blob/develop/src/riak_api_ssl.erl#L101-L105 fails certificate validation without one.

This is impacting JRuby + our ruby client, and may well impact other applications attempting client-side certificate-based authentication.

cc @Vagabond in case he'd care to chime in. Not sure what the standards say here.

macintux commented 10 years ago

riak.conf workaround for chains without CRL: check_crl=off

Thanks @Vagabond

/cc @bkerley

bkerley commented 10 years ago

check_crl = off breaks previously-working client cert auth in the ruby-client running on C ruby.

macintux commented 10 years ago

https://github.com/basho/riak_api/blob/develop/src/riak_api_ssl.erl#L90-L91

We're hiding errors if validation is turned on.

macintux commented 10 years ago

Here's the default verification function from public_key.hrl in the OTP public_key library:

-define(DEFAULT_VERIFYFUN,
    {fun(_,{bad_cert, _} = Reason, _) ->
         {fail, Reason};
        (_,{extension, _}, UserState) ->
         {unknown, UserState};
        (_, valid, UserState) ->
         {valid, UserState};
        (_, valid_peer, UserState) ->
         {valid, UserState}
     end, []}).
macintux commented 10 years ago

And yet, I can't prove that in riak_test. Quite befuddled.

Vagabond commented 10 years ago

I think you're correct, you should be handling bad_cert and extension explicitly and remove that catch-all clause.

macintux commented 10 years ago

When I create a parallel certificate structure that Riak doesn't know about, validate_function is not invoked, so the impact isn't as severe as I feared.

Still unclear what's happening with @bkerley and his Ruby client.

bkerley commented 10 years ago

As near as I can tell, it has two code paths:

Ruby with C OpenSSL

  1. Client sends Riak client cert only
  2. Riak validates client cert against configured CA
  3. Riak validates client cert CN

JRuby with BouncyCastle "OpenSSL"

  1. Client sends Riak client cert and client cert's issuer/CA
  2. Riak might validate client cert against configured CA?
  3. Riak attempts to validate CRL on client-provided CA
  4. If no CRL entry on client-provided CA, close connection.
macintux commented 10 years ago

Is this matrix accurate, Bryce?

My theory (admittedly weak) is your certs are broken somehow and we're masking it with the catch-all function header in validate_function.

check_crl CRuby JRuby
on :thumbsup: :thumbsdown:
off :thumbsdown: :thumbsdown:
Vagabond commented 10 years ago

Fixing the verify fun should help, see what you get when you do that.

Vagabond commented 10 years ago

It is very easy to generate bad ssl certificate chains.

macintux commented 10 years ago

Bryce has a fixed validate_function for testing.

The original bug (mandating CRLs) also needs to be addressed if that is the correct diagnosis; I suspect the conflation of two problems isn't helping with comprehension.

macintux commented 10 years ago

Ok, at this point there are two changes that seem useful but have yet to be proved one way or another. Bryce is struggling to get consistent results, but the standard tests can't reproduce what he's seeing, so I'm going to assume there are other "SSL is bloody hard" problems there.

Leaving target at 2.0.1, working on tracing the OTP call chain to better understand what happens with the current code and the implications of changing it.

Vagabond commented 10 years ago

Trying the ruby client with certificated generated by the riak_tests would go a long way to clarifying if it is a certificate problem.