TOOLS-2598 Fix client certificate chain bug

[mukerjee]

If the user provides a client certificate chain (i.e., a client cert + an
intermediate cert), currently mongodump will ignore all certs except the last
one (i.e., dropping the client cert) and then compare the intermediate cert to
the private key, resulting in a fatal error about the key and the cert not
matching. Additionally, since we drop all but the last cert, we don't present
the server with the correct client cert.

Here we append all certs in the cert/key file together + use the first cert for
the subject name, which is standard practice. This fixes X.509 auth for
mongodump.

[tfogo]

Hi @mukerjee! Thank you for submitting a contribution.

I'm going to link this PR to https://jira.mongodb.org/browse/TOOLS-2598 since it looks like this would be a fix for that ticket. I'm going to consult with our Go Driver team about this behavior and get back to you next week some time. They originally wrote this logic so I want to get their opinion.

In the meantime, please could you sign the MongoDB Contributor Agreement? This will allow us to review and accept contributions from you. For more details see http://www.mongodb.org/about/contributors/.

[mukerjee]

Great-- thanks @tfogo . I've signed the CLA.

It sounds like that ticket matches up quite well, thanks for digging around for it. My guess is that other mongo projects already fixed this issue? I believe the go driver team fixed this in 1.8.3 (PR here: mongodb/mongo-go-driver#834 ) a few months ago. Thanks, and have a good weekend!

[mukerjee]

I noticed this PR is still open -- @tfogo any chance we can merge this? thanks!

[tfogo]

Hi @mukerjee, sorry for the delay with this! I've been a bit swamped, so I've asked another team member to take a look instead. We'll try to get this merged as soon as possible.

[edobranov]

Thanks for opening a PR @mukerjee! I think we should be able to go ahead with this, since the behavior is already fixed in the Go Driver like you pointed out.

My only minor concern is whether the certificates will get parsed correctly when we create the X509 key pair at the end. I noticed that you're aggregating / appending all the certificate bytes in certBlock before they get directly passed to tls.X509KeyPair(). Does that work on your end (if you've tested it)? If not, it might be easiest to just copy the Go Driver's implementation from that PR, which joins the certificates with a newline (I believe this is necessary when pem.Decode gets eventually called and checks for a line delineation).

And if possible, it'd be good to have a unit test for this. I think we can again copy the one-pk-multiple-certs.pem file from the Go Driver PR and verify that it works with the new code. You should be able to duplicate this Convey test block and just change the target PEM file (plus test description :)

Be sure to let us know if you run into issues or have any questions!

[edobranov]

@tfogo After some confusion about what the behavior should be vs. what it actually is, I just pushed a commit here which I believe puts everything in the correct state.

To summarize, mongodb/mongo-go-driver#834 says it's loading all certificates in a PEM file, but in reality it's only loading the first one. Still, this is okay since it lines up with OpenSSL's and other libraries' behavior. One of the changes I'm making here is simply applying the diff from that Go driver PR, so that we're consistent with the driver.

The other change is the one you and me discovered separately, which is that the incorrect certificate subject is being returned at the end of the function after parsing the PEM file. I filed GODRIVER-2650 to fix this in the driver. It's scheduled at the time of writing, so I believe they'll have that fix done soon. I'm applying that fix in this tools PR directly by changing this line in the for-loop:

certDecodedBlock = currentBlock.Bytes 

To this:

if certDecodedBlock == nil {
    certDecodedBlock = currentBlock.Bytes
}

This makes it so that the first certificate block is used (instead of the last one) when returning the subject at the end.

I also added an SSL integration test and a new client PEM file testing the behavior. The comments at the top of that file explain the contents. Here's an Evergreen patch for that: link.

Lmk if there's other changes you think should be done.

[mukerjee]

Thanks for picking this up! I didn't have time to work through this again and it is no longer needed for what I'm working on.

I want to be a little more clear about the actual bug I was trying to fix before. In my scenario I had client cert + key. The client cert was signed by an intermediate CA, and the intermediate CA was signed by a root CA.

So the trust chain was CA cert --> intermediate CA cert --> client cert.

I had a file which was:

client cert
client key
intermediate CA cert

and mongo had only the root CA cert, with client cert authentication required

when a client connects to mongo in this setup, the client should provide the client cert as well as the intermediate CA cert for verification by the mongo server.

The current behavior on main is broken in two ways (iirc):

• the mongodump tool assumes that last cert in the file is the client cert; standard practice is that it's the first cert that's the client cert. mongodump locally compares the last cert with the private key and complains about the mismatch.
• If you put the client cert at the end of the file, mongodump still has issues, as it only sends a single cert to the server (the last cert). It should send all certs to the server (so that the server can verify the whole chain). This is important if you have an intermediate CA cert that the mongo server doesn't know about.

My original fix solved this issue for me.

From skimming the new fix, this looks good, I think it will solve both problems above. That said, I think there should be a second test that tests cert chains explicitly.

I don't know how your testing framework works, but I'm assuming ca-ia.pem is "certificate authority, intermediate authority". It would be good to replicate the test you made and just change SSLCAFile to use the root CA cert (maybe ca.pem if i had to guess?).

From decoding the certs in your testdata, it looks like the chain is probably ca.pem --> ca-ia.pem --> client cert. So making a new test like i suggested will test that intermediate certs work. (i.e., mongo is booted with only ca.pem and it has to verify that the client cert given is indirectly signed by ca.pem (via the client providing ca-iam.pem as part of the chain)).

Also, I'm not sure what DBGetAuthOptions() does, but it might be good to boot mongo with the equivalent --tlsMode requireTLS ( e.g., https://www.mongodb.com/docs/manual/tutorial/configure-x509-client-authentication/#deploy-with-x-509-authentication ) to really make sure the authenticity of client certs is being checked on the mongo side.

thanks again for the help on this!

[tfogo]

@mukerjee thank you for spending the time providing so much detail. We appreciate it!

@edobranov it looks like test-client-multiple-certs.pem is Client Cert + Client Key + IA Cert. We want to make sure that all the certs in SSLPEMKeyFile get used to verify the chain. We are doing this correctly since we're adding all the certs to cfg.Certificates. But we need to change the test. I think we can just change SSLCAFile to ca.pem in TestServerMultipleCertificateVerification... or add another convey block with that setup.

[edobranov]

Thanks for all the help @mukerjee!

@tfogo As discussed offline, we'll add the integration test later since it's a bit tough to set up an Evergreen replica set with the correct PEM files just for this test. I filed TOOLS-3227 to eventually do this, but made sure to verify that the fix works locally.

If everything looks good to you, either I or Matt can merge this fix.