Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRL validation improvements #482

Merged
merged 6 commits into from
Jan 21, 2022
Merged

CRL validation improvements #482

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e6204a3
Improve CRL validator performance [skip ci]
iamnotacake Jan 18, 2022
af65bd4
Improve CRL validator performance [skip ci]
iamnotacake Jan 18, 2022
99505cb
Solve testing issues
iamnotacake Jan 19, 2022
f348f8b
Update CHANGELOG_DEV, avoid one struct copy in code
iamnotacake Jan 19, 2022
8a4c54c
Updates after review
iamnotacake Jan 20, 2022
741aeb2
Merge branch 'master' into anatolii/T1830-crl-caching-improvements
iamnotacake Jan 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 71 additions & 56 deletions network/crl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,15 @@ import (
"crypto/x509"
"crypto/x509/pkix"
"errors"
log "github.com/sirupsen/logrus"
"io/ioutil"
"math/big"
"net/http"
url_ "net/url"
"sync"
"time"

log "github.com/sirupsen/logrus"

"github.com/golang/groupcache/lru"
)

Expand Down Expand Up @@ -232,8 +234,9 @@ func (c DefaultCRLClient) Fetch(url string, allowLocal bool) ([]byte, error) {

// CRLCacheItem is combination of fetched+parsed+verified CRL with fetch time
type CRLCacheItem struct {
Fetched time.Time // When this CRL was fetched and cached
CRL pkix.CertificateList
Fetched time.Time // When this CRL was fetched and cached
CRL pkix.CertificateList // Parsed CRL itself
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, why we store it as value and make several copy operations if v.Client.Fetch(url, allowLocal) returns pointer? can we store just pointer and avoid extra copies?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plus, what about to encapsulate all this fields and use CRLCacheItem as interface, and initialize it with constructor?
because previously we initialized with cacheItem := &CRLCacheItem{Fetched: time.Now(), CRL: *crl}
now we prepare map for it, set nil for a field of CRL result and only after that create struct value with:

cacheItem := &CRLCacheItem{
		Fetched:             time.Now(),
		CRL:                 *crl,
		RevokedCertificates: revokedCertificates,
	}

But it's not DefaultCRLVerifier responsibility to know how CacheItem works. It should initialize it with CRL result and CacheItem prepare itself and his map as it wants. And just provide simple API how to create item and how to get info about revoked cert: CacheItem.Get(cert *x509.Certificate)(revokedCert, ok)

And it is encapsulates that it uses SerialNumber of certificate for map, not some hash or any other cert fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do it in next CRL-related PR? Gonna mention your comment in the task.

RevokedCertificates map[*big.Int]pkix.RevokedCertificate // Copy of CRL.TBSCertList.RevokedCertificates with SerialNumber as key
}

// CRLCache is used to store fetched CRLs to avoid downloading the same URL more than once,
Expand Down Expand Up @@ -297,7 +300,7 @@ type DefaultCRLVerifier struct {
}

// Tries to find cached CRL, fetches using v.Client if not found, checks the signature of CRL using issuerCert
func (v DefaultCRLVerifier) getCachedOrFetch(url string, allowLocal bool, issuerCert *x509.Certificate) (*pkix.CertificateList, error) {
func (v DefaultCRLVerifier) getCachedOrFetch(url string, allowLocal bool, issuerCert *x509.Certificate) (*CRLCacheItem, error) {
// Try v.Cache first, but only if caching is enabled (cache time > 0)
if v.Config.isCachingEnabled() {
cacheItem, err := v.Cache.Get(v.Config.url)
Expand All @@ -307,8 +310,8 @@ func (v DefaultCRLVerifier) getCachedOrFetch(url string, allowLocal bool, issuer
return nil, err
}

if time.Now().Before(cacheItem.Fetched.Add(v.Config.cacheTime)) {
return &cacheItem.CRL, nil
if time.Now().Before(cacheItem.Fetched.Add(v.Config.cacheTime)) && time.Now().Before(cacheItem.CRL.TBSCertList.NextUpdate) {
return cacheItem, nil
}
}
}
Expand All @@ -331,23 +334,36 @@ func (v DefaultCRLVerifier) getCachedOrFetch(url string, allowLocal bool, issuer
return nil, err
}

revokedCertificates := make(map[*big.Int]pkix.RevokedCertificate, len(crl.TBSCertList.RevokedCertificates))
for _, cert := range crl.TBSCertList.RevokedCertificates {
revokedCertificates[cert.SerialNumber] = cert
}

if crl.TBSCertList.NextUpdate.Before(time.Now()) {
log.Warnf("CRL: CRL at %s is outdated", url)
return nil, ErrOutdatedCRL
}

// We don't need this array anymore as all revoked certificates are now inside the `map`
crl.TBSCertList.RevokedCertificates = make([]pkix.RevokedCertificate, 0)
iamnotacake marked this conversation as resolved.
Show resolved Hide resolved

cacheItem := &CRLCacheItem{
Fetched: time.Now(),
CRL: *crl,
iamnotacake marked this conversation as resolved.
Show resolved Hide resolved
RevokedCertificates: revokedCertificates,
}

if v.Config.isCachingEnabled() {
cacheItem := &CRLCacheItem{Fetched: time.Now(), CRL: *crl}
v.Cache.Put(url, cacheItem)
}

return crl, nil
return cacheItem, nil
}

// Returns `nil` if certificate was not cound in CRL, returns error if it was there
// or if there was unknown Object ID in revoked certificate extensions
func checkCertWithCRL(cert *x509.Certificate, crl *pkix.CertificateList) error {
for _, extension := range crl.TBSCertList.Extensions {
func checkCertWithCRL(cert *x509.Certificate, cacheItem *CRLCacheItem) error {
for _, extension := range cacheItem.CRL.TBSCertList.Extensions {
// For CRL v2 (RFC 5280 section 5.2), CRL issuers are REQUIRED to include
// the authority key identifier (Section 5.2.1) and the CRL number (Section 5.2.3).
// TODO handle all these extensions; this will require some refactoring:
Expand Down Expand Up @@ -379,55 +395,54 @@ func checkCertWithCRL(cert *x509.Certificate, crl *pkix.CertificateList) error {
}
}

for _, revokedCertificate := range crl.TBSCertList.RevokedCertificates {
if revokedCertificate.SerialNumber.Cmp(cert.SerialNumber) == 0 {
log.WithField("extensions", revokedCertificate.Extensions).Debugln("Revoked certificate extensions")
for _, extension := range revokedCertificate.Extensions {
// One can use https://www.alvestrand.no/objectid/top.html to convert string OIDs (like id-ce-keyUsage)
// into numerical format (like 2.5.29.15), though RFC also allows easy reconstruction of OIDs.

// Some of these extensions values may be ignored, but we still have to
// include them (their OIDs) here so that we're not blindly ignoring them.
// Convert to string to be able to use switch.
switch extension.Id.String() {
// As per RFC 5280 section 4.2, aplications MUST recognize following extensions:
case "2.5.29.15":
// section 4.2.1.3, id-ce-keyUsage
case "2.5.29.32":
// section 4.2.1.4, id-ce-certificatePolicies
case "2.5.29.17":
// section 4.2.1.6, id-ce-subjectAltName
case "2.5.29.19":
// section 4.2.1.9, id-ce-basicConstraints
case "2.5.29.30":
// section 4.2.1.10, id-ce-nameConstraints
case "2.5.29.36":
// section 4.2.1.11, id-ce-policyConstraints
case "2.5.29.37":
// section 4.2.1.12, id-ce-extKeyUsage
case "2.5.29.54":
// section 4.2.1.14, id-ce-inhibitAnyPolicy

// As per RFC 5280 section 4.2, aplications SHOULD recognize following extensions:
case "2.5.29.35":
// section 4.2.1.1, id-ce-authorityKeyIdentifier
case "2.5.29.14":
// section 4.2.1.2, id-ce-subjectKeyIdentifier
case "2.5.29.33":
// section 4.2.1.5, id-ce-policyMappings

default:
if extension.Critical {
log.WithField("oid", extension.Id.String()).Warnln("CRL: Unable to process critical extension with unknown Object ID")
return ErrUnknownCRLExtensionOID
}
log.WithField("oid", extension.Id.String()).Debugln("CRL: Unable to process non-critical extension with unknown Object ID")
revokedCertificate, ok := cacheItem.RevokedCertificates[cert.SerialNumber]
if ok {
log.WithField("extensions", revokedCertificate.Extensions).Debugln("Revoked certificate extensions")
for _, extension := range revokedCertificate.Extensions {
// One can use https://www.alvestrand.no/objectid/top.html to convert string OIDs (like id-ce-keyUsage)
// into numerical format (like 2.5.29.15), though RFC also allows easy reconstruction of OIDs.

// Some of these extensions values may be ignored, but we still have to
// include them (their OIDs) here so that we're not blindly ignoring them.
// Convert to string to be able to use switch.
switch extension.Id.String() {
// As per RFC 5280 section 4.2, aplications MUST recognize following extensions:
case "2.5.29.15":
// section 4.2.1.3, id-ce-keyUsage
case "2.5.29.32":
// section 4.2.1.4, id-ce-certificatePolicies
case "2.5.29.17":
// section 4.2.1.6, id-ce-subjectAltName
case "2.5.29.19":
// section 4.2.1.9, id-ce-basicConstraints
case "2.5.29.30":
// section 4.2.1.10, id-ce-nameConstraints
case "2.5.29.36":
// section 4.2.1.11, id-ce-policyConstraints
case "2.5.29.37":
// section 4.2.1.12, id-ce-extKeyUsage
case "2.5.29.54":
// section 4.2.1.14, id-ce-inhibitAnyPolicy

// As per RFC 5280 section 4.2, aplications SHOULD recognize following extensions:
case "2.5.29.35":
// section 4.2.1.1, id-ce-authorityKeyIdentifier
case "2.5.29.14":
// section 4.2.1.2, id-ce-subjectKeyIdentifier
case "2.5.29.33":
// section 4.2.1.5, id-ce-policyMappings

default:
if extension.Critical {
log.WithField("oid", extension.Id.String()).Warnln("CRL: Unable to process critical extension with unknown Object ID")
return ErrUnknownCRLExtensionOID
}
log.WithField("oid", extension.Id.String()).Debugln("CRL: Unable to process non-critical extension with unknown Object ID")
}

log.WithField("serial", cert.SerialNumber).WithField("revoked_at", revokedCertificate.RevocationTime).Warnln("CRL: Certificate was revoked")
return ErrCertWasRevoked
}

log.WithField("serial", cert.SerialNumber).WithField("revoked_at", revokedCertificate.RevocationTime).Warnln("CRL: Certificate was revoked")
return ErrCertWasRevoked
}

return nil
Expand Down
Loading