trust: Support operator field, support multiple services #1407
Conversation
* Change the SigningConfig API so that it returns actual clients (like RekorClient): this may seem unusual but makes sense because SigningConfig must know which services this client supports, this means the code is simplest if it directly returns correct clients (e.g. when rekor has two separate clients for v1 and v2). This allows keeping version, operator, etc as SigningConfig imlementation details * There is one exception to previous point: get_oidc_url() returns string, not Issuer object. I could make this change as well to be consistent, it just requires a small refactor (because currently Issuer makes a http request on construction and that seems bad, especially for testing) * Support operator field: This is used to ensure we only return one service version per operator Signed-off-by: Jussi Kukkonen <[email protected]>
|
oops, apparently I did not test with production :) |
Signed-off-by: Jussi Kukkonen <[email protected]>
I don't think we'll be seeing anything else than ANY for a while but for completeness, support all selector modes for TSA and Rekor. Signed-off-by: Jussi Kukkonen <[email protected]>
jku
force-pushed
the
signing-config-service-select
branch
from
e904663 to
2543a7d
Compare
Signed-off-by: Jussi Kukkonen <[email protected]>
Signed-off-by: Jussi Kukkonen <[email protected]>
* Abstract the ServiceConfiguration handling as suggested in review (so both tsa and rekor are handled in the same way) * This creates some issues as TSAs are still optional... I decided that it is reasonable to require "ANY" selector to be used with at least one service, meaning I have to change the placeholder signingconfig for production. Signed-off-by: Jussi Kukkonen <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: Jussi Kukkonen <[email protected]>
|
Added the changelog entry I had forgot: it's not a great changelog entry but I think we'll need to rewrite the changelog after all the rekor v2 work anyway to make it useful: as long as we have all changes listed it should be doable. |
|
@haydentherapper you're probably the only other person to have implemented this so far: let me know if you see anything fishy in |
|
ramon found multiple bugs: I 'll take that as indication that I need to add the tests in this PR and not later... |
* we want a single service per operator * selector UNDEFINED should be an error * variable in this function should not refer to "logs" but "services" * Method is actually static Signed-off-by: Jussi Kukkonen <[email protected]>
Signed-off-by: Jussi Kukkonen <[email protected]>
Test the service selection with SigningConfig._get_valid_services() Signed-off-by: Jussi Kukkonen <[email protected]>
jku
force-pushed
the
signing-config-service-select
branch
from
944c5ec to
eb657c3
Compare
Signed-off-by: Jussi Kukkonen <[email protected]>
| if not url: | ||
| raise Error("No valid Fulcio CA found in signing config") | ||
| return url | ||
| return FulcioClient(self._fulcios[0].url) | ||
|
|
||
| def get_oidc_url(self) -> str: |
There was a problem hiding this comment.
get_oidc_url() still returns string, not Issuer object.
This method is now the odd one out: it could return Issuer but as mentioned that will need a small refactoring in Issuer -- I can handle that in a followup if the design here is reasonable to reviewers
There was a problem hiding this comment.
This design looks reasonable to me; doing it in a followup sounds good!
| * SigningConfig now has methods that return actual clients (like `RekorClient`) instead of | ||
| just URLs. The returned clients are also filtered according to SigningConfig contents. | ||
| [#1407](https://github.com/sigstore/sigstore-python/pull/1407) |
There was a problem hiding this comment.
Nitpick: SigningConfig is a private API, so perhaps we should omit it from the CHANGELOG? Alternatively, if you think it's important to include from a developer/maintainer visibility perspective, maybe we should include "this is a private API" in some form in the CHANGELOG entry so users don't think we're committing to a public interface here.
There was a problem hiding this comment.
yeah, I was thinking of this as well while adding the entry. In a way SigningConfig semi-public since the signing_config property in ClientTrustConfig is currently public -- and ClientTrustConfig is in practice exposed through the sign and verify modules...
I think the intent of the API now is that ClientTrustConfig is public (so users can e.g. call from_production()) but the internals of ClientTrustConfig are not... so in that way this is not an API change but it's debatable.
I might leave it as is for now: We need to rewrite the changelog anyway for next release (combine related changes, try to make a coherent log).
| @@ -356,54 +373,73 @@ def from_file( | |||
| return cls(inner) | |||
|
|
|||
| @staticmethod | |||
| def _get_valid_service_url(services: list[Service]) -> str | None: | |||
| def _get_valid_services( | |||
There was a problem hiding this comment.
@jku Sorry about the late review. The only change I might suggest is to sort by newest to oldest rather than assuming it's already sorted. In the Sigstore TUF trusted root, we'll have them sorted already.
This is something I am adding to sigstore-go's implementation now, sigstore-java already added this - sigstore/sigstore-go@main...haydentherapper:sigstore-go:operator#diff-35123d504dffe1b547e48e14acb7c7174416a48566d50e5a6b75e898c5667a4fR121-R123
Getting
_get_valid_services()right (and simple) was surprisingly tricky, but I think it's there now -- thanks to ramon for feedback. I think the decision to keep this code contained in trust.py is correct: it's not trivial and sign.py is complicated enough already.@ramonpetgrave64 I think this fits your plans well:
get_tlogs()returns either v1 client or v2 depending on major_version