1040130 - Socket transport API to specify client cert

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

For WiFi debugging, I want to connect to a TLS server using a known, specific client cert, but I do not want a client cert dialog to be displayed.

The socket transport API should offer a way to set a client cert to use without prompting.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Roughly speaking, I think I would add a |cliertCert| attribute to nsISocketTransport.  The cert would be passed down to the securityInfo object (nsNSSSocketInfo).  If the server asks for a client cert and one has been set in this way, it will be used instead of prompting the user.

Honza, does this seem okay at a high level?

Comment 2

[Honza Bambas (:mayhemer)]

There is a PSM code that may select the cert for you automatically.  We should hack there and not in socket transport - it's I believe a frozen IDL and putting 'clientCert' attribute on it sounds crazy.

You should put that on nsNSSSocketInfo object, best the nsISSLSocketControl interface.  The code that selects the client certificate is using ClientAuthDataRunnable, just mxr for it.

The nss info object is available on the socket after it is built, which is after we start connecting it, which is after you open input or output stream of the socket (just open it, not use it).  Let you know that ssl negotiation doesn't happen sooner you start writing/reading the socket.  So there is a well defined window under your control to setup the client certificate on nsISSLSocketControl.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Allow specifying a client cert for sockets

Honza, does this look okay?

I am trying to write a test for this, but it's been a fight against our current client auth testing infrastructure so far.  I'll keep making some more attempts while you look at the code.  Let me know how critical you feel a test is here in the mean time.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8464748 [details] [diff] [review]
Allow specifying a client cert for sockets

David, maybe you could look at this since Honza is away?

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8464748 [details] [diff] [review]
Allow specifying a client cert for sockets

Review of attachment 8464748 [details] [diff] [review]:
-----------------------------------------------------------------

