Make SA and PA fields in CA unexported. (#2894)
An early design mistake meant that some fields of our services were exported unnecessarily. In particular, fields storing handles of other services (e.g. "SA" or "PA") were exported. This introduces the possibility of race conditions, though in practice these fields are set at startup and never modified concurrently. We'd like to go through our codebase and change these all to unexported fields, set at construction time. This is one step in that process.
This commit is contained in:
Jacob Hoffman-Andrews
Daniel McCarney
parent
ac63c78313
commit
f8e9bf1144
12
ca/ca.go
12
ca/ca.go
|
|
@ -101,8 +101,8 @@ type CertificateAuthorityImpl struct {
|
|||
issuers map[string]*internalIssuer
|
||||
// The common name of the default issuer cert
|
||||
defaultIssuer *internalIssuer
|
||||
SA certificateStorage
|
||||
PA core.PolicyAuthority
|
||||
sa certificateStorage
|
||||
pa core.PolicyAuthority
|
||||
keyPolicy goodkey.KeyPolicy
|
||||
clk clock.Clock
|
||||
log blog.Logger
|
||||
|
|
@ -172,6 +172,8 @@ func makeInternalIssuers(
|
|||
// for any of the issuer certificates provided.
|
||||
func NewCertificateAuthorityImpl(
|
||||
config cmd.CAConfig,
|
||||
sa certificateStorage,
|
||||
pa core.PolicyAuthority,
|
||||
clk clock.Clock,
|
||||
stats metrics.Scope,
|
||||
issuers []Issuer,
|
||||
|
|
@ -234,6 +236,8 @@ func NewCertificateAuthorityImpl(
|
|||
stats.MustRegister(signatureCount)
|
||||
|
||||
ca = &CertificateAuthorityImpl{
|
||||
sa: sa,
|
||||
pa: pa,
|
||||
issuers: internalIssuers,
|
||||
defaultIssuer: defaultIssuer,
|
||||
rsaProfile: rsaProfile,
|
||||
|
|
@ -400,7 +404,7 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(ctx context.Context, issueR
|
|||
csr,
|
||||
ca.maxNames,
|
||||
&ca.keyPolicy,
|
||||
ca.PA,
|
||||
ca.pa,
|
||||
ca.forceCNFromSAN,
|
||||
regID,
|
||||
); err != nil {
|
||||
|
|
@ -520,7 +524,7 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(ctx context.Context, issueR
|
|||
}
|
||||
|
||||
// Store the cert with the certificate authority, if provided
|
||||
_, err = ca.SA.AddCertificate(ctx, certDER, regID, ocspResp)
|
||||
_, err = ca.sa.AddCertificate(ctx, certDER, regID, ocspResp)
|
||||
if err != nil {
|
||||
err = berrors.InternalServerError(err.Error())
|
||||
// Note: This log line is parsed by cmd/orphan-finder. If you make any
|
||||
|
|
|
|||
|
|
@ -239,6 +239,8 @@ func TestFailNoSerial(t *testing.T) {
|
|||
testCtx.caConfig.SerialPrefix = 0
|
||||
_, err := NewCertificateAuthorityImpl(
|
||||
testCtx.caConfig,
|
||||
nil,
|
||||
nil,
|
||||
testCtx.fc,
|
||||
testCtx.stats,
|
||||
testCtx.issuers,
|
||||
|
|
@ -249,8 +251,11 @@ func TestFailNoSerial(t *testing.T) {
|
|||
|
||||
func TestIssueCertificate(t *testing.T) {
|
||||
testCtx := setup(t)
|
||||
sa := &mockSA{}
|
||||
ca, err := NewCertificateAuthorityImpl(
|
||||
testCtx.caConfig,
|
||||
sa,
|
||||
testCtx.pa,
|
||||
testCtx.fc,
|
||||
testCtx.stats,
|
||||
testCtx.issuers,
|
||||
|
|
@ -258,9 +263,6 @@ func TestIssueCertificate(t *testing.T) {
|
|||
testCtx.logger)
|
||||
test.AssertNotError(t, err, "Failed to create CA")
|
||||
ca.forceCNFromSAN = false
|
||||
ca.PA = testCtx.pa
|
||||
sa := &mockSA{}
|
||||
ca.SA = sa
|
||||
|
||||
// Sign CSR
|
||||
issuedCert, err := ca.IssueCertificate(ctx, &caPB.IssueCertificateRequest{Csr: CNandSANCSR, RegistrationID: &arbitraryRegID})
|
||||
|
|
@ -308,16 +310,17 @@ func TestIssueCertificateMultipleIssuers(t *testing.T) {
|
|||
Cert: caCert,
|
||||
},
|
||||
}
|
||||
sa := &mockSA{}
|
||||
ca, err := NewCertificateAuthorityImpl(
|
||||
testCtx.caConfig,
|
||||
sa,
|
||||
testCtx.pa,
|
||||
testCtx.fc,
|
||||
testCtx.stats,
|
||||
newIssuers,
|
||||
testCtx.keyPolicy,
|
||||
testCtx.logger)
|
||||
test.AssertNotError(t, err, "Failed to remake CA")
|
||||
ca.PA = testCtx.pa
|
||||
ca.SA = &mockSA{}
|
||||
|
||||
issuedCert, err := ca.IssueCertificate(ctx, &caPB.IssueCertificateRequest{Csr: CNandSANCSR, RegistrationID: &arbitraryRegID})
|
||||
test.AssertNotError(t, err, "Failed to sign certificate")
|
||||
|
|
@ -331,16 +334,17 @@ func TestIssueCertificateMultipleIssuers(t *testing.T) {
|
|||
|
||||
func TestOCSP(t *testing.T) {
|
||||
testCtx := setup(t)
|
||||
sa := &mockSA{}
|
||||
ca, err := NewCertificateAuthorityImpl(
|
||||
testCtx.caConfig,
|
||||
sa,
|
||||
testCtx.pa,
|
||||
testCtx.fc,
|
||||
testCtx.stats,
|
||||
testCtx.issuers,
|
||||
testCtx.keyPolicy,
|
||||
testCtx.logger)
|
||||
test.AssertNotError(t, err, "Failed to create CA")
|
||||
ca.PA = testCtx.pa
|
||||
ca.SA = &mockSA{}
|
||||
|
||||
issueReq := caPB.IssueCertificateRequest{Csr: CNandSANCSR, RegistrationID: &arbitraryRegID}
|
||||
|
||||
|
|
@ -382,14 +386,14 @@ func TestOCSP(t *testing.T) {
|
|||
}
|
||||
ca, err = NewCertificateAuthorityImpl(
|
||||
testCtx.caConfig,
|
||||
sa,
|
||||
testCtx.pa,
|
||||
testCtx.fc,
|
||||
testCtx.stats,
|
||||
newIssuers,
|
||||
testCtx.keyPolicy,
|
||||
testCtx.logger)
|
||||
test.AssertNotError(t, err, "Failed to remake CA")
|
||||
ca.PA = testCtx.pa
|
||||
ca.SA = &mockSA{}
|
||||
|
||||
// Now issue a new cert, signed by newIssuerCert
|
||||
newCert, err := ca.IssueCertificate(ctx, &issueReq)
|
||||
|
|
@ -466,16 +470,17 @@ func TestInvalidCSRs(t *testing.T) {
|
|||
}
|
||||
|
||||
testCtx := setup(t)
|
||||
sa := &mockSA{}
|
||||
ca, err := NewCertificateAuthorityImpl(
|
||||
testCtx.caConfig,
|
||||
sa,
|
||||
testCtx.pa,
|
||||
testCtx.fc,
|
||||
testCtx.stats,
|
||||
testCtx.issuers,
|
||||
testCtx.keyPolicy,
|
||||
testCtx.logger)
|
||||
test.AssertNotError(t, err, "Failed to create CA")
|
||||
ca.PA = testCtx.pa
|
||||
ca.SA = &mockSA{}
|
||||
|
||||
for _, testCase := range testCases {
|
||||
t.Run(testCase.name, func(t *testing.T) {
|
||||
|
|
@ -489,16 +494,17 @@ func TestInvalidCSRs(t *testing.T) {
|
|||
|
||||
func TestRejectValidityTooLong(t *testing.T) {
|
||||
testCtx := setup(t)
|
||||
sa := &mockSA{}
|
||||
ca, err := NewCertificateAuthorityImpl(
|
||||
testCtx.caConfig,
|
||||
sa,
|
||||
testCtx.pa,
|
||||
testCtx.fc,
|
||||
testCtx.stats,
|
||||
testCtx.issuers,
|
||||
testCtx.keyPolicy,
|
||||
testCtx.logger)
|
||||
test.AssertNotError(t, err, "Failed to create CA")
|
||||
ca.PA = testCtx.pa
|
||||
ca.SA = &mockSA{}
|
||||
|
||||
// This time is a few minutes before the notAfter in testdata/ca_cert.pem
|
||||
future, err := time.Parse(time.RFC3339, "2025-02-10T00:30:00Z")
|
||||
|
|
@ -513,8 +519,11 @@ func TestRejectValidityTooLong(t *testing.T) {
|
|||
|
||||
func TestAllowNoCN(t *testing.T) {
|
||||
testCtx := setup(t)
|
||||
sa := &mockSA{}
|
||||
ca, err := NewCertificateAuthorityImpl(
|
||||
testCtx.caConfig,
|
||||
sa,
|
||||
testCtx.pa,
|
||||
testCtx.fc,
|
||||
testCtx.stats,
|
||||
testCtx.issuers,
|
||||
|
|
@ -522,8 +531,6 @@ func TestAllowNoCN(t *testing.T) {
|
|||
testCtx.logger)
|
||||
test.AssertNotError(t, err, "Couldn't create new CA")
|
||||
ca.forceCNFromSAN = false
|
||||
ca.PA = testCtx.pa
|
||||
ca.SA = &mockSA{}
|
||||
|
||||
issueReq := caPB.IssueCertificateRequest{Csr: NoCNCSR, RegistrationID: &arbitraryRegID}
|
||||
issuedCert, err := ca.IssueCertificate(ctx, &issueReq)
|
||||
|
|
@ -555,15 +562,16 @@ func TestAllowNoCN(t *testing.T) {
|
|||
func TestProfileSelection(t *testing.T) {
|
||||
testCtx := setup(t)
|
||||
testCtx.caConfig.MaxNames = 3
|
||||
sa := &mockSA{}
|
||||
ca, _ := NewCertificateAuthorityImpl(
|
||||
testCtx.caConfig,
|
||||
sa,
|
||||
testCtx.pa,
|
||||
testCtx.fc,
|
||||
testCtx.stats,
|
||||
testCtx.issuers,
|
||||
testCtx.keyPolicy,
|
||||
testCtx.logger)
|
||||
ca.PA = testCtx.pa
|
||||
ca.SA = &mockSA{}
|
||||
|
||||
testCases := []struct {
|
||||
CSR []byte
|
||||
|
|
@ -603,15 +611,16 @@ func TestExtensions(t *testing.T) {
|
|||
testCtx := setup(t)
|
||||
testCtx.caConfig.MaxNames = 3
|
||||
|
||||
sa := &mockSA{}
|
||||
ca, err := NewCertificateAuthorityImpl(
|
||||
testCtx.caConfig,
|
||||
sa,
|
||||
testCtx.pa,
|
||||
testCtx.fc,
|
||||
testCtx.stats,
|
||||
testCtx.issuers,
|
||||
testCtx.keyPolicy,
|
||||
testCtx.logger)
|
||||
ca.PA = testCtx.pa
|
||||
ca.SA = &mockSA{}
|
||||
|
||||
sign := func(csr []byte) *x509.Certificate {
|
||||
ca.csrExtensionCount.Reset()
|
||||
|
|
|
|||
|
|
@ -156,16 +156,6 @@ func main() {
|
|||
kp, err := goodkey.NewKeyPolicy(c.CA.WeakKeyFile)
|
||||
cmd.FailOnError(err, "Unable to create key policy")
|
||||
|
||||
cai, err := ca.NewCertificateAuthorityImpl(
|
||||
c.CA,
|
||||
clock.Default(),
|
||||
scope,
|
||||
issuers,
|
||||
kp,
|
||||
logger)
|
||||
cmd.FailOnError(err, "Failed to create CA impl")
|
||||
cai.PA = pa
|
||||
|
||||
var tls *tls.Config
|
||||
if c.CA.TLS.CertFile != nil {
|
||||
tls, err = c.CA.TLS.Load()
|
||||
|
|
@ -174,7 +164,18 @@ func main() {
|
|||
|
||||
conn, err := bgrpc.ClientSetup(c.CA.SAService, tls, scope)
|
||||
cmd.FailOnError(err, "Failed to load credentials and create gRPC connection to SA")
|
||||
cai.SA = bgrpc.NewStorageAuthorityClient(sapb.NewStorageAuthorityClient(conn))
|
||||
sa := bgrpc.NewStorageAuthorityClient(sapb.NewStorageAuthorityClient(conn))
|
||||
|
||||
cai, err := ca.NewCertificateAuthorityImpl(
|
||||
c.CA,
|
||||
sa,
|
||||
pa,
|
||||
clock.Default(),
|
||||
scope,
|
||||
issuers,
|
||||
kp,
|
||||
logger)
|
||||
cmd.FailOnError(err, "Failed to create CA impl")
|
||||
|
||||
var caSrv *grpc.Server
|
||||
if c.CA.GRPCCA != nil {
|
||||
|
|
|
|||
Loading…
Reference in New Issue