X509KeyPair (crypto/tls): On success, why not set tls.Certificate.Leaf before returning?

[ram...@yahoo.com]

Here's the relevant code snippet in 1.5.3 (https://golang.org/src/crypto/tls/tls.go?s=5139:5210#L214):

   214		cert.PrivateKey, err = parsePrivateKey(keyDERBlock.Bytes)
   215		if err != nil {
   216			return fail(err)
   217		}
   218	
   219		// We don't need to parse the public key for TLS, but we so do anyway
   220		// to check that it looks sane and matches the private key.
   221		x509Cert, err := x509.ParseCertificate(cert.Certificate[0])
   222		if err != nil {
   223			return fail(err)
   224		}

I definitely see the comment on Lines 219-220, but on Line 221, but why not just set the result of x509.ParseCertificate to cert.Leaf instead of using x509Cert if it's going to be parsed regardless?

This still seems to be the case in the latest source: https://github.com/golang/go/blob/master/src/crypto/tls/tls.go

And this seems to have been the case ever since the initial commit back in July 2010: https://github.com/golang/go/blob/fc23def67f4c24fe295c4e389e584d244eee1530/src/pkg/crypto/tls/tls.go

I was unable to find any comments on the topic in the the initial code review: https://codereview.appspot.com/1684051/diff/1/src/pkg/crypto/tls/tls.go

BACKGROUND: I'm a noob to GO overall, so doubly so when it comes to GO's crypto.  I needed to load a certificate and its corresponding private key from files on disk.  In my program, I was actually calling LoadX509Pair, which calls X509KeyPair.  After calling it, I was trying to access the Leaf.Subject and Leaf.Issuer from the returned cert object.  I was always checking for errors after calling LoadX509KeyPair, but initially, I was not checking for the Leaf to be nil if successful.  It took me a while to figure out why Leaf was nil even though there were no errors being returned from LoadX509Pair / X509KeyPair. I ended up doing all sorts of troubleshooting (in my environment, with my cert files, etc.) before I dug into the source and finally realized that Leaf is simply never set on success.

Now that I know this, obviously, I can parse it again on my own after making a successful call to LoadX509Pair, but given the names and descriptions of both the LoadX509Pair / X509KeyPair functions, I was definitely expecting Leaf to be set upon success.

If this behavior cannot be changed, then perhaps the doc could be clarified to explicitly state that Certificate.Leaf will always be nil following this function.  That definitely would have saved me some time.

Regards,

Omar

[Adam Langley]

On Sun, Jan 17, 2016 at 3:36 AM, ramoas via golang-dev
<golan...@googlegroups.com> wrote:
> If this behavior cannot be changed, then perhaps the doc could be clarified
> to explicitly state that Certificate.Leaf will always be nil following this
> function. That definitely would have saved me some time.

This is a fair point and I can, at least, explain some of the thinking here:

The Leaf member is only needed for clients doing
client-authentication. This is rare compared to the common case of
loading certificates for serving. In the latter case, the parsed form
isn't needed because the server just sends the blob to the client and
doesn't generally care what's in it.

If X509KeyPair retained the parsed form then I worry that servers that
are loading large numbers of certificates would suddenly have much
greater memory usage because the parsed form of the certs, which is
useless to them, would be kept around.

I've created https://go-review.googlesource.com/#/c/18734/ about this.


Cheers

AGL

[Russ Cox]

There was an issue filed about this during the 1.6 cycle, which we closed for much the same reasons. I can't find it right now, but thanks for adding the comments. I think that will help avoid the issue coming up again.

Russ

--
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Omar]

Thanks for creating the issue to clarify the doc.

Some more related background / general comments / enhancement ideas:

- Interacting with certificates and keys stored in files is quite common, and there are lots of use cases for certs / keys besides TLS.  For example, in my use case, I actually did not need TLS in this particular module -- I was just doing some certificate / key validation. Therefore, I originally went looking for and was expecting to find these utility "load from file"-type functions in crypto/x509, not crypto/tls.  Perhaps it makes sense to re-factor / re-layer / re-abstract some of this file-based functionality into crypto/x509 to conveniently open up more general use cases and where the parsed status of any certs / keys would always be explicit and/or self-documenting.  In doing so, crypto/tls could continue to preserve the lowest memory footprint assuming the more common server-side TLS use case.

- Along the lines of LoadX509KeyPair, in crypto/x509, it would also be very convenient if "to/from file" functionality ("Load" / "Store") could be extended to certs and all the public/private key types (PKCS1, PKCS8, EC, PKIX, etc.).  Not having to lookup the correct PEM-block type or write the file-wrapper functions (or invoke openssl commands) means less mistakes, less duplicate code, and just less time spent.  This would also definitely help developers coming from other, older languages (e.g., Java, .NET) where such utility functions have become part of their built-in crypto libraries.

I understand resources are limited, and the crypto investments put into GO so far have been the minimum needed to support more common / popular like TLS 1.2.  But, hopefully, further investment, especially on the convenience front, is not completely off the table or at the very bottom of the priority stack.

I'm not a cryptographer, but I'm happy to contribute however I can.

Thanks again for all your efforts.

Regards,

Omar