code review 49830048: crypto/x509: Add support for parsing and serialisation ...

crypto/x509: Add support for parsing and serialisation of PKCS #10 certificate signature requests.

[gokyle, 2014-01-31 22:12:40]

Hello golang-codereviews@googlegroups.com (cc: agl@chromium.org, nick@cloudflare.com), I'd like you to review this change to https://code.google.com/p/go

[agl1, 2014-02-03 16:07:00]

https://codereview.appspot.com/49830048/diff/30004/src/pkg/crypto/x509/pkix/p... File src/pkg/crypto/x509/pkix/pkix.go (right): https://codereview.appspot.com/49830048/diff/30004/src/pkg/crypto/x509/pkix/p... src/pkg/crypto/x509/pkix/pkix.go:54: SerialNumber string `asn1:"omitempty"` // This field should not appear in certificate requests. This structure is not pass to the ASN.1 package I think, so the asn1 tags don't do anything. https://codereview.appspot.com/49830048/diff/30004/src/pkg/crypto/x509/x509.go File src/pkg/crypto/x509/x509.go (right): https://codereview.appspot.com/49830048/diff/30004/src/pkg/crypto/x509/x509.g... src/pkg/crypto/x509/x509.go:1577: // Certificate signature request information. This comment is placed as if it's remarking on "Subject", but it's not. I think I'd just delete it. https://codereview.appspot.com/49830048/diff/30004/src/pkg/crypto/x509/x509.g... src/pkg/crypto/x509/x509.go:1630: switch priv := priv.(type) { this is a big copy-and-paste from the code above, suggesting that it should be its own function. https://codereview.appspot.com/49830048/diff/30004/src/pkg/crypto/x509/x509.g... src/pkg/crypto/x509/x509.go:1685: extensions[0][0].Type = oidExtensionSubjectAltName copy and paste code again.

[gokyle, 2014-02-08 08:20:21]

I've got a fix ready, but I keep running into problems with codereview.py. Once I get these sorted out, I'll be able to submit them.

[gokyle, 2014-02-08 08:27:14]

Hello golang-codereviews@googlegroups.com, agl@golang.org (cc: agl@chromium.org, golang-codereviews@googlegroups.com, nick@cloudflare.com), Please take another look.

[agl1, 2014-02-13 17:53:27]

I didn't feel that this was sufficiently close that it could be kicked into shape via the review process. So I worked on it locally. It's not possible for others to upload diffs I'm afraid so I'll submit it with my changes. Could you review the result? I dropped one function (see below), but that can be changed. https://codereview.appspot.com/49830048/diff/70001/src/pkg/crypto/x509/x509.go File src/pkg/crypto/x509/x509.go (right): https://codereview.appspot.com/49830048/diff/70001/src/pkg/crypto/x509/x509.g... src/pkg/crypto/x509/x509.go:1756: func ParseCertificateRequests(asn1Data []byte) ([]*CertificateRequest, error) { I dropped this function because exposed API has a high bar. Do concatenated, DER CSRs actually happen? Happy to add it back if there's a clear use.

[agl1, 2014-02-13 17:54:00]

LGTM (in order to keep the scripts happy)

[agl1, 2014-02-13 17:54:17]

*** Submitted as https://code.google.com/p/go/source/detail?r=3ac4616250ed *** crypto/x509: Add certificate signature request (CSR) support. This change adds support for parsing and serialisation of PKCS #10, certificate signature requests. LGTM=agl R=golang-codereviews, agl CC=agl, golang-codereviews, nick https://codereview.appspot.com/49830048 Committer: Adam Langley <agl@golang.org>

[gobot, 2014-02-13 17:58:12]

This CL appears to have broken the linux-amd64 builder.

[agl1, 2014-02-13 18:04:44]

On Thu, Feb 13, 2014 at 12:58 PM, <gobot@golang.org> wrote: > This CL appears to have broken the linux-amd64 builder. Nope, GC crashed :( ALG