signer: add basic sigstore signer and verifier #522

lukpueh · 2023-02-28T16:10:17Z

Adds SigstoreSigner and SigstoreKey implementations to the securesystemslib signer interface. They can be used to create and verify signatures using an OIDC token and an identity/issuer pair respectively, as described in the TAP:
"Ephemeral identity verification using sigstore's Fulcio for TUF developer key management"

IMPORTANT NOTES:

~~the created signature does not strictly follow the typing requirements of the interface~~
currently, only offline verification is supported

This patch also adds a GitHub Action workflow that runs a tailored test, using ambient credentials for signing.

This should already work, but I'd like to fix a few things before I mark this "ready for review".

fix signature interface incompatibility
~~fix circular imports and inline-import workaround~~ (local scope import seems like the right choice here)
make sure signer remains importable even if sigstore is not installed
implement from_priv_key_uri (maybe in follow-up PR)
~~make online/offline verification configurable (maybe in follow-up PR)~~ (let's discuss this in the TAP)

lukpueh · 2023-02-28T16:13:47Z

Run Sigstore Signer tests / test-sigstore (pull_request) Failing after 46s

need to update permissions ...

jku · 2023-02-28T16:50:14Z

securesystemslib/signer/_sigstore_signer.py

+            NOTE: The ``signature`` attribute of the returned object
+            contains the ``dict`` representation of a sigstore ``Bundle`.
+            This is incompatible with the TUF specification and the
+            ``Signature` interface, which expect the attribute to be of type `str`.


Could store the bundle in a separate "bundle" field in the signature, nothing forces us to use "sig", right?

To keep spec conformance at 100% could even fill "sig" with the actual signature value -- even if it's useless without the bundle

securesystemslib/signer/_sigstore_signer.py

Adds SigstoreSigner and SigstoreKey implementations to the securesystemslib signer interface. They can be used to create and verify signatures using an OIDC token and an identity/issuer pair respectively, as described in the TAP: "Ephemeral identity verification using sigstore's Fulcio for TUF developer key management" https://github.com/theupdateframework/taps/blob/3b0d8b108758266e36790b3a782b943d633ce585/candidate-fulcio-tap.md IMPORTANT NOTES: - the created signature does not strictly follow the typing requirements of the interface - currently, only offline verification is supported This patch also adds a GitHub Action workflow that runs a tailored test, using ambient credentials for signing. Signed-off-by: Lukas Puehringer <[email protected]>

Signed-off-by: Lukas Puehringer <[email protected]>

The sigstore library is an optional and circular dependency in securesystemslib. Importing it in the methods, where it is needed, allows importing the module from within sigstore (circular import), or if sigstore is not installed. This commit wraps the local scope imports in try/except to fail gracefully if the methods are called w/o sigstore available, and locally disables the related linter warning. The commit also disables a linter import warning in the related test, which requires sigstore. Alternatively, we could install sigstore in the lint job, which does not seem worth the cycles, just for one method call. Signed-off-by: Lukas Puehringer <[email protected]>

Key.verify_signature implementations should only raise UnverifiedSignatureError or VerificationError, even if an optional dependency is missing. This commit wraps everything in the corresponding try block and removes the previously added UnsupportedLibraryError on ImportError. Plus some minor renames for horizontal density. Signed-off-by: Lukas Puehringer <[email protected]>

The Signature.signature field is defined as hex string of the signature byte data. But for sigstore we need to store a complex signature bundle, which also contains signature byte data. To stay compatible with the API, we store both, the signature byte data from the bundle in the Signature.signature field, and the whole bundle under unrecognized_fields. Signed-off-by: Lukas Puehringer <[email protected]>

Signed-off-by: Lukas Puehringer <[email protected]>

Appease linter w/o installing optional requirement in linter run. Signed-off-by: Lukas Puehringer <[email protected]>

Signed-off-by: Lukas Puehringer <[email protected]>

lukpueh · 2023-03-07T14:41:20Z

Okay, I think this is mergable as self-contained feature. To be a fully usable signer, we need to add from_priv_key_uri, but I suggest to do that in a follow-up PR.

The "on: pull_request" test-sigstore job should start working once this PR is merged. A successful run is already available in my fork: https://github.com/lukpueh/securesystemslib/actions/runs/4355175849/jobs/7611437818

jku

Looks good.

I think it's likely that the key format is not final but I think we should get this merged and just keep it "unstable":

TAP really needs an implementation so this serves that need
the implementation should work fine already as long as you don't want anything else than the public good instance

Thinking about the future changes: we can just add keytypes/schemes if we want to support this implementation and a newer one at some point

jku · 2023-03-09T07:22:58Z

securesystemslib/signer/_sigstore_signer.py

+            if not result:
+                logger.info(
+                    "Key %s failed to verify sig: %s", self.keyid, result.reason
+                )
+                raise UnverifiedSignatureError(
+                    f"Failed to verify signature by {self.keyid}"
+                )
+        except UnverifiedSignatureError:
+            raise


Nit: you could probably handle result outside of the try block to avoid this raise-except-raise pattern.

Good point! Can you check if the commit I just pushed resolves this to your liking.

yeah that looks good to me

jku · 2023-03-09T07:26:37Z

tox.ini

+    GITHUB_ACTIONS
+    ACTIONS_ID_TOKEN_REQUEST_TOKEN
+    ACTIONS_ID_TOKEN_REQUEST_URL


these are used by ambient credentials detection?

jku · 2023-03-09T07:36:17Z

securesystemslib/signer/_sigstore_signer.py

+            from sigstore.verify.policy import Identity
+            from sigstore_protobuf_specs.dev.sigstore.bundle.v1 import Bundle
+
+            verifier = Verifier.production()


seeing this code it seems clear that the content in our key should include everything needed to choose the sigstore "instance".

That said, it's quite unclear to me what that all means (rekor and tuf urls? fulcio?): this is really more a question for the TAP than this implementation which should focus on supporting the default instance for now

So no action requested here.

jku · 2023-03-09T07:45:28Z

Should the module or the classes be marked "unstable API" or something?

jku · 2023-03-09T07:48:21Z

.github/workflows/test-sigstore.yml

+  push:
+    branches:
+      - main
+  pull_request:


should remove this as it will unfortunately keep failing for PRs

We should also maybe file an issue for managing these tests that can be only run on upstream branches... so that an issue would be raised when a test fails

oh, I actually did that in kms-tests: you can probably copy the file-ticket-on-failure step from there

should remove this as it will unfortunately keep failing for PRs

Is that the case? Shouldn't it work, once the workflow is merged?

I don't think so: pull requests have read only permissions and you need id-token: write

or in more OIDC terms, pull requests aren't allowed to "authenticate" as the project (even though sometimes it would make sense, since the authentication is not for the whole project but for the specific workflow which could be dedicated for PRs...)

but feel free to test by merging: we can fix with another PR if it does not work

@jku: Looks like this actually works:
https://github.com/secure-systems-lab/securesystemslib/actions/runs/4382839569

Build was triggered via pull request and apparently had the required permissions.

Or maybe it only worked because the pr didn't come from a fork. 🤷

maybe it only worked because the pr didn't come from a fork. shrug

yes, correct: I expect branches in upstream repo to work, but branches in forks to fail

As per Jussi's CR: - Add comments to explain env vars - Add comments to mark API as unstable - Refactor raise-except-raise pattern Signed-off-by: Lukas Puehringer <[email protected]>

jku reviewed Feb 28, 2023

View reviewed changes

lukpueh commented Mar 1, 2023

View reviewed changes

securesystemslib/signer/_sigstore_signer.py Show resolved Hide resolved

lukpueh mentioned this pull request Mar 1, 2023

Ssh keys #451

Closed

3 tasks

lukpueh added 3 commits March 7, 2023 11:11

signer: refactor sigstore signer exceptions import

10ac123

Signed-off-by: Lukas Puehringer <[email protected]>

lukpueh force-pushed the sigstore-signer branch from ac098de to ba73dc9 Compare March 7, 2023 11:10

lukpueh added 4 commits March 7, 2023 13:30

mypy: ignore missing imports for sigstore

7aa2123

Signed-off-by: Lukas Puehringer <[email protected]>

signer: bump sigstore dependency

2b19bae

Signed-off-by: Lukas Puehringer <[email protected]>

lukpueh force-pushed the sigstore-signer branch from 041fb34 to e147564 Compare March 7, 2023 13:59

lint: disable sigstore verify import warning

b7b4191

Appease linter w/o installing optional requirement in linter run. Signed-off-by: Lukas Puehringer <[email protected]>

lukpueh force-pushed the sigstore-signer branch from e147564 to a05d8fa Compare March 7, 2023 14:27

ci: adopt recent branch rename

717f73a

Signed-off-by: Lukas Puehringer <[email protected]>

lukpueh force-pushed the sigstore-signer branch from a05d8fa to 717f73a Compare March 7, 2023 14:30

lukpueh marked this pull request as ready for review March 7, 2023 14:32

lukpueh requested a review from jku March 7, 2023 14:41

jku approved these changes Mar 9, 2023

View reviewed changes

jku reviewed Mar 9, 2023

View reviewed changes

signer: update sigstore docs + minor refactor

345d428

As per Jussi's CR: - Add comments to explain env vars - Add comments to mark API as unstable - Refactor raise-except-raise pattern Signed-off-by: Lukas Puehringer <[email protected]>

lukpueh merged commit e17d946 into secure-systems-lab:main Mar 10, 2023

lukpueh mentioned this pull request Mar 16, 2023

support short lived signing certificates (proof-of-concept) #399

Closed

MVrachev mentioned this pull request Aug 30, 2023

Release v0.29.0 #631

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

signer: add basic sigstore signer and verifier #522

signer: add basic sigstore signer and verifier #522

lukpueh commented Feb 28, 2023 •

edited

Loading

lukpueh commented Feb 28, 2023

jku Feb 28, 2023

lukpueh commented Mar 7, 2023

jku left a comment

jku Mar 9, 2023

lukpueh Mar 9, 2023

jku Mar 9, 2023

jku Mar 9, 2023

jku Mar 9, 2023

jku commented Mar 9, 2023

jku Mar 9, 2023

jku Mar 9, 2023

lukpueh Mar 9, 2023

jku Mar 9, 2023 •

edited

Loading

jku Mar 9, 2023

lukpueh Mar 10, 2023

jku Mar 10, 2023

signer: add basic sigstore signer and verifier #522

signer: add basic sigstore signer and verifier #522

Conversation

lukpueh commented Feb 28, 2023 • edited Loading

lukpueh commented Feb 28, 2023

Choose a reason for hiding this comment

lukpueh commented Mar 7, 2023

jku left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jku commented Mar 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jku Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukpueh commented Feb 28, 2023 •

edited

Loading

jku Mar 9, 2023 •

edited

Loading