This seems reasonable to me.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +198,5 @@
>  
>  NS_IMETHODIMP
> +nsNSSSocketInfo::GetClientCert(nsIX509Cert** aClientCert)
> +{
> +  *aClientCert = mClientCert;

NS_ENSURE_ARG_POINTER would probably be good

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

David, any thoughts on how I should handle testing this?  It's tricky today, as there's not a lot of testing infrastructure in place for client cert authentication.

Here's what I've found so far:

* XPCShell is likely the best fit testing framework
* I could make use of the "tlsserver"-type of tests already in security/manager
* However, XPCShell is not currently loaded with a profile that contains any client certs

Setting up XPCShell to optionally have a pre-built NSS cert DB sounds like a large amount of work at first glance.  How necessary is it to test this code path here in security/manager?  I'll later write integration tests against my use case of this, so I am not sure if it's needed here.  Alternatively, once my cert generator from bug 1038991 lands, I could use that here to generate a cert during the test.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

One thing you can do is write a script that generates a user certificate and exports it as a PKCS12 file or something. The script and the generated file can get checked in to the tree. Then, in an xpcshell test, call nsIX509CertDB.importUserCertificate on that file and use it with a tlsserver that requires a user certificate. This is more or less what we do with most of the PSM xpcshell tests.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Actually, I think it would be nsIX509CertDB.importPKCS12File, since importUserCertificate requires the private key already be in the database.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Allow specifying a client cert for sockets (v2)

David, I've added the extra pointer check you mentioned.

Additionally, there's now a test, using the approach you suggested with |importPKCS12File|.  Actually, I had found that route a few days back, but kept stumbling over all the dialogs PSM wants to show, which don't work so well in XPCShell.  After your confirmation of the approach, it seemed best to just override the dialogs as needed for test purposes, so that's where we are now.

The .p12 cert and key here is just a copy of the file /build/pgo/certs/mochitest.client.

Patrick, I'm adding a new attribute to the nsISSLSocketControl interface for setting a client cert to be used without showing a UI dialog.  Since the interface lives in netwerk/socket, I wanted to make sure someone from the Necko module was okay with this.

Try: https://tbpl.mozilla.org/?tree=Try&rev=3b13d757e0dd

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 8467444 [details] [diff] [review]
Allow specifying a client cert for sockets (v2)

Review of attachment 8467444 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with the idl change.. but you've got an android orange on the new test there, so it looks like the total patch isn't quite right

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Allow specifying a client cert for sockets (v3, mcmanus: r+)

Looks like the Android timeouts affect all tests using these custom servers.  I've disabled this one on Android as well for now.

Try: https://tbpl.mozilla.org/?tree=Try&rev=09da30f84f44

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8467894 [details] [diff] [review]
Allow specifying a client cert for sockets (v3, mcmanus: r+)

Review of attachment 8467894 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. r=me with nits addressed. In particular, I think it would be a good idea to verify on the server side that the client certificate sent is the one we're expecting.
Also, maybe rather than copying the client certificate/key from elsewhere, it would be nice to have a script to re-generate it if that's ever necessary (if you do that, you should probably add a directory test_client_cert and put the script, the pkcs12 file, and cert_dialog.js/cert_dialog.manifest there). A bash script that uses openssl should be sufficient for that (although, if you do add a generator script, I should probably have a look at it before it gets checked in).

::: security/manager/ssl/tests/unit/cert_dialog.js
@@ +1,1 @@
> +/* Any copyright is dedicated to the Public Domain.

nit: modeline
Also, we generally use MPL2 for files in this directory.

@@ +9,5 @@
> +  classID: Components.ID("{a70153f2-3590-4317-93e9-73b3e7ffca5d}"),
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsICertificateDialogs]),
> +
> +  getPKCS12FilePassword: function() {
> +    return true; // Accept empty password

maybe "// simulates entering an empty password"

::: security/manager/ssl/tests/unit/test_client_cert.js
@@ +1,1 @@
> +/* Any copyright is dedicated to the Public Domain.

nit: modeline

@@ +9,5 @@
> +  do_get_profile();
> +
> +  // Init key token (to prevent password prompt)
> +  const tokenDB = Cc["@mozilla.org/security/pk11tokendb;1"]
> +                  .getService(Ci.nsIPK11TokenDB);

nit: we usually line the second line up with the first [ here

@@ +21,5 @@
> +  do_load_manifest("cert_dialog.manifest");
> +
> +  // Load the user cert and look it up in XPCOM format
> +  const certDB = Cc["@mozilla.org/security/x509certdb;1"]
> +                 .getService(Ci.nsIX509CertDB);

same here

@@ +22,5 @@
> +
> +  // Load the user cert and look it up in XPCOM format
> +  const certDB = Cc["@mozilla.org/security/x509certdb;1"]
> +                 .getService(Ci.nsIX509CertDB);
> +  let clientCertFile = do_get_file("tlsserver/default-client.p12", false);

Do we know how this file was generated? Usually we include a script for re-generating inputs like this for test files.

@@ +36,5 @@
> +  add_connection_test("requestclientauth.example.com", Cr.NS_OK,
> +                      null, null, transport => {
> +    do_print("Setting client cert on transport");
> +    let sslSocketControl = transport.securityInfo
> +                           .QueryInterface(Ci.nsISSLSocketControl);

nit: indent this line two spaces or so it lines up with .securityInfo

::: security/manager/ssl/tests/unit/tlsserver/cmd/ClientAuthServer.cpp
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

nit: modeline

@@ +38,5 @@
> +SECStatus
> +AuthCertificateHook(void* arg, PRFileDesc* fd, PRBool checkSig,
> +                    PRBool isServer)
> +{
> +  // Allow any client cert

It would be nice to check that the certificate received here is what it was set to in the test. An easy way to do this might be to get the peer certificate (SSL_PeerCertificate I think) and check that its sha256 fingerprint matches an expected value (and it would be good to document in the test that if the client certificate changes, the hash has to be updated).

@@ +43,5 @@
> +  return SECSuccess;
> +}
> +
> +int32_t
> +DoSNISocketConfig(PRFileDesc *aFd, const SECItem *aSrvNameArr,

nit: PRFileDesc*, etc.

@@ +56,5 @@
> +  if (gDebugLevel >= DEBUG_VERBOSE) {
> +    fprintf(stderr, "found pre-defined host '%s'\n", host->mHostName);
> +  }
> +
> +  if (SECSuccess != ConfigSecureServerWithNamedCert(aFd, DEFAULT_CERT_NICKNAME,

nit: usually the style is:

if (ConfigSecure...(...) != SECSucess) {
...

I realize it probably wraps in an ugly way here, though.

@@ +69,5 @@
> +    SSL_OptionSet(aFd, SSL_REQUIRE_CERTIFICATE, SSL_REQUIRE_NEVER);
> +  }
> +
> +  // Override default client auth hook to allow any cert
> +  if (SECSuccess != SSL_AuthCertificateHook(aFd, AuthCertificateHook,

same

@@ +78,5 @@
> +  return 0;
> +}
> +
> +int
> +main(int argc, char *argv[])

nit: char* argv[]

@@ +87,5 @@
> +  }
> +
> +  return StartServer(argv[1], DoSNISocketConfig, nullptr);
> +}
> +

nit: unnecessary trailing blank line

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Allow specifying a client cert for sockets (v4, mcmanus: r+)

I've resolved all the nits you mentioned.

I am now generating my own cert as suggested.  Basically I am just passing defaults into the CertUtils helper functions, which seems like it's good enough here.  Since I've added a generation script, flagging you for review again.

Also, the test server validates the SHA-256 of the cert.

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=e0cf188fd290

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8468959 [details] [diff] [review]
Allow specifying a client cert for sockets (v4, mcmanus: r+)

Review of attachment 8468959 [details] [diff] [review]:
-----------------------------------------------------------------

Great! r=me with comments addressed.

::: security/manager/ssl/tests/unit/test_client_cert/generate.py
@@ +1,1 @@
> +#!/usr/bin/python

nit: modeline and license boilerplate

@@ +13,5 @@
> +dest_dir = os.getcwd()
> +db = tempfile.mkdtemp()
> +
> +serial = random.randint(100, 40000000)
> +prefix = "client-cert"

nit: maybe "name" instead of "prefix"?

@@ +16,5 @@
> +serial = random.randint(100, 40000000)
> +prefix = "client-cert"
> +[key, cert] = CertUtils.generate_cert_generic(db, dest_dir, serial, "rsa",
> +                                              prefix, "")
> +CertUtils.generate_pkcs12(db, dest_dir, cert, key, prefix)

We don't need the resulting client-cert.der file, do we? It would be nice to remove it after outputting client-cert.p12 so we don't accidentally check it in.

::: security/manager/ssl/tests/unit/tlsserver/cmd/ClientAuthServer.cpp
@@ +60,5 @@
> +  if (rv != SECSuccess) {
> +    return rv;
> +  }
> +
> +  bool match = !memcmp(certFingerprint, sClientCertFingerprint,

can we static_assert(sizeof(sClientCertFingerprint) == SHA256_LENGTH)?

@@ +92,5 @@
> +  } else {
> +    SSL_OptionSet(aFd, SSL_REQUIRE_CERTIFICATE, SSL_REQUIRE_NEVER);
> +  }
> +
> +  // Override default client auth hook to allow any cert

nit: update comment

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Allow specifying a client cert for sockets (v5, keeler: r+, mcmanus: r+)

All review comments addressed.

Sorry for the repeated modeline / license nits...  That's what I get for copying old files. :)

Try: https://tbpl.mozilla.org/?tree=Try&rev=d9c795a70b42

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04500acd1bb7

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/04500acd1bb7