Repository: google/certificate-transparency-go
Pull request #903 (2022-04-07 18:09:26)

Move PEMCertPool from CTFE package to x509util
Currently many things indirectly depend on the entire CTFE package
(including HTTP handlers and some flags) because they use the cert pool.
For example, the `ctclient` CLI tool imports `loglist`, which imports `ctfe`.
This is a large overkill, and brings irrelevant flags to the CLI tool.

This change moves the cert pool type to `x509util`, because it's a wrapper
around the pool provided in the `x509` package.

<!---
Describe your changes in detail here.
If this fixes an issue, please write "Fixes #123", substituting the issue number.
-->

### Checklist

<!---
Go over all the following points, and put an `x` in all the boxes that apply.
Feel free to not tick any boxes that don't apply to this PR (e.g. refactoring may not need a CHANGELOG update).
If you're unsure about any of these, don't hesitate to ask. We're here to help!
-->

- [x] I have updated the [CHANGELOG](CHANGELOG.md).
  - Adjust the draft version number according to [semantic versioning](https://semver.org/) rules.
- [ ] I have updated [documentation](docs/) accordingly.


pphaneuf: The whole of the "internals" of CTFE (such as the `ctfe` package) should probably all be moved into an `internal` directory, while we're breaking things, but that's something else.
pavelkalinnikov: Updated changelog.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c23ec457..610d6353 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,7 +9,11 @@
 
 ### JSONClient
 
-  * PostAndParseWithRetry now does backoff-and-retry upon receiving HTTP 429.
+ * PostAndParseWithRetry now does backoff-and-retry upon receiving HTTP 429.
+
+### Cleanup
+ * `ctfe.PEMCertPool` type has been moved to `x509util.PEMCertPool` to reduce
+   dependencies (#903).
 
 ## v1.1.2
 
diff --git a/loglist/logfilter.go b/loglist/logfilter.go
index a5a82990..e6459f89 100644
--- a/loglist/logfilter.go
+++ b/loglist/logfilter.go
@@ -18,13 +18,13 @@ import (
 	"time"
 
 	"github.com/golang/glog"
-	"github.com/google/certificate-transparency-go/trillian/ctfe"
 	"github.com/google/certificate-transparency-go/x509"
+	"github.com/google/certificate-transparency-go/x509util"
 )
 
 // LogRoots maps Log-URLs (stated at LogList) to the pools of their accepted
 // root-certificates.
-type LogRoots map[string]*ctfe.PEMCertPool
+type LogRoots map[string]*x509util.PEMCertPool
 
 // ActiveLogs creates a new LogList containing only non-disqualified non-frozen
 // logs from the original.
diff --git a/loglist2/logfilter.go b/loglist2/logfilter.go
index 6dd5ab4c..2851821f 100644
--- a/loglist2/logfilter.go
+++ b/loglist2/logfilter.go
@@ -16,13 +16,13 @@ package loglist2
 
 import (
 	"github.com/golang/glog"
-	"github.com/google/certificate-transparency-go/trillian/ctfe"
 	"github.com/google/certificate-transparency-go/x509"
+	"github.com/google/certificate-transparency-go/x509util"
 )
 
 // LogRoots maps Log-URLs (stated at LogList) to the pools of their accepted
 // root-certificates.
-type LogRoots map[string]*ctfe.PEMCertPool
+type LogRoots map[string]*x509util.PEMCertPool
 
 // Compatible creates a new LogList containing only Logs matching the temporal,
 // root-acceptance and Log-status conditions.
diff --git a/loglist3/logfilter.go b/loglist3/logfilter.go
index 7c141165..efc07965 100644
--- a/loglist3/logfilter.go
+++ b/loglist3/logfilter.go
@@ -16,13 +16,13 @@ package loglist3
 
 import (
 	"github.com/golang/glog"
-	"github.com/google/certificate-transparency-go/trillian/ctfe"
 	"github.com/google/certificate-transparency-go/x509"
+	"github.com/google/certificate-transparency-go/x509util"
 )
 
 // LogRoots maps Log-URLs (stated at LogList) to the pools of their accepted
 // root-certificates.
-type LogRoots map[string]*ctfe.PEMCertPool
+type LogRoots map[string]*x509util.PEMCertPool
 
 // Compatible creates a new LogList containing only Logs matching the temporal,
 // root-acceptance and Log-status conditions.
diff --git a/submission/distributor.go b/submission/distributor.go
index c5a6a2ff..5b5c0d42 100644
--- a/submission/distributor.go
+++ b/submission/distributor.go
@@ -30,6 +30,7 @@ import (
 	"github.com/google/certificate-transparency-go/loglist2"
 	"github.com/google/certificate-transparency-go/trillian/ctfe"
 	"github.com/google/certificate-transparency-go/x509"
+	"github.com/google/certificate-transparency-go/x509util"
 	"github.com/google/trillian/monitoring"
 
 	ct "github.com/google/certificate-transparency-go"
@@ -86,7 +87,7 @@ type Distributor struct {
 	// helper structs produced out of ll during init.
 	logClients map[string]client.AddLogClient
 	logRoots   loglist2.LogRoots
-	rootPool   *ctfe.PEMCertPool
+	rootPool   *x509util.PEMCertPool
 
 	rootDataFull bool
 
@@ -102,7 +103,7 @@ type Distributor struct {
 func (d *Distributor) RefreshRoots(ctx context.Context) map[string]error {
 	type RootsResult struct {
 		LogURL string
-		Roots  *ctfe.PEMCertPool
+		Roots  *x509util.PEMCertPool
 		Err    error
 	}
 	ch := make(chan RootsResult, len(d.logClients))
@@ -120,7 +121,7 @@ func (d *Distributor) RefreshRoots(ctx context.Context) map[string]error {
 				ch <- res
 				return
 			}
-			res.Roots = ctfe.NewPEMCertPool()
+			res.Roots = x509util.NewPEMCertPool()
 			for _, r := range roots {
 				parsed, err := x509.ParseCertificate(r.Data)
 				if x509.IsFatal(err) {
@@ -160,7 +161,7 @@ func (d *Distributor) RefreshRoots(ctx context.Context) map[string]error {
 	d.logRoots = freshRoots
 	d.rootDataFull = len(d.logRoots) == len(d.logClients)
 	// Merge individual root-pools into a unified one
-	d.rootPool = ctfe.NewPEMCertPool()
+	d.rootPool = x509util.NewPEMCertPool()
 	for _, pool := range d.logRoots {
 		for _, c := range pool.RawCertificates() {
 			d.rootPool.AddCert(c)
@@ -362,7 +363,7 @@ func NewDistributor(ll *loglist2.LogList, plc ctpolicy.CTPolicy, lcBuilder LogCl
 	d.pendingLogsPolicy = pendingLogsPolicy{}
 	d.logClients = make(map[string]client.AddLogClient)
 	d.logRoots = make(loglist2.LogRoots)
-	d.rootPool = ctfe.NewPEMCertPool()
+	d.rootPool = x509util.NewPEMCertPool()
 
 	// Build clients for each of the Logs. Also build log-to-id map.
 	if err := d.buildLogClients(lcBuilder, d.usableLl); err != nil {
diff --git a/trillian/ctfe/cert_checker.go b/trillian/ctfe/cert_checker.go
index e9fe1e47..4fd43f4b 100644
--- a/trillian/ctfe/cert_checker.go
+++ b/trillian/ctfe/cert_checker.go
@@ -22,6 +22,7 @@ import (
 
 	"github.com/google/certificate-transparency-go/asn1"
 	"github.com/google/certificate-transparency-go/x509"
+	"github.com/google/certificate-transparency-go/x509util"
 )
 
 // IsPrecertificate tests if a certificate is a pre-certificate as defined in CT.
@@ -49,7 +50,7 @@ func IsPrecertificate(cert *x509.Certificate) (bool, error) {
 func ValidateChain(rawChain [][]byte, validationOpts CertValidationOpts) ([]*x509.Certificate, error) {
 	// First make sure the certs parse as X.509
 	chain := make([]*x509.Certificate, 0, len(rawChain))
-	intermediatePool := NewPEMCertPool()
+	intermediatePool := x509util.NewPEMCertPool()
 
 	for i, certBytes := range rawChain {
 		cert, err := x509.ParseCertificate(certBytes)
diff --git a/trillian/ctfe/handlers.go b/trillian/ctfe/handlers.go
index 9dfd339a..66b59101 100644
--- a/trillian/ctfe/handlers.go
+++ b/trillian/ctfe/handlers.go
@@ -35,6 +35,7 @@ import (
 	"github.com/google/certificate-transparency-go/tls"
 	"github.com/google/certificate-transparency-go/trillian/util"
 	"github.com/google/certificate-transparency-go/x509"
+	"github.com/google/certificate-transparency-go/x509util"
 	"github.com/google/trillian"
 	"github.com/google/trillian/monitoring"
 	"github.com/google/trillian/types"
@@ -210,7 +211,7 @@ func (a AppHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 // CertValidationOpts contains various parameters for certificate chain validation
 type CertValidationOpts struct {
 	// trustedRoots is a pool of certificates that defines the roots the CT log will accept
-	trustedRoots *PEMCertPool
+	trustedRoots *x509util.PEMCertPool
 	// currentTime is the time used for checking a certificate's validity period
 	// against. If it's zero then time.Now() is used. Only for testing.
 	currentTime time.Time
@@ -234,7 +235,7 @@ type CertValidationOpts struct {
 }
 
 // NewCertValidationOpts builds validation options based on parameters.
-func NewCertValidationOpts(trustedRoots *PEMCertPool, currentTime time.Time, rejectExpired bool, rejectUnexpired bool, notAfterStart *time.Time, notAfterLimit *time.Time, acceptOnlyCA bool, extKeyUsages []x509.ExtKeyUsage) CertValidationOpts {
+func NewCertValidationOpts(trustedRoots *x509util.PEMCertPool, currentTime time.Time, rejectExpired bool, rejectUnexpired bool, notAfterStart *time.Time, notAfterLimit *time.Time, acceptOnlyCA bool, extKeyUsages []x509.ExtKeyUsage) CertValidationOpts {
 	var vOpts CertValidationOpts
 	vOpts.trustedRoots = trustedRoots
 	vOpts.currentTime = currentTime
diff --git a/trillian/ctfe/instance.go b/trillian/ctfe/instance.go
index ba2847ce..c717afdd 100644
--- a/trillian/ctfe/instance.go
+++ b/trillian/ctfe/instance.go
@@ -29,6 +29,7 @@ import (
 	"github.com/google/certificate-transparency-go/schedule"
 	"github.com/google/certificate-transparency-go/trillian/util"
 	"github.com/google/certificate-transparency-go/x509"
+	"github.com/google/certificate-transparency-go/x509util"
 	"github.com/google/trillian"
 	"github.com/google/trillian/crypto/keys"
 	"github.com/google/trillian/monitoring"
@@ -112,7 +113,7 @@ func setUpLogInfo(ctx context.Context, opts InstanceOptions) (*logInfo, error) {
 		return nil, errors.New("need to specify RootsPemFile")
 	}
 	// Load the trusted roots.
-	roots := NewPEMCertPool()
+	roots := x509util.NewPEMCertPool()
 	for _, pemFile := range cfg.RootsPemFile {
 		if err := roots.AppendCertsFromPEMFile(pemFile); err != nil {
 			return nil, fmt.Errorf("failed to read trusted roots: %v", err)
diff --git a/trillian/integration/copier.go b/trillian/integration/copier.go
index 731257f7..9fc9b8b8 100644
--- a/trillian/integration/copier.go
+++ b/trillian/integration/copier.go
@@ -25,9 +25,9 @@ import (
 	"github.com/golang/glog"
 	"github.com/google/certificate-transparency-go/client"
 	"github.com/google/certificate-transparency-go/scanner"
-	"github.com/google/certificate-transparency-go/trillian/ctfe"
 	"github.com/google/certificate-transparency-go/trillian/ctfe/configpb"
 	"github.com/google/certificate-transparency-go/x509"
+	"github.com/google/certificate-transparency-go/x509util"
 
 	ct "github.com/google/certificate-transparency-go"
 )
@@ -36,7 +36,7 @@ import (
 // from a source log.
 type CopyChainGenerator struct {
 	start, limit             time.Time
-	sourceRoots, targetRoots *ctfe.PEMCertPool
+	sourceRoots, targetRoots *x509util.PEMCertPool
 	certs, precerts          chan []ct.ASN1Cert
 }
 
@@ -90,7 +90,7 @@ func NewCopyChainGeneratorFromOpts(ctx context.Context, client *client.LogClient
 		limit = cfg.NotAfterLimit.AsTime()
 	}
 
-	targetPool := ctfe.NewPEMCertPool()
+	targetPool := x509util.NewPEMCertPool()
 	for _, pemFile := range cfg.RootsPemFile {
 		if err := targetPool.AppendCertsFromPEMFile(pemFile); err != nil {
 			return nil, fmt.Errorf("failed to read trusted roots for target log: %v", err)
@@ -107,7 +107,7 @@ func NewCopyChainGeneratorFromOpts(ctx context.Context, client *client.LogClient
 	if err != nil {
 		return nil, fmt.Errorf("failed to read trusted roots for source log: %v", err)
 	}
-	sourcePool := ctfe.NewPEMCertPool()
+	sourcePool := x509util.NewPEMCertPool()
 	for _, root := range srcRoots {
 		cert, err := x509.ParseCertificate(root.Data)
 		if x509.IsFatal(err) {
diff --git a/trillian/ctfe/pem_cert_pool.go b/x509util/pem_cert_pool.go
similarity index 99%
rename from trillian/ctfe/pem_cert_pool.go
rename to x509util/pem_cert_pool.go
index 1d07c638..0f814ad5 100644
--- a/trillian/ctfe/pem_cert_pool.go
+++ b/x509util/pem_cert_pool.go
@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package ctfe
+package x509util
 
 import (
 	"crypto/sha256"


Test patch:
diff --git a/loglist/logfilter_test.go b/loglist/logfilter_test.go
index c65b044f..64e03e46 100644
--- a/loglist/logfilter_test.go
+++ b/loglist/logfilter_test.go
@@ -17,7 +17,6 @@ import (
 	"testing"
 
 	"github.com/google/certificate-transparency-go/testdata"
-	"github.com/google/certificate-transparency-go/trillian/ctfe"
 	"github.com/google/certificate-transparency-go/x509"
 	"github.com/google/certificate-transparency-go/x509util"
 
@@ -60,10 +59,10 @@ func TestActiveLogs(t *testing.T) {
 
 func artificialRoots(source string) LogRoots {
 	roots := LogRoots{
-		"log.bob.io":                   ctfe.NewPEMCertPool(),
-		"ct.googleapis.com/racketeer/": ctfe.NewPEMCertPool(),
-		"ct.googleapis.com/rocketeer/": ctfe.NewPEMCertPool(),
-		"ct.googleapis.com/aviator/":   ctfe.NewPEMCertPool(),
+		"log.bob.io":                   x509util.NewPEMCertPool(),
+		"ct.googleapis.com/racketeer/": x509util.NewPEMCertPool(),
+		"ct.googleapis.com/rocketeer/": x509util.NewPEMCertPool(),
+		"ct.googleapis.com/aviator/":   x509util.NewPEMCertPool(),
 	}
 	roots["log.bob.io"].AppendCertsFromPEM([]byte(source))
 	return roots
diff --git a/loglist2/logfilter_test.go b/loglist2/logfilter_test.go
index 1b72e7b2..3505033f 100644
--- a/loglist2/logfilter_test.go
+++ b/loglist2/logfilter_test.go
@@ -19,7 +19,6 @@ import (
 	"time"
 
 	"github.com/google/certificate-transparency-go/testdata"
-	"github.com/google/certificate-transparency-go/trillian/ctfe"
 	"github.com/google/certificate-transparency-go/x509"
 	"github.com/google/certificate-transparency-go/x509util"
 
@@ -96,11 +95,11 @@ func TestSelectPendingAndQualified(t *testing.T) {
 
 func artificialRoots(source string) LogRoots {
 	roots := LogRoots{
-		"https://log.bob.io":                        ctfe.NewPEMCertPool(),
-		"https://ct.googleapis.com/racketeer/":      ctfe.NewPEMCertPool(),
-		"https://ct.googleapis.com/rocketeer/":      ctfe.NewPEMCertPool(),
-		"https://ct.googleapis.com/aviator/":        ctfe.NewPEMCertPool(),
-		"https://ct.googleapis.com/logs/argon2020/": ctfe.NewPEMCertPool(),
+		"https://log.bob.io":                        x509util.NewPEMCertPool(),
+		"https://ct.googleapis.com/racketeer/":      x509util.NewPEMCertPool(),
+		"https://ct.googleapis.com/rocketeer/":      x509util.NewPEMCertPool(),
+		"https://ct.googleapis.com/aviator/":        x509util.NewPEMCertPool(),
+		"https://ct.googleapis.com/logs/argon2020/": x509util.NewPEMCertPool(),
 	}
 	roots["https://log.bob.io"].AppendCertsFromPEM([]byte(source))
 	return roots
diff --git a/loglist3/logfilter_test.go b/loglist3/logfilter_test.go
index 0eeb527b..94097db4 100644
--- a/loglist3/logfilter_test.go
+++ b/loglist3/logfilter_test.go
@@ -19,7 +19,6 @@ import (
 	"time"
 
 	"github.com/google/certificate-transparency-go/testdata"
-	"github.com/google/certificate-transparency-go/trillian/ctfe"
 	"github.com/google/certificate-transparency-go/x509"
 	"github.com/google/certificate-transparency-go/x509util"
 
@@ -96,11 +95,11 @@ func TestSelectPendingAndQualified(t *testing.T) {
 
 func artificialRoots(source string) LogRoots {
 	roots := LogRoots{
-		"https://log.bob.io":                        ctfe.NewPEMCertPool(),
-		"https://ct.googleapis.com/racketeer/":      ctfe.NewPEMCertPool(),
-		"https://ct.googleapis.com/rocketeer/":      ctfe.NewPEMCertPool(),
-		"https://ct.googleapis.com/aviator/":        ctfe.NewPEMCertPool(),
-		"https://ct.googleapis.com/logs/argon2020/": ctfe.NewPEMCertPool(),
+		"https://log.bob.io":                        x509util.NewPEMCertPool(),
+		"https://ct.googleapis.com/racketeer/":      x509util.NewPEMCertPool(),
+		"https://ct.googleapis.com/rocketeer/":      x509util.NewPEMCertPool(),
+		"https://ct.googleapis.com/aviator/":        x509util.NewPEMCertPool(),
+		"https://ct.googleapis.com/logs/argon2020/": x509util.NewPEMCertPool(),
 	}
 	roots["https://log.bob.io"].AppendCertsFromPEM([]byte(source))
 	return roots
diff --git a/trillian/ctfe/cert_checker_test.go b/trillian/ctfe/cert_checker_test.go
index 44f5212d..7be1c21d 100644
--- a/trillian/ctfe/cert_checker_test.go
+++ b/trillian/ctfe/cert_checker_test.go
@@ -101,7 +101,7 @@ func TestIsPrecertificate(t *testing.T) {
 }
 
 func TestValidateChain(t *testing.T) {
-	fakeCARoots := NewPEMCertPool()
+	fakeCARoots := x509util.NewPEMCertPool()
 	if !fakeCARoots.AppendCertsFromPEM([]byte(testonly.FakeCACertPEM)) {
 		t.Fatal("failed to load fake root")
 	}
@@ -279,7 +279,7 @@ func TestValidateChain(t *testing.T) {
 }
 
 func TestCA(t *testing.T) {
-	fakeCARoots := NewPEMCertPool()
+	fakeCARoots := x509util.NewPEMCertPool()
 	if !fakeCARoots.AppendCertsFromPEM([]byte(testonly.FakeCACertPEM)) {
 		t.Fatal("failed to load fake root")
 	}
@@ -337,7 +337,7 @@ func TestCA(t *testing.T) {
 }
 
 func TestNotAfterRange(t *testing.T) {
-	fakeCARoots := NewPEMCertPool()
+	fakeCARoots := x509util.NewPEMCertPool()
 	if !fakeCARoots.AppendCertsFromPEM([]byte(testonly.FakeCACertPEM)) {
 		t.Fatal("failed to load fake root")
 	}
@@ -401,7 +401,7 @@ func TestNotAfterRange(t *testing.T) {
 }
 
 func TestRejectExpiredUnexpired(t *testing.T) {
-	fakeCARoots := NewPEMCertPool()
+	fakeCARoots := x509util.NewPEMCertPool()
 	// Validity period: Jul 11, 2016 - Jul 11, 2017.
 	if !fakeCARoots.AppendCertsFromPEM([]byte(testonly.FakeCACertPEM)) {
 		t.Fatal("failed to load fake root")
@@ -570,7 +570,7 @@ func TestCMPreIssuedCert(t *testing.T) {
 	if err != nil {
 		t.Fatalf("failed to parse root cert: %v", err)
 	}
-	cmRoot := NewPEMCertPool()
+	cmRoot := x509util.NewPEMCertPool()
 	cmRoot.AddCert(root)
 
 	for _, tc := range []struct {
diff --git a/trillian/ctfe/handlers_test.go b/trillian/ctfe/handlers_test.go
index f812a599..bba5efba 100644
--- a/trillian/ctfe/handlers_test.go
+++ b/trillian/ctfe/handlers_test.go
@@ -109,7 +109,7 @@ const remoteQuotaUser = "Moneybags"
 
 type handlerTestInfo struct {
 	mockCtrl *gomock.Controller
-	roots    *PEMCertPool
+	roots    *x509util.PEMCertPool
 	client   *mockclient.MockTrillianLogClient
 	li       *logInfo
 }
@@ -154,7 +154,7 @@ func setupTest(t *testing.T, pemRoots []string, signer crypto.Signer) handlerTes
 	t.Helper()
 	info := handlerTestInfo{
 		mockCtrl: gomock.NewController(t),
-		roots:    NewPEMCertPool(),
+		roots:    x509util.NewPEMCertPool(),
 	}
 
 	info.client = mockclient.NewMockTrillianLogClient(info.mockCtrl)
@@ -2172,7 +2172,7 @@ func TestGetEntryAndProof(t *testing.T) {
 	}
 }
 
-func createJSONChain(t *testing.T, p PEMCertPool) io.Reader {
+func createJSONChain(t *testing.T, p x509util.PEMCertPool) io.Reader {
 	t.Helper()
 	var req ct.AddChainRequest
 	for _, rawCert := range p.RawCertificates() {
@@ -2273,9 +2273,9 @@ func makeGetRootResponseForTest(t *testing.T, stamp, treeSize int64, hash []byte
 	}
 }
 
-func loadCertsIntoPoolOrDie(t *testing.T, certs []string) *PEMCertPool {
+func loadCertsIntoPoolOrDie(t *testing.T, certs []string) *x509util.PEMCertPool {
 	t.Helper()
-	pool := NewPEMCertPool()
+	pool := x509util.NewPEMCertPool()
 	for _, cert := range certs {
 		if !pool.AppendCertsFromPEM([]byte(cert)) {
 			t.Fatalf("couldn't parse test certs: %v", certs)
diff --git a/trillian/ctfe/pem_cert_pool_test.go b/x509util/pem_cert_pool_test.go
similarity index 99%
rename from trillian/ctfe/pem_cert_pool_test.go
rename to x509util/pem_cert_pool_test.go
index 896e70b4..c07e451c 100644
--- a/trillian/ctfe/pem_cert_pool_test.go
+++ b/x509util/pem_cert_pool_test.go
@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package ctfe
+package x509util
 
 import (
 	"encoding/pem"