From 0edece506a9c366c747bdbc7a46d95c74a4f6002 Mon Sep 17 00:00:00 2001 From: Daniel Adam Date: Thu, 31 Oct 2024 13:35:21 +0100 Subject: [PATCH] fixup! Verification by CRL --- .../store/mongodb/revocationList.go | 5 +- certificate-authority/store/revocationList.go | 8 -- .../certManager/general/certManager.go | 49 ++------ pkg/security/certManager/general/crl_cache.go | 109 ++++++++++++++++++ pkg/security/tls/crl.go | 7 ++ 5 files changed, 130 insertions(+), 48 deletions(-) create mode 100644 pkg/security/certManager/general/crl_cache.go diff --git a/certificate-authority/store/mongodb/revocationList.go b/certificate-authority/store/mongodb/revocationList.go index 50016bad4..191eff61d 100644 --- a/certificate-authority/store/mongodb/revocationList.go +++ b/certificate-authority/store/mongodb/revocationList.go @@ -10,6 +10,7 @@ import ( "github.com/google/uuid" "github.com/plgd-dev/hub/v2/certificate-authority/store" "github.com/plgd-dev/hub/v2/pkg/mongodb" + pkgTls "github.com/plgd-dev/hub/v2/pkg/security/tls" "go.mongodb.org/mongo-driver/bson" "go.mongodb.org/mongo-driver/mongo" "go.mongodb.org/mongo-driver/mongo/options" @@ -98,7 +99,7 @@ func (s *Store) getRevocationListUpdate(ctx context.Context, query *store.Update s.logger.Debugf("skipping duplicate serial number(%v)", c.Serial) delete(cmap, c.Serial) } - if len(cmap) == 0 && (!query.UpdateIfExpired || !rl.IsExpired()) { + if len(cmap) == 0 && (!query.UpdateIfExpired || !pkgTls.IsExpired(rl.ValidUntil)) { return revocationListUpdate{ originalRevocationList: rl.RevocationList, }, false, nil @@ -229,7 +230,7 @@ func (s *Store) GetLatestIssuedOrIssueRevocationList(ctx context.Context, issuer if err != nil { return nil, err } - if rl.IssuedAt > 0 && !rl.IsExpired() { + if rl.IssuedAt > 0 && !pkgTls.IsExpired(rl.ValidUntil) { return rl, nil } issuedAt := time.Now() diff --git a/certificate-authority/store/revocationList.go b/certificate-authority/store/revocationList.go index a89136eb9..fda3d7c94 100644 --- a/certificate-authority/store/revocationList.go +++ b/certificate-authority/store/revocationList.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "math/big" - "time" "github.com/google/uuid" ) @@ -59,13 +58,6 @@ func ParseBigInt(s string) (*big.Int, error) { return &number, nil } -// IsExpired checks whether the revocation list is expired -func (rl *RevocationList) IsExpired() bool { - // the crl is expiring soon, so we treat it as already expired - const delta = time.Minute - return rl.ValidUntil <= time.Now().Add(delta).UnixNano() -} - func (rl *RevocationList) Validate() error { if _, err := uuid.Parse(rl.Id); err != nil { return fmt.Errorf("invalid ID(%v): %w", rl.Id, err) diff --git a/pkg/security/certManager/general/certManager.go b/pkg/security/certManager/general/certManager.go index c7704996c..c99253305 100644 --- a/pkg/security/certManager/general/certManager.go +++ b/pkg/security/certManager/general/certManager.go @@ -8,8 +8,6 @@ import ( "crypto/x509" "errors" "fmt" - "io" - "net/http" "strings" "sync" "time" @@ -19,7 +17,6 @@ import ( "github.com/plgd-dev/hub/v2/pkg/fn" "github.com/plgd-dev/hub/v2/pkg/fsnotify" "github.com/plgd-dev/hub/v2/pkg/log" - pkgHttpClient "github.com/plgd-dev/hub/v2/pkg/net/http/client" pkgTls "github.com/plgd-dev/hub/v2/pkg/security/tls" pkgX509 "github.com/plgd-dev/hub/v2/pkg/security/x509" pkgTime "github.com/plgd-dev/hub/v2/pkg/time" @@ -65,7 +62,7 @@ type CertManager struct { logger log.Logger onFileChangeFunc func(event fsnotify.Event) done atomic.Bool - httpClient *pkgHttpClient.Client + crlCache *CRLCache private struct { mutex sync.Mutex @@ -90,13 +87,13 @@ func tryToWatchFile(file urischeme.URIScheme, fileWatcher *fsnotify.Watcher, rem return nil } -func newCertManager(config Config, fileWatcher *fsnotify.Watcher, logger log.Logger, verifyClientCertificate tls.ClientAuthType, httpClient *pkgHttpClient.Client, cleanUpOnError fn.FuncList) (*CertManager, error) { +func newCertManager(config Config, fileWatcher *fsnotify.Watcher, logger log.Logger, verifyClientCertificate tls.ClientAuthType, crlCache *CRLCache, cleanUpOnError fn.FuncList) (*CertManager, error) { c := &CertManager{ fileWatcher: fileWatcher, config: config, verifyClientCertificate: verifyClientCertificate, logger: logger, - httpClient: httpClient, + crlCache: crlCache, } _, err := c.loadCAs() if err != nil { @@ -132,17 +129,17 @@ func New(config Config, fileWatcher *fsnotify.Watcher, logger log.Logger, tracer } var cleanUpOnError fn.FuncList - var httpClient *pkgHttpClient.Client + var crlCache *CRLCache if config.CRL.Enabled { var err error - httpClient, err = NewHTTPClient(config.CRL.HTTP, fileWatcher, logger, tracerProvider) + crlCache, err = NewCRLCache(config.CRL.HTTP, fileWatcher, logger, tracerProvider) if err != nil { return nil, err } - cleanUpOnError.AddFunc(httpClient.Close) + cleanUpOnError.AddFunc(crlCache.Close) } - c, err := newCertManager(config, fileWatcher, logger, verifyClientCertificate, httpClient, cleanUpOnError) + c, err := newCertManager(config, fileWatcher, logger, verifyClientCertificate, crlCache, cleanUpOnError) if err != nil { cleanUpOnError.Execute() return nil, err @@ -189,8 +186,8 @@ func (a *CertManager) Close() { if !a.done.CompareAndSwap(false, true) { return } - if a.httpClient != nil { - a.httpClient.Close() + if a.crlCache != nil { + a.crlCache.Close() } for _, ca := range a.config.CAPool { if !ca.IsFile() { @@ -397,37 +394,13 @@ func (a *CertManager) onFileChange(event fsnotify.Event) { } } -func (a *CertManager) downloadCRL(ctx context.Context, cdp string) (*x509.RevocationList, error) { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, cdp, nil) - if err != nil { - return nil, err - } - req.Close = true - resp, err := a.httpClient.HTTP().Do(req) - if err != nil { - return nil, err - } - defer func() { - if errC := resp.Body.Close(); errC != nil { - a.logger.Errorf("failed to close response body stream: %w", errC) - } - }() - respBody, err := io.ReadAll(resp.Body) - if err != nil { - return nil, err - } - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("unexpected status code %v while downloading CRL from %s", resp.StatusCode, cdp) - } - return x509.ParseRevocationList(respBody) -} - func (a *CertManager) VerifyByCRL(ctx context.Context, certificate *x509.Certificate, cdps []string) error { if !a.config.CRL.Enabled { return nil } + for _, dp := range cdps { - crl, err := a.downloadCRL(ctx, dp) + crl, err := a.crlCache.GetRevocationList(ctx, dp) if err != nil { a.logger.Errorf("failed to download CRL from distribution point(%v): %v", dp, err) continue diff --git a/pkg/security/certManager/general/crl_cache.go b/pkg/security/certManager/general/crl_cache.go new file mode 100644 index 000000000..e7425fc71 --- /dev/null +++ b/pkg/security/certManager/general/crl_cache.go @@ -0,0 +1,109 @@ +package general + +import ( + "context" + "crypto/x509" + "fmt" + "io" + "net/http" + "sync" + + "github.com/plgd-dev/hub/v2/pkg/fsnotify" + "github.com/plgd-dev/hub/v2/pkg/log" + "github.com/plgd-dev/hub/v2/pkg/net/http/client" + pkgHttpUri "github.com/plgd-dev/hub/v2/pkg/net/http/uri" + pkgTls "github.com/plgd-dev/hub/v2/pkg/security/tls" + "github.com/plgd-dev/hub/v2/pkg/sync/task/future" + "go.opentelemetry.io/otel/trace" +) + +type CRLCache struct { + revocationLists map[string]*future.Future + mutex sync.Mutex + httpClient *client.Client + logger log.Logger +} + +func NewCRLCache(config pkgTls.HTTPConfigurer, fileWatcher *fsnotify.Watcher, logger log.Logger, tracerProvider trace.TracerProvider) (*CRLCache, error) { + httpClient, err := NewHTTPClient(config, fileWatcher, logger, tracerProvider) + if err != nil { + return nil, err + } + c := &CRLCache{ + httpClient: httpClient, + revocationLists: make(map[string]*future.Future), + logger: logger, + } + return c, nil +} + +func (c *CRLCache) getFutureRevocationList(key string, old *future.Future) (*future.Future, future.SetFunc) { + c.mutex.Lock() + defer c.mutex.Unlock() + f, ok := c.revocationLists[key] + if !ok || f == old { + fu, set := future.New() + c.revocationLists[key] = fu + return fu, set + } + return f, nil +} + +func (c *CRLCache) GetRevocationListByHTTP(ctx context.Context, distributionPoint string) (*x509.RevocationList, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, distributionPoint, nil) + if err != nil { + return nil, err + } + req.Close = true + resp, err := c.httpClient.HTTP().Do(req) + if err != nil { + return nil, err + } + defer func() { + if errC := resp.Body.Close(); errC != nil { + c.logger.Errorf("failed to close response body stream: %w", errC) + } + }() + respBody, err := io.ReadAll(resp.Body) + if err != nil { + return nil, err + } + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("unexpected status code %v while downloading CRL from %s", resp.StatusCode, distributionPoint) + } + return x509.ParseRevocationList(respBody) +} + +func (c *CRLCache) GetRevocationList(ctx context.Context, distributionPoint string) (*x509.RevocationList, error) { + dp := pkgHttpUri.CanonicalURI(distributionPoint) + + var oldF *future.Future + var setF future.SetFunc + for { + f, set := c.getFutureRevocationList(dp, oldF) + if set != nil { + setF = set + break + } + v, err := f.Get(ctx) + if err == nil { + rl, ok := v.(*x509.RevocationList) + if !ok { + return nil, fmt.Errorf("invalid object type(%T) in a future", v) + } + if !pkgTls.IsExpired(rl.NextUpdate.UnixNano()) { + return rl, nil + } + } + oldF = f + } + rl, err := c.GetRevocationListByHTTP(ctx, dp) + setF(rl, err) + return rl, err +} + +// TODO: add expired CRL clean-up + +func (c *CRLCache) Close() { + c.httpClient.Close() +} diff --git a/pkg/security/tls/crl.go b/pkg/security/tls/crl.go index f60801fda..8fa4bd5be 100644 --- a/pkg/security/tls/crl.go +++ b/pkg/security/tls/crl.go @@ -163,3 +163,10 @@ func (c *CRLConfig) UnmarshalYAML(value *yaml.Node) error { c.HTTP = &cc.HTTP return nil } + +// IsExpired checks whether the revocation list is expired +func IsExpired(validUntil int64) bool { + // the crl is expiring soon, so we treat it as already expired + const delta = time.Minute + return validUntil <= time.Now().Add(delta).UnixNano() +}