Skip to content

Commit

Permalink
Update CA RPC interface to proto3 (#4951)
Browse files Browse the repository at this point in the history 
This updates the ca.proto to use proto3 syntax, and updates
all clients of the autogenerated code to use the new types. In
particular, it removes indirection from built-in types (proto3
uses ints, rather than pointers to ints, for example).

It also updates a few instances where tests were being
conducted to see if various object fields were nil to instead
check for those fields' new zero-value.

Fixes #4940
  • Loading branch information
@aarongable
aarongable committed Jul 14, 2020
1 parent dea2f6e commit 24e782e
Show file tree
Hide file tree
Showing 9 changed files with 110 additions and 125 deletions.
25 changes: 12 additions & 13 deletions ca/ca.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -437,16 +437,16 @@ func (ca *CertificateAuthorityImpl) GenerateOCSP(ctx context.Context, req *caPB.
// that didn't have an IssuerID set when they were created. Once this feature
// has been enabled for a full OCSP lifetime cycle we can remove this
// functionality.
if features.Enabled(features.StoreIssuerInfo) && req.IssuerID != nil {
serialInt, err := core.StringToSerial(*req.Serial)
if features.Enabled(features.StoreIssuerInfo) && req.IssuerID != 0 {
serialInt, err := core.StringToSerial(req.Serial)
if err != nil {
return nil, err
}
serial = serialInt
var ok bool
issuer, ok = ca.idToIssuer[*req.IssuerID]
issuer, ok = ca.idToIssuer[req.IssuerID]
if !ok {
return nil, fmt.Errorf("This CA doesn't have an issuer cert with ID %d", *req.IssuerID)
return nil, fmt.Errorf("This CA doesn't have an issuer cert with ID %d", req.IssuerID)
}
} else {
cert, err := x509.ParseCertificate(req.CertDER)
Expand All @@ -471,14 +471,14 @@ func (ca *CertificateAuthorityImpl) GenerateOCSP(ctx context.Context, req *caPB.

now := ca.clk.Now().Truncate(time.Hour)
tbsResponse := ocsp.Response{
Status: ocspStatusToCode[*req.Status],
Status: ocspStatusToCode[req.Status],
SerialNumber: serial,
ThisUpdate: now,
NextUpdate: now.Add(ca.ocspLifetime),
}
if tbsResponse.Status == ocsp.Revoked {
tbsResponse.RevokedAt = time.Unix(0, *req.RevokedAt)
tbsResponse.RevocationReason = int(*req.Reason)
tbsResponse.RevokedAt = time.Unix(0, req.RevokedAt)
tbsResponse.RevocationReason = int(req.Reason)
}

ocspResponse, err := ocsp.CreateResponse(issuer.cert, issuer.cert, tbsResponse, issuer.ocspSigner)
Expand All @@ -495,7 +495,7 @@ func (ca *CertificateAuthorityImpl) IssuePrecertificate(ctx context.Context, iss
return nil, err
}

regID := *issueReq.RegistrationID
regID := issueReq.RegistrationID

serialHex := core.SerialToString(serialBigInt)
nowNanos := ca.clk.Now().UnixNano()
Expand All @@ -515,10 +515,9 @@ func (ca *CertificateAuthorityImpl) IssuePrecertificate(ctx context.Context, iss
return nil, err
}

status := string(core.OCSPStatusGood)
ocspResp, err := ca.GenerateOCSP(ctx, &caPB.GenerateOCSPRequest{
CertDER: precertDER,
Status: &status,
Status: string(core.OCSPStatusGood),
})
if err != nil {
err = berrors.InternalServerError(err.Error())
Expand All @@ -545,7 +544,7 @@ func (ca *CertificateAuthorityImpl) IssuePrecertificate(ctx context.Context, iss
// Note: This log line is parsed by cmd/orphan-finder. If you make any
// changes here, you should make sure they are reflected in orphan-finder.
ca.log.AuditErrf("Failed RPC to store at SA, orphaning precertificate: serial=[%s] cert=[%s] err=[%v], regID=[%d], orderID=[%d]",
serialHex, hex.EncodeToString(precertDER), err, *issueReq.RegistrationID, *issueReq.OrderID)
serialHex, hex.EncodeToString(precertDER), err, issueReq.RegistrationID, issueReq.OrderID)
if ca.orphanQueue != nil {
ca.queueOrphan(&orphanedCert{
DER: precertDER,
Expand Down Expand Up @@ -623,7 +622,7 @@ func (ca *CertificateAuthorityImpl) IssueCertificateForPrecertificate(ctx contex
ca.log.AuditInfof("Signing success: serial=[%s] names=[%s] certificate=[%s]",
serialHex, strings.Join(precert.DNSNames, ", "), hex.EncodeToString(req.DER),
hex.EncodeToString(certDER))
return ca.storeCertificate(ctx, *req.RegistrationID, *req.OrderID, precert.SerialNumber, certDER)
return ca.storeCertificate(ctx, req.RegistrationID, req.OrderID, precert.SerialNumber, certDER)
}

type validity struct {
Expand Down Expand Up @@ -667,7 +666,7 @@ func (ca *CertificateAuthorityImpl) issuePrecertificateInner(ctx context.Context
&ca.keyPolicy,
ca.pa,
ca.forceCNFromSAN,
*issueReq.RegistrationID,
issueReq.RegistrationID,
); err != nil {
ca.log.AuditErr(err.Error())
// VerifyCSR returns berror instances that can be passed through as-is
Expand Down
67 changes: 30 additions & 37 deletions ca/ca_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,6 @@ var (
// * DNSNames = example.com, example2.com
ECDSACSR = mustRead("./testdata/ecdsa.der.csr")

// This is never modified, but it must be a var instead of a const so we can make references to it.
arbitraryRegID int64 = 1001

// OIDExtensionCTPoison is defined in RFC 6962 s3.1.
OIDExtensionCTPoison = asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 11129, 2, 4, 3}

Expand All @@ -125,6 +122,8 @@ var (
}
)

const arbitraryRegID int64 = 1001

// CFSSL config
const rsaProfileName = "rsaEE"
const ecdsaProfileName = "ecdsaEE"
Expand Down Expand Up @@ -348,7 +347,7 @@ func TestIssuePrecertificate(t *testing.T) {
req, err := x509.ParseCertificateRequest(testCase.csr)
test.AssertNotError(t, err, "Certificate request failed to parse")

issueReq := &caPB.IssueCertificateRequest{Csr: testCase.csr, RegistrationID: &arbitraryRegID}
issueReq := &caPB.IssueCertificateRequest{Csr: testCase.csr, RegistrationID: arbitraryRegID}

var certDER []byte
response, err := ca.IssuePrecertificate(ctx, issueReq)
Expand Down Expand Up @@ -456,7 +455,7 @@ func TestMultipleIssuers(t *testing.T) {
nil)
test.AssertNotError(t, err, "Failed to remake CA")

issuedCert, err := ca.IssuePrecertificate(ctx, &caPB.IssueCertificateRequest{Csr: CNandSANCSR, RegistrationID: &arbitraryRegID})
issuedCert, err := ca.IssuePrecertificate(ctx, &caPB.IssueCertificateRequest{Csr: CNandSANCSR, RegistrationID: arbitraryRegID})
test.AssertNotError(t, err, "Failed to issue certificate")

cert, err := x509.ParseCertificate(issuedCert.DER)
Expand All @@ -481,16 +480,15 @@ func TestOCSP(t *testing.T) {
nil)
test.AssertNotError(t, err, "Failed to create CA")

issueReq := caPB.IssueCertificateRequest{Csr: CNandSANCSR, RegistrationID: &arbitraryRegID}
issueReq := caPB.IssueCertificateRequest{Csr: CNandSANCSR, RegistrationID: arbitraryRegID}

cert, err := ca.IssuePrecertificate(ctx, &issueReq)
test.AssertNotError(t, err, "Failed to issue")
parsedCert, err := x509.ParseCertificate(cert.DER)
test.AssertNotError(t, err, "Failed to parse cert")
status := string(core.OCSPStatusGood)
ocspResp, err := ca.GenerateOCSP(ctx, &caPB.GenerateOCSPRequest{
CertDER: cert.DER,
Status: &status,
Status: string(core.OCSPStatusGood),
})
test.AssertNotError(t, err, "Failed to generate OCSP")
parsed, err := ocsp.ParseResponse(ocspResp.Response, caCert)
Expand All @@ -502,7 +500,7 @@ func TestOCSP(t *testing.T) {
// Test that signatures are checked.
_, err = ca.GenerateOCSP(ctx, &caPB.GenerateOCSPRequest{
CertDER: append(cert.DER, byte(0)),
Status: &status,
Status: string(core.OCSPStatusGood),
})
test.AssertError(t, err, "Generated OCSP for cert with bad signature")

Expand Down Expand Up @@ -545,7 +543,7 @@ func TestOCSP(t *testing.T) {
// should be signed by caCert.
ocspResp2, err := ca.GenerateOCSP(ctx, &caPB.GenerateOCSPRequest{
CertDER: append([]byte(nil), cert.DER...),
Status: &status,
Status: string(core.OCSPStatusGood),
})
test.AssertNotError(t, err, "Failed to sign second OCSP response")
_, err = ocsp.ParseResponse(ocspResp2.Response, caCert)
Expand All @@ -555,7 +553,7 @@ func TestOCSP(t *testing.T) {
// and should be signed by newIssuer.
newCertOcspResp, err := ca.GenerateOCSP(ctx, &caPB.GenerateOCSPRequest{
CertDER: newCert.DER,
Status: &status,
Status: string(core.OCSPStatusGood),
})
test.AssertNotError(t, err, "Failed to generate OCSP")
parsedNewCertOcspResp, err := ocsp.ParseResponse(newCertOcspResp.Response, newIssuerCert)
Expand Down Expand Up @@ -631,7 +629,7 @@ func TestInvalidCSRs(t *testing.T) {

t.Run(testCase.name, func(t *testing.T) {
serializedCSR := mustRead(testCase.csrPath)
issueReq := &caPB.IssueCertificateRequest{Csr: serializedCSR, RegistrationID: &arbitraryRegID}
issueReq := &caPB.IssueCertificateRequest{Csr: serializedCSR, RegistrationID: arbitraryRegID}
_, err = ca.IssuePrecertificate(ctx, issueReq)

test.Assert(t, berrors.Is(err, testCase.errorType), "Incorrect error type returned")
Expand Down Expand Up @@ -666,7 +664,7 @@ func TestRejectValidityTooLong(t *testing.T) {
test.AssertNotError(t, err, "Failed to parse time")
testCtx.fc.Set(future)
// Test that the CA rejects CSRs that would expire after the intermediate cert
_, err = ca.IssuePrecertificate(ctx, &caPB.IssueCertificateRequest{Csr: NoCNCSR, RegistrationID: &arbitraryRegID})
_, err = ca.IssuePrecertificate(ctx, &caPB.IssueCertificateRequest{Csr: NoCNCSR, RegistrationID: arbitraryRegID})
test.AssertError(t, err, "Cannot issue a certificate that expires after the intermediate certificate")
test.Assert(t, berrors.Is(err, berrors.InternalServer), "Incorrect error type returned")
}
Expand Down Expand Up @@ -842,7 +840,7 @@ func TestIssueCertificateForPrecertificate(t *testing.T) {
test.AssertNotError(t, err, "Failed to create CA")

orderID := int64(0)
issueReq := caPB.IssueCertificateRequest{Csr: CNandSANCSR, RegistrationID: &arbitraryRegID, OrderID: &orderID}
issueReq := caPB.IssueCertificateRequest{Csr: CNandSANCSR, RegistrationID: arbitraryRegID, OrderID: orderID}
precert, err := ca.IssuePrecertificate(ctx, &issueReq)
test.AssertNotError(t, err, "Failed to issue precert")
parsedPrecert, err := x509.ParseCertificate(precert.DER)
Expand All @@ -866,8 +864,8 @@ func TestIssueCertificateForPrecertificate(t *testing.T) {
cert, err := ca.IssueCertificateForPrecertificate(ctx, &caPB.IssueCertificateForPrecertificateRequest{
DER: precert.DER,
SCTs: sctBytes,
RegistrationID: &arbitraryRegID,
OrderID: new(int64),
RegistrationID: arbitraryRegID,
OrderID: 0,
})
test.AssertNotError(t, err, "Failed to issue cert from precert")
parsedCert, err := x509.ParseCertificate(cert.DER)
Expand Down Expand Up @@ -929,14 +927,14 @@ func TestIssueCertificateForPrecertificateDuplicateSerial(t *testing.T) {
}

orderID := int64(0)
issueReq := caPB.IssueCertificateRequest{Csr: CNandSANCSR, RegistrationID: &arbitraryRegID, OrderID: &orderID}
issueReq := caPB.IssueCertificateRequest{Csr: CNandSANCSR, RegistrationID: arbitraryRegID, OrderID: orderID}
precert, err := ca.IssuePrecertificate(ctx, &issueReq)
test.AssertNotError(t, err, "Failed to issue precert")
_, err = ca.IssueCertificateForPrecertificate(ctx, &caPB.IssueCertificateForPrecertificateRequest{
DER: precert.DER,
SCTs: sctBytes,
RegistrationID: &arbitraryRegID,
OrderID: new(int64),
RegistrationID: arbitraryRegID,
OrderID: 0,
})
if err == nil {
t.Error("Expected error issuing duplicate serial but got none.")
Expand All @@ -963,8 +961,8 @@ func TestIssueCertificateForPrecertificateDuplicateSerial(t *testing.T) {
_, err = errorca.IssueCertificateForPrecertificate(ctx, &caPB.IssueCertificateForPrecertificateRequest{
DER: precert.DER,
SCTs: sctBytes,
RegistrationID: &arbitraryRegID,
OrderID: new(int64),
RegistrationID: arbitraryRegID,
OrderID: 0,
})
if err == nil {
t.Fatal("Expected error issuing duplicate serial but got none.")
Expand Down Expand Up @@ -1038,10 +1036,9 @@ func TestPrecertOrphanQueue(t *testing.T) {
t.Fatalf("Unexpected error, wanted %q, got %q", goque.ErrEmpty, err)
}

var one int64 = 1
_, err = ca.IssuePrecertificate(context.Background(), &caPB.IssueCertificateRequest{
RegistrationID: &one,
OrderID: &one,
RegistrationID: int64(1),
OrderID: int64(1),
Csr: CNandSANCSR,
})
test.AssertError(t, err, "Expected IssuePrecertificate to fail with `failSA`")
Expand Down Expand Up @@ -1229,7 +1226,7 @@ func TestIssuePrecertificateLinting(t *testing.T) {
// Attempt to issue a pre-certificate
_, err = ca.IssuePrecertificate(ctx, &caPB.IssueCertificateRequest{
Csr: CNandSANCSR,
RegistrationID: &arbitraryRegID,
RegistrationID: arbitraryRegID,
})
// It should error
test.AssertError(t, err, "expected err from IssuePrecertificate with linttrapSigner")
Expand Down Expand Up @@ -1262,32 +1259,28 @@ func TestGenerateOCSPWithIssuerID(t *testing.T) {
test.AssertNotError(t, err, "Failed to create CA")

// GenerateOCSP with feature enabled + req contains bad IssuerID
issuerID := int64(666)
serial := "DEADDEADDEADDEADDEADDEADDEADDEADDEAD"
status := string(core.OCSPStatusGood)
_, err = ca.GenerateOCSP(context.Background(), &caPB.GenerateOCSPRequest{
IssuerID: &issuerID,
Serial: &serial,
Status: &status,
IssuerID: int64(666),
Serial: "DEADDEADDEADDEADDEADDEADDEADDEADDEAD",
Status: string(core.OCSPStatusGood),
})
test.AssertError(t, err, "GenerateOCSP didn't fail with invalid IssuerID")

// GenerateOCSP with feature enabled + req contains good IssuerID
issuerID = idForIssuer(ca.defaultIssuer.cert)
_, err = ca.GenerateOCSP(context.Background(), &caPB.GenerateOCSPRequest{
IssuerID: &issuerID,
Serial: &serial,
Status: &status,
IssuerID: idForIssuer(ca.defaultIssuer.cert),
Serial: "DEADDEADDEADDEADDEADDEADDEADDEADDEAD",
Status: string(core.OCSPStatusGood),
})
test.AssertNotError(t, err, "GenerateOCSP failed")

// GenerateOCSP with feature enabled + req doesn't contain IssuerID
issueReq := caPB.IssueCertificateRequest{Csr: CNandSANCSR, RegistrationID: &arbitraryRegID}
issueReq := caPB.IssueCertificateRequest{Csr: CNandSANCSR, RegistrationID: arbitraryRegID}
cert, err := ca.IssuePrecertificate(ctx, &issueReq)
test.AssertNotError(t, err, "Failed to issue")
_, err = ca.GenerateOCSP(context.Background(), &caPB.GenerateOCSPRequest{
CertDER: cert.DER,
Status: &status,
Status: string(core.OCSPStatusGood),
})
test.AssertNotError(t, err, "GenerateOCSP failed")
}

0 comments on commit 24e782e

Please sign in to comment.