CRL validation improvements #482
Conversation
CRL contains revoked certificates as a simple list, which means validating certificates is O(N). Most certificates will be valid which means validator would have to search through the whole list each time. This improvement is about making verification O(1) operations thanks to `map` being used instead of plain array.
Continuing development of this improvement. Failing tests are currently commented.
network/crl.go
Outdated
| 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Let's do it in next CRL-related PR? Gonna mention your comment in the task.
network/crl.go
Outdated
| 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.Text(16)] |
There was a problem hiding this comment.
if we use same integer in several places, lets use constant for that to avoid remembering in future changes
There was a problem hiding this comment.
BTW, you also proposed using base62 for this to have smaller strings. But seems like .Text() has special implementation for power-of-two bases, which makes me think this should work faster with no division involved. Then I will make the constant and set it to 32. A bit longer that base62, but hashing should be faster.
CRL contains revoked certificates as a simple list, which means validating certificates is O(N).
Most certificates will be valid which means validator would have to search through the whole list each time.
This improvement is about making verification O(1) operations thanks to
mapbeing used instead of plain array.Checklist
Public API has proper documentation in the Acra documentation site or has PR on documentation repositorywith new changes
CHANGELOG.md is updated (in case of notable or breaking changes)Benchmark results are attached (if applicable)Example projects and code samples are up-to-date (in case of API changes)