[go] crypto/x509: add new CRL parser, deprecate old one
475 views
Skip to first unread message
Roland Shoemaker (Gerrit)
Mar 8, 2022, 2:48:20 PM3/8/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Roland Shoemaker has uploaded this change for review.
crypto/x509: add new CRL parser, deprecate old one
Adds a new, cryptobyte based, CRL parser, which returns a
x509.RevocaitonList, rather than a pkix.CertificateList. This allows us
to return much more detailed information, as well as leaving open the
option of adding further information since RevocationList is not a
direct ASN.1 representation like pkix.CertificateList. Additionally
a new method is added to RevocationList, CheckSignatureFrom, which is
analogous to the method with the same name on Certificate, which
properly checks that the signature is from an issuing certiifcate.
This change also deprecates a number of older CRL related functions and
types, which have been replaced with the new functionality introduced
in this change:
* crypto/x509.ParseCRL
* crypto/x509.ParseDERCRL
* crypto/x509.CheckCRLSignature
* crypto/x509/pkix.CertificateList
* crypto/x509/pkix.TBSCertificateList
Fixes #50674
Change-Id: I27dc219e39bef09a396e666b4fccaa32578fd913
---
M src/crypto/x509/parser.go
M src/crypto/x509/pkix/pkix.go
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
4 files changed, 374 insertions(+), 55 deletions(-)
diff --git a/src/crypto/x509/parser.go b/src/crypto/x509/parser.go
index bb60cea..81e7bb6 100644
--- a/src/crypto/x509/parser.go
+++ b/src/crypto/x509/parser.go
@@ -164,53 +164,29 @@
return ai, nil
}
-func parseValidity(der cryptobyte.String) (time.Time, time.Time, error) {
- extract := func() (time.Time, error) {
- var t time.Time
- switch {
- case der.PeekASN1Tag(cryptobyte_asn1.UTCTime):
- // TODO(rolandshoemaker): once #45411 is fixed, the following code
- // should be replaced with a call to der.ReadASN1UTCTime.
- var utc cryptobyte.String
- if !der.ReadASN1(&utc, cryptobyte_asn1.UTCTime) {
- return t, errors.New("x509: malformed UTCTime")
- }
- s := string(utc)
-
- formatStr := "0601021504Z0700"
- var err error
- t, err = time.Parse(formatStr, s)
- if err != nil {
- formatStr = "060102150405Z0700"
- t, err = time.Parse(formatStr, s)
- }
- if err != nil {
- return t, err
- }
-
- if serialized := t.Format(formatStr); serialized != s {
- return t, errors.New("x509: malformed UTCTime")
- }
-
- if t.Year() >= 2050 {
- // UTCTime only encodes times prior to 2050. See https://tools.ietf.org/html/rfc5280#section-4.1.2.5.1
- t = t.AddDate(-100, 0, 0)
- }
- case der.PeekASN1Tag(cryptobyte_asn1.GeneralizedTime):
- if !der.ReadASN1GeneralizedTime(&t) {
- return t, errors.New("x509: malformed GeneralizedTime")
- }
- default:
- return t, errors.New("x509: unsupported time format")
+func parseTime(der *cryptobyte.String) (time.Time, error) {
+ var t time.Time
+ switch {
+ case der.PeekASN1Tag(cryptobyte_asn1.UTCTime):
+ if !der.ReadASN1UTCTime(&t) {
+ return t, errors.New("x509: malformed UTCTime")
}
- return t, nil
+ case der.PeekASN1Tag(cryptobyte_asn1.GeneralizedTime):
+ if !der.ReadASN1GeneralizedTime(&t) {
+ return t, errors.New("x509: malformed GeneralizedTime")
+ }
+ default:
+ return t, errors.New("x509: unsupported time format")
}
+ return t, nil
+}
- notBefore, err := extract()
+func parseValidity(der cryptobyte.String) (time.Time, time.Time, error) {
+ notBefore, err := parseTime(&der)
if err != nil {
return time.Time{}, time.Time{}, err
}
- notAfter, err := extract()
+ notAfter, err := parseTime(&der)
if err != nil {
return time.Time{}, time.Time{}, err
}
@@ -1011,3 +987,160 @@
}
return certs, nil
}
+
+// ParseRevocationList parses a X509 v2 Certificate Revocation List from the given
+// ASN.1 DER data.
+func ParseRevocationList(der []byte) (*RevocationList, error) {
+ rl := &RevocationList{}
+
+ input := cryptobyte.String(der)
+ // we read the SEQUENCE including length and tag bytes so that
+ // 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")
+ }
+ rl.Raw = input
+ if !input.ReadASN1(&input, cryptobyte_asn1.SEQUENCE) {
+ return nil, errors.New("x509: malformed certificate")
+ }
+
+ 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")
+ }
+ rl.RawTBSRevocationList = tbs
+ if !tbs.ReadASN1(&tbs, cryptobyte_asn1.SEQUENCE) {
+ return nil, errors.New("x509: malformed tbs certificate")
+ }
+
+ var version int
+ if !tbs.PeekASN1Tag(cryptobyte_asn1.INTEGER) {
+ return nil, errors.New("x509: unsupported crl version")
+ }
+ if !tbs.ReadASN1Integer(&version) {
+ return nil, errors.New("x509: malformed crl")
+ }
+ if version != 1 {
+ return nil, fmt.Errorf("x509: unsupported crl version: %d", version)
+ }
+
+ var sigAISeq cryptobyte.String
+ if !tbs.ReadASN1(&sigAISeq, cryptobyte_asn1.SEQUENCE) {
+ return nil, errors.New("x509: malformed signature algorithm identifier")
+ }
+ // Before parsing the inner algorithm identifier, extract
+ // the outer algorithm identifier and make sure that they
+ // match.
+ var outerSigAISeq cryptobyte.String
+ if !input.ReadASN1(&outerSigAISeq, cryptobyte_asn1.SEQUENCE) {
+ return nil, errors.New("x509: malformed algorithm identifier")
+ }
+ if !bytes.Equal(outerSigAISeq, sigAISeq) {
+ return nil, errors.New("x509: inner and outer signature algorithm identifiers don't match")
+ }
+ sigAI, err := parseAI(sigAISeq)
+ if err != nil {
+ return nil, err
+ }
+ rl.SignatureAlgorithm = getSignatureAlgorithmFromAI(sigAI)
+
+ var issuerSeq cryptobyte.String
+ if !tbs.ReadASN1Element(&issuerSeq, cryptobyte_asn1.SEQUENCE) {
+ return nil, errors.New("x509: malformed issuer")
+ }
+ rl.RawIssuer = issuerSeq
+ issuerRDNs, err := parseName(issuerSeq)
+ if err != nil {
+ return nil, err
+ }
+ rl.Issuer.FillFromRDNSequence(issuerRDNs)
+
+ rl.ThisUpdate, err = parseTime(&tbs)
+ if err != nil {
+ return nil, err
+ }
+ if tbs.PeekASN1Tag(cryptobyte_asn1.GeneralizedTime) || tbs.PeekASN1Tag(cryptobyte_asn1.UTCTime) {
+ rl.NextUpdate, err = parseTime(&tbs)
+ if err != nil {
+ return nil, err
+ }
+ }
+
+ if tbs.PeekASN1Tag(cryptobyte_asn1.SEQUENCE) {
+ var revokedSeq cryptobyte.String
+ if !tbs.ReadASN1(&revokedSeq, cryptobyte_asn1.SEQUENCE) {
+ return nil, errors.New("x509: malformed crl")
+ }
+ for !revokedSeq.Empty() {
+ var certSeq cryptobyte.String
+ if !revokedSeq.ReadASN1(&certSeq, cryptobyte_asn1.SEQUENCE) {
+ return nil, errors.New("x509: malformed crl")
+ }
+ rc := pkix.RevokedCertificate{}
+ rc.SerialNumber = new(big.Int)
+ if !certSeq.ReadASN1Integer(rc.SerialNumber) {
+ return nil, errors.New("x509: malformed serial number")
+ }
+ rc.RevocationTime, err = parseTime(&certSeq)
+ if err != nil {
+ return nil, err
+ }
+ var extensions cryptobyte.String
+ var present bool
+ if !tbs.ReadOptionalASN1(&extensions, &present, cryptobyte_asn1.SEQUENCE) {
+ return nil, errors.New("x509: malformed extensions")
+ }
+ if present {
+ if !extensions.ReadASN1(&extensions, cryptobyte_asn1.SEQUENCE) {
+ return nil, errors.New("x509: malformed extensions")
+ }
+ for !extensions.Empty() {
+ var extension cryptobyte.String
+ if !extensions.ReadASN1(&extension, cryptobyte_asn1.SEQUENCE) {
+ return nil, errors.New("x509: malformed extension")
+ }
+ ext, err := parseExtension(extension)
+ if err != nil {
+ return nil, err
+ }
+ rc.Extensions = append(rc.Extensions, ext)
+ }
+ }
+
+ rl.RevokedCertificates = append(rl.RevokedCertificates, rc)
+ }
+ }
+
+ var extensions cryptobyte.String
+ var present bool
+ if !tbs.ReadOptionalASN1(&extensions, &present, cryptobyte_asn1.Tag(0).Constructed().ContextSpecific()) {
+ return nil, errors.New("x509: malformed extensions")
+ }
+ if present {
+ if !extensions.ReadASN1(&extensions, cryptobyte_asn1.SEQUENCE) {
+ return nil, errors.New("x509: malformed extensions")
+ }
+ for !extensions.Empty() {
+ var extension cryptobyte.String
+ if !extensions.ReadASN1(&extension, cryptobyte_asn1.SEQUENCE) {
+ return nil, errors.New("x509: malformed extension")
+ }
+ ext, err := parseExtension(extension)
+ if err != nil {
+ return nil, err
+ }
+ rl.Extensions = append(rl.Extensions, ext)
+ }
+ }
+
+ var signature asn1.BitString
+ if !input.ReadASN1BitString(&signature) {
+ return nil, errors.New("x509: malformed signature")
+ }
+ rl.Signature = signature.RightAlign()
+
+ return rl, nil
+}
diff --git a/src/crypto/x509/pkix/pkix.go b/src/crypto/x509/pkix/pkix.go
index e9179ed..da57b66 100644
--- a/src/crypto/x509/pkix/pkix.go
+++ b/src/crypto/x509/pkix/pkix.go
@@ -296,6 +296,8 @@
// TBSCertificateList represents the ASN.1 structure of the same name. See RFC
// 5280, section 5.1.
+//
+// Deprecated: x509.RevocationList should be used instead.
type TBSCertificateList struct {
Raw asn1.RawContent
Version int `asn1:"optional,default:0"`
@@ -309,6 +311,8 @@
// RevokedCertificate represents the ASN.1 structure of the same name. See RFC
// 5280, section 5.1.
+//
+// Deprecated: x509.RevocationList should be used instead.
type RevokedCertificate struct {
SerialNumber *big.Int
RevocationTime time.Time
diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go
index 47be77d..b9e2df6 100644
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -883,6 +883,8 @@
}
// CheckCRLSignature checks that the signature in crl is from c.
+//
+// Deprecated: Use RevocationList.CheckSignatureFrom instead.
func (c *Certificate) CheckCRLSignature(crl *pkix.CertificateList) error {
algo := getSignatureAlgorithmFromAI(crl.SignatureAlgorithm)
return c.CheckSignature(algo, crl.TBSCertList.Raw, crl.SignatureValue.RightAlign())
@@ -1622,6 +1624,8 @@
// encoded CRLs will appear where they should be DER encoded, so this function
// will transparently handle PEM encoding as long as there isn't any leading
// garbage.
+//
+// Deprecated: Use ParseRevocationList instead.
func ParseCRL(crlBytes []byte) (*pkix.CertificateList, error) {
if bytes.HasPrefix(crlBytes, pemCRLPrefix) {
block, _ := pem.Decode(crlBytes)
@@ -1633,6 +1637,8 @@
}
// ParseDERCRL parses a DER encoded CRL from the given bytes.
+//
+// Deprecated: Use ParseRevocationList instead.
func ParseDERCRL(derBytes []byte) (*pkix.CertificateList, error) {
certList := new(pkix.CertificateList)
if rest, err := asn1.Unmarshal(derBytes, certList); err != nil {
@@ -1646,7 +1652,7 @@
// CreateCRL returns a DER encoded CRL, signed by this Certificate, that
// contains the given list of revoked certificates.
//
-// Note: this method does not generate an RFC 5280 conformant X.509 v2 CRL.
+// Deprecated: this method does not generate an RFC 5280 conformant X.509 v2 CRL.
// To generate a standards compliant CRL, use CreateRevocationList instead.
func (c *Certificate) CreateCRL(rand io.Reader, priv any, revokedCerts []pkix.RevokedCertificate, now, expiry time.Time) (crlBytes []byte, err error) {
key, ok := priv.(crypto.Signer)
@@ -2088,6 +2094,14 @@
// RevocationList contains the fields used to create an X.509 v2 Certificate
// Revocation list with CreateRevocationList.
type RevocationList struct {
+ Raw []byte
+ RawTBSRevocationList []byte
+ RawIssuer []byte
+ Issuer pkix.Name
+ Signature []byte
+ AuthorityKeyId []byte
+ Extensions []pkix.Extension
+
// SignatureAlgorithm is used to determine the signature algorithm to be
// used when signing the CRL. If 0 the default algorithm for the signing
// key will be used.
@@ -2222,3 +2236,20 @@
SignatureValue: asn1.BitString{Bytes: signature, BitLength: len(signature) * 8},
})
}
+
+func (rl *RevocationList) CheckSignatureFrom(parent *Certificate) error {
+ if parent.Version == 3 && !parent.BasicConstraintsValid ||
+ parent.BasicConstraintsValid && !parent.IsCA {
+ return ConstraintViolationError{}
+ }
+
+ if parent.KeyUsage != 0 && parent.KeyUsage&KeyUsageCRLSign == 0 {
+ return ConstraintViolationError{}
+ }
+
+ if parent.PublicKeyAlgorithm == UnknownPublicKeyAlgorithm {
+ return ErrUnsupportedAlgorithm
+ }
+
+ return parent.CheckSignature(rl.SignatureAlgorithm, rl.RawTBSRevocationList, rl.Signature)
+}
diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index 69dcd11..69a45ae 100644
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -2631,24 +2631,24 @@
return
}
- parsedCRL, err := ParseDERCRL(crl)
+ parsedCRL, err := ParseRevocationList(crl)
if err != nil {
t.Fatalf("Failed to parse generated CRL: %s", err)
}
if tc.template.SignatureAlgorithm != UnknownSignatureAlgorithm &&
- parsedCRL.SignatureAlgorithm.Algorithm.Equal(signatureAlgorithmDetails[tc.template.SignatureAlgorithm].oid) {
+ parsedCRL.SignatureAlgorithm != tc.template.SignatureAlgorithm {
t.Fatalf("SignatureAlgorithm mismatch: got %v; want %v.", parsedCRL.SignatureAlgorithm,
tc.template.SignatureAlgorithm)
}
- if !reflect.DeepEqual(parsedCRL.TBSCertList.RevokedCertificates, tc.template.RevokedCertificates) {
+ if !reflect.DeepEqual(parsedCRL.RevokedCertificates, tc.template.RevokedCertificates) {
t.Fatalf("RevokedCertificates mismatch: got %v; want %v.",
- parsedCRL.TBSCertList.RevokedCertificates, tc.template.RevokedCertificates)
+ parsedCRL.RevokedCertificates, tc.template.RevokedCertificates)
}
- if len(parsedCRL.TBSCertList.Extensions) != 2+len(tc.template.ExtraExtensions) {
- t.Fatalf("Generated CRL has wrong number of extensions, wanted: %d, got: %d", 2+len(tc.template.ExtraExtensions), len(parsedCRL.TBSCertList.Extensions))
+ if len(parsedCRL.Extensions) != 2+len(tc.template.ExtraExtensions) {
+ t.Fatalf("Generated CRL has wrong number of extensions, wanted: %d, got: %d", 2+len(tc.template.ExtraExtensions), len(parsedCRL.Extensions))
}
expectedAKI, err := asn1.Marshal(authKeyId{Id: tc.issuer.SubjectKeyId})
if err != nil {
@@ -2658,9 +2658,9 @@
Id: oidExtensionAuthorityKeyId,
Value: expectedAKI,
}
- if !reflect.DeepEqual(parsedCRL.TBSCertList.Extensions[0], akiExt) {
+ if !reflect.DeepEqual(parsedCRL.Extensions[0], akiExt) {
t.Fatalf("Unexpected first extension: got %v, want %v",
- parsedCRL.TBSCertList.Extensions[0], akiExt)
+ parsedCRL.Extensions[0], akiExt)
}
expectedNum, err := asn1.Marshal(tc.template.Number)
if err != nil {
@@ -2670,18 +2670,18 @@
Id: oidExtensionCRLNumber,
Value: expectedNum,
}
- if !reflect.DeepEqual(parsedCRL.TBSCertList.Extensions[1], crlExt) {
+ if !reflect.DeepEqual(parsedCRL.Extensions[1], crlExt) {
t.Fatalf("Unexpected second extension: got %v, want %v",
- parsedCRL.TBSCertList.Extensions[1], crlExt)
+ parsedCRL.Extensions[1], crlExt)
}
- if len(parsedCRL.TBSCertList.Extensions[2:]) == 0 && len(tc.template.ExtraExtensions) == 0 {
+ if len(parsedCRL.Extensions[2:]) == 0 && len(tc.template.ExtraExtensions) == 0 {
// If we don't have anything to check return early so we don't
// hit a [] != nil false positive below.
return
}
- if !reflect.DeepEqual(parsedCRL.TBSCertList.Extensions[2:], tc.template.ExtraExtensions) {
+ if !reflect.DeepEqual(parsedCRL.Extensions[2:], tc.template.ExtraExtensions) {
t.Fatalf("Extensions mismatch: got %v; want %v.",
- parsedCRL.TBSCertList.Extensions[2:], tc.template.ExtraExtensions)
+ parsedCRL.Extensions[2:], tc.template.ExtraExtensions)
}
})
}
@@ -3348,3 +3348,125 @@
t.Fatalf("ParseCertificate to failed to parse certificate with large OID: %s", err)
}
}
+
+func TestParseRevocationList(t *testing.T) {
+ derBytes := fromBase64(derCRLBase64)
+ certList, err := ParseRevocationList(derBytes)
+ if err != nil {
+ t.Errorf("error parsing: %s", err)
+ return
+ }
+ numCerts := len(certList.RevokedCertificates)
+ expected := 88
+ if numCerts != expected {
+ t.Errorf("bad number of revoked certificates. got: %d want: %d", numCerts, expected)
+ }
+}
+
+func TestRevocationListCheckSignatureFrom(t *testing.T) {
+ goodKey, err := ecdsa.GenerateKey(elliptic.P224(), rand.Reader)
+ if err != nil {
+ t.Fatalf("failed to generate test key: %s", err)
+ }
+ badKey, err := ecdsa.GenerateKey(elliptic.P224(), rand.Reader)
+ if err != nil {
+ t.Fatalf("failed to generate test key: %s", err)
+ }
+ tests := []struct {
+ name string
+ issuer *Certificate
+ err string
+ }{
+ {
+ name: "valid",
+ issuer: &Certificate{
+ Version: 3,
+ BasicConstraintsValid: true,
+ IsCA: true,
+ PublicKeyAlgorithm: ECDSA,
+ PublicKey: goodKey.Public(),
+ },
+ },
+ {
+ name: "valid, key usage set",
+ issuer: &Certificate{
+ Version: 3,
+ BasicConstraintsValid: true,
+ IsCA: true,
+ PublicKeyAlgorithm: ECDSA,
+ PublicKey: goodKey.Public(),
+ KeyUsage: KeyUsageCRLSign,
+ },
+ },
+ {
+ name: "invalid issuer, wrong key usage",
+ issuer: &Certificate{
+ Version: 3,
+ BasicConstraintsValid: true,
+ IsCA: true,
+ PublicKeyAlgorithm: ECDSA,
+ PublicKey: goodKey.Public(),
+ KeyUsage: KeyUsageCertSign,
+ },
+ err: "x509: invalid signature: parent certificate cannot sign this kind of certificate",
+ },
+ {
+ name: "invalid issuer, no basic constraints/ca",
+ issuer: &Certificate{
+ Version: 3,
+ PublicKeyAlgorithm: ECDSA,
+ PublicKey: goodKey.Public(),
+ },
+ err: "x509: invalid signature: parent certificate cannot sign this kind of certificate",
+ },
+ {
+ name: "invalid issuer, unsupported public key type",
+ issuer: &Certificate{
+ Version: 3,
+ BasicConstraintsValid: true,
+ IsCA: true,
+ PublicKeyAlgorithm: UnknownPublicKeyAlgorithm,
+ PublicKey: goodKey.Public(),
+ },
+ err: "x509: cannot verify signature: algorithm unimplemented",
+ },
+ {
+ name: "wrong key",
+ issuer: &Certificate{
+ Version: 3,
+ BasicConstraintsValid: true,
+ IsCA: true,
+ PublicKeyAlgorithm: ECDSA,
+ PublicKey: badKey.Public(),
+ },
+ err: "x509: ECDSA verification failure",
+ },
+ }
+
+ crlIssuer := &Certificate{
+ BasicConstraintsValid: true,
+ IsCA: true,
+ PublicKeyAlgorithm: ECDSA,
+ PublicKey: goodKey.Public(),
+ KeyUsage: KeyUsageCRLSign,
+ SubjectKeyId: []byte{1, 2, 3},
+ }
+ for _, tc := range tests {
+ t.Run(tc.name, func(t *testing.T) {
+ crlDER, err := CreateRevocationList(rand.Reader, &RevocationList{Number: big.NewInt(1)}, crlIssuer, goodKey)
+ if err != nil {
+ t.Fatalf("failed to generate CRL: %s", err)
+ }
+ crl, err := ParseRevocationList(crlDER)
+ if err != nil {
+ t.Fatalf("failed to parse test CRL: %s", err)
+ }
+ err = crl.CheckSignatureFrom(tc.issuer)
+ if err != nil && err.Error() != tc.err {
+ t.Errorf("unexpected error: got %s, want %s", err, tc.err)
+ } else if err == nil && tc.err != "" {
+ t.Errorf("CheckSignatureFrom did not fail: want %s", tc.err)
+ }
+ })
+ }
+}
To view, visit change 390834. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Mar 8, 2022, 2:48:42 PM3/8/22
to goph...@pubsubhelper.golang.org, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Filippo Valsorda.
Patch set 1:Run-TryBot +1Trust +1
Roland Shoemaker (Gerrit)
Mar 9, 2022, 12:08:40 PM3/9/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Filippo Valsorda.
Roland Shoemaker uploaded patch set #2 to this change.
4 files changed, 382 insertions(+), 55 deletions(-)
To view, visit change 390834. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Mar 30, 2022, 4:13:33 PM3/30/22
to goph...@pubsubhelper.golang.org, Damien Neil, Julie Qiu, Gopher Robot, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Damien Neil, Julie Qiu, Filippo Valsorda.
Patch set 2:Trust +1
Damien Neil (Gerrit)
Mar 31, 2022, 6:07:00 PM3/31/22
to Roland Shoemaker, goph...@pubsubhelper.golang.org, Julie Qiu, Gopher Robot, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Julie Qiu, Roland Shoemaker, Filippo Valsorda.
Patch set 2:Code-Review +2
2 comments:
File src/crypto/x509/parser.go:
Patch Set #2, Line 1026:
if version != 1 {Optional: I'd find this clearer by defining a constant, since "v2" is confusingly "1":
const x509v2 = 1
if version != x509v2 { ... }Patch Set #2, Line 1038:
if !input.ReadASN1(&outerSigAISeq, cryptobyte_asn1.SEQUENCE) {Optional: The parsing is jumping around between tbs and input. I'd find this easier to follow if input was fully parsed before moving on to tbs.
To view, visit change 390834. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Apr 4, 2022, 10:20:29 PM4/4/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Julie Qiu, Roland Shoemaker, Filippo Valsorda.
Roland Shoemaker uploaded patch set #3 to this change.
4 files changed, 386 insertions(+), 55 deletions(-)
To view, visit change 390834. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Apr 4, 2022, 10:21:14 PM4/4/22
to goph...@pubsubhelper.golang.org, Damien Neil, Julie Qiu, Gopher Robot, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Julie Qiu, Filippo Valsorda.
Patch set 3:Run-TryBot +1Auto-Submit +1Trust +1
2 comments:
File src/crypto/x509/parser.go:
Patch Set #2, Line 1026:
if version != 1 {
Optional: I'd find this clearer by defining a constant, since "v2" is confusingly "1": […]
Done.
Patch Set #2, Line 1038:
if !input.ReadASN1(&outerSigAISeq, cryptobyte_asn1.SEQUENCE) {
Optional: The parsing is jumping around between tbs and input. […]
Done.
To view, visit change 390834. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Apr 5, 2022, 6:57:11 PM4/5/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Julie Qiu, Roland Shoemaker, Filippo Valsorda.
Roland Shoemaker uploaded patch set #4 to this change.
crypto/x509: add new CRL parser, deprecate old one
Adds a new, cryptobyte based, CRL parser, which returns a
x509.RevocaitonList, rather than a pkix.CertificateList. This allows us
to return much more detailed information, as well as leaving open the
option of adding further information since RevocationList is not a
direct ASN.1 representation like pkix.CertificateList. Additionally
a new method is added to RevocationList, CheckSignatureFrom, which is
analogous to the method with the same name on Certificate, which
properly checks that the signature is from an issuing certiifcate.
This change also deprecates a number of older CRL related functions and
types, which have been replaced with the new functionality introduced
in this change:
* crypto/x509.ParseCRL
* crypto/x509.ParseDERCRL
* crypto/x509.CheckCRLSignature
* crypto/x509/pkix.CertificateList
* crypto/x509/pkix.TBSCertificateList
Fixes #50674
Change-Id: I27dc219e39bef09a396e666b4fccaa32578fd913
---
A api/next/50674.txt
M src/crypto/x509/parser.go
M src/crypto/x509/pkix/pkix.go
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
5 files changed, 395 insertions(+), 55 deletions(-)
To view, visit change 390834. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Apr 5, 2022, 6:57:19 PM4/5/22
to goph...@pubsubhelper.golang.org, Gopher Robot, Damien Neil, Julie Qiu, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Julie Qiu, Filippo Valsorda.
Patch set 4:Run-TryBot +1Trust +1
To view, visit change 390834. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Apr 5, 2022, 7:20:38 PM4/5/22
to goph...@pubsubhelper.golang.org, Gopher Robot, Damien Neil, Julie Qiu, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Julie Qiu, Filippo Valsorda.
Patch set 5:Run-TryBot +1Auto-Submit +1Trust +1
Gopher Robot (Gerrit)
Apr 5, 2022, 7:33:05 PM4/5/22
to Roland Shoemaker, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Damien Neil, Julie Qiu, Filippo Valsorda, golang-co...@googlegroups.com
Gopher Robot submitted this change.
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/crypto/x509/x509_test.go
Insertions: 111, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: api/next/50674.txt
Insertions: 9, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: src/crypto/x509/parser.go
Insertions: 11, Deletions: 7.
The diff is too large to show. Please review the diff.
```
Approvals:
Damien Neil: Looks good to me, approved
Roland Shoemaker: Trusted; Run TryBots; Automatically submit change
Gopher Robot: TryBots succeeded
crypto/x509: add new CRL parser, deprecate old one
Adds a new, cryptobyte based, CRL parser, which returns a
x509.RevocaitonList, rather than a pkix.CertificateList. This allows us
to return much more detailed information, as well as leaving open the
option of adding further information since RevocationList is not a
direct ASN.1 representation like pkix.CertificateList. Additionally
a new method is added to RevocationList, CheckSignatureFrom, which is
analogous to the method with the same name on Certificate, which
properly checks that the signature is from an issuing certiifcate.
This change also deprecates a number of older CRL related functions and
types, which have been replaced with the new functionality introduced
in this change:
* crypto/x509.ParseCRL
* crypto/x509.ParseDERCRL
* crypto/x509.CheckCRLSignature
* crypto/x509/pkix.CertificateList
* crypto/x509/pkix.TBSCertificateList
Fixes #50674
Change-Id: I27dc219e39bef09a396e666b4fccaa32578fd913
Reviewed-on: https://go-review.googlesource.com/c/go/+/390834
Reviewed-by: Damien Neil <dn...@google.com>
Trust: Roland Shoemaker <rol...@golang.org>
Run-TryBot: Roland Shoemaker <rol...@golang.org>
Auto-Submit: Roland Shoemaker <rol...@golang.org>
TryBot-Result: Gopher Robot <go...@golang.org>
---
A api/next/50674.txt
M src/crypto/x509/parser.go
M src/crypto/x509/pkix/pkix.go
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
5 files changed, 401 insertions(+), 55 deletions(-)
diff --git a/api/next/50674.txt b/api/next/50674.txt
new file mode 100644
index 0000000..6b5bca3
--- /dev/null
+++ b/api/next/50674.txt
@@ -0,0 +1,9 @@
+pkg crypto/x509, func ParseRevocationList([]uint8) (*RevocationList, error) #50674
+pkg crypto/x509, method (*RevocationList) CheckSignatureFrom(*Certificate) error #50674
+pkg crypto/x509, type RevocationList struct, AuthorityKeyId []uint8 #50674
+pkg crypto/x509, type RevocationList struct, Extensions []pkix.Extension #50674
+pkg crypto/x509, type RevocationList struct, Issuer pkix.Name #50674
+pkg crypto/x509, type RevocationList struct, Raw []uint8 #50674
+pkg crypto/x509, type RevocationList struct, RawIssuer []uint8 #50674
+pkg crypto/x509, type RevocationList struct, RawTBSRevocationList []uint8 #50674
+pkg crypto/x509, type RevocationList struct, Signature []uint8 #50674
diff --git a/src/crypto/x509/parser.go b/src/crypto/x509/parser.go
index 333991b..9dd7c25 100644
@@ -1011,3 +987,164 @@
}
return certs, nil
}
+
+// The X.509 standards confusingly 1-indexed the version names, but 0-indexed
+// the actual encoded version, so the version for X.509v2 is 1.
+const x509v2Version = 1
+ if version != x509v2Version {
+ return nil, fmt.Errorf("x509: unsupported crl version: %d", version)
+ }
+
+ var sigAISeq cryptobyte.String
+ if !tbs.ReadASN1(&sigAISeq, cryptobyte_asn1.SEQUENCE) {
+ return nil, errors.New("x509: malformed signature algorithm identifier")
+ }
+ // Before parsing the inner algorithm identifier, extract
+ // the outer algorithm identifier and make sure that they
+ // match.
+ var outerSigAISeq cryptobyte.String
+ if !input.ReadASN1(&outerSigAISeq, cryptobyte_asn1.SEQUENCE) {
+ return nil, errors.New("x509: malformed algorithm identifier")
+ }
+ if !bytes.Equal(outerSigAISeq, sigAISeq) {
+ return nil, errors.New("x509: inner and outer signature algorithm identifiers don't match")
+ }
+ sigAI, err := parseAI(sigAISeq)
+ if err != nil {
+ return nil, err
+ }
+ rl.SignatureAlgorithm = getSignatureAlgorithmFromAI(sigAI)
+
index cb43079..41d85c4 100644
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -880,6 +880,8 @@
}
// CheckCRLSignature checks that the signature in crl is from c.
+//
+// Deprecated: Use RevocationList.CheckSignatureFrom instead.
func (c *Certificate) CheckCRLSignature(crl *pkix.CertificateList) error {
algo := getSignatureAlgorithmFromAI(crl.SignatureAlgorithm)
return c.CheckSignature(algo, crl.TBSCertList.Raw, crl.SignatureValue.RightAlign())
@@ -1607,6 +1609,8 @@
// encoded CRLs will appear where they should be DER encoded, so this function
// will transparently handle PEM encoding as long as there isn't any leading
// garbage.
+//
+// Deprecated: Use ParseRevocationList instead.
func ParseCRL(crlBytes []byte) (*pkix.CertificateList, error) {
if bytes.HasPrefix(crlBytes, pemCRLPrefix) {
block, _ := pem.Decode(crlBytes)
@@ -1618,6 +1622,8 @@
}
// ParseDERCRL parses a DER encoded CRL from the given bytes.
+//
+// Deprecated: Use ParseRevocationList instead.
func ParseDERCRL(derBytes []byte) (*pkix.CertificateList, error) {
certList := new(pkix.CertificateList)
if rest, err := asn1.Unmarshal(derBytes, certList); err != nil {
@@ -1631,7 +1637,7 @@
// CreateCRL returns a DER encoded CRL, signed by this Certificate, that
// contains the given list of revoked certificates.
//
-// Note: this method does not generate an RFC 5280 conformant X.509 v2 CRL.
+// Deprecated: this method does not generate an RFC 5280 conformant X.509 v2 CRL.
// To generate a standards compliant CRL, use CreateRevocationList instead.
func (c *Certificate) CreateCRL(rand io.Reader, priv any, revokedCerts []pkix.RevokedCertificate, now, expiry time.Time) (crlBytes []byte, err error) {
key, ok := priv.(crypto.Signer)
@@ -2073,6 +2079,14 @@
// RevocationList contains the fields used to create an X.509 v2 Certificate
// Revocation list with CreateRevocationList.
type RevocationList struct {
+ Raw []byte
+ RawTBSRevocationList []byte
+ RawIssuer []byte
+
+ Issuer pkix.Name
+ AuthorityKeyId []byte
+
+ Signature []byte
// SignatureAlgorithm is used to determine the signature algorithm to be
// used when signing the CRL. If 0 the default algorithm for the signing
// key will be used.
@@ -2087,6 +2101,7 @@
// which should be a monotonically increasing sequence number for a given
// CRL scope and CRL issuer.
Number *big.Int
+
// ThisUpdate is used to populate the thisUpdate field in the CRL, which
// indicates the issuance date of the CRL.
ThisUpdate time.Time
@@ -2094,6 +2109,11 @@
// indicates the date by which the next CRL will be issued. NextUpdate
// must be greater than ThisUpdate.
NextUpdate time.Time
+
+ // Extensions contains raw X.509 extensions. When creating a CRL,
+ // the Extensions field is ignored, see ExtraExtensions.
+ Extensions []pkix.Extension
+
// ExtraExtensions contains any additional extensions to add directly to
// the CRL.
ExtraExtensions []pkix.Extension
@@ -2207,3 +2227,22 @@
SignatureValue: asn1.BitString{Bytes: signature, BitLength: len(signature) * 8},
})
}
+
+// CheckSignatureFrom verifies that the signature on rl is a valid signature
+// from issuer.
+func (rl *RevocationList) CheckSignatureFrom(parent *Certificate) error {
+ if parent.Version == 3 && !parent.BasicConstraintsValid ||
+ parent.BasicConstraintsValid && !parent.IsCA {
+ return ConstraintViolationError{}
+ }
+
+ if parent.KeyUsage != 0 && parent.KeyUsage&KeyUsageCRLSign == 0 {
+ return ConstraintViolationError{}
+ }
+
+ if parent.PublicKeyAlgorithm == UnknownPublicKeyAlgorithm {
+ return ErrUnsupportedAlgorithm
+ }
+
+ return parent.CheckSignature(rl.SignatureAlgorithm, rl.RawTBSRevocationList, rl.Signature)
+}
diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index d2889fc..b26ace8 100644@@ -3444,3 +3444,125 @@
t.Errorf("unexpected error: %s", err)
To view, visit change 390834. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages