Skip to content

Commit e201632

Browse files
rolandshoemakerrolandshoemaker
committed
Merge pull request letsencrypt#812 from letsencrypt/proper-errors
Fix CA.IssueCertificate error types
2 parents 0480bdb + d89dfa3 commit e201632

File tree

3 files changed

+30
-27
lines changed

3 files changed

+30
-27
lines changed

‎ca/certificate-authority.go‎

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -229,19 +229,19 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest
229229
var err error
230230
key, ok := csr.PublicKey.(crypto.PublicKey)
231231
if !ok {
232-
err = fmt.Errorf("Invalid public key in CSR.")
232+
err = core.MalformedRequestError("Invalid public key in CSR.")
233233
// AUDIT[ Certificate Requests ] 11917fa4-10ef-4e0d-9105-bacbe7836a3c
234234
ca.log.AuditErr(err)
235235
return emptyCert, err
236236
}
237237
if err = core.GoodKey(key); err != nil {
238-
err = fmt.Errorf("Invalid public key in CSR: %s", err.Error())
238+
err = core.MalformedRequestError(fmt.Sprintf("Invalid public key in CSR: %s", err.Error()))
239239
// AUDIT[ Certificate Requests ] 11917fa4-10ef-4e0d-9105-bacbe7836a3c
240240
ca.log.AuditErr(err)
241241
return emptyCert, err
242242
}
243243
if badSignatureAlgorithms[csr.SignatureAlgorithm] {
244-
err = fmt.Errorf("Invalid signature algorithm in CSR")
244+
err = core.MalformedRequestError("Invalid signature algorithm in CSR")
245245
// AUDIT[ Certificate Requests ] 11917fa4-10ef-4e0d-9105-bacbe7836a3c
246246
ca.log.AuditErr(err)
247247
return emptyCert, err
@@ -258,7 +258,7 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest
258258
} else if len(hostNames) > 0 {
259259
commonName = hostNames[0]
260260
} else {
261-
err = fmt.Errorf("Cannot issue a certificate without a hostname.")
261+
err = core.MalformedRequestError("Cannot issue a certificate without a hostname.")
262262
// AUDIT[ Certificate Requests ] 11917fa4-10ef-4e0d-9105-bacbe7836a3c
263263
ca.log.AuditErr(err)
264264
return emptyCert, err
@@ -267,23 +267,23 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest
267267
// Collapse any duplicate names. Note that this operation may re-order the names
268268
hostNames = core.UniqueNames(hostNames)
269269
if ca.MaxNames > 0 && len(hostNames) > ca.MaxNames {
270-
err = fmt.Errorf("Certificate request has %d > %d names", len(hostNames), ca.MaxNames)
270+
err = core.MalformedRequestError(fmt.Sprintf("Certificate request has %d > %d names", len(hostNames), ca.MaxNames))
271271
ca.log.WarningErr(err)
272272
return emptyCert, err
273273
}
274274

275275
// Verify that names are allowed by policy
276276
identifier := core.AcmeIdentifier{Type: core.IdentifierDNS, Value: commonName}
277277
if err = ca.PA.WillingToIssue(identifier); err != nil {
278-
err = fmt.Errorf("Policy forbids issuing for name %s", commonName)
278+
err = core.MalformedRequestError(fmt.Sprintf("Policy forbids issuing for name %s", commonName))
279279
// AUDIT[ Certificate Requests ] 11917fa4-10ef-4e0d-9105-bacbe7836a3c
280280
ca.log.AuditErr(err)
281281
return emptyCert, err
282282
}
283283
for _, name := range hostNames {
284284
identifier = core.AcmeIdentifier{Type: core.IdentifierDNS, Value: name}
285285
if err = ca.PA.WillingToIssue(identifier); err != nil {
286-
err = fmt.Errorf("Policy forbids issuing for name %s", name)
286+
err = core.MalformedRequestError(fmt.Sprintf("Policy forbids issuing for name %s", name))
287287
// AUDIT[ Certificate Requests ] 11917fa4-10ef-4e0d-9105-bacbe7836a3c
288288
ca.log.AuditErr(err)
289289
return emptyCert, err
@@ -293,8 +293,8 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest
293293
notAfter := ca.Clk.Now().Add(ca.ValidityPeriod)
294294

295295
if ca.NotAfter.Before(notAfter) {
296+
err = core.InternalServerError("Cannot issue a certificate that expires after the intermediate certificate.")
296297
// AUDIT[ Certificate Requests ] 11917fa4-10ef-4e0d-9105-bacbe7836a3c
297-
err = errors.New("Cannot issue a certificate that expires after the intermediate certificate.")
298298
ca.log.AuditErr(err)
299299
return emptyCert, err
300300
}
@@ -308,13 +308,15 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest
308308
// Get the next serial number
309309
tx, err := ca.DB.Begin()
310310
if err != nil {
311+
err = core.InternalServerError(err.Error())
311312
// AUDIT[ Error Conditions ] 9cc4d537-8534-4970-8665-4b382abe82f3
312313
ca.log.AuditErr(err)
313314
return emptyCert, err
314315
}
315316

316317
serialDec, err := ca.DB.IncrementAndGetSerial(tx)
317318
if err != nil {
319+
err = core.InternalServerError(err.Error())
318320
// AUDIT[ Error Conditions ] 9cc4d537-8534-4970-8665-4b382abe82f3
319321
ca.log.Audit(fmt.Sprintf("Serial increment failed, rolling back: err=[%v]", err))
320322
tx.Rollback()
@@ -335,14 +337,15 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest
335337

336338
certPEM, err := ca.Signer.Sign(req)
337339
if err != nil {
340+
err = core.InternalServerError(err.Error())
338341
// AUDIT[ Error Conditions ] 9cc4d537-8534-4970-8665-4b382abe82f3
339342
ca.log.Audit(fmt.Sprintf("Signer failed, rolling back: serial=[%s] err=[%v]", serialHex, err))
340343
tx.Rollback()
341344
return emptyCert, err
342345
}
343346

344347
if len(certPEM) == 0 {
345-
err = fmt.Errorf("No certificate returned by server")
348+
err = core.InternalServerError("No certificate returned by server")
346349
// AUDIT[ Error Conditions ] 9cc4d537-8534-4970-8665-4b382abe82f3
347350
ca.log.Audit(fmt.Sprintf("PEM empty from Signer, rolling back: serial=[%s] err=[%v]", serialHex, err))
348351
tx.Rollback()
@@ -351,8 +354,7 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest
351354

352355
block, _ := pem.Decode(certPEM)
353356
if block == nil || block.Type != "CERTIFICATE" {
354-
err = fmt.Errorf("Invalid certificate value returned")
355-
357+
err = core.InternalServerError("Invalid certificate value returned")
356358
// AUDIT[ Error Conditions ] 9cc4d537-8534-4970-8665-4b382abe82f3
357359
ca.log.Audit(fmt.Sprintf("PEM decode error, aborting and rolling back issuance: pem=[%s] err=[%v]", certPEM, err))
358360
tx.Rollback()
@@ -366,6 +368,7 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest
366368

367369
// This is one last check for uncaught errors
368370
if err != nil {
371+
err = core.InternalServerError(err.Error())
369372
// AUDIT[ Error Conditions ] 9cc4d537-8534-4970-8665-4b382abe82f3
370373
ca.log.Audit(fmt.Sprintf("Uncaught error, aborting and rolling back issuance: pem=[%s] err=[%v]", certPEM, err))
371374
tx.Rollback()
@@ -375,13 +378,15 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest
375378
// Store the cert with the certificate authority, if provided
376379
_, err = ca.SA.AddCertificate(certDER, regID)
377380
if err != nil {
381+
err = core.InternalServerError(err.Error())
378382
// AUDIT[ Error Conditions ] 9cc4d537-8534-4970-8665-4b382abe82f3
379383
ca.log.Audit(fmt.Sprintf("Failed RPC to store at SA, orphaning certificate: pem=[%s] err=[%v]", certPEM, err))
380384
tx.Rollback()
381385
return emptyCert, err
382386
}
383387

384388
if err = tx.Commit(); err != nil {
389+
err = core.InternalServerError(err.Error())
385390
// AUDIT[ Error Conditions ] 9cc4d537-8534-4970-8665-4b382abe82f3
386391
ca.log.Audit(fmt.Sprintf("Failed to commit, orphaning certificate: pem=[%s] err=[%v]", certPEM, err))
387392
return emptyCert, err

‎ca/certificate-authority_test.go‎

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -330,9 +330,9 @@ func TestRejectNoName(t *testing.T) {
330330
// Test that the CA rejects CSRs with no names
331331
csr, _ := x509.ParseCertificateRequest(NoNameCSR)
332332
_, err = ca.IssueCertificate(*csr, ctx.reg.ID)
333-
if err == nil {
334-
t.Errorf("CA improperly agreed to create a certificate with no name")
335-
}
333+
test.AssertError(t, err, "CA improperly agreed to create a certificate with no name")
334+
_, ok := err.(core.MalformedRequestError)
335+
test.Assert(t, ok, "Incorrect error type returned")
336336
}
337337

338338
func TestRejectTooManyNames(t *testing.T) {
@@ -347,7 +347,9 @@ func TestRejectTooManyNames(t *testing.T) {
347347
// Test that the CA rejects a CSR with too many names
348348
csr, _ := x509.ParseCertificateRequest(TooManyNameCSR)
349349
_, err = ca.IssueCertificate(*csr, ctx.reg.ID)
350-
test.Assert(t, err != nil, "Issued certificate with too many names")
350+
test.AssertError(t, err, "Issued certificate with too many names")
351+
_, ok := err.(core.MalformedRequestError)
352+
test.Assert(t, ok, "Incorrect error type returned")
351353
}
352354

353355
func TestDeduplication(t *testing.T) {
@@ -363,15 +365,9 @@ func TestDeduplication(t *testing.T) {
363365
csr, _ := x509.ParseCertificateRequest(DupeNameCSR)
364366
cert, err := ca.IssueCertificate(*csr, ctx.reg.ID)
365367
test.AssertNotError(t, err, "Failed to gracefully handle a CSR with duplicate names")
366-
if err != nil {
367-
return
368-
}
369368

370369
parsedCert, err := x509.ParseCertificate(cert.DER)
371370
test.AssertNotError(t, err, "Error parsing certificate produced by CA")
372-
if err != nil {
373-
return
374-
}
375371

376372
correctName := "a.not-example.com"
377373
correctNames := len(parsedCert.DNSNames) == 1 &&
@@ -394,6 +390,8 @@ func TestRejectValidityTooLong(t *testing.T) {
394390
ca.NotAfter = ctx.fc.Now()
395391
_, err = ca.IssueCertificate(*csr, 1)
396392
test.AssertEquals(t, err.Error(), "Cannot issue a certificate that expires after the intermediate certificate.")
393+
_, ok := err.(core.InternalServerError)
394+
test.Assert(t, ok, "Incorrect error type returned")
397395
}
398396

399397
func TestShortKey(t *testing.T) {
@@ -407,7 +405,9 @@ func TestShortKey(t *testing.T) {
407405
// Test that the CA rejects CSRs that would expire after the intermediate cert
408406
csr, _ := x509.ParseCertificateRequest(ShortKeyCSR)
409407
_, err = ca.IssueCertificate(*csr, ctx.reg.ID)
410-
test.Assert(t, err != nil, "Issued a certificate with too short a key.")
408+
test.AssertError(t, err, "Issued a certificate with too short a key.")
409+
_, ok := err.(core.MalformedRequestError)
410+
test.Assert(t, ok, "Incorrect error type returned")
411411
}
412412

413413
func TestRejectBadAlgorithm(t *testing.T) {
@@ -421,5 +421,7 @@ func TestRejectBadAlgorithm(t *testing.T) {
421421
// Test that the CA rejects CSRs that would expire after the intermediate cert
422422
csr, _ := x509.ParseCertificateRequest(BadAlgorithmCSR)
423423
_, err = ca.IssueCertificate(*csr, ctx.reg.ID)
424-
test.Assert(t, err != nil, "Issued a certificate based on a CSR with a weak algorithm.")
424+
test.AssertError(t, err, "Issued a certificate based on a CSR with a weak algorithm.")
425+
_, ok := err.(core.MalformedRequestError)
426+
test.Assert(t, ok, "Incorrect error type returned")
425427
}

‎ra/registration-authority.go‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -382,11 +382,7 @@ func (ra *RegistrationAuthorityImpl) NewCertificate(req core.CertificateRequest,
382382

383383
// Create the certificate and log the result
384384
if cert, err = ra.CA.IssueCertificate(*csr, regID); err != nil {
385-
// While this could be InternalServerError for certain conditions, most
386-
// of the failure reasons (such as GoodKey failing) are caused by malformed
387-
// requests.
388385
logEvent.Error = err.Error()
389-
err = core.MalformedRequestError("Certificate request was invalid")
390386
return emptyCert, err
391387
}
392388

0 commit comments

Comments
 (0)