code review 149770044: crypto/x509: accept CRLs without an expiry.

crypto/x509: accept CRLs without an expiry.

RFC5280 says that the nextUpdate field is optional.

Fixes issue 8085.

[agl1, 2014-09-19 22:28:51]

Hello bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2014-09-21 02:17:27]

What's with the Raw addition?

We're in the Go 1.4 freeze now. Does this fix an open bug? If not, you
might need to create a bug. Is it worth fixing before 1.4?


On Fri, Sep 19, 2014 at 6:28 PM, <agl@golang.org> wrote:

> Reviewers: bradfitz,
>
> Message:
> Hello bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> crypto/x509: accept CRLs without an expiry.
>
> RFC5280 says that the nextUpdate field is optional.
>
> Please review this at https://codereview.appspot.com/149770044/
>
> Affected files (+13, -1 lines):
>   M src/crypto/x509/pkix/pkix.go
>   M src/crypto/x509/x509_test.go
>
>
> Index: src/crypto/x509/pkix/pkix.go
> ===================================================================
> --- a/src/crypto/x509/pkix/pkix.go
> +++ b/src/crypto/x509/pkix/pkix.go
> @@ -15,6 +15,7 @@
>  // AlgorithmIdentifier represents the ASN.1 structure of the same name.
> See RFC
>  // 5280, section 4.1.1.2.
>  type AlgorithmIdentifier struct {
> +       Raw        asn1.RawContent
>         Algorithm  asn1.ObjectIdentifier
>         Parameters asn1.RawValue `asn1:"optional"`
>  }
> @@ -164,7 +165,7 @@
>         Signature           AlgorithmIdentifier
>         Issuer              RDNSequence
>         ThisUpdate          time.Time
> -       NextUpdate          time.Time
> +       NextUpdate          time.Time            `asn1:"optional"`
>         RevokedCertificates []RevokedCertificate `asn1:"optional"`
>         Extensions          []Extension          `asn1:"tag:0,optional,
> explicit"`
>  }
> Index: src/crypto/x509/x509_test.go
> ===================================================================
> --- a/src/crypto/x509/x509_test.go
> +++ b/src/crypto/x509/x509_test.go
> @@ -707,6 +707,17 @@
>         // Can't check the signature here without a package cycle.
>  }
>
> +func TestCRLWithoutExpiry(t *testing.T) {
> +       derBytes := fromBase64("MIHYMIGZMAkGByqGSM44BAMwEjEQMA
> 4GA1UEAxMHQ2FybERTUxcNOTkwODI3MDcwMDAwWjBpMBMCAgDIFw05OTA4Mj
> IwNzAwMDBaMBMCAgDJFw05OTA4MjIwNzAwMDBaMBMCAgDTFw05OTA4MjIwNz
> AwMDBaMBMCAgDSFw05OTA4MjIwNzAwMDBaMBMCAgDUFw05OTA4MjQwNzAwMD
> BaMAkGByqGSM44BAMDLwAwLAIUfmVSdjP+NHMX0feW+aDU2G1cfT0CFAJ6W7fVWxjBz4fvfto
> k8yqDnDWh")
> +       certList, err := ParseDERCRL(derBytes)
> +       if err != nil {
> +               t.Fatal(err)
> +       }
> +       if !certList.TBSCertList.NextUpdate.IsZero() {
> +               t.Errorf("NextUpdate is not the zero value")
> +       }
> +}
> +
>  func TestParsePEMCRL(t *testing.T) {
>         pemBytes := fromBase64(pemCRLBase64)
>         certList, err := ParseCRL(pemBytes)
>
>
>

[agl1, 2014-09-22 22:02:18]

On 2014/09/21 02:17:27, bradfitz wrote:
> What's with the Raw addition?

Gah, thanks. A leftover from debugging something else.

> We're in the Go 1.4 freeze now. Does this fix an open bug? If not, you
> might need to create a bug. Is it worth fixing before 1.4?

Yes, fixes issue 8085. (I only remembered to add that after sending out the
email.)

How frozen are things at the moment?

[bradfitz, 2014-09-22 22:15:45]

I think this is fine if it fixes a bug and the Raw addition was just
debugging.


On Mon, Sep 22, 2014 at 6:02 PM, <agl@golang.org> wrote:

> On 2014/09/21 02:17:27, bradfitz wrote:
>
>> What's with the Raw addition?
>>
>
> Gah, thanks. A leftover from debugging something else.
>
>  We're in the Go 1.4 freeze now. Does this fix an open bug? If not, you
>> might need to create a bug. Is it worth fixing before 1.4?
>>
>
> Yes, fixes issue 8085. (I only remembered to add that after sending out
> the email.)
>
> How frozen are things at the moment?
>
> https://codereview.appspot.com/149770044/
>

[agl1, 2014-09-29 19:26:58]

*** Submitted as https://code.google.com/p/go/source/detail?r=2080ff7fb64e ***

crypto/x509: accept CRLs without an expiry.

RFC5280 says that the nextUpdate field is optional.

Fixes issue 8085.

R=bradfitz
CC=golang-codereviews
https://codereview.appspot.com/149770044