Skip to content
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

Embed SCTs in issued certificates #507

Merged
merged 4 commits into from Apr 13, 2022
Merged

Conversation

haydentherapper
Copy link
Contributor

@haydentherapper haydentherapper commented Apr 6, 2022

Summary

This adds support for embedding SCTs in certificates instead of
returning a header with a detached SCTs. This is done by implementing an
SCT interface for a signer. For example, GCP CA Service will not
support embedded SCTs, but KMS will.

This heavily leverages the Go CT library. I've removed the custom
client in favor of the CT library client, which includes more
verification and retry logic. Note that there's a TODO to include the
public key of the CT log in Fulcio so that the SCT is checked before
returning a response.

A certificate is signed twice, which adds an extra remote call to KMS.
The first certificate is added to the CT log via AddPreChain instead of
AddChain.

The Cosign client will need to be updated to support embedded SCTs.

Signed-off-by: Hayden Blauzvern hblauzvern@google.com

Ticket Link

Fixes #42, #310

Release Note

Added support for embedded SCTs for intermediate CA implementation. The CA will not return the SCT detached in a header. The client must verify the SCT using the SCT embedded in the certificate.

@haydentherapper
Copy link
Contributor Author

If you want to take a look, please only review the second commit! Waiting until we submit #496.

@codecov-commenter
Copy link

codecov-commenter commented Apr 7, 2022

Codecov Report

Merging #507 (36849e6) into main (4db32b6) will increase coverage by 0.58%.
The diff coverage is 58.67%.

@@            Coverage Diff             @@
##             main     #507      +/-   ##
==========================================
+ Coverage   43.32%   43.91%   +0.58%     
==========================================
  Files          18       16       -2     
  Lines        1221     1273      +52     
==========================================
+ Hits          529      559      +30     
- Misses        617      633      +16     
- Partials       75       81       +6     
Impacted Files Coverage Δ
pkg/api/ca.go 46.63% <28.12%> (-5.02%) ⬇️
pkg/ca/intermediateca/intermediateca.go 72.51% <64.70%> (-8.44%) ⬇️
pkg/ctl/utils.go 85.71% <85.71%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4db32b6...36849e6. Read the comment docs.

@dlorenc
Copy link
Member

dlorenc commented Apr 7, 2022

The Cosign client will need to be updated to support embedded SCTs.

How will this rollout work? Can cosign support both for awhile?

@haydentherapper
Copy link
Contributor Author

haydentherapper commented Apr 7, 2022

#42 (comment) - Cosign will need to be updated to support both. It'll conditionally check if the SCT is embedded or in a header, and verify based on that. Then we can rollout this change, but it'll be a breaking change for anyone who hasn't updated Cosign. It's not possible for the current version of Cosign to successfully verify a certificate with an embedded SCT (because an SCT is signed using the To-Be-Signed portion of a certificate and removing either the poison ext or the SCT ext, and you need to set a flag for the verifier to remove an ext when verifying).

@dlorenc
Copy link
Member

dlorenc commented Apr 7, 2022

Cool! OK, so we can change cosign to support both, get a release out with a version bump (1.8?), then deploy this change.

This adds support for embedding SCTs in certificates instead of
returning a header with a detached SCTs. This is done by implementing an
SCT interface for a signer. For example, GCP CA Service will not
support embedded SCTs, but KMS will.

This heavily leverages the Go CT library. I've removed the custom
client in favor of the CT library client, which includes more
verification and retry logic. Note that there's a TODO to include the
public key of the CT log in Fulcio so that the SCT is checked before
returning a response.

A certificate is signed twice, which adds an extra remote call to KMS.
The first certificate is added to the CT log via AddPreChain instead of
AddChain.

The Cosign client will need to be updated to support embedded SCTs.

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
This is because Cosign expects an AddChainResponse and not an SCT. The
new client returns an SCT instead, so we have to manually convert it.

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@dlorenc
Copy link
Member

dlorenc commented Apr 10, 2022

Do we have an issue or PR in flight to add support to cosign for verifying both?

@haydentherapper
Copy link
Contributor Author

sigstore/cosign#1730 for the issue, sigstore/cosign#1731 for the WIP PR - The logic is all there, just working on testing (generating SCTs in tests is fun!)

@haydentherapper
Copy link
Contributor Author

Any other comments, or can this be submitted?

@dlorenc dlorenc mentioned this pull request Apr 12, 2022
Copy link
Member

@bobcallaway bobcallaway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally LGTM, a couple questions inline

cmd/app/serve.go Show resolved Hide resolved
pkg/api/ca.go Outdated Show resolved Hide resolved
pkg/api/ca.go Outdated Show resolved Hide resolved
pkg/ca/intermediateca/intermediateca.go Outdated Show resolved Hide resolved
pkg/ca/intermediateca/intermediateca.go Outdated Show resolved Hide resolved
pkg/ca/intermediateca/intermediateca.go Outdated Show resolved Hide resolved
pkg/ca/intermediateca/intermediateca.go Outdated Show resolved Hide resolved
pkg/ca/intermediateca/intermediateca.go Show resolved Hide resolved
pkg/ca/intermediateca/intermediateca.go Show resolved Hide resolved
PEM encoding handles adding a newline.

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@dlorenc
Copy link
Member

dlorenc commented Apr 13, 2022

I'll defer on submitting this one to @priyawadhwa and @k4leung4.

The code is fine, but if we submit this before the cosign 1.8 release we won't be able to deploy the main branch to prod, making it a bit unsafe.

@haydentherapper
Copy link
Contributor Author

We can deploy the main branch to prod, this only affects the intermediate CA implementation, which is not what’s rolled out in prod. The CA Service CA will continue to return detached SCTs.

@priyawadhwa
Copy link
Contributor

Is the plan to switch to the intermediate CA implementation when we migrate clusters in prod?

@haydentherapper
Copy link
Contributor Author

I wanted to switch once we cut 1.8. Though if you prefer, I’m happy to do testing in staging first.

@dlorenc
Copy link
Member

dlorenc commented Apr 13, 2022

We can deploy the main branch to prod, this only affects the intermediate CA implementation, which is not what’s rolled out in prod. The CA Service CA will continue to return detached SCTs.

Ahh, got it.

@dlorenc
Copy link
Member

dlorenc commented Apr 13, 2022

This can merge now then @bobcallaway can rebase :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Figure out how to handle the CT log proofs in clients
5 participants