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 certificate validity with only current time #277

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

haydentherapper
Copy link
Contributor

Using a certificate's NBF will always pass the time verification. We should be using only the current time to try to verify a certificate's validity. This is likely to only work with long-lived certificates or where verification happens immediately after signing.

Fixes #276

Summary

Release Note

Documentation

Using a certificate's NBF will always pass the time verification. We
should be using only the current time to try to verify a certificate's
validity. This is likely to only work with long-lived certificates or
where verification happens immediately after signing.

Fixes sigstore#276

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

@woodruffw Do you know if we have any conformance tests around verifying certificates against current time?

@haydentherapper
Copy link
Contributor Author

Well, right after I wrote that, a conformance test failed, so I guess yes.

@haydentherapper
Copy link
Contributor Author

I think the issue is we're conflating two things in sigstore-go - verification of the certificate and that a signing event occurred when the certificate is valid.

In Cosign, the way this was dealt with is that the certificate is always verified with NotBefore, and then later when we compare the time of signing against the certificate, we use current time as a fallback.

I think we should do the same here. This mirrors what sigstore-python does (I assume other clients as well given this is in the conformance suite) - sigstore-python uses NBF to verify the certificate with a later check against provided timestamps.

@haydentherapper
Copy link
Contributor Author

The other question is what we want this fallback behavior to be. If we require current time to be used as a fallback, that would mean that you MUST provide a signed timestamp for short lived certs, which I think is the behavior we want. If the fallback would use NBF, then we are effectively skipping the timestamp check for a certificate - we do say this is an unsafe behavior as per the policy flag. I’d like to hear more from clients if we want to have this - it seems only useful for testing, which I would rather not have policy flags for just that.

@haydentherapper
Copy link
Contributor Author

Another question - how should conformance pass when given only a signature and certificate? I guess all clients are using NBF, because otherwise it wouldn’t be possible to verify a short lived cert?

@woodruffw
Copy link
Member

@woodruffw Do you know if we have any conformance tests around verifying certificates against current time?

I don't think we do yet, unfortunately -- I believe everything in the conformance suite currently verifies using the signed time source when present (or the client is expected to retrieve it, if not present). But the latter isn't enforceable, so in theory some clients could be allowing verification to succeed based on NBF being correct.

how should conformance pass when given only a signature and certificate? I guess all clients are using NBF, because otherwise it wouldn’t be possible to verify a short lived cert?

At least for sigstore-python, we currently perform an online lookup if we're given only detached materials, so we don't use just the notBefore time -- we use whatever signed time we get back from the transparency service. But I'm not sure about other clients 🙂

@haydentherapper
Copy link
Contributor Author

haydentherapper commented Aug 19, 2024

