Key management improvement #936

javanlacerda · 2024-03-14T13:43:48Z

Closes #927

It makes a refactor on code for a better key management. It makes the trusted root fully responsible by the key management, concentrating the responsibilities there.

It also removes the unused flags from the CLI. These properties are already provided by the trusted root.
Removed flags:

--certificate-chain
--rekor-root-pubkey
-ctfe

Summary

It is a refactor to make the code cleaner and cohesive.

Release Note

Documentation

cc @woodruffw @jku

jku

I left some specific comments in code. The main feedback is that these items from william are the core ones:

SigningContext and Verifier should probably take a TrustedRoot directly.

RekorClient should have no key material within it at all.

Just to add to those:

SigningContext and Verifier could still have staging() and production() helpers to make things easier for API users but the normal constructor call should look something like Verifier(rekor_client, trusted_root)
CLI should create the TrustedRoot, and pass it to Verifier and SigningContext
the CLI legacy args could be supported by "modifying" the TrustedRoot based on those args
Verifier and SigningContext know that their purposes are verifying and signing: this does not need to be an argument in any of their public methods
RekorClient should not need a trust root or a purpose

It's a long list of things but I think when we manage that , the code is hopefully significantly easier to understand.

sigstore/_internal/trustroot.py

sigstore/_cli.py

jku · 2024-03-15T14:08:13Z

I did not address the actual key choices in different situations... It's friday afternoon and I'm pretty sure I'll get it wrong. If you can document in a comment which keys are used in which situations in your PR that would probably be useful.

The basic idea with KeyringPurpose seems good.

woodruffw · 2024-03-15T14:30:26Z

Thanks @javanlacerda! I agree with @jku's feedback, and I'll add some of my own in a bit as well.

For deconflicting: this will probably interact a bit with #937. In particular, that changeset may end up removing RekorClient from Verifier, since the Verifier will become totally offline and will only need the Rekor keyring, not Rekor REST APIs.

sigstore/_internal/trustroot.py

woodruffw · 2024-03-18T15:34:07Z

@javanlacerda JFYI: it's easier to review the diffs here if you don't force-push each time -- it's okay to have a messy commit history while the PR is in progress, since we can squash or amend it before merge 🙂

javanlacerda · 2024-03-18T15:39:02Z

@javanlacerda JFYI: it's easier to review the diffs here if you don't force-push each time -- it's okay to have a messy commit history while the PR is in progress, since we can squash or amend it before merge 🙂

I apologize!! I had to do that as I forgot to sign one of the commits in the middle 😞

woodruffw · 2024-03-18T15:45:09Z

I apologize!! I had to do that as I forgot to sign one of the commits in the middle 😞

No worries! I personally do alias gc=git commit --signoff, but also don't worry too much about missing ones: I can also fix that during squash.

woodruffw · 2024-03-18T16:51:48Z

/gcbrun

woodruffw · 2024-03-18T16:55:08Z

This is looking pretty good to me, modulo the design question about the CLI flags. That boils down to whether we want to remove the CLI flags outright (fine by me, personally), or whether we want to keep them around for a bit and internally convert them into a TrustRoot.

I personally lean towards the former: we've been wanting to get rid of them for a while, and doing so will make future improvements much more straightforward. If @jku agrees, I think we can just go ahead and delete all of that 🙂

jku

Looks really nice, this seems like the right direction. As for CLI arguments: I think I'm fine with removing them -- we could ping the slack channel with the specific list of arguments we intend to remove to see if anyone complains...

Are we going to lose testing coverage if the arguments are removed? probably not?

I might not have time this week to do proper testing/review but I left some minor comments, generally this looks good. I would really love more "description of changes" (as a simple example, if you move a bunch of code from one file to another, maybe mention if it's a direct copy or somehow modified).

jku · 2024-03-19T10:03:11Z

sigstore/_internal/trustroot.py

+        trusted_root = cls.from_file(path)
+        trusted_root.args = args
+        trusted_root.purpose = purpose
+        return trusted_root


Suggested change

trusted_root = cls.from_file(path)

trusted_root.args = args

trusted_root.purpose = purpose

return trusted_root

return cls.from_file(path, args, purpose)

same suggestion applies to all other helper constructors

jku · 2024-03-19T10:07:59Z

sigstore/_internal/trustroot.py

+    def rekor_keyring(self) -> RekorKeyring:
+        """Return public key contents given certificate authorities."""
+
+        return RekorKeyring(self._get_rekor_keys())


I believe you can just remove this wrapper method, and change
_get_rekor_keys(self) -> Keyring
into
rekor_keyring(self) -> RekorKeyring
right?

it makes sense

jku · 2024-03-19T10:11:50Z

sigstore/_internal/rekor/client.py

+            rekor_keyring=trust_root.rekor_keyring(),
+            ct_keyring=trust_root.ct_keyring(),
        )


I was kind of expecting RekorClient to not need at least one of these (that the users of these keyrings actually have access to a proper keyring and should be using these)... but maybe that's fine to leave for future PR?

I just removed any trustedroot and keyring dependency from rekor client :)

javanlacerda · 2024-03-19T14:57:00Z

Looks really nice, this seems like the right direction. As for CLI arguments: I think I'm fine with removing them -- we could ping the slack channel with the specific list of arguments we intend to remove to see if anyone complains...

Are we going to lose testing coverage if the arguments are removed? probably not?

I might not have time this week to do proper testing/review but I left some minor comments, generally this looks good. I would really love more "description of changes" (as a simple example, if you move a bunch of code from one file to another, maybe mention if it's a direct copy or somehow modified).

Awesome! I will start making more comments about decisions.

Do you guys think we should create a different issue/PR for removing the flags? cc @jku @woodruffw

woodruffw · 2024-03-19T15:42:12Z

Are we going to lose testing coverage if the arguments are removed? probably not?

I believe not -- we basically don't hit the _cli.py routines at all in tests at the moment.

Do you guys think we should create a different issue/PR for removing the flags?

I'm okay with either way. If we do it in a separate PR, we should merge that one first to ensure we don't have to adapt these changes too much. But if you find it easier to just update this one, I don't mind a larger diff here 🙂

javanlacerda · 2024-03-20T17:12:29Z

I did these forced push because I just removed a remaining print 😆 @woodruffw

woodruffw · 2024-03-20T18:32:48Z

sigstore/_cli.py

I think this file also needs deletions for the CLI flags themselves: the _parser() function needs to be updated to remove each of the arg definitions that have had their usages removed below.

sigstore/_internal/trustroot.py

CHANGELOG.md

README.md

CHANGELOG.md

woodruffw · 2024-03-21T16:29:03Z

/gcbrun

woodruffw · 2024-03-21T16:29:34Z

Thanks @javanlacerda, this is looking pretty good! I'm going to give these changes a spin locally and do a final review afterwards, with the goal of merging today.

(Mind updating the branch again?)

javanlacerda · 2024-03-21T16:31:57Z

Thanks @javanlacerda, this is looking pretty good! I'm going to give these changes a spin locally and do a final review afterwards, with the goal of merging today.

(Mind updating the branch again?)

Done! :)

woodruffw · 2024-03-21T17:02:18Z

/gcbrun

refactors and adding trusted_root to Verifier and SigningContext move purpose from rekor client to trusted_root Signed-off-by: Javan lacerda <[email protected]>

Signed-off-by: Javan lacerda <[email protected]>

Co-authored-by: William Woodruff <[email protected]> Signed-off-by: Javan Lacerda <[email protected]>

Signed-off-by: Javan lacerda <[email protected]>

jku · 2024-03-25T13:08:57Z

please leave comments about changes (especially when force pushing): even a "rebase, no code changes" is useful.

/gcbrun

woodruffw

LGTM and didn't observe any breakage in local testing. Thanks @javanlacerda!

javanlacerda marked this pull request as draft March 14, 2024 13:43

jku reviewed Mar 15, 2024

View reviewed changes

woodruffw reviewed Mar 15, 2024

View reviewed changes

sigstore/_internal/trustroot.py Outdated Show resolved Hide resolved

javanlacerda force-pushed the javan.ref.internal_key_management_improvement branch from 261457c to 397e166 Compare March 15, 2024 22:46

javanlacerda marked this pull request as ready for review March 18, 2024 15:12

woodruffw reviewed Mar 18, 2024

View reviewed changes

sigstore/_internal/trustroot.py Outdated Show resolved Hide resolved

javanlacerda force-pushed the javan.ref.internal_key_management_improvement branch from 0b1cb47 to 21eb980 Compare March 18, 2024 15:25

woodruffw added component:signing Core signing functionality component:verification Core verification functionality labels Mar 18, 2024

javanlacerda force-pushed the javan.ref.internal_key_management_improvement branch from 9c63913 to ee94ba2 Compare March 18, 2024 19:50

jku reviewed Mar 19, 2024

View reviewed changes

javanlacerda force-pushed the javan.ref.internal_key_management_improvement branch 2 times, most recently from 4014282 to 28bd775 Compare March 20, 2024 17:09