Add support for specifying accepted Client CAs

[nosnilmot]

Add support for specifying which CAs will be accepted as issuers of Client Certificates (OpenSSL API SSL_CTX_set_client_CA_list).

This is necessary for correct SSL negotiation of Client Certificate where client is expected to only send Client Certificate during handshake if it has one issued by an accepted CA.

OpenSSL verification is enabled (unless 'verify_none' is specified).

Unit tests are added for various combinations of self-signed and CA issued Client and Server certs, along with script for creating test CA, Server & Client certs.

I will also submit a PR for ejabberd to enable using this.

[zinid]

I don't understand the meaning of the PR: why existing cafile is not enough?

[nosnilmot]

This option specifies which CAs should be permitted to issue Client Certificates. That may well be different from the list of CAs trusted to issue other server certificates.

As an example, MegaCorp may only want to accept client certificates issued by the MegaCorp CA, but is happy to connect to 3rd party servers with certs issued by a wider range of CAs (DigiCert, Let's Encrypt, GeoTrust, etc.).

Additionally, clients use the list of accepted CAs to determine whether to present (or which to present) a Client Certificate during SSL/TLS handshake. It would also be inefficient for the server to present all CAs that may be listed in cafile during the handshake.

[zinid]

As an example, MegaCorp may only want to accept client certificates issued by the MegaCorp CA

I still don't understand: you can put MegaCorp CA into cafile and get the same result.

[nosnilmot]

That would mean client certs from ALL CAs were accepted, not just MegaCorp CA.

Also consider the case where client has multiple certs, it needs to know which CAs are acceptable to select which to send (and even if we didn't care too much about accepting certs from other CAs, sending all of those in cafile would significantly increase the size and likely time for every handshake).

... [some thinking time passes] ...

I suppose different cafile for client and server case might work, and we can presumably do that with c2s_cafile, s2s_cafile and listener-specific cafile... so probably all I need from this PR is passing this to SSL_CTX_set_client_CA_list for listener sockets and suitable verification (plus some bonus tests!).

[nosnilmot]

@zinid thanks for helping me realise I had overthought this, are the remaining simplified changes in this PR ok? Would you prefer me to rebase the branch?

[zinid]

I don't understand the meaning of the changes. Seems like in the case when cafile is set, the code now ignores SNI completely and I think this is not desirable.

[zinid]

Ah, no, I read it wrongly due to cumbersome indentation. Anyway, seems like the code for verification has changed, why?

[nosnilmot]

seems like the code for verification has changed, why?

I want connections that do not provide Client Certificates issued by an accepted CA to be rejected when cafile is set (-> SSL_VERIFY_FAIL_IF_NO_PEER_CERT). The verify_callback was removed because it provides no benefit as far as I could determine (besides possibly allowing unwanted connections to continue), I do not believe there is any performance penalty from this removal as was hinted in #29.

I can see this could be an unexpected change in behaviour when leveraging the existing cafile option instead of what I was originally doing with the new client_cafile option. Perhaps I need to add a separate config to indicate if client SSL certs are optional vs. required? (similar to Apache httpd's SSLVerifyClient optional|required if you are familiar with that).

[zinid]

besides possibly allowing unwanted connections to continue

But we need this behaviour for ejabberd s2s connections (when mod_s2s_dialback is loaded). Whatever, I think the verify callback is completely irrelevant to the CA_list you want to have. Just keep CA_list, don't touch verify callback and I will merge the PR.

[zinid]

Sigh... Now, why did you add this? Once again, the driver should NOT fail connections just because there is a problem with a certificate, because ejabberd will do it by itself with generating human-readable error messages and sending them to the client inside SASL <failure/> element. Otherwise it will be completely impossible to track the problem which will become a hell for sysadmins and they will create load for our support team.

[nosnilmot]

Sigh...

Sorry!

Now, why did you add this? (SSL_VERIFY_FAIL_IF_NO_PEER_CERT) Once again, the driver should NOT fail connections just because there is a problem with a certificate,

But that is what I want to happen :) I thought I was clear about that in https://github.com/processone/fast_tls/pull/32#issuecomment-370921780

I am not using SSL Client Certificates for full authentication, only as a form of authorisation for now. The actual authentication will use regular SASL PLAIN mechanism, so what ejabberd offers today doesn't quite meet my needs. Maybe that helps you understand what I am trying to achieve? Do you have any suggestions on how to accomplish this in a more acceptable way?

Thanks for the explanation for the reasoning behind wanting to accept bad certs and having ejabberd do the rejection - it would have been helpful if #29 had that detail.

[zinid]

I am not using SSL Client Certificates for full authentication, only as a form of authorisation for now. The actual authentication will use regular SASL PLAIN mechanism, so what ejabberd offers today doesn't quite meet my needs.

Seems like that's the problem of XMPP itself: what you need is a sort of 2FA, i.e. SASL EXTERNAL + SASL PLAIN. I don't feel like TLS driver is a correct thing to fix it. But I think it's possible to patch ejabberd to support round-trips in SASL authentication.

[nosnilmot]

But I think it's possible to patch ejabberd to support round-trips in SASL authentication.

That sounds like an interesting idea, are you thinking something like this for SASL EXTERNAL?

• if Cert verifies & includes JID -> Auth success
• if Cert verifies but no JID -> Skip mechanism but allow others to continue
• if No cert or verification fails -> Reject auth

[nosnilmot]

something like this for SASL EXTERNAL?

That actually won't work for me unless we can also enforce that EXTERNAL mech is attempted before others.

[zinid]

Looks fine now. Could you please rebase/open a new PR so I can apply it?

[nosnilmot]

I have rebased, do you need a new PR too?

[zinid]

No need. But now I have another question ;) Regarding this line: https://github.com/processone/fast_tls/pull/32/files#diff-5bd992290c2dd875ba417180efc336b3L159 I think this is wrong, this will block incoming connections when the certificate is not set.

[nosnilmot]

That line was incorrect before, it was saying:

if a certificate is specified or this is a listening/accept socket -> proceed else -> error

but that's incorrect, certificate is optional for client/connecting socket but required for listening/accept sockets, so I corrected the logic:

if a certificate is specified or this is a client/connecting socket -> proceed else -> error

[zinid]

but that's incorrect, certificate is optional for client/connecting socket but required for listening/accept sockets, so I corrected the logic

No. If there is no certificate provided, it will fetched by SNI. See add_certfile/2, get_certfile/1 and delete_certfile/1.

[nosnilmot]

ok, so it was wrong before and it is wrong now too? :) certificate should be optional for both client and server sockets

[zinid]

ok, so it was wrong before and it is wrong now too? :) certificate should be optional for both client and server sockets

I'm not sure, but I think the driver (fast_tls.c) expects a certificate for a client connection. Whatever, this PR is not the right place to fix this issue. If you're curious, you can check the client behaviour without a certificate and create a separate PR if you want it to be fixed.

[nosnilmot]

I'm fairly sure the driver works fine without cert for client connection, as I made this change while expanding the test cases (which will now fail if I just revert that one line). I think I'll move that change and the test case expansion to a separate PR