[go] crypto/x509: populate Number and AKI of parsed CRLs
Aaron Gable (Gerrit)
Aaron Gable has uploaded this change for review.
crypto/x509: populate Number and AKI of parsed CRLs
The `x509.RevocationList` type has two fields which correspond to
extensions, rather than native fields, of the underlying ASN.1 CRL:
the `.Number` field corresponds to the `crlNumber` extension, and
the `.AuthorityKeyId` field corresponds to the `authorityKeyIdentifier`
extension.
The `x509.CreateRevocationList()` function uses these fields to
populate their respective extensions in the resulting CRL. However,
The `x509.ParseRevocationList()` function does not perform the
reverse operation: the fields retain their zero-values even after
parsing a CRL which contains the relevant extensions.
Add code which populates these fields when parsing their extensions.
Add assertions to the existing tests to confirm that the values are
populated appropriate.
Fixes #53726
Change-Id: Ie5b71081e53034e0b5b9ff3c122065c62f15cf23
---
M src/crypto/x509/parser.go
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
3 files changed, 55 insertions(+), 6 deletions(-)
diff --git a/src/crypto/x509/parser.go b/src/crypto/x509/parser.go
index e0e8f61..49720d5 100644
--- a/src/crypto/x509/parser.go
+++ b/src/crypto/x509/parser.go
@@ -1008,22 +1008,22 @@
// we can populate RevocationList.Raw, before unwrapping the
// SEQUENCE so it can be operated on
if !input.ReadASN1Element(&input, cryptobyte_asn1.SEQUENCE) {
- return nil, errors.New("x509: malformed certificate")
+ return nil, errors.New("x509: malformed crl")
}
rl.Raw = input
if !input.ReadASN1(&input, cryptobyte_asn1.SEQUENCE) {
- return nil, errors.New("x509: malformed certificate")
+ return nil, errors.New("x509: malformed crl")
}
var tbs cryptobyte.String
// do the same trick again as above to extract the raw
// bytes for Certificate.RawTBSCertificate
if !input.ReadASN1Element(&tbs, cryptobyte_asn1.SEQUENCE) {
- return nil, errors.New("x509: malformed tbs certificate")
+ return nil, errors.New("x509: malformed tbs crl")
}
rl.RawTBSRevocationList = tbs
if !tbs.ReadASN1(&tbs, cryptobyte_asn1.SEQUENCE) {
- return nil, errors.New("x509: malformed tbs certificate")
+ return nil, errors.New("x509: malformed tbs crl")
}
var version int
@@ -1148,6 +1148,16 @@
if err != nil {
return nil, err
}
+ if ext.Id.Equal(oidExtensionAuthorityKeyId) {
+ rl.AuthorityKeyId = ext.Value
+ } else if ext.Id.Equal(oidExtensionCRLNumber) {
+ number := new(big.Int)
+ value := cryptobyte.String(ext.Value)
+ if !value.ReadASN1Integer(number) {
+ return nil, errors.New("x509: malformed crl number")
+ }
+ rl.Number = number
+ }
rl.Extensions = append(rl.Extensions, ext)
}
}
diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go
index 87eb1f7..7dcebfa 100644
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -2109,7 +2109,9 @@
// Issuer contains the DN of the issuing certificate.
Issuer pkix.Name
// AuthorityKeyId is used to identify the public key associated with the
- // issuing certificate.
+ // issuing certificate. It is populated from the authorityKeyIdentifier
+ // extension when parsing a CRL. It is ignored when creating a CRL; the
+ // extension is populated from the issuing certificate itself.
AuthorityKeyId []byte
Signature []byte
@@ -2125,7 +2127,8 @@
// Number is used to populate the X.509 v2 cRLNumber extension in the CRL,
// which should be a monotonically increasing sequence number for a given
- // CRL scope and CRL issuer.
+ // CRL scope and CRL issuer. It is also populated from the cRLNumber
+ // extension when parsing a CRL.
Number *big.Int
// ThisUpdate is used to populate the thisUpdate field in the CRL, which
diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index 8ef6115..c0a555a 100644
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -2681,6 +2681,15 @@
t.Fatalf("Extensions mismatch: got %v; want %v.",
parsedCRL.Extensions[2:], tc.template.ExtraExtensions)
}
+
+ if tc.template.Number != nil && tc.template.Number.String() != parsedCRL.Number.String() {
+ t.Fatalf("Generated CRL has wrong Number: got %s, want %s",
+ parsedCRL.Number.String(), tc.template.Number.String())
+ }
+ if !bytes.Equal(parsedCRL.AuthorityKeyId, expectedAKI) {
+ t.Fatalf("Generated CRL has wrong Number: got %x, want %x",
+ parsedCRL.AuthorityKeyId, expectedAKI)
+ }
})
}
}
To view, visit change 416354. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Attention is currently required from: Aaron Gable, Filippo Valsorda.
Patch set 1:Code-Review +2
5 comments:
Commit Message:
Patch Set #1, Line 9:
The `x509.RevocationList` type has two fields which correspond tonit: markdown in these messages (basically) never gets rendered, stripping it out makes them slightly easier to read.
Patch Set #1, Line 23:
appropriateappropriately
File src/crypto/x509/parser.go:
Patch Set #1, Line 1154:
number := new(big.Int)nit: you can just use `rl.Number` here and avoid the need for a second assignment below.
File src/crypto/x509/x509_test.go:
Patch Set #1, Line 2685:
tc.template.Number.String() != parsedCRL.Number.String()`tc.template.Number.Cmp(parsedCRl.Number) != 0`
Patch Set #1, Line 2685:
tc.template.Number != nilThere should probably also be an earlier test for `tc.template.Number != nil && parsedCRL.Number == nil` so the test doesn't panic if there is a regression and the number encoding/parsing breaks.
To view, visit change 416354. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Gable (Gerrit)
Attention is currently required from: Aaron Gable, Filippo Valsorda.
Aaron Gable uploaded patch set #2 to this change.
crypto/x509: populate Number and AKI of parsed CRLs
The x509.RevocationList type has two fields which correspond to
extensions, rather than native fields, of the underlying ASN.1 CRL:
the .Number field corresponds to the crlNumber extension, and
the .AuthorityKeyId field corresponds to the authorityKeyIdentifier
extension.
The x509.CreateRevocationList() function uses these fields to populate
their respective extensions in the resulting CRL. However, the
x509.ParseRevocationList() function does not perform the reverse
operation: the fields retain their zero-values even after parsing a CRL
which contains the relevant extensions.
Add code which populates these fields when parsing their extensions.
Add assertions to the existing tests to confirm that the values are
populated appropriately.
Fixes #53726
Change-Id: Ie5b71081e53034e0b5b9ff3c122065c62f15cf23
---
M src/crypto/x509/parser.go
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
3 files changed, 58 insertions(+), 6 deletions(-)
To view, visit change 416354. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Gable (Gerrit)
Attention is currently required from: Filippo Valsorda.
5 comments:
Commit Message:
Patch Set #1, Line 9:
The `x509.RevocationList` type has two fields which correspond to
nit: markdown in these messages (basically) never gets rendered, stripping it out makes them slightl […]
oh NO I've been fully corrupted by github
Patch Set #1, Line 23:
appropriateappropriately
Done
File src/crypto/x509/parser.go:
Patch Set #1, Line 1154:
number := new(big.Int)nit: you can just use `rl.Number` here and avoid the need for a second assignment below.
Done
File src/crypto/x509/x509_test.go:
Patch Set #1, Line 2685:
tc.template.Number != nilThere should probably also be an earlier test for `tc.template.Number != nil && parsedCRL. […]
Done
Patch Set #1, Line 2685:
tc.template.Number.String() != parsedCRL.Number.String()
`tc.template.Number.Cmp(parsedCRl. […]
Done
To view, visit change 416354. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Attention is currently required from: Aaron Gable, Filippo Valsorda.
Patch set 2:Run-TryBot +1Auto-Submit +1Code-Review +2
2 comments:
Commit Message:
Patch Set #1, Line 9:
The `x509.RevocationList` type has two fields which correspond to
oh NO I've been fully corrupted by github
😞
Patchset:
Thanks for the patch! Glad to know someone will actually use this functionality 😄.
To view, visit change 416354. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Gable (Gerrit)
Attention is currently required from: Filippo Valsorda.
1 comment:
Patchset:
Thanks for the patch! Glad to know someone will actually use this functionality 😄.
You have no idea how excited I was when I saw that the CRL functionality had been switched to cryptobyte just as we started needing to use it! Thanks for making my life so much easier :D
To view, visit change 416354. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Aaron Gable, Filippo Valsorda.
Patch set 2:Code-Review +1
Gopher Robot (Gerrit)
Gopher Robot submitted this change.
crypto/x509: populate Number and AKI of parsed CRLs
The x509.RevocationList type has two fields which correspond to
extensions, rather than native fields, of the underlying ASN.1 CRL:
the .Number field corresponds to the crlNumber extension, and
the .AuthorityKeyId field corresponds to the authorityKeyIdentifier
extension.
The x509.CreateRevocationList() function uses these fields to populate
their respective extensions in the resulting CRL. However, the
x509.ParseRevocationList() function does not perform the reverse
operation: the fields retain their zero-values even after parsing a CRL
which contains the relevant extensions.
Add code which populates these fields when parsing their extensions.
Add assertions to the existing tests to confirm that the values are
populated appropriately.
Fixes #53726
Change-Id: Ie5b71081e53034e0b5b9ff3c122065c62f15cf23
Reviewed-on: https://go-review.googlesource.com/c/go/+/416354
Run-TryBot: Roland Shoemaker <rol...@golang.org>
Auto-Submit: Roland Shoemaker <rol...@golang.org>
Reviewed-by: Roland Shoemaker <rol...@golang.org>
TryBot-Result: Gopher Robot <go...@golang.org>
Reviewed-by: Damien Neil <dn...@google.com>
---
M src/crypto/x509/parser.go
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
3 files changed, 64 insertions(+), 6 deletions(-)
diff --git a/src/crypto/x509/parser.go b/src/crypto/x509/parser.go
index e0e8f61..cd87044 100644
--- a/src/crypto/x509/parser.go
+++ b/src/crypto/x509/parser.go
@@ -1008,22 +1008,22 @@
// we can populate RevocationList.Raw, before unwrapping the
// SEQUENCE so it can be operated on
if !input.ReadASN1Element(&input, cryptobyte_asn1.SEQUENCE) {
- return nil, errors.New("x509: malformed certificate")
+ return nil, errors.New("x509: malformed crl")
}
rl.Raw = input
if !input.ReadASN1(&input, cryptobyte_asn1.SEQUENCE) {
- return nil, errors.New("x509: malformed certificate")
+ return nil, errors.New("x509: malformed crl")
}
var tbs cryptobyte.String
// do the same trick again as above to extract the raw
// bytes for Certificate.RawTBSCertificate
if !input.ReadASN1Element(&tbs, cryptobyte_asn1.SEQUENCE) {
- return nil, errors.New("x509: malformed tbs certificate")
+ return nil, errors.New("x509: malformed tbs crl")
}
rl.RawTBSRevocationList = tbs
if !tbs.ReadASN1(&tbs, cryptobyte_asn1.SEQUENCE) {
- return nil, errors.New("x509: malformed tbs certificate")
+ return nil, errors.New("x509: malformed tbs crl")
}
var version int
@@ -1148,6 +1148,15 @@
if err != nil {
return nil, err
}
+ if ext.Id.Equal(oidExtensionAuthorityKeyId) {
+ rl.AuthorityKeyId = ext.Value
+ } else if ext.Id.Equal(oidExtensionCRLNumber) {
+ value := cryptobyte.String(ext.Value)
+ rl.Number = new(big.Int)
+ if !value.ReadASN1Integer(rl.Number) {
+ return nil, errors.New("x509: malformed crl number")
+ }
+ }
rl.Extensions = append(rl.Extensions, ext)
}
}
diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go
index 87eb1f7..7dcebfa 100644
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -2109,7 +2109,9 @@
// Issuer contains the DN of the issuing certificate.
Issuer pkix.Name
// AuthorityKeyId is used to identify the public key associated with the
- // issuing certificate.
+ // issuing certificate. It is populated from the authorityKeyIdentifier
+ // extension when parsing a CRL. It is ignored when creating a CRL; the
+ // extension is populated from the issuing certificate itself.
AuthorityKeyId []byte
Signature []byte
@@ -2125,7 +2127,8 @@
// Number is used to populate the X.509 v2 cRLNumber extension in the CRL,
// which should be a monotonically increasing sequence number for a given
- // CRL scope and CRL issuer.
+ // CRL scope and CRL issuer. It is also populated from the cRLNumber
+ // extension when parsing a CRL.
Number *big.Int
// ThisUpdate is used to populate the thisUpdate field in the CRL, which
diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index 8ef6115..594ee1d 100644
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -2681,6 +2681,19 @@
t.Fatalf("Extensions mismatch: got %v; want %v.",
parsedCRL.Extensions[2:], tc.template.ExtraExtensions)
}
+
+ if tc.template.Number != nil && parsedCRL.Number == nil {
+ t.Fatalf("Generated CRL missing Number: got nil, want %s",
+ tc.template.Number.String())
+ }
+ if tc.template.Number != nil && tc.template.Number.Cmp(parsedCRL.Number) != 0 {
+ t.Fatalf("Generated CRL has wrong Number: got %s, want %s",
+ parsedCRL.Number.String(), tc.template.Number.String())
+ }
+ if !bytes.Equal(parsedCRL.AuthorityKeyId, expectedAKI) {
+ t.Fatalf("Generated CRL has wrong Number: got %x, want %x",
+ parsedCRL.AuthorityKeyId, expectedAKI)
+ }
})
}
}
To view, visit change 416354. To unsubscribe, or for help writing mail filters, visit settings.