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

pyproject: bump sigstore-protobuf-specs #705

Merged
merged 18 commits into from
Jul 21, 2023
Merged

Conversation

woodruffw
Copy link
Member

@woodruffw woodruffw commented Jul 12, 2023

This bumps sigstore-protobuf-specs to the 0.2.x series which, among other things, changes the required status of a few fields.

Draft until I figure out what needs to change on our client side.

Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw self-assigned this Jul 12, 2023
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
...but only if inclusion_proof is not None.

Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
Copy link
Member Author

To summarize what's happening here:

  1. We now check the bundle version explicitly, and apply different requirements based on the version. More specifically, we require the inclusion proof on 0.2, or the inclusion promise on 0.1.
  2. Internally, we enforce that verification begins with an inclusion promise, an inclusion proof, or both, but not neither. In other words: the weakest verification we'll do is an offline verification with just the inclusion promise or proof.

I need to think a bit more about (2) and ensure that we're doing what's stated above, but this should be consistent with both bundle formats.

@woodruffw woodruffw marked this pull request as ready for review July 12, 2023 18:24
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
Copy link
Member Author

One edge case that (I think) still needs to be covered here is around checkpoints. Here are the different states we can be in (limited to just inclusion proofs and checkpoints, not considering inclusion promises):

  1. 0.1 bundle: no inclusion proof, no checkpoint
  2. 0.1 bundle: inclusion proof and checkpoint
  3. 0.1 bundle: inclusion proof but no checkpoint
  4. 0.2 bundle: inclusion proof and checkpoint

When in online mode, all of these variants are handle-able: (1) and (3) should cause us to perform an online entry lookup, while (2) and (4) will be handled using the supplied inclusion proof.

When in offline mode, (1) and (3) are murky: it's unclear whether they should be error cases, whether we should verify the inclusion promise instead (if present), or something else. My intuition is that they should be error cases unless the inclusion promise is present, but it'd be nice to have some guidance/normative behavior here.

CC @znewman01 for opinions on the client behavior side + @haydentherapper for behavioral opinions as well 🙂

Signed-off-by: William Woodruff <william@trailofbits.com>
Apparently Pydantic needs this now.

Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
@haydentherapper
Copy link
Contributor

My intuition is that they should be error cases unless the inclusion promise is present, but it'd be nice to have some guidance/normative behavior here.

I agree with this, but we should decide at what point we deprecate this behavior and mandate inclusion proofs. Do we have a deprecation policy in place yet?

For justification, an inclusion proof without a checkpoint should be considered invalid since there's no link to the log. If you can't query online or do not have a promise (both checkpoints and promises are signed with the same key, so while each commits to something different, they're still both commitments from the log), the client must return an error.

@woodruffw
Copy link
Member Author

I agree with this, but we should decide at what point we deprecate this behavior and mandate inclusion proofs. Do we have a deprecation policy in place yet?

Agreed! I don't think we have a deprecation policy in place; CC @znewman01 as clients czar 🙂

@znewman01
Copy link

I think deprecation is up to each individual client. For instance, Go mandates a strict adherence to semver. Other ecosystems may have other conventions. You can look at the Cosign versioning policy for an example: https://github.com/sigstore/cosign/blob/main/VERSIONING.md

My preference is probably:

  • library API should be pretty stable, new major version for brekaing changes (releasing new library major version is cheap)
  • CLI behavior should try not to break without warning. But a rolling deprecation train is way easier than trying to cram every breaking change in at the same time.

+1 to offline-mode being totally offline and erroring out if additional information is needed.

Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw requested review from tetsuo-cpp and removed request for tetsuo-cpp July 17, 2023 16:14
@woodruffw woodruffw requested a review from tnytown July 17, 2023 16:14
@woodruffw
Copy link
Member Author

This should be good to go now, and will unblock fixing the conformance tests; CC @di and @tnytown for review.

@haydentherapper
Copy link
Contributor

@jku Do you want to take a look too?

Comment on lines 279 to +286
# 6) Verify the Signed Entry Timestamp (SET) supplied by Rekor for this artifact
try:
verify_set(self._rekor, entry)
except InvalidSETError as inval_set:
return VerificationFailure(reason=f"invalid Rekor entry SET: {inval_set}")
if entry.inclusion_promise:
try:
verify_set(self._rekor, entry)
except InvalidSETError as inval_set:
return VerificationFailure(
reason=f"invalid Rekor entry SET: {inval_set}"
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the inclusion promise/SET optional for 0.2 bundles?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. 0.2 inverts the behavior here:

  1. With 0.1, the SET is mandatory and the inclusion proof is optional;
  2. With 0.2, the SET is optional and the inclusion proof is mandatory.

Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw requested a review from di July 21, 2023 15:08
@woodruffw
Copy link
Member Author

CC @di for review, if you have the time 🙂

@woodruffw woodruffw merged commit 52b2f6f into main Jul 21, 2023
23 checks passed
@woodruffw woodruffw deleted the ww/bump-protobuf-specs branch July 21, 2023 18:01
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.

5 participants