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% 36.28% -3.83%
==========================================
Files 155 204 +49
Lines 10044 13029 +2985
==========================================
+ Hits 4028 4727 +699
- Misses 5530 7716 +2186
- Partials 486 586 +100 ☔ View full report in Codecov by Sentry. |
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've caught up on all the discussion, thanks Zach for the thorough chart! #3548 was not clear on what was in scope for the TUF v2 upgrade. When I wrote it, my goal was to move to TUF v2 independent of the trusted root changes. This was when the client was quite new, so I wanted to be able to test out TUF v2 without making any other changes, leveraging the client to fetch root material by name. As noted in the issue, we'd still need to support TUF v1 to fallback to the custom metadata and for private deployments that are using custom metadata. There is less motivation for doing this change by itself as we have more confidence in the TUF client now, though I had hoped we could make the change by itself still because it will keep these PRs smaller. The original version of this PR - where you initialize metadata with the new TUF client and fallback to the old TUF client - was what I had in my head when I wrote the issue. My concern with using env vars isn't necessarily that users have to set it - functionally, TUF v1 and v2 should be equivalent, so setting these are more for our testing during development than the end users. The concern is that we have to rely on the hardcoded target names without custom metadata. If we have to rotate the roots in the next N months, then the TUF v2 client won't pick up these changes unless we hardcode more filenames. This is why I had proposed to concurrently support TUF v2 and v1 clients - leverage TUF v2 for harcoded targets, TUF v1 for custom metadata, and merge. I also recognize that this is functionally a no-op, that we get none of the benefits of the trusted root (validity windows) and still require provided TUF root to use custom metadata (though a separate topic, we will have to for quite awhile or else we'll break old Cosign clients). At this point, I think we need to make a call on next steps. Are we going to interlink TUF v2 and the trusted root work, and say that a) TUF v1 is only for custom metadata and only support "legacy" verification with detached material/the old bundle format and b) if you provide either a trust root or verification bundle, the client will use TUF v2? My only hesitance is around the sequencing of the refactoring. I want to make sure we keep PRs small so we can iterate faster. Having some intermediate PRs like this lets us test out well-scoped changes. Tangentially, addressing:
I think it would be worthwhile to do this. This is the only way we can minimize the likelihood of introducing unexpected breaking changes. I'll respond on #3854 about this. |
|
I see now how trying to switch the internals of cosign's individual key material fetching functions isn't going to result in a seamless outcome. I'm afraid, however, that conditioning the which-TUF-client decision on what kind of bundle is in use is also going to miss several important pathways, namely signing (there is no bundle yet) and image verification (also no protobuf bundle). I've attempted to take a step back and enumerate everywhere we are obtaining and using trusted keys in some form: Key type: CT Log
ProposalWhere Caveat: cosign's version of Key type: Fulcio CA
The Fulcio CA is NOT retrieved from TUF for verifying the certificate for blobs or blob attestations - the Fulcio certificate must be passed in by a flag or extracted from the bundle. ProposalWhere Key type: Rekor pub
ProposalWhere
I'm in agreement with @steiza now, I think separating TUF v2 from trusted root is a non-starter at this point. TUF v2 does not support custom metadata, so it's not just a no-op, it's really a big step backwards and makes users less secure. Really the only reason to use the TUF v2 client is to enable support for fetching the trusted root.
In this PR's current design (which we do not want to follow anymore), users would have had to set those env vars in order to opt into trusted root and TUF v2. In the proposal I've outlined above, I think we can make them optional, since we will always fall back to TUF v1 if we can't safely initialize a TUF v2 client or the TUF mirror does not have a trusted root. |
|
I've updated this PR to approximately implement the plan I laid out in my comment. I've done some light manual testing but it will need significant e2e and unit testing. I wanted to make sure there is buy-in on this approach before setting to work on that. I think we should also add support for setting |
|
I think this approach is great! But I'll admit I don't fully understand the constraints of OCI verification, so we should probably get feedback from @codysoyland as well. I think these changes around getting the trusted root could be used to improve the implementation in #3796 or (like you say) bring back some elements of #3854, but I agree that that can wait for a later pull request. Nice work! |
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 <colleenmurphy@google.com>
|
@cmurphy Fantastic summary of the work necessary to implement this! I'm supportive of all of the above.
For verification, root certificates should be pulled through TUF through an API in sigstore/sigstore. We had started a refactor a long time ago to move TUF logic into this shared repo but never finished. Or is this referring to bring-your-own PKI where roots are provided via flag?
That makes sense, and if it's less work to not try and separate these two goals, let's not try to. Our goal is to adopt TUF v2 wholly, so no point in trying to have an intermediate no-op state. |
This was referring to the slightly different logic between |
There was a problem hiding this comment.
I read over the history, and (perhaps I missed it) has this option been considered:
Is there a way to just build a trusted_root.json from the provided materials. We provide sensible defaults for config (like if provided on the command line -- they are just valid forever). Then only use the tursted_root code path for everything?
If that sounds dumb then I'll just finish up a regular code review here.
| // Detached SCTs cannot be verified with this function. | ||
| certs, err := cryptoutils.UnmarshalCertificatesFromPEM(fs.Cert) | ||
| if err != nil || len(certs) < 1 { | ||
| return nil, fmt.Errorf("unmarshalling SCT from PEM: %w", err) |
There was a problem hiding this comment.
Would "unmarshalling certificate from PEM for SCT verification" make more sense? or am I misreading this?
Yeah, so there are several things happening in parallel, if I understand your question correctly. This pull request is "just" about upgrading cosign's TUF client from the existing version which fetches verification material as individual files to TUFv2 which fetches all the verification material in one I think your question is "in the future, once cosign verification paths support making use of a My answer is that we could, although we might also delete those old verification paths and ask people to generate their own trusted root with something like #3876. But I think the work laid out in this pull request is a prerequisite, either way. |
|
@codysoyland and I talked about this pull request yesterday at the sigstore-go meeting. We think this is compatible with the work he's doing on OCI and is good to proceed! |
|
As @steiza mentioned, I've had a bit of time to look at the latest changes here, and I'm pleased with the recent changes! It will complement my work on container image bundle verification quite well (we even both added |
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