crypto/x509: continue to recognise MaxPathLen of zero as "no value".

In [1] the behaviour of encoding/asn1 with respect to marshaling
optional integers was changed. Previously, a zero valued integer would
be omitted when marshaling. After the change, if a default value was
set then the integer would only be omitted if it was the default value.

This changed the behaviour of crypto/x509 because
Certificate.MaxPathLen has a default value of -1 and thus zero valued
MaxPathLens would no longer be omitted when marshaling. This is
arguably a bug-fix -- a value of zero for MaxPathLen is valid and
meaningful and now could be expressed. However it broke users
(including Docker) who were not setting MaxPathLen at all.

This change again causes a zero-valued MaxPathLen to be omitted and
introduces a ZeroMathPathLen member that indicates that, yes, one
really does want a zero. This is ugly, but we value not breaking users.

[1] https://code.google.com/p/go/source/detail?r=4218b3544610e8d9771b89126553177e32687adf

LGTM=rsc
R=rsc
CC=golang-codereviews, golang-dev
https://golang.org/cl/153420045

diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go
index 6e57e913ac27a1c8ef54f74209bae36e983a55ac..69a62e57d2193673dd95d85e8c91ee459287d303 100644 (file)
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -494,6 +494,11 @@ type Certificate struct {
        BasicConstraintsValid bool // if true then the next two fields are valid.
        IsCA                  bool
        MaxPathLen            int
+       // MaxPathLenZero indicates that BasicConstraintsValid==true and
+       // MaxPathLen==0 should be interpreted as an actual maximum path length
+       // of zero. Otherwise, that combination is interpreted as MaxPathLen
+       // not being set.
+       MaxPathLenZero bool
 
        SubjectKeyId   []byte
        AuthorityKeyId []byte
@@ -913,6 +918,7 @@ func parseCertificate(in *certificate) (*Certificate, error) {
                                        out.BasicConstraintsValid = true
                                        out.IsCA = constraints.IsCA
                                        out.MaxPathLen = constraints.MaxPathLen
+                                       out.MaxPathLenZero = out.MaxPathLen == 0
                                        continue
                                }
                        case 17:
@@ -1227,8 +1233,15 @@ func buildExtensions(template *Certificate) (ret []pkix.Extension, err error) {
        }
 
        if template.BasicConstraintsValid && !oidInExtensions(oidExtensionBasicConstraints, template.ExtraExtensions) {
+               // Leaving MaxPathLen as zero indicates that no maximum path
+               // length is desired, unless MaxPathLenZero is set. A value of
+               // -1 causes encoding/asn1 to omit the value as desired.
+               maxPathLen := template.MaxPathLen
+               if maxPathLen == 0 && !template.MaxPathLenZero {
+                       maxPathLen = -1
+               }
                ret[n].Id = oidExtensionBasicConstraints
-               ret[n].Value, err = asn1.Marshal(basicConstraints{template.IsCA, template.MaxPathLen})
+               ret[n].Value, err = asn1.Marshal(basicConstraints{template.IsCA, maxPathLen})
                ret[n].Critical = true
                if err != nil {
                        return

diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index abe86216f9c8b787a4e038594e33a9b6609e3487..4f5173fb5dd27bbcb8589207be04dc1479d2217c 100644 (file)
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -953,6 +953,69 @@ func TestParseCertificateRequest(t *testing.T) {
        }
 }
 
+func TestMaxPathLen(t *testing.T) {
+       block, _ := pem.Decode([]byte(pemPrivateKey))
+       rsaPriv, err := ParsePKCS1PrivateKey(block.Bytes)
+       if err != nil {
+               t.Fatalf("Failed to parse private key: %s", err)
+       }
+
+       template := &Certificate{
+               SerialNumber: big.NewInt(1),
+               Subject: pkix.Name{
+                       CommonName: "Σ Acme Co",
+               },
+               NotBefore: time.Unix(1000, 0),
+               NotAfter:  time.Unix(100000, 0),
+
+               BasicConstraintsValid: true,
+               IsCA: true,
+       }
+
+       serialiseAndParse := func(template *Certificate) *Certificate {
+               derBytes, err := CreateCertificate(rand.Reader, template, template, &rsaPriv.PublicKey, rsaPriv)
+               if err != nil {
+                       t.Fatalf("failed to create certificate: %s", err)
+                       return nil
+               }
+
+               cert, err := ParseCertificate(derBytes)
+               if err != nil {
+                       t.Fatalf("failed to parse certificate: %s", err)
+                       return nil
+               }
+
+               return cert
+       }
+
+       cert1 := serialiseAndParse(template)
+       if m := cert1.MaxPathLen; m != -1 {
+               t.Errorf("Omitting MaxPathLen didn't turn into -1, got %d", m)
+       }
+       if cert1.MaxPathLenZero {
+               t.Errorf("Omitting MaxPathLen resulted in MaxPathLenZero")
+       }
+
+       template.MaxPathLen = 1
+       cert2 := serialiseAndParse(template)
+       if m := cert2.MaxPathLen; m != 1 {
+               t.Errorf("Setting MaxPathLen didn't work. Got %d but set 1", m)
+       }
+       if cert2.MaxPathLenZero {
+               t.Errorf("Setting MaxPathLen resulted in MaxPathLenZero")
+       }
+
+       template.MaxPathLen = 0
+       template.MaxPathLenZero = true
+       cert3 := serialiseAndParse(template)
+       if m := cert3.MaxPathLen; m != 0 {
+               t.Errorf("Setting MaxPathLenZero didn't work, got %d", m)
+       }
+       if !cert3.MaxPathLenZero {
+               t.Errorf("Setting MaxPathLen to zero didn't result in MaxPathLenZero")
+       }
+}
+
 // This CSR was generated with OpenSSL:
 //  openssl req -out CSR.csr -new -newkey rsa:2048 -nodes -keyout privateKey.key -config openssl.cnf
 //