Skip to content
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

CRL handling is over-aggressive #65

Open
macintux opened this issue Aug 26, 2014 · 14 comments
Open

CRL handling is over-aggressive #65

macintux opened this issue Aug 26, 2014 · 14 comments
Milestone

Comments

@macintux
Copy link
Contributor

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 macintux added this to the 2.0.1 milestone Aug 26, 2014
@macintux
Copy link
Contributor Author

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

Thanks @Vagabond

/cc @bkerley

@bkerley
Copy link

bkerley commented Aug 27, 2014

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

@macintux
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@Vagabond
Copy link
Contributor

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

@macintux
Copy link
Contributor Author

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
Copy link

bkerley commented Aug 28, 2014

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
Copy link
Contributor Author

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 👍 👎
off 👎 👎

@Vagabond
Copy link
Contributor

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

@Vagabond
Copy link
Contributor

It is very easy to generate bad ssl certificate chains.

@macintux
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

  • Make CRL handling optional
  • Handle bad certs sent to validate_function

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants