New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SCT embedding #3521

Merged
merged 21 commits into from Mar 12, 2018

Conversation

Projects
None yet
6 participants
@rolandshoemaker
Contributor

rolandshoemaker commented Mar 3, 2018

Adds SCT embedding to the certificate issuance flow. When a issuance is requested a precertificate (the requested certificate but poisoned with the critical CT extension) is issued and submitted to the required CT logs. Once the SCTs for the precertificate have been collected a new certificate is issued with the poison extension replace with a SCT list extension containing the retrieved SCTs.

Fixes #2244, fixes #3492 and fixes #3429.

@rolandshoemaker rolandshoemaker requested a review from letsencrypt/boulder-developers as a code owner Mar 3, 2018

@rolandshoemaker rolandshoemaker changed the title from Embed scts to Add SCT embedding Mar 3, 2018

@jsha

This comment has been minimized.

Contributor

jsha commented Mar 6, 2018

Can you break out the cloudflare dep update into its own PR?

@cpu

Few things to ask about:

  1. I think you're missing an update to test/config-next for the EmbedSCTs feature flag

  2. I think the GRPC wrappers need to be updated to check the changed IssuePrecertificateResponse and IssueCertificateForPrecertificateRequest fields, no?

ca/ca.go Outdated
}, nil
}
// IssueCertificateForPrecertificate does a thing

This comment has been minimized.

@cpu

cpu Mar 6, 2018

Member

Probably WIP - but this comment needs updating :-)

@@ -870,5 +885,6 @@ func findExtension(extensions []pkix.Extension, id asn1.ObjectIdentifier) *pkix.
}
func signatureCountByPurpose(signatureType string, signatureCount *prometheus.CounterVec) int {
fmt.Println(signatureCount.MetricVec)

This comment has been minimized.

@cpu

cpu Mar 6, 2018

Member

Probably WIP: leftover fmt.Println?

return emptyCert, err
}
scts, err := ra.getSCTs(ctx, precert.DER)
if err != nil {

This comment has been minimized.

@cpu

cpu Mar 6, 2018

Member

Does this conditional body also need a logEvent.Error = err.Error() like the err checking from ra.CA.IssuePrecertificate and ra.CA.IssueCertificate?

This comment has been minimized.

@rolandshoemaker

rolandshoemaker Mar 6, 2018

Contributor

Done.

ca/ca.go Outdated
@@ -454,20 +456,39 @@ func (ca *CertificateAuthorityImpl) IssuePrecertificate(ctx context.Context, iss
return nil, err
}
precertDER, err := ca.issueCertificateOrPrecertificate(ctx, issueReq, serialBigInt, validity, "cert", &ctPoisonExtension)
precertDER, err := ca.issueCertificateOrPrecertificate(ctx, issueReq, serialBigInt, validity, "precert")

This comment has been minimized.

@jsha

jsha Mar 6, 2018

Contributor

Let's make "cert" / "precert" into a typed enum.

@@ -90,7 +90,7 @@ func (ctp *CTPolicy) race(ctx context.Context, cert core.CertDER, group cmd.CTGr
// GetSCTs attempts to retrieve a SCT from each configured grouping of logs and returns
// the set of SCTs to the caller.
func (ctp *CTPolicy) GetSCTs(ctx context.Context, cert core.CertDER) ([]core.SCTDER, error) {
func (ctp *CTPolicy) GetSCTs(ctx context.Context, cert core.CertDER) ([][]byte, error) {

This comment has been minimized.

@jsha

jsha Mar 6, 2018

Contributor

Why did this type change back?

This comment has been minimized.

@rolandshoemaker

rolandshoemaker Mar 6, 2018

Contributor

Ah, I meant to add a comment about this. The protobuf messages we generate translate repeated bytes to [][]byte and golang won't let you use a []typedBytes (where type typedBytes []byte) for that. We could add a translation in the gRPC wrappers on each side of the RPC that took a []core.SCTDER and turned it into a [][]byte and vice-versa but that seems rather memory inefficient just to gain a named type.

This comment has been minimized.

@jsha

jsha Mar 6, 2018

Contributor

What about creating a new named type, SCTDERs that maps to [][]byte? Then we could just cast to and from that.

Out of date

ca/ca.go Outdated
const (
precertificate = certificateType("precertificate")
certificate = certificateType("certificate")

This comment has been minimized.

@jsha

jsha Mar 7, 2018

Contributor

Let's name these enums precertType and certType. Otherwise they look like variables holding a precertificate (or a certificate) in the calls below.

@jsha

Design seems sound; waiting on final review while you write the last few tests.

@rolandshoemaker

This comment has been minimized.

Contributor

rolandshoemaker commented Mar 8, 2018

Integration tests will fail until #3536 is merged.

@jsha

Publisher changes look great, but please break them out into a separate PR and add a unittest.

@@ -216,9 +218,15 @@ func (pub *Impl) SubmitToSingleCTWithResult(ctx context.Context, req *pubpb.Requ
return nil, err
}
isPrecert := false
if req.Precert != nil && *req.Precert {

This comment has been minimized.

@jsha

jsha Mar 8, 2018

Contributor

Slightly better:

if req.Precert != nil {
  isPrecert = *req.Precert
}

This comment has been minimized.

@rolandshoemaker

rolandshoemaker Mar 9, 2018

Contributor

Done in #3537.

cpu added a commit that referenced this pull request Mar 8, 2018

Update github.com/cloudflare/cfssl (#3536)
Pulls in SCT list serialization fix, unblocks #3521.

```
ok  	github.com/cloudflare/cfssl/api/client	1.137s	coverage: 52.2% of statements
ok  	github.com/cloudflare/cfssl/api/crl	1.110s	coverage: 75.0% of statements
ok  	github.com/cloudflare/cfssl/api/gencrl	1.062s	coverage: 72.5% of statements
ok  	github.com/cloudflare/cfssl/api/generator	1.304s	coverage: 33.3% of statements
ok  	github.com/cloudflare/cfssl/api/info	1.133s	coverage: 84.1% of statements
ok  	github.com/cloudflare/cfssl/api/initca	1.068s	coverage: 90.5% of statements
ok  	github.com/cloudflare/cfssl/api/ocsp	1.152s	coverage: 93.8% of statements
ok  	github.com/cloudflare/cfssl/api/revoke	2.574s	coverage: 75.0% of statements
ok  	github.com/cloudflare/cfssl/api/scan	2.885s	coverage: 62.1% of statements
ok  	github.com/cloudflare/cfssl/api/sign	3.188s	coverage: 83.3% of statements
ok  	github.com/cloudflare/cfssl/api/signhandler	1.179s	coverage: 26.3% of statements
ok  	github.com/cloudflare/cfssl/auth	1.012s	coverage: 68.2% of statements
ok  	github.com/cloudflare/cfssl/bundler	15.700s	coverage: 84.5% of statements
ok  	github.com/cloudflare/cfssl/certdb/dbconf	1.016s	coverage: 84.2% of statements
ok  	github.com/cloudflare/cfssl/certdb/ocspstapling	1.415s	coverage: 69.2% of statements
ok  	github.com/cloudflare/cfssl/certdb/sql	1.248s	coverage: 70.5% of statements
ok  	github.com/cloudflare/cfssl/cli	1.013s	coverage: 61.9% of statements
ok  	github.com/cloudflare/cfssl/cli/bundle	1.012s	coverage: 0.0% of statements [no tests to run]
ok  	github.com/cloudflare/cfssl/cli/crl	1.091s	coverage: 57.8% of statements
ok  	github.com/cloudflare/cfssl/cli/gencert	11.960s	coverage: 83.6% of statements
ok  	github.com/cloudflare/cfssl/cli/gencrl	1.089s	coverage: 73.3% of statements
ok  	github.com/cloudflare/cfssl/cli/gencsr	1.064s	coverage: 70.3% of statements
ok  	github.com/cloudflare/cfssl/cli/genkey	6.415s	coverage: 70.0% of statements
ok  	github.com/cloudflare/cfssl/cli/ocsprefresh	1.060s	coverage: 64.3% of statements
ok  	github.com/cloudflare/cfssl/cli/revoke	1.033s	coverage: 88.2% of statements
ok  	github.com/cloudflare/cfssl/cli/scan	1.013s	coverage: 36.0% of statements
ok  	github.com/cloudflare/cfssl/cli/selfsign	2.029s	coverage: 73.2% of statements
ok  	github.com/cloudflare/cfssl/cli/serve	1.073s	coverage: 39.0% of statements
ok  	github.com/cloudflare/cfssl/cli/sign	1.054s	coverage: 54.8% of statements
ok  	github.com/cloudflare/cfssl/cli/version	1.012s	coverage: 100.0% of statements
ok  	github.com/cloudflare/cfssl/cmd/cfssl	1.036s	coverage: 0.0% of statements [no tests to run]
ok  	github.com/cloudflare/cfssl/cmd/cfssljson	1.018s	coverage: 3.4% of statements
ok  	github.com/cloudflare/cfssl/cmd/mkbundle	1.012s	coverage: 0.0% of statements [no tests to run]
ok  	github.com/cloudflare/cfssl/config	1.029s	coverage: 67.7% of statements
ok  	github.com/cloudflare/cfssl/crl	1.056s	coverage: 68.3% of statements
ok  	github.com/cloudflare/cfssl/csr	31.882s	coverage: 89.6% of statements
ok  	github.com/cloudflare/cfssl/errors	1.016s	coverage: 79.6% of statements
ok  	github.com/cloudflare/cfssl/helpers	1.251s	coverage: 82.8% of statements
ok  	github.com/cloudflare/cfssl/helpers/testsuite	6.974s	coverage: 65.8% of statements
ok  	github.com/cloudflare/cfssl/initca	207.580s	coverage: 73.2% of statements
ok  	github.com/cloudflare/cfssl/log	1.010s	coverage: 59.3% of statements
ok  	github.com/cloudflare/cfssl/multiroot/config	1.161s	coverage: 77.4% of statements
ok  	github.com/cloudflare/cfssl/ocsp	1.230s	coverage: 77.4% of statements
ok  	github.com/cloudflare/cfssl/revoke	1.336s	coverage: 77.9% of statements
ok  	github.com/cloudflare/cfssl/scan	1.016s	coverage: 1.1% of statements
ok  	github.com/cloudflare/cfssl/selfsign	1.059s	coverage: 70.0% of statements
ok  	github.com/cloudflare/cfssl/signer	1.014s	coverage: 19.4% of statements
ok  	github.com/cloudflare/cfssl/signer/local	3.355s	coverage: 77.9% of statements
ok  	github.com/cloudflare/cfssl/signer/remote	2.371s	coverage: 70.0% of statements
ok  	github.com/cloudflare/cfssl/signer/universal	2.163s	coverage: 67.7% of statements
ok  	github.com/cloudflare/cfssl/transport	1.012s
ok  	github.com/cloudflare/cfssl/transport/ca/localca	1.043s	coverage: 94.9% of statements
ok  	github.com/cloudflare/cfssl/transport/core	1.030s	coverage: 90.9% of statements
ok  	github.com/cloudflare/cfssl/transport/kp	1.032s	coverage: 37.1% of statements
ok  	github.com/cloudflare/cfssl/ubiquity	1.034s	coverage: 88.3% of statements
ok  	github.com/cloudflare/cfssl/whitelist	2.879s	coverage: 100.0% of statements
```
@rolandshoemaker

This comment has been minimized.

Contributor

rolandshoemaker commented Mar 9, 2018

Note to self: the feature flag should also disable the missing SCT part of ocsp-updater.

@cpu

Two very minor nits in the integration test. Really excited to see this work just about finished. The branch turned out really nicely, thanks @rolandshoemaker !

raise Exception("certificate contains CT poison extension")
except x509.ExtensionNotFound:
# do nothing
print("no poison extension")

This comment has been minimized.

@cpu

cpu Mar 12, 2018

Member

Nit: Could you make this a pass instead of a print? I don't think it will be useful output.

if len(sctList.value) != 2:
raise Exception("SCT list contains wrong number of SCTs")
for sct in sctList.value:
print("Sct is", sct.log_id, sct.entry_type)

This comment has been minimized.

@cpu

cpu Mar 12, 2018

Member

Nit: leftover print?

raise Exception("certificate contains CT poison extension")
except x509.ExtensionNotFound:
# do nothing
print("no poison extension")

This comment has been minimized.

@cpu

cpu Mar 12, 2018

Member

ditto RE: my v2 integration test nits.

@cpu

cpu approved these changes Mar 12, 2018

🎉 🎈

@jsha

jsha approved these changes Mar 12, 2018

@jsha jsha merged commit 9c9e944 into master Mar 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jsha jsha deleted the embed-scts branch Mar 12, 2018

@Knight1

This comment has been minimized.

Knight1 commented Mar 12, 2018

ETA for staging and live?

@jsha

This comment has been minimized.

Contributor

jsha commented Mar 12, 2018

Staging probably tomorrow; we'll post on the forum.

@mikebrickwall

This comment has been minimized.

mikebrickwall commented Mar 16, 2018

I didn't see any announcements on the Forum regarding SCT embedding. Is there an updated ETA?

@Knight1

This comment has been minimized.

Knight1 commented Mar 20, 2018

@mikebrickwall

This comment has been minimized.

mikebrickwall commented Mar 20, 2018

Nice work! Tested the staging environment and it worked!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment