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

Verify embedded SCTs #1731

Merged
merged 3 commits into from
Apr 15, 2022
Merged

Verify embedded SCTs #1731

merged 3 commits into from
Apr 15, 2022

Conversation

haydentherapper
Copy link
Contributor

@haydentherapper haydentherapper commented Apr 10, 2022

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

Summary

This adds support for verifying SCTs embedded in certificates in
addition to being detached. Embedded SCTs will be verified while parsing
the certificate on verification (for verify, verify-blob, and
verify-attestation), on signing if a certificate chain is provided, and on signing
when a certificate is returned from Fulcio.

This also includes a policy flag to enforce an embedded SCT.

This maintains support for verifying detached SCTs.

Verified this is working with:

# Test against prod
COSIGN_EXPERIMENTAL=1 cosign sign <container>
...
Successfully verified SCT...

# Test against local instance
# Spin up local Fulcio with docker-compose, using `kmsca`
# Generate local TUF metadata with local log verification key
# Initialize cosign with local TUF root
COSIGN_EXPERIMENTAL=1 cosign sign --fulcio-url http://localhost:5555 <container>
...
Successfully verified SCT...

Ticket Link

Fixes #1730

Release Note

Added verification of embedded SCTs. This will be required to use certificates from Fulcio.

@haydentherapper haydentherapper changed the title Verify embedded SCTs WIP: Verify embedded SCTs Apr 10, 2022
@dlorenc
Copy link
Member

dlorenc commented Apr 10, 2022

Thanks for getting this started!

@haydentherapper haydentherapper force-pushed the embed-sct branch 3 times, most recently from 566c421 to 5f39c86 Compare April 11, 2022 22:09
@haydentherapper
Copy link
Contributor Author

@mattmoor - I'm getting depcheck failures due a logging framework being pulled in with Trillian. I think you originally added the depcheck. Do you have concerns about this being removed?

@haydentherapper haydentherapper force-pushed the embed-sct branch 2 times, most recently from ace5b8c to 62af999 Compare April 11, 2022 22:42
@codecov-commenter
Copy link

codecov-commenter commented Apr 11, 2022

Codecov Report

Merging #1731 (26f873f) into main (650dc80) will increase coverage by 0.72%.
The diff coverage is 51.49%.

@@            Coverage Diff             @@
##             main    #1731      +/-   ##
==========================================
+ Coverage   30.13%   30.86%   +0.72%     
==========================================
  Files         143      144       +1     
  Lines        8607     8778     +171     
==========================================
+ Hits         2594     2709     +115     
- Misses       5712     5740      +28     
- Partials      301      329      +28     
Impacted Files Coverage Δ
cmd/cosign/cli/dockerfile.go 0.00% <0.00%> (ø)
cmd/cosign/cli/manifest.go 0.00% <0.00%> (ø)
cmd/cosign/cli/options/certificate.go 0.00% <0.00%> (ø)
cmd/cosign/cli/verify.go 0.00% <0.00%> (ø)
cmd/cosign/cli/verify/verify.go 0.00% <0.00%> (ø)
cmd/cosign/cli/verify/verify_attestation.go 0.00% <0.00%> (ø)
cmd/cosign/cli/verify/verify_blob.go 10.42% <0.00%> (-0.07%) ⬇️
cmd/cosign/cli/sign/sign.go 14.36% <16.66%> (-0.17%) ⬇️
cmd/cosign/cli/fulcio/fulcioverifier/ctl/verify.go 47.85% <47.85%> (ø)
pkg/cosign/verify.go 24.34% <60.86%> (+0.64%) ⬆️
... and 3 more

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 650dc80...26f873f. Read the comment docs.

@mattmoor
Copy link
Member

@haydentherapper The history here is that klog is a fork of glog and both define the same flag, so if both libraries are pulled into a single binary, it will panic during initialization. I believe that this is a problem for both tekton/chains and the cosigned admission webhook which have a lot of Kubernetes dependencies that pull in klog.

@mattmoor
Copy link
Member

I suspect the cosigned e2e tests will fail, and you'll see the controller crash looping and the logs will show a panic about the duplicate flag.

Honestly, the Trillian / certificate-transparency-go dependency graph is a nightmare, so if we can factor out the pieces we need from there into their own module without these dependencies, everyone downstream of them (and us) will have a much better time.

I was just talking to @dlorenc about this yesterday because I was battling its dependency graph which pulls in a fairly old version of otelgrpc by way of etcd 😅

@mattmoor
Copy link
Member

--- cosign-system pods ---
NAME                              READY   STATUS             RESTARTS   AGE
policy-webhook-75dcbd5745-6hn65   0/1     CrashLoopBackOff   5          5m1s
webhook-79986cb869-ctv79          0/1     CrashLoopBackOff   5          5m1s

and

2022-04-11T22:52:40.635116763Z stderr F /ko-app/policy_webhook flag redefined: log_dir
2022-04-11T22:52:40.637400307Z stderr F panic: /ko-app/policy_webhook flag redefined: log_dir
2022-04-11T22:52:40.637413608Z stderr F 
2022-04-11T22:52:40.637417208Z stderr F goroutine 1 [running]:
2022-04-11T22:52:40.637420208Z stderr F flag.(*FlagSet).Var(0xc000106180, {0x272f260, 0x3ae6160}, {0x22bcc0d, 0x7}, {0x231d316, 0x2f})
2022-04-11T22:52:40.637432509Z stderr F 	flag/flag.go:879 +0x2f4
2022-04-11T22:52:40.637435709Z stderr F flag.(*FlagSet).StringVar(...)
2022-04-11T22:52:40.637438509Z stderr F 	flag/flag.go:762
2022-04-11T22:52:40.637441809Z stderr F k8s.io/klog/v2.InitFlags(0xc0002a8080)
2022-04-11T22:52:40.63744491Z stderr F 	k8s.io/klog/v2@v2.60.1-0.20220317184644-43cc75f9ae89/klog.go:407 +0x55
2022-04-11T22:52:40.63744801Z stderr F knative.dev/pkg/injection.ParseAndGetRESTConfigOrDie()
2022-04-11T22:52:40.63745091Z stderr F 	knative.dev/pkg@v0.0.0-20220325200448-1f7514acd0c2/injection/config.go:33 +0x45
2022-04-11T22:52:40.637519414Z stderr F knative.dev/pkg/injection/sharedmain.MainWithContext({0x2750e60, 0xc000739290}, {0x22d2867, 0x12}, {0xc00081fea0, 0x4, 0x4})
2022-04-11T22:52:40.637540216Z stderr F 	knative.dev/pkg@v0.0.0-20220325200448-1f7514acd0c2/injection/sharedmain/main.go:163 +0x78
2022-04-11T22:52:40.637544816Z stderr F main.main()
2022-04-11T22:52:40.637548316Z stderr F 	github.com/sigstore/cosign/cmd/cosign/policy_webhook/main.go:54 +0x2e9

@haydentherapper
Copy link
Contributor Author

Thanks for the context, I'll take a look at pulling out what we need.

	ct "github.com/google/certificate-transparency-go"
	"github.com/google/certificate-transparency-go/ctutil"
	ctx509 "github.com/google/certificate-transparency-go/x509"
	"github.com/google/certificate-transparency-go/x509util"

This is what was used previously without issue. I added .../trillian/ctfe, but this is calling a trivial function I can extract.

@haydentherapper
Copy link
Contributor Author

Excellent, got it working! I don't love this approach, but I made a copy of two files that we rely on, to avoid pulling in the rest of the package (I'll copy in the tests too). Any concerns with this approach?

@haydentherapper
Copy link
Contributor Author

PR is ready to go!

@haydentherapper haydentherapper changed the title WIP: Verify embedded SCTs Verify embedded SCTs Apr 12, 2022
@haydentherapper
Copy link
Contributor Author

Added a flag to enforce an SCT on verification. This defaults to not requiring the SCT to not break existing clients.

@haydentherapper haydentherapper force-pushed the embed-sct branch 3 times, most recently from 71fb752 to 26f873f Compare April 13, 2022 21:31
@haydentherapper
Copy link
Contributor Author

Any comments @bobcallaway @asraa?

This adds support for verifying SCTs embedded in certificates in
addition to being detached. Embedded SCTs will be verified while parsing
the certificate on verification (for verify, verify-blob, and
verify-attestation), and on signing if a certificate chain is provided.

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@haydentherapper
Copy link
Contributor Author

@dlorenc I think this is ready to go!

Copy link
Member

@dlorenc dlorenc left a comment

Choose a reason for hiding this comment

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

Nicely done!

@dlorenc dlorenc merged commit 920dcfa into sigstore:main Apr 15, 2022
@github-actions github-actions bot added this to the v1.8.0 milestone Apr 15, 2022
@haydentherapper haydentherapper deleted the embed-sct branch April 15, 2022 01:04
mlieberman85 pushed a commit to mlieberman85/cosign that referenced this pull request May 6, 2022
* Add verification for embedded SCTs

This adds support for verifying SCTs embedded in certificates in
addition to being detached. Embedded SCTs will be verified while parsing
the certificate on verification (for verify, verify-blob, and
verify-attestation), and on signing if a certificate chain is provided.

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

* Add policy flag to enforce SCT

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

* Added comment for imported package

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
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.

Add support for verifying embedded SCTs
5 participants