Add functionality for scanning OID matchers in a CT log entry #527
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #527 +/- ##
==========================================
- Coverage 64.02% 62.81% -1.22%
==========================================
Files 4 17 +13
Lines 303 1307 +1004
==========================================
+ Hits 194 821 +627
- Misses 78 410 +332
- Partials 31 76 +45 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
linus-sun
force-pushed
the
linussun/ct-log-identity-monitoring
branch
3 times, most recently
from
844c10e to
74be5d5
Compare
linus-sun
requested review from
haydentherapper and
mihaimaruseac
and removed request for
haydentherapper
| subject := logEntry.X509Cert.Subject.String() | ||
| foundEntries := []*identity.LogEntry{} | ||
| matchedEntries := []*identity.LogEntry{} | ||
| for _, monitoredSub := range monitoredSubjects { | ||
| regex, err := regexp.Compile(monitoredSub) |
There was a problem hiding this comment.
This is jumping ahead by one PR, but I think you'll want to match on CertificateIdentities rather than Subjects since we'd want to match on issuer as well.
There was a problem hiding this comment.
going to address this in a follow-up PR
linus-sun
force-pushed
the
linussun/ct-log-identity-monitoring
branch
4 times, most recently
from
15c7202 to
b794951
Compare
linus-sun
force-pushed
the
linussun/ct-log-identity-monitoring
branch
from
b794951 to
425f8bd
Compare
pkg/identity/identity.go
Outdated
| // if true, it returns the OID extension and extension value that were matched on | ||
| func OIDMatchesPolicy[Certificate *x509.Certificate | *google_x509.Certificate](certificate Certificate, oid asn1.ObjectIdentifier, extensionValues []string) (bool, asn1.ObjectIdentifier, string, error) { | ||
| switch cert := any(certificate).(type) { | ||
| case *x509.Certificate: |
There was a problem hiding this comment.
Given the contents of the two case statements is the same, do you need the two case statements? Or can you just pass cert to getExtension without the case?
There was a problem hiding this comment.
Without the switch statement, Golang won't detect that the Certificate generic constraint will always contain an Extensions field; the switch statement in OIDMatchesPolicy is extraneous and can be removed, but getExtension/getDeprecatedExtension both require the cases
| return "", nil | ||
| case *google_x509.Certificate: | ||
| for _, ext := range cert.Extensions { | ||
| if !ext.Id.Equal((google_asn1.ObjectIdentifier)(oid)) { |
There was a problem hiding this comment.
Like the other comment, it seems like these two case statements are nearly identical, except for this line about comparing OIDs. If we implemented a function that also had a type constraint for either google_asn1.ObjectIdentifier or asn1.ObjectIdentifier, could we use that and then remove the separate case statements?
There was a problem hiding this comment.
same as above- would it be preferable to have two separate functions for getting extension between asn1/Google or is the switch statement preferable?
There was a problem hiding this comment.
This is good with me, this duplication is easy to spot then in case there's a later language feature to remove the duplication.
Signed-off-by: linus-sun <[email protected]>
Signed-off-by: linus-sun <[email protected]>
linus-sun
force-pushed
the
linussun/ct-log-identity-monitoring
branch
from
425f8bd to
be5ee9c
Compare
Summary
Per changes outlined in this doc, this PR implements support for scanning asn1.ObjectIdentifier extensions and matching values within a certificate transparency log. A certificate's extensions can be matched on and filtered for specific extension values, (future PRs will handle saving this output and writing to any of various notification platforms).
This PR also refactors functions related to scanning OID extensions and matching on them for specific values.
Release Note
NONE
Documentation
N/A