Upgrade to TUF v2 client #3844
Upgrade to TUF v2 client #3844
Conversation
|
There is still work to do here but I will be out for a couple of weeks so it might be worth getting some eyes on in the meantime. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3844 +/- ##
==========================================
- Coverage 40.10% 34.16% -5.94%
==========================================
Files 155 211 +56
Lines 10044 14028 +3984
==========================================
+ Hits 4028 4793 +765
- Misses 5530 8627 +3097
- Partials 486 608 +122 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cmurphy
force-pushed
the
upgrade-tuf
branch
2 times, most recently
from
e59faee to
07e1c38
Compare
cmurphy
force-pushed
the
upgrade-tuf
branch
4 times, most recently
from
00b4edb to
820d6a8
Compare
sorry, I missed this at the time: I will have a look today or monday |
Can you specify what cosign does with sigstore-go? I can see there are some invidual target fetches in the code but does cosign now use trusted_root.json by default? |
Cosign uses sigstore-go piecemeal for various things, but the goal of this PR is to adopt sigstore-go's TUF client wrapper instead of the client wrapper provided by sigstore/sigstore.
It does not, that's part of the purpose of this PR and #3548. There is another PR in progress #3854 that adds a --trusted-root flag that I will have to adjust this PR to conform with.
I may now have to wait for that other PR to be fleshed out before I can make active progress on this, so don't feel rushed to review this. I can ping you again when it's in a more stable state. |
pkg/cosign/fulcio.go
Outdated
| const ( | ||
| // This is the root in the fulcio project. | ||
| fulcioTargetStr = `fulcio.crt.pem` | ||
| // This is the v1 migrated root. | ||
| fulcioV1TargetStr = `fulcio_v1.crt.pem` | ||
| // This is the untrusted v1 intermediate CA certificate, used or chain building. | ||
| fulcioV1IntermediateTargetStr = `fulcio_intermediate_v1.crt.pem` | ||
| ) |
There was a problem hiding this comment.
I wonder if these filenames should be configurable somewhere for private Sigstore TUF operators. The existing metadata format allows for additional targets to be added and discovered, but TUF v2 does not allow iterating over targets, so that strategy is unsupported, meaning this current diff does not necessarily support all private TUF deployments.
We could also provide a CLI utility to convert an old TUF v1 layout to a trusted_root.json and require private TUF maintainers to use it to generate a trusted root in order to support the next version of cosign. I kind of like this option as it fast tracks the adoption of the trusted root.
There was a problem hiding this comment.
I wonder if these filenames should be configurable somewhere for private Sigstore TUF operators
These filenames are hardcoded in the TUF v1 code so recreating them here was an attempt to align with the old way of retrieving targets.
The existing metadata format allows for additional targets to be added and discovered, but TUF v2 does not allow iterating over targets, so that strategy is unsupported, meaning this current diff does not necessarily support all private TUF deployments.
You're right, that was an oversight on my part. I had an earlier version of this that could support discovering targets from custom metadata that I will restore.
We could also provide a CLI utility to convert an old TUF v1 layout to a trusted_root.json
We could do that as a last resort, but my hope was to maintain full backwards compatibility and ease users gently toward using trusted_root.json.
pkg/cosign/tuf.go
Outdated
| func checkValidityPeriod(start, end time.Time) tufStatus { | ||
| now := time.Now() | ||
| if now.Before(start) { | ||
| return inactive | ||
| } | ||
| if now.After(end) { | ||
| return inactive | ||
| } | ||
| return active | ||
| } |
There was a problem hiding this comment.
The validity period in the TrustedRoot should be compared to the timestamp provided by the timestamping service or transparency log. I understand this is not something that is known at the time that you are assembling the CheckOpts, and that makes this a hard problem, but I'm not sure we want to enforce this in this version of the code. I believe we still want to be able to verify a signature produced prior to a timestamping service's validity end date, even if the current time is after that date.
There was a problem hiding this comment.
My goal here was to have something to substitute for the Active/Expired status from TUF v1 that does not seem to have an equivalent in TUF v2. Open to discarding this entirely or finding an alternative.
There was a problem hiding this comment.
For what it's worth, I don't think Cosign is effectively using Active/Expired now, as Cosign is only printing a message to stdout if you are verifying an entry with an "expired" key. This isn't really helpful to the user because there's nothing wrong with using an expired key, it's if the key is used to verify something outside of its validity window, which isn't currently implemented in Cosign.
If it'd be easier to just not deal with Active/Expired, I'd be supportive of that. We can either rely on sigstore-go down the line to do that check, or we can file an issue to implement that feature in Cosign's verification API when using the trusted root file. I'd lean towards the latter in line with #3879.
cmurphy
force-pushed
the
upgrade-tuf
branch
3 times, most recently
from
b992556 to
84e4712
Compare
|
@steiza , @codysoyland , @haydentherapper and I met to discuss a path forward on this and we decided to shrink the scope of this PR by removing the fetch of trusted_root.json, to be addressed separately. This PR dropped the fetch of trusted_root.json, but it now fails conformance tests because the SCT returned during signing from p-g-i is signed by the ctfe_2022.pub key, which the sigstore-go TUF client has no way to know about. This worked fine with p-g-i when trusted_root.json was considered, but shows that this approach will break any deployment that has rotated keys relying on custom metadata. I'm starting to think that using the new client must also be synchronized with using trusted_root.json and that the whole thing should be guarded by a flag that users can opt into. Thoughts? |
Oof, what a mess we've made for ourselves. Yeah, this is what makes the cosign modernization effort we're working on so tricky.
This is what I'm thinking as well. #3854 could be updated to use |
cmurphy
force-pushed
the
upgrade-tuf
branch
2 times, most recently
from
f5d1221 to
b6f5dc4
Compare
|
The latest change adds back the |
|
I think we're still at odds a bit in terms of approach (but it's also possible I'm missing something!) Ultimately, we have to decide what verification path we use in different scenarios, Here's my understanding of the current state of things (which we can of course change):
It is certainly possible to use We could make this pull request address those cases. But ultimately, we need #3844 and #3854 to agree on what the verification path should be when you're using TUF v2 - |
|
I was confused again this morning, and worried I lumped together too many use-cases, so I made a more detailed table:
I think I ended up in the same place though. In the case where we don't have a new protocol buffer bundle, and we have a trusted root that we fetched with TUFv2, we need to decide if the verification path is going to be:
|
|
thanks for writing that up zach, I'm trying to catch up and this is useful |
|
@steiza thanks for the summary, I have a couple of minor clarifying questions:
Do you mean ideally, or currently? Using the TUF v2 client doesn't guarantee you've created a trusted_root.json and added it to your TUF repository.
Technically this PR does very little in |
How do we expect these to get set? By end users? It feels wrong if users need to set an environment variable to make the software use the recommended trust root mechanism. |
Aha, I think I finally understand our different perspectives! I thought that when we use a TUF v2 client we were only going to fetch the If we are allowing fetching files like But if we're fetching For example, we verify the certificate with |
I believe I have a fix: sigstore/sigstore-conformance#179 |
|
Conformance fix is now merged. |
This comment was marked as outdated.
This comment was marked as outdated.
|
Just need to rebase past #4011 |
|
Hi folks, thank you all for all your hard work on this, this is awesome. I gave this PR a test with the private Sigstore instances that I'm working on and I ran into one problem: We're using RSA PSS keys to sign our TUF metadata and it seems that go-tuf v2 only got a support for these recently in theupdateframework/go-tuf#625 - would it be possible to upgrade to a version of go-tuf that includes this PR? I'd be happy to help doing the updates once a new version of go-tuf is released, but not sure if/when that's going to happen. Edit: one thing that would be really useful to help debugging this kind of issues is if there was a (debug) message saying that the trusted_root.json was in fact found, but failed to be parsed (and why). Right now, the warning message emitted only seems to suggest that the file is not present at all: |
Almost certainly: go-tuf just needs to make that release.
it sounds like the failure is that go-tuf considers the repository invalid so it never even got to downloading |
This does happen if the repository is considered invalid, but it also happens when |
cmurphy
force-pushed
the
upgrade-tuf
branch
2 times, most recently
from
087305e to
9b50755
Compare
|
Done:
Blocked: |
cmurphy
force-pushed
the
upgrade-tuf
branch
2 times, most recently
from
38bee43 to
bbe5f24
Compare
cmurphy
force-pushed
the
upgrade-tuf
branch
3 times, most recently
from
114b8d2 to
00192be
Compare
|
This is passing tests now and should be ready for review. Apologies for the delay. |
| trustedMaterial, err := cosign.TrustedRoot() | ||
| if err != nil { | ||
| ui.Warnf(context.Background(), "Could not fetch trusted_root.json from the TUF repository. Continuing with individual targets. Error from TUF: %v", err) | ||
| } |
There was a problem hiding this comment.
I'm still working my way through this PR, but I'm curious if this behavior is consistent with the existing code. I noticed that each signing/verification command starts by loading the Trusted Root from TUF, but if the user supplies their own Trusted Root JSON, I would expect that we wouldn't issue a TUF update (both for air-gapped mode and for speed). Do we currently always fetch TUF updates even if the user doesn't need it?
There was a problem hiding this comment.
++, although I think this comment makes more sense on the verify commands where we have --offline as a strong single if we're doing air-gapped verification.
You could be doing attest/sign commands in a restricted network environment (maybe there's a HSM attached to the network) although I'm not sure we have a flag we can key off of (like --offline for verify commands). I don't know enough about cosign usage to know if we need to support signing in restricted network environments.
There was a problem hiding this comment.
This is an interesting point, because the sign commands don't have a --trusted-root flag. The trusted root is needed for signing because it does SCT verification as part of the fulcio workflow, and cosign has always been looking in the cached TUF root for this CT public key. If you try to verify a bundle and pass in a different trusted root with --trusted-root, the SCT check will fail. The sign commands need to have a --trusted-root flag if we want verification to work in general and especially without a new TUF fetch.
There was a problem hiding this comment.
I left the sign commands as-is because there is no flag to indicate where to get an offline trusted root. There should be, but I think that's outside of the scope of this already very large PR. I did rearrange the verify commands to make sure they don't try to fetch the TUF root online if a trusted root file is already provided.
There was a problem hiding this comment.
I think this is fine as-is for signing, and like you say we can add a flag if people need offline signing in another PR.
There was a problem hiding this comment.
What happens now when you sign with a provided key rather than using an identity/fetching a certificate? Unless you're initializing a signer using Fulcio I'm fairly certain that there is no attempt to fetch TUF targets in the background. (If there is, that's arguably a bug in Cosign).
With just a key and signing a blob, signing should be offline (signing is not offline of course if writing to an OCI registry or if --issue-certificate is set to fetch a fulcio cert). It's also worth noting that Cosign may be used behind a firewall and even if signing requires a network connection (signing/attesting a container with a key), there may be a limited set of allowed domains for outbound traffic. This isn't hypothetical either, we've gotten bug reports/FRs about this in the past.
Is there a set of flag values we can use to determine if TUF targets are needed?
For Cosign v3, we do need to clarify what --offline does to actually mean "there are no outbound connections" and disallow certain flag combos.
There was a problem hiding this comment.
There's not an existing great set of flags for signing that can be used to declare that it needs to be airgapped. --key isn't adequate because it could refer to a KMS or K8s key and isn't necessarily offline. --tlog-upload=false and --issue-certificate=false also don't not fully encompass the scope of offline-ness.
I can try to move this closer to where it is actually needed.
| trustedMaterial, err := cosign.TrustedRoot() | ||
| if err != nil { | ||
| ui.Warnf(context.Background(), "Could not fetch trusted_root.json from the TUF repository. Continuing with individual targets. Error from TUF: %v", err) | ||
| } |
There was a problem hiding this comment.
++, although I think this comment makes more sense on the verify commands where we have --offline as a strong single if we're doing air-gapped verification.
You could be doing attest/sign commands in a restricted network environment (maybe there's a HSM attached to the network) although I'm not sure we have a flag we can key off of (like --offline for verify commands). I don't know enough about cosign usage to know if we need to support signing in restricted network environments.
| trustedMaterial, err := cosign.TrustedRoot() | ||
| if err != nil { | ||
| ui.Warnf(context.Background(), "Could not fetch trusted_root.json from the TUF repository. Continuing with individual targets. Error from TUF: %v", err) | ||
| } |
There was a problem hiding this comment.
I think this is fine as-is for signing, and like you say we can add a flag if people need offline signing in another PR.
|
#4216 for the e2e test failures. |
Use sigstore-go's TUF client to fetch the trusted_root.json from the TUF mirror, if available. Where possible, use sigstore-go's verifiers which natively accept the trusted root as its trusted material. Where there is no trusted root available in TUF or sigstore-go doesn't support a use case, fall back to the sigstore/sigstore TUF v1 client and the existing verifiers in cosign. Signed-off-by: Colleen Murphy <[email protected]>
| trustedMaterial, err := cosign.TrustedRoot() | ||
| if err != nil { | ||
| ui.Warnf(context.Background(), "Could not fetch trusted_root.json from the TUF repository. Continuing with individual targets. Error from TUF: %v", err) | ||
| } |
There was a problem hiding this comment.
What happens now when you sign with a provided key rather than using an identity/fetching a certificate? Unless you're initializing a signer using Fulcio I'm fairly certain that there is no attempt to fetch TUF targets in the background. (If there is, that's arguably a bug in Cosign).
With just a key and signing a blob, signing should be offline (signing is not offline of course if writing to an OCI registry or if --issue-certificate is set to fetch a fulcio cert). It's also worth noting that Cosign may be used behind a firewall and even if signing requires a network connection (signing/attesting a container with a key), there may be a limited set of allowed domains for outbound traffic. This isn't hypothetical either, we've gotten bug reports/FRs about this in the past.
Is there a set of flag values we can use to determine if TUF targets are needed?
For Cosign v3, we do need to clarify what --offline does to actually mean "there are no outbound connections" and disallow certain flag combos.
| // Verify SCT if present in the leaf certificate. | ||
| contains, err := cosign.ContainsSCT(leafCert.Raw) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if contains { |
There was a problem hiding this comment.
FWIW, this check isn't really meaningful and I'm tempted to say we just scrap it, to simplify this and limit when we need TUF metadata.
On signing with Fulcio, we verify the SCT which is more of just an early check that verification will succeed when done by the consumer.
Here, this is when a certificate was fetched out of band, e.g private PKI, and will be attached to the container. It doesn't matter if the certificate contains an SCT or not, this is just provided by the signer. If it doesn't contain an SCT, then verifiers must later specify the ignoreSCT flag regardless.
Arguably, neither of these checks matter, as verification policy doesn't necessarily match what the signer might trust. I think this check here is even less meaningful since it's really just meant for private PKI which very likely doesn't use CT.
There was a problem hiding this comment.
it's really just meant for private PKI which very likely doesn't use CT.
"very likely" doesn't mean "never". I wouldn't feel good about burying a behavior change like this inside this giant PR. I can open a separate PR to remove this so discussion can be had on it independently and it can be referred back to if someone has questions about it later.
| } | ||
|
|
||
| func setTUFMirror(opts *tuf.Options) error { | ||
| if tufMirror := env.Getenv(env.VariableTUFMirror); tufMirror != "" { //nolint:forbidigo |
There was a problem hiding this comment.
We currently have a flag for the mirror with cosign initialize, --mirror, and I believe also one for the root --root. Can we reuse these along with the env var?
There was a problem hiding this comment.
It would be a little cumbersome to add two new flags for every sign and verify command and pipe those values through to here. I feel like environment variables are friendly to the user because they could just source an environment file and then all their cosign commands work for that TUF environment. How strongly do you feel about adding flags?
| } | ||
|
|
||
| func (s *sigContent) EnvelopeContent() verify.EnvelopeContent { | ||
| panic("not implemented") |
There was a problem hiding this comment.
Can we return nil? I'm hesitant to put any panics in here that might inadvertently get called.
There was a problem hiding this comment.
If it gets called it's a programmer error, returning nil would hide that and make it harder to fix the bug. I could change the error message?
Use sigstore-go's TUF client to fetch the trusted_root.json from the TUF
mirror, if available. Where possible, use sigstore-go's verifiers which
natively accept the trusted root as its trusted material. Where there is
no trusted root available in TUF or sigstore-go doesn't support a use
case, fall back to the sigstore/sigstore TUF v1 client and the existing
verifiers in cosign.
TODO:
Fixes #3548
Summary
Release Note
Documentation