Refactor issuance chain service (#1512)

Primary goal is to have 2 separate implementations of the different services, instead of having both implementations inside the same class with conditional switching. This is realized by introducing a _direct_ chain service that performs the legacy implementation of storing chains directly inside the extra data in Trillian. The logic for the new feature is now in an _indirect_ chain service that requires the storage and cache that chains are stored in.

trillian/ctfe/handlers.go

@@ -256,6 +256,11 @@ func NewCertValidationOpts(trustedRoots *x509util.PEMCertPool, currentTime time.
 	return vOpts
 }
 
+type leafChainBuilder interface {
+	BuildLogLeaf(ctx context.Context, chain []*x509.Certificate, logPrefix string, merkleLeaf *ct.MerkleTreeLeaf, isPrecert bool) (*trillian.LogLeaf, error)
+	FixLogLeaf(ctx context.Context, leaf *trillian.LogLeaf) error
+}
+
 // logInfo holds information for a specific log instance.
 type logInfo struct {
 	// LogPrefix is a pre-formatted string identifying the log for diagnostics
@@ -279,7 +284,7 @@ type logInfo struct {
 	// sthGetter provides STHs for the log
 	sthGetter STHGetter
 	// issuanceChainService provides the issuance chain add and get operations
-	issuanceChainService *issuanceChainService
+	issuanceChainService leafChainBuilder
 }
 
 // newLogInfo creates a new instance of logInfo.
@@ -288,7 +293,7 @@ func newLogInfo(
 	validationOpts CertValidationOpts,
 	signer crypto.Signer,
 	timeSource util.TimeSource,
-	issuanceChainService *issuanceChainService,
+	issuanceChainService leafChainBuilder,
 ) *logInfo {
 	vCfg := instanceOpts.Validated
 	cfg := vCfg.Config
@@ -388,6 +393,10 @@ func (li *logInfo) getSTH(ctx context.Context) (*ct.SignedTreeHead, error) {
 	return sth, nil
 }
 
+func (li *logInfo) buildLeaf(ctx context.Context, chain []*x509.Certificate, merkleLeaf *ct.MerkleTreeLeaf, isPrecert bool) (*trillian.LogLeaf, error) {
+	return li.issuanceChainService.BuildLogLeaf(ctx, chain, li.LogPrefix, merkleLeaf, isPrecert)
+}
+
 // ParseBodyAsJSONChain tries to extract cert-chain out of request.
 func ParseBodyAsJSONChain(r *http.Request) (ct.AddChainRequest, error) {
 	body, err := io.ReadAll(r.Body)
@@ -470,7 +479,7 @@ func addChainInternal(ctx context.Context, li *logInfo, w http.ResponseWriter, r
 	if err != nil {
 		return http.StatusBadRequest, fmt.Errorf("failed to build MerkleTreeLeaf: %s", err)
 	}
-	leaf, err := li.issuanceChainService.BuildLogLeaf(ctx, chain, li.LogPrefix, merkleLeaf, isPrecert)
+	leaf, err := li.buildLeaf(ctx, chain, merkleLeaf, isPrecert)
 	if err != nil {
 		return http.StatusInternalServerError, err
 	}
@@ -949,14 +958,6 @@ func verifyAddChain(li *logInfo, req ct.AddChainRequest, expectingPrecert bool)
 	return validPath, nil
 }
 
-func extractRawCerts(chain []*x509.Certificate) []ct.ASN1Cert {
-	raw := make([]ct.ASN1Cert, len(chain))
-	for i, cert := range chain {
-		raw[i] = ct.ASN1Cert{Data: cert.Raw}
-	}
-	return raw
-}
-
 // marshalAndWriteAddChainResponse is used by add-chain and add-pre-chain to create and write
 // the JSON response to the client
 func marshalAndWriteAddChainResponse(sct *ct.SignedCertificateTimestamp, signer crypto.Signer, w http.ResponseWriter) error {

trillian/ctfe/handlers_test.go

@@ -165,7 +165,7 @@ func setupTest(t *testing.T, pemRoots []string, signer crypto.Signer) handlerTes
 	cfg := &configpb.LogConfig{LogId: 0x42, Prefix: "test", IsMirror: false}
 	vCfg := &ValidatedLogConfig{Config: cfg}
 	iOpts := InstanceOptions{Validated: vCfg, Client: info.client, Deadline: time.Millisecond * 500, MetricFactory: monitoring.InertMetricFactory{}, RequestLog: new(DefaultRequestLog)}
-	info.li = newLogInfo(iOpts, vOpts, signer, fakeTimeSource, newIssuanceChainService(nil, nil))
+	info.li = newLogInfo(iOpts, vOpts, signer, fakeTimeSource, &directIssuanceChainService{})
 
 	for _, pemRoot := range pemRoots {
 		if !info.roots.AppendCertsFromPEM([]byte(pemRoot)) {

trillian/ctfe/instance.go

@@ -185,13 +185,17 @@ func setUpLogInfo(ctx context.Context, opts InstanceOptions) (*logInfo, error) {
 	if err != nil {
 		return nil, err
 	}
-	// issuanceChainCache is nil if the cache related flags are not defined.
+	if issuanceChainStorage == nil {
+		return newLogInfo(opts, validationOpts, signer, new(util.SystemTimeSource), &directIssuanceChainService{}), nil
+	}
+
+	// We are storing chains outside of Trillian, so set up cache and service.
 	issuanceChainCache, err := cache.NewIssuanceChainCache(ctx, opts.CacheType, opts.CacheOption)
 	if err != nil {
 		return nil, err
 	}
 
-	issuanceChainService := newIssuanceChainService(issuanceChainStorage, issuanceChainCache)
+	issuanceChainService := newIndirectIssuanceChainService(issuanceChainStorage, issuanceChainCache)
 
 	logInfo := newLogInfo(opts, validationOpts, signer, new(util.SystemTimeSource), issuanceChainService)
 	return logInfo, nil

trillian/ctfe/services.go

@@ -17,7 +17,6 @@ package ctfe
 import (
 	"context"
 	"crypto/sha256"
-	"errors"
 	"fmt"
 
 	"github.com/google/certificate-transparency-go/asn1"
@@ -32,127 +31,76 @@ import (
 	ct "github.com/google/certificate-transparency-go"
 )
 
-type issuanceChainService struct {
-	storage storage.IssuanceChainStorage
-	cache   cache.IssuanceChainCache
+// directIssuanceChainService does no special work, relying on the chains
+// being serialized in the extraData directly.
+type directIssuanceChainService struct {
 }
 
-func newIssuanceChainService(s storage.IssuanceChainStorage, c cache.IssuanceChainCache) *issuanceChainService {
-	service := &issuanceChainService{
-		storage: s,
-		cache:   c,
+func (s *directIssuanceChainService) BuildLogLeaf(ctx context.Context, chain []*x509.Certificate, logPrefix string, merkleLeaf *ct.MerkleTreeLeaf, isPrecert bool) (*trillian.LogLeaf, error) {
+	raw := extractRawCerts(chain)
+	// Trillian gRPC
+	leaf, err := util.BuildLogLeaf(logPrefix, *merkleLeaf, 0, raw[0], raw[1:], isPrecert)
+	if err != nil {
+		return nil, fmt.Errorf("failed to build LogLeaf: %s", err)
 	}
-
-	return service
+	return leaf, nil
 }
 
-func (s *issuanceChainService) isCTFEStorageEnabled() bool {
-	return s.storage != nil
+func (s *directIssuanceChainService) FixLogLeaf(ctx context.Context, leaf *trillian.LogLeaf) error {
+	return nil
 }
 
-// GetByHash returns the issuance chain with hash as the input.
-func (s *issuanceChainService) GetByHash(ctx context.Context, hash []byte) ([]byte, error) {
-	// Return err if CTFE storage backend is not enabled.
-	if !s.isCTFEStorageEnabled() {
-		return nil, errors.New("failed to GetByHash when storage is nil")
-	}
-
-	// Return if found in cache.
-	chain, err := s.cache.Get(ctx, hash)
-	if chain != nil || err != nil {
-		return chain, err
+func newIndirectIssuanceChainService(s storage.IssuanceChainStorage, c cache.IssuanceChainCache) *indirectIssuanceChainService {
+	if s == nil || c == nil {
+		panic("storage and cache are required")
 	}
-
-	// Find in storage if cache miss.
-	chain, err = s.storage.FindByKey(ctx, hash)
-	if err != nil {
-		return nil, err
+	service := &indirectIssuanceChainService{
+		storage: s,
+		cache:   c,
 	}
 
-	// If there is any error from cache set, do not return the error because
-	// the chain is still available for read.
-	go func(ctx context.Context, hash, chain []byte) {
-		if err := s.cache.Set(ctx, hash, chain); err != nil {
-			klog.Errorf("failed to set hash and chain into cache: %v", err)
-		}
-	}(ctx, hash, chain)
-
-	return chain, nil
+	return service
 }
 
-// add adds the issuance chain into the storage and cache and returns the hash
-// of the chain.
-func (s *issuanceChainService) add(ctx context.Context, chain []byte) ([]byte, error) {
-	// Return err if CTFE storage backend is not enabled.
-	if !s.isCTFEStorageEnabled() {
-		return nil, errors.New("failed to Add when storage is nil")
-	}
-
-	hash := issuanceChainHash(chain)
-
-	if err := s.storage.Add(ctx, hash, chain); err != nil {
-		return nil, err
-	}
-
-	// If there is any error from cache set, do not return the error because
-	// the chain is already stored.
-	go func(ctx context.Context, hash, chain []byte) {
-		if err := s.cache.Set(ctx, hash, chain); err != nil {
-			klog.Errorf("failed to set hash and chain into cache: %v", err)
-		}
-	}(ctx, hash, chain)
-
-	return hash, nil
+// indirectIssuanceChainService uses an external storage mechanism to store the chains.
+// This feature allows for deduplication of chains, but requires more work to inject the
+// chain data back inside the leaves.
+type indirectIssuanceChainService struct {
+	storage storage.IssuanceChainStorage
+	cache   cache.IssuanceChainCache
 }
 
 // BuildLogLeaf builds the MerkleTreeLeaf that gets sent to the backend, and make a trillian.LogLeaf for it.
-func (s *issuanceChainService) BuildLogLeaf(ctx context.Context, chain []*x509.Certificate, logPrefix string, merkleLeaf *ct.MerkleTreeLeaf, isPrecert bool) (*trillian.LogLeaf, error) {
+func (s *indirectIssuanceChainService) BuildLogLeaf(ctx context.Context, chain []*x509.Certificate, logPrefix string, merkleLeaf *ct.MerkleTreeLeaf, isPrecert bool) (*trillian.LogLeaf, error) {
 	raw := extractRawCerts(chain)
 
-	// If CTFE storage is enabled for issuance chain, add the chain to storage
-	// and cache, and then build log leaf. If Trillian gRPC is enabled for
-	// issuance chain, build the log leaf.
-	if s.isCTFEStorageEnabled() {
-		issuanceChain, err := asn1.Marshal(raw[1:])
-		if err != nil {
-			return nil, fmt.Errorf("failed to marshal issuance chain: %s", err)
-		}
-		hash, err := s.add(ctx, issuanceChain)
-		if err != nil {
-			return nil, fmt.Errorf("failed to add issuance chain into CTFE storage: %s", err)
-		}
-		leaf, err := util.BuildLogLeafWithChainHash(logPrefix, *merkleLeaf, 0, raw[0], hash, isPrecert)
-		if err != nil {
-			return nil, fmt.Errorf("failed to build LogLeaf: %s", err)
-		}
-		return leaf, nil
+	// Add the chain to storage and cache, and then build log leaf
+	issuanceChain, err := asn1.Marshal(raw[1:])
+	if err != nil {
+		return nil, fmt.Errorf("failed to marshal issuance chain: %s", err)
 	}
-
-	// Trillian gRPC
-	leaf, err := util.BuildLogLeaf(logPrefix, *merkleLeaf, 0, raw[0], raw[1:], isPrecert)
+	hash, err := s.add(ctx, issuanceChain)
+	if err != nil {
+		return nil, fmt.Errorf("failed to add issuance chain into CTFE storage: %s", err)
+	}
+	leaf, err := util.BuildLogLeafWithChainHash(logPrefix, *merkleLeaf, 0, raw[0], hash, isPrecert)
 	if err != nil {
 		return nil, fmt.Errorf("failed to build LogLeaf: %s", err)
 	}
 	return leaf, nil
-
 }
 
 // FixLogLeaf recreates and populates the LogLeaf.ExtraData if CTFE storage
 // backend is enabled and the type of LogLeaf.ExtraData contains any hash
 // (e.g. PrecertChainEntryHash, CertificateChainHash).
-func (s *issuanceChainService) FixLogLeaf(ctx context.Context, leaf *trillian.LogLeaf) error {
-	// Skip if CTFE storage backend is not enabled.
-	if !s.isCTFEStorageEnabled() {
-		return nil
-	}
-
+func (s *indirectIssuanceChainService) FixLogLeaf(ctx context.Context, leaf *trillian.LogLeaf) error {
 	// As the struct stored in leaf.ExtraData is unknown, the only way is to try to unmarshal with each possible struct.
 	// Try to unmarshal with ct.PrecertChainEntryHash struct.
 	var precertChainHash ct.PrecertChainEntryHash
 	if rest, err := tls.Unmarshal(leaf.ExtraData, &precertChainHash); err == nil && len(rest) == 0 {
 		var chain []ct.ASN1Cert
 		if len(precertChainHash.IssuanceChainHash) > 0 {
-			chainBytes, err := s.GetByHash(ctx, precertChainHash.IssuanceChainHash)
+			chainBytes, err := s.getByHash(ctx, precertChainHash.IssuanceChainHash)
 			if err != nil {
 				return err
 			}
@@ -182,7 +130,7 @@ func (s *issuanceChainService) FixLogLeaf(ctx context.Context, leaf *trillian.Lo
 	if rest, err := tls.Unmarshal(leaf.ExtraData, &certChainHash); err == nil && len(rest) == 0 {
 		var entries []ct.ASN1Cert
 		if len(certChainHash.IssuanceChainHash) > 0 {
-			chainBytes, err := s.GetByHash(ctx, certChainHash.IssuanceChainHash)
+			chainBytes, err := s.getByHash(ctx, certChainHash.IssuanceChainHash)
 			if err != nil {
 				return err
 			}
@@ -219,8 +167,61 @@ func (s *issuanceChainService) FixLogLeaf(ctx context.Context, leaf *trillian.Lo
 	return fmt.Errorf("unknown extra data type in log leaf: %s", string(leaf.MerkleLeafHash))
 }
 
+// getByHash returns the issuance chain with hash as the input.
+func (s *indirectIssuanceChainService) getByHash(ctx context.Context, hash []byte) ([]byte, error) {
+	// Return if found in cache.
+	chain, err := s.cache.Get(ctx, hash)
+	if chain != nil || err != nil {
+		return chain, err
+	}
+
+	// Find in storage if cache miss.
+	chain, err = s.storage.FindByKey(ctx, hash)
+	if err != nil {
+		return nil, err
+	}
+
+	// If there is any error from cache set, do not return the error because
+	// the chain is still available for read.
+	go func(ctx context.Context, hash, chain []byte) {
+		if err := s.cache.Set(ctx, hash, chain); err != nil {
+			klog.Errorf("failed to set hash and chain into cache: %v", err)
+		}
+	}(ctx, hash, chain)
+
+	return chain, nil
+}
+
+// add adds the issuance chain into the storage and cache and returns the hash
+// of the chain.
+func (s *indirectIssuanceChainService) add(ctx context.Context, chain []byte) ([]byte, error) {
+	hash := issuanceChainHash(chain)
+
+	if err := s.storage.Add(ctx, hash, chain); err != nil {
+		return nil, err
+	}
+
+	// If there is any error from cache set, do not return the error because
+	// the chain is already stored.
+	go func(ctx context.Context, hash, chain []byte) {
+		if err := s.cache.Set(ctx, hash, chain); err != nil {
+			klog.Errorf("failed to set hash and chain into cache: %v", err)
+		}
+	}(ctx, hash, chain)
+
+	return hash, nil
+}
+
 // issuanceChainHash returns the SHA-256 hash of the chain.
 func issuanceChainHash(chain []byte) []byte {
 	checksum := sha256.Sum256(chain)
 	return checksum[:]
 }
+
+func extractRawCerts(chain []*x509.Certificate) []ct.ASN1Cert {
+	raw := make([]ct.ASN1Cert, len(chain))
+	for i, cert := range chain {
+		raw[i] = ct.ASN1Cert{Data: cert.Raw}
+	}
+	return raw
+}

trillian/ctfe/services_test.go

@@ -36,15 +36,15 @@ func TestIssuanceChainServiceAddAndGet(t *testing.T) {
 	ctx := context.Background()
 	storage := &fakeIssuanceChainStorage{}
 	cache := &fakeIssuanceChainCache{}
-	issuanceChainService := newIssuanceChainService(storage, cache)
+	issuanceChainService := newIndirectIssuanceChainService(storage, cache)
 
 	for _, test := range tests {
 		hash, err := issuanceChainService.add(ctx, test.chain)
 		if err != nil {
 			t.Errorf("IssuanceChainService.Add(): %v", err)
 		}
 
-		got, err := issuanceChainService.GetByHash(ctx, hash)
+		got, err := issuanceChainService.getByHash(ctx, hash)
 		if err != nil {
 			t.Errorf("IssuanceChainService.GetByHash(): %v", err)
 		}