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

Don't require CT log keys if using a key/sk #3415

Merged
merged 1 commit into from Dec 5, 2023

Conversation

haydentherapper
Copy link
Contributor

Fixes #3386. The logic was inverted for this check.

Summary

Release Note

Documentation

Fixes sigstore#3386. The logic was inverted for this check.

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

codecov bot commented Dec 5, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (d329bf7) 29.87% compared to head (46605fa) 30.38%.

Files Patch % Lines
cmd/cosign/cli/verify/verify.go 0.00% 1 Missing ⚠️
cmd/cosign/cli/verify/verify_attestation.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3415      +/-   ##
==========================================
+ Coverage   29.87%   30.38%   +0.51%     
==========================================
  Files         155      155              
  Lines        9971     9971              
==========================================
+ Hits         2979     3030      +51     
+ Misses       6560     6490      -70     
- Partials      432      451      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@hectorj2f hectorj2f left a comment

Choose a reason for hiding this comment

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

lgtm

@haydentherapper haydentherapper merged commit 3329d81 into sigstore:main Dec 5, 2023
28 checks passed
@github-actions github-actions bot added this to the v2.3.0 milestone Dec 5, 2023
@Mukuls77
Copy link
Contributor

Mukuls77 commented Dec 8, 2023

This code change has impacted BYO PKI case.
We are using our company PKI to generate signatures and attach those in registry. We pass the Root certificate to the Verifier and using the root certificate we are able to verify the Certificate chain -> Leaf cert -> Signature,
We dont use any of the Tlog and SCT feature as our server are working mostly in an Airgap mode no connection to external internet servers.

So the problem is that even we are passing the flags like "--insecure-ignore-sct=true --insecure-ignore-tlog=true" still cosign is trying to fetch the ctlog public key for verification which we dont want to use.

cosign verify harbor.demo-ncd.services.te0014-demo-ncd.dyn.nesc.net/ncd-orb/orbs/ncd-ncd_fp6_generic-799@sha256:c08f847db8877aeefa3852ae9ee471fa7c421be4089b855fd0e545d521e2d87c --certificate-identity-regexp='.' --certificate-oidc-issuer-regexp='.' --insecure-ignore-sct=true --insecure-ignore-tlog=true --cert-chain=rootCA.crt --verbose=true
WARNING: Skipping tlog verification is an insecure practice that lacks of transparency and auditability verification for the signature.
Error: getting ctlog public keys: updating local metadata and targets: error updating to TUF remote mirror: tuf: failed to download 8.root.json: Get https://tuf-repo-cdn.sigstore.dev/8.root.json: dial tcp 34.117.62.14:443: i/o timeout
remote status:{
"mirror": https://tuf-repo-cdn.sigstore.dev/,
"metadata": {}
}
main.go:74: error during command execution: getting ctlog public keys: updating local metadata and targets: error updating to TUF remote mirror: tuf: failed to download 8.root.json: Get https://tuf-repo-cdn.sigstore.dev/8.root.json: dial tcp 34.117.62.14:443: i/o timeout
remote status:{
"mirror": https://tuf-repo-cdn.sigstore.dev/,
"metadata": {}
}

The same result, if I provide the rootCA.crt file as an env variable:

export SIGSTORE_ROOT_FILE=/home/sanyi/cosign-2/rootCA.crt
cosign verify harbor.demo-ncd.services.te0014-demo-ncd.dyn.nesc.net/ncd-orb/orbs/ncd-ncd_fp6_generic-799@sha256:c08f847db8877aeefa3852ae9ee471fa7c421be4089b855fd0e545d521e2d87c --certificate-identity-regexp='.' --certificate-oidc-issuer-regexp='.' --insecure-ignore-tlog=true --insecure-ignore-sct=true --verbose=true
WARNING: Skipping tlog verification is an insecure practice that lacks of transparency and auditability verification for the signature.
Error: getting ctlog public keys: updating local metadata and targets: error updating to TUF remote mirror: tuf: failed to download 8.root.json: Get https://tuf-repo-cdn.sigstore.dev/8.root.json: dial tcp 34.117.62.14:443: i/o timeout
remote status:{
"mirror": https://tuf-repo-cdn.sigstore.dev/,
"metadata": {}
}
main.go:74: error during command execution: getting ctlog public keys: updating local metadata and targets: error updating to TUF remote mirror: tuf: failed to download 8.root.json: Get https://tuf-repo-cdn.sigstore.dev/8.root.json: dial tcp 34.117.62.14:443: i/o timeout
remote status:{
"mirror": https://tuf-repo-cdn.sigstore.dev/,
"metadata": {}
}

The code change done

https://github.com/sigstore/cosign/blob/main/cmd/cosign/cli/verify/verify.go (209):
// Ignore Signed Certificate Timestamp if the flag is set or a key is provided
if !c.IgnoreSCT || keylessVerification(c.KeyRef, c.Sk) {
co.CTLogPubKeys, err = cosign.GetCTLogPubs(ctx)
if err != nil {
return fmt.Errorf("getting ctlog public keys: %w", err)
}
}

goes into the section to get CT Log Key if IgnoreSCT is false or the keylessVerification function return True.
As in my case i am using Keyless functionality for verify and dont want to use SCT but due to this cosign is still trying to fetch the ctlog public key, this has broken the existing running functionality for BYO PKI.

I propose to enhance this check to also see if user is not passing any CertChain in verify command else we should not go for CTLog Public Key.

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.

--insecure-ignore-sct=true required to verify in v2.2.1 when using own keys
3 participants