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

Refactor client trust/trust root management #1010

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

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.

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 woodruffw added the refactoring Refactoring tasks. label May 10, 2024
@woodruffw woodruffw added this to the 3.0 milestone May 10, 2024
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw mentioned this pull request May 14, 2024
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>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw marked this pull request as ready for review May 15, 2024 15:19
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>
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.

Signed-off-by: William Woodruff <william@trailofbits.com>
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
Copy link
Member

@jku jku left a comment

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).

"""
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.)

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.)

Signed-off-by: William Woodruff <william@trailofbits.com>
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:api Public APIs component:signing Core signing functionality component:verification Core verification functionality refactoring Refactoring tasks.
Projects
None yet
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