Skip to content

Refactor client trust/trust root management #1010

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 20 commits into from
May 16, 2024
Merged

Refactor client trust/trust root management #1010

merged 20 commits into from
May 16, 2024

Conversation

woodruffw
Copy link
Member

@woodruffw woodruffw commented May 10, 2024 •
edited
Loading

This rips out a lot of our dedicated state, replacing it with a smaller adapter over the now-standard ClientTrustConfig message.

TODO:

  • Unit tests
  • Documentation (incl. README)

Closes #324.

Closes #916.

woodruffw added 3 commits May 10, 2024 14:18
@woodruffw
sigstore: refactor trust state management
efd4119 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
test: rename
b94888b 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
docstring
e5d24b0 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw added the refactoring Refactoring tasks. label May 10, 2024
@woodruffw woodruffw added this to the 3.0 milestone May 10, 2024
woodruffw added 2 commits May 10, 2024 14:25
@woodruffw
sigstore, test: propagate rename
0be13aa 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
_internal: hackety hack
172fc0e 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw mentioned this pull request May 14, 2024
Closed
woodruffw added 7 commits May 14, 2024 14:54
@woodruffw
Merge branch 'main' into ww/refactor-trust
34c7f4b
@woodruffw
sigstore: refactor purpose handling
8999094 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
trust: docstring
fa05d27 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
fixup trust tests
a1c0784 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
test_sign: fixup
239397a 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
hook up client trust config
5c6b4f1 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
README: update --help
02d121b 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw marked this pull request as ready for review May 15, 2024 15:19
woodruffw added 5 commits May 15, 2024 11:31
@woodruffw
README: document BYO PKI
e283cf7 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
sigstore: enforce client trust config media type
670a3cf 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
fix type
4bb731c 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
enforce media types, unit tests
8f6e42f 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
test: more trust tests
979ac6d 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw requested review from jku, javanlacerda and di May 15, 2024 16:04
@woodruffw
Copy link
Member Author

This should be good to go -- the diff is huge because of the checked-in JSON assets, but the core change is pretty minimal:

  • TrustedRoot is no longer a subclass of the proto TrustedRoot, but its own class with a pImpl-style pattern for the inner proto.
  • We now pass KeyringPurpose on individual APIs on TrustedRoot, rather than initializing the root with a purpose. This is more consistent with how the APIs are actually used, and makes the wrapper class a little simpler (plus, it allows for a future refactor where we only initialize the trust root once at client start, not separately for signing/verification).
  • ClientTrustConfig now exists and is plumbed through via --trust-config. It's also documented in the README.

@woodruffw
CHANGELOG: record changes
f7f3307 
Signed-off-by: William Woodruff <william@trailofbits.com>
javanlacerda
javanlacerda approved these changes May 15, 2024
View reviewed changes
Copy link
Contributor

@javanlacerda javanlacerda left a comment

Choose a reason for hiding this comment

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

Great update! LGTM

@jku
Copy link
Member

jku commented May 16, 2024

On a first pass this looks great (will have another look after more coffee).

The various construction methods (production() etc) still use a lot of hard coded values and that makes e.g. SigningContext construction seem wildly different in the different cases... but as long as the client config is not yet available via TUF, some of those values are required. So I agree it makes sense to simplify that later rather than try to do some of it now

jku
jku previously approved these changes May 16, 2024
View reviewed changes
Copy link
Member

@jku jku left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good.

There seems to be a clear path to integrating the ClientTrustConfig from tuf once that's available. I believe that will further simplify the construction options of SigningContext and Verifier (as you said the trust/config initialization can then be done once in a single place).

sigstore/_internal/trust.py
"""
Construct a new `TrustedRoot`.

@api private
Copy link
Member

Choose a reason for hiding this comment

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

I've never seen @api private and can't find any docs (maye because "api" is one of those ruined search words). What does this do?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a pdoc (our document generator) thing -- it marks an otherwise public API (like the ctor) as not receiving public documentation, so that we don't commit to 3p proto types in our public APIs 🙂

(Then again, it's entirely redundant here since _internal.trust is already undocumented as a completely private API.)

sigstore/sign.py
Comment on lines +337 to +342
def _from_trust_config(cls, trust_config: ClientTrustConfig) -> SigningContext:
"""
Create a `SigningContext` from the given `ClientTrustConfig`.

@api private
"""
Copy link
Member

Choose a reason for hiding this comment

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

This should not be private?

(same comment for verifier.py)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we want to eventually make it public, but doing so right now will result in a visibility mismatch: the ClientTrustConfig type is in the _internal namespace, so a public API here will need to know how to instantiate a private type.

(My thinking was that it could be left as private for 3.0, and made public with a refactor in 3.1.)

@woodruffw
Merge branch 'main' into ww/refactor-trust
7c891db 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw mentioned this pull request May 16, 2024
Open
@woodruffw
README: fix --help
5c49a6d 
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw requested a review from jku May 16, 2024 14:45
@woodruffw woodruffw added component:signing Core signing functionality component:verification Core verification functionality component:api Public APIs labels May 16, 2024
@woodruffw woodruffw merged commit d425770 into main May 16, 2024
25 checks passed
@woodruffw woodruffw deleted the ww/refactor-trust branch May 16, 2024 15:04
@haydentherapper haydentherapper mentioned this pull request May 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jku jku jku approved these changes

@javanlacerda javanlacerda javanlacerda approved these changes

@di di Awaiting requested review from di

Assignees
No one assigned
Labels
component:api Public APIs component:signing Core signing functionality component:verification Core verification functionality refactoring Refactoring tasks.
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

Bring your own PKI CLI: Limit user confusion over Rekor/Fulcio instance and state URLs
3 participants
@woodruffw @jku @javanlacerda