Add option to sign multiple artifacts with the same key and certificate

[mayaCostantini]

Summary

Resolves #514

Add a --single-cert option to thesign subcommand to sign multiple artifacts by generating one private key and requesting a single certificate from Fulcio.

Release Note

• Add a --single-cert option to sign multiple artifacts with a single key and certificate

Documentation

Added corresponding documentation in README.md (sign command output).

[mayaCostantini]

Converting to draft for fixing CI

[woodruffw]

Thanks @mayaCostantini! I'll have some time to review this later today.

[jku]

Based on Williams useful explanation in #514 (comment), should we consider:

1. Making multiple signatures with single CSR (so single_certificate=True) the default
2. Even not having the option at all -- this could be added later if someone actually needs it

[mayaCostantini]

We could go for 1., I didn't make it the default mode here just in case I failed to identify a potential use case for having one CSR per artifact.
However this approach could fail if signing a large number of files, but I doubt this would happen given the certificate validity window (10min seems to be way enough). I could run some benchmarks if necessary to remove any doubt.

As for 2., we could leave the parameter in the Signer constructor with True as the default, but we could hide it from the CLI to avoid confusion. What do you think?

[jku]

As for 2., we could leave the parameter in the Signer constructor with True as the default, but we could hide it from the CLI to avoid confusion. What do you think?

I was thinking that for the API user the Signer argument is not really necessary as the caller can just create several Signers if they want several CSRs to be used (while still reusing the same identity token).

However, looking at the API again I'm reminded the token is not a constructor argument to Signer, but an argument to sign()... so one Signer can be used to sign with multiple identities -- which means if Signer wants to store the signing certificate for re-signing, it would have to cache a certificate per each identity token and only reuse the cert if the tokens match.

I wonder if it it would be cleaner to have another signer class instead (like I think William may have suggested at one point), one that would take the token as a constructor argument and would cache the CSR?

[woodruffw]

@mayaCostantini given that this changes the internal state of the Signer quite a bit, what do you think about one of these two designs:

• Breaking this out into a SingleKeySigner/CachingSigner, per Support signing multiple artifacts with a single certificate #514 (comment)
• Flipping the behavior and making this the default, with Signer(caching=false) or similar

I'm okay with either of these (with a slight preference for the second), and IMO either would result in a cleaner diff/easier to maintain internal invariants.

If we go the second route, we'll probably want the instance to be "self-maintaining" -- accessing the signing key and materials should always produce materials that are valid at the current time, so both should be properties that transparently refresh as needed under the hood (except for in the caching=False case, where we should always produce a new key and certificate).

[woodruffw]

However, looking at the API again I'm reminded the token is not a constructor argument to Signer, but an argument to sign()... so one Signer can be used to sign with multiple identities -- which means if Signer wants to store the signing certificate for re-signing, it would have to cache a certificate per each identity token and only reuse the cert if the tokens match.

Oh, this is a good point -- I forgot that I had designed it this way...

Yeah, a more general refactor is possibly in order here: sign(...) should(?) conceptually only take the input being signed over, with the Signer maintaining all other state.

There's some weird impedance mismatch there, though -- constructing a Signer with an internal identity token means that the token will expire at some point; at that point the Signer instance will enter an invalid state, even if we refresh the CSR and short-lived key. That might be pretty confusing for end users, especially since different IdPs might have different expiration policies and timespans.

On the other hand: maybe that's okay? It's no different from the current sign(...) error behavior if the user passes in an invalid or expired identity token; the only change is the "distance" between the state's introduction and where the error happens.

[jku]

Yeah, a more general refactor is possibly in order here: sign(...) should(?) conceptually only take the input being signed over, with the Signer maintaining all other state.

I think I agree with this -- it makes sense for the signer to be tied to a single identity -- but also with your other observations in this comment... Maybe the easy/correct solution is a new signer-class that implements this slightly different concept? Naming is hard, so I have not come up with a good name for this SingleCertificateSigner. If it becomes clear that the new class provides everything users need, then the old one can be deprecated and removed later.

[woodruffw]

If it becomes clear that the new class provides everything users need, then the old one can be deprecated and removed later.

These changes will land with 2.0, so I'm personally okay with a decent bit of breakage here! So long as we can come up with a suitable API 🙂

Just to write the constraints down in a single place:

1. The Signer instance itself should (ideally) never become "invalid", at least not over reasonable object lifetimes (minutes-to-hours, not weeks-to-years);
2. A Signer should be bound to a single identity: multiple calls to sign(...) with different identity tokens are confusing (if not currently dangerous, since the underlying key material is always refreshed);
3. A Signer should, by default, reuse its ephemeral key material (and certificate) for as many signing operations as possible.

One idea:

1. Signer is renamed to SigningContext
2. SigningContext.sign is renamed to SigningContext.with_signer(identity_token=...), which is a context manager for a new Signer class
3. That new Signer class handles the invariants above: it can internally cache keys, and ensures that the identity token is bound in rather than passed in.

As an example:

# access the production Sigstore instances
production = SigningContext.production()

with production.with_signer(identity_token=...) as signer:
    result = signer.sign(...)

Thoughts? I'm still not 100% happy with that (maybe it shouldn't be a context manager?), but it's food for thought 🙂

[mayaCostantini]

Thanks for your thoughts on this. I personally like the idea of a context manager; I find it good to make API users understand that resources associated to signing (key and certificate) are ephemeral and should remain within a certain scope to avoid mixing up identities.

Does something like the following implement what you were thinking of @woodruffw ?

from contextlib import contextmanager

# New `Signer` class
class Signer:
    def __init__(self, identity_token: str, cache: bool=True):
        self._identity_token: str = identity_token
        self._signing_certificate: Optional[FulcioCertificateSigningResponse] = None
        self._private_key: Optional[ec.EllipticCurvePrivateKey] = None
        self.cache : bool = cache
        
    @property
    def private_key(self) -> ec.EllipticCurvePrivateKey:
        # Get or generate a private key
        ...
        
    @property
    def signing_cert(
        self,
    ) -> Callable[[str, ec.EllipticCurvePrivateKey], FulcioCertificateSigningResponse]:
        # Get or request a signing certificate for Fulcio with the provided identity token
        ...
    
    def sign(self, input_: IO[bytes]) -> SigningResult:
        # Sign the artifact
        ...


class SigningContext:
    def __init__(self, *, fulcio: FulcioClient, rekor: RekorClient):
        ...
    def production(cls) -> SigningContext:
        ...
    def staging(cls) -> SigningContext:
        ...
    @contextmanager
    def with_signer(self, identity_token: str):
        signer = Signer(identity_token: str, cache=True)
        try:
            yield signer
        finally:
            del signer

[woodruffw]

Does something like the following implement what you were thinking of @woodruffw ?

Yep, looks close to what I was thinking!

We should think a bit more about the lifetimes here (maybe it makes sense for the new Signer to explicitly raise when the identity token is expired?), but I think this is a good starting point 🙂

[mayaCostantini]

maybe it makes sense for the new Signer to explicitly raise when the identity token is expired?

+1. We could add a try... except statement in the sign method when using the certificate to handle certificate expiration errors.
Why not also check if the token is expired before requesting a certificate (even if this is unlikely, as users might not want to initialize the signing_cert attribute on its own but rather undirectly via the sign method).

[woodruffw]

Why not also check if the token is expired before requesting a certificate (even if this is unlikely, as users might not want to initialize the signing_cert attribute on its own but rather undirectly via the sign method).

I'm not directly opposed to that, although I think it produces an unidiomatic structure in end-users of the API: they'll end up wrapping the context manager in a try...except, when in principle all "exceptional" handling ideally happens at the smallest unit of work (i.e. within the manager instead of around it).

Since we'll need to do the check on every signing operation anyways, I think we should encourage end-user usage like this:

with ctx.with_signer(identity_token=..., cache=True):
    try:
        signer.sign(...)
    except ExpiredIdentity:
        ...
    except ExpiredCertificate:
        # NOTE: Can only happen when cache=True
        ...

[mayaCostantini]

Sounds good! Should I proceed with the implementation?

[woodruffw]

Sounds good! Should I proceed with the implementation?

Works for me! I assume you'll need an IdentityToken.expired or similar API, but we can address that when it comes up 🙂

[mayaCostantini]

@woodruffw @jku feel free to take a look at ff30b09
If this looks good to you, I can commit the updated tests and docs as well.

[woodruffw]

Thanks! I'll take a look today.

[woodruffw]

/gcbrun

[woodruffw]

LGTM, great work @mayaCostantini!

I'd like @di to also give this a review before merge.

[woodruffw]

/gcbrun

[mayaCostantini]

Thanks for the thorough reviews and help @woodruffw! I am excited as well to see this in v2.0 🙂

[jku]

This looks good to me.

As a nit-pick, if we don't have a good use case for the CLI flag, maybe it just does not need to exist?

• API users can always handle their signers like they want
• CLI users can invoke sign multiple times if they really need individual certs (admittedly this approach currently has other downsides like repeated TUF initializations)

[woodruffw]

As a nit-pick, if we don't have a good use case for the CLI flag, maybe it just does not need to exist?

Agreed, removing.

[woodruffw]

/gcbrun

[woodruffw]

/gcbrun

[di]

/gcbrun

[woodruffw]

Thanks a ton @mayaCostantini! Excited to land this.

[mayaCostantini]

Thanks a lot to you @woodruffw !