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