In chatting with some other clients and thinking more on this, there are a few cases to handle:

  • Verifying short-lived certificates as part of conformance tests
  • Verifying short-lived certificates when no timestamp is presented
  • Verifying long-lived certificates (not a part of Sigstore's spec, but for bring-your-own PKI or modified Fulcio instances)

For the first and second, I think the right solution is to require a signed timestamp. For conformance tests, either

  1. we need to keep the current code to include a policy to skip the timestamp check (exposing a significant footgun to typical users)
  2. don't integrate with the detached material conformance test (maybe this is fine, the bundle tests should be sufficient)
  3. follow what sigstore-java is doing and fetch an SET on demand during conformance to get a signed timestamp

For the third, this is where I think we should fallback to current time when no time is provided. Either we could remove WithoutAnyObserverTimestampsUnsafe and fallback by default, or rename/alias this to WithCurrentTime since it isn't unsafe as long as it is doing some timestamp check.

@steiza @codysoyland is there a use case for the WithoutAnyObserverTimestampsUnsafe policy flag that I'm unaware of?

@kommendorkapten
Copy link
Member

This is great @haydentherapper!

I'm not aware of any use cases for WithoutAnyObserverTimestampsUnsafe. And my reasoning is aligned with yours, if you are using "classical" long-lived certificates, you do want to verify against current time, as that's more of a typical verification flow when using certificates.

Replacing WithoutAnyObserverTimestampsUnsafe with a new policy WithCurrentTime is a good idea, as it's explicit on what it does, and does not offer a method to bypass a timestamp check.

@steiza
Copy link
Member

steiza commented Aug 26, 2024

There's a lot here! Let me try to walk through the Sigstore client spec, conformance tests, and then we can talk about what we should do here.

From the Sigstore client spec, I think there was some misunderstanding when we were implementing sigstore-go as to what was meant under "Establishing a Time for the Signature":

The Verifier MUST perform certification path validation (RFC 5280 §6) of the certificate chain with the pre-distributed Fulcio root certificate(s) as a trust anchor, but with a fake “current time.” If a timestamp from the timestamping service is available, the Verifier MUST perform path validation using the timestamp from the Timestamping Service. If a timestamp from the Transparency Service is available, the Verifier MUST perform path validation using the timestamp from the Transparency Service. If both are available, the Verifier performs path validation twice. If either fails, verification fails.

At the very least, we should probably call out here what to do if there's no timestamp from the timestamping service or the transparency service. Some options for sigstore-go to consider:

  • Attempt to query the transparency service online (it sounds like this is what sigstore-python does). This would allow the conformance test to pass, but maybe we want to move away from online lookups from the transparency service?

  • Fail (which is what this pull request does). This isn't a bad option, although we should probably update the conformance test to not use detached materials (which we want to do anyways).

  • Fall back to ensuring the current time just matches the certificate's NBF. If we decide to go down this route, we should definitely update the client specification.

It's not clear to me what the next step is here. Maybe we should agree on what the behavior should be, make any necessary updates to the client specification and / or conformance tests, and then update the client behavior?

I'm leaning towards "fail", although it'll make sigstore-go harder to use with detached materials, which will exist for a while even as we move away from them. I think we can work around it by ensuring cosign does online transparency service queries when constructing a bundle to verify.

@woodruffw
Copy link
Member

I'm leaning towards "fail", although it'll make sigstore-go harder to use with detached materials, which will exist for a while even as we move away from them. I think we can work around it by ensuring cosign does online transparency service queries when constructing a bundle to verify.

Big +1 for this approach: this is what we settled on for sigstore-python, as a compromise between loosening the client's default timeliness guarantees and requiring an online lookup as part of verification for detached materials.

@kommendorkapten
Copy link
Member

At the very least, we should probably call out here what to do if there's no timestamp from the timestamping service or the transparency service

This is interesting, I need to refresh my memory on the client spec. But this is the scenario when neither a RFC 3161 signed timestamp nor a SET exists. I'm tempted to update the client spec to explicitly state that the verification should fail. As the client spec should define the behaviour of a default sigstore client.

But that does not mean that sigstore-go or other client's does not allow for policies like WithCurrentTime as that can be useful for certain deployments like BYOPKI where more classical X.509 PKI is used.

@haydentherapper
Copy link
Contributor Author

haydentherapper commented Aug 27, 2024

It sounds like we're all in agreement! So to confirm:

  1. We will update the client spec to be explicit that at least one signed timestamp must be provided by default (Client spec: Specify that certificate verification should fail without a signed time source, current time with explicit policy architecture-docs#15). (I also added Client spec: Timestamp verification should be best effort, not hard fail architecture-docs#16 to document that a client should not fail on individual timestamp verification, that it's best effort to find as many timestamps as possible - if you disagree, feel free to chime in there)
  2. This PR will be updated to have a WithCurrentTime policy flag, and fail if no signed timestamp is verified.
  3. I will update conformance testing for detached tests to fetch a proof, like what sigstore-python does. We will remove detached tests entirely as a separate discussion.

I'm leaning towards "fail", although it'll make sigstore-go harder to use with detached materials, which will exist for a while even as we move away from them. I think we can work around it by ensuring cosign does online transparency service queries when constructing a bundle to verify.

If we did want to keep detached conformance testing, what about if we update the conformance test to also support providing a proof as detached material? Cosign already persists this. If online queries are only for conformance testing, that's fine-ish (this is what sigstore-python does, as William told me), but a) we should strongly discourage online lookups and I'd love to just remove online lookups from clients entirely, and b) tile-based logs remove the ability to query by hash, so we'll need to move towards proofs as detached material regardless (or remove detached conformance tests, so this would be moot).

@steiza
Copy link
Member

steiza commented Aug 28, 2024

If we did want to keep detached conformance testing, what about if we update the conformance test to also support providing a proof as detached material?

I'd rather just remove detached materials from the conformance tests. If we add a flag to https://github.com/sigstore/sigstore-conformance/blob/main/docs/cli_protocol.md#verify to provide a proof separately, then all the clients have to update in order to support that new flag. Let's start moving away from detached materials!

@haydentherapper haydentherapper marked this pull request as draft August 29, 2024 22:28
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.

Certificate time is always valid when no timestamp is provided
4 participants