Enable signing using a custom sigstore instance #9

prezha · 2024-06-14T02:38:07Z

This PR extends plugin schema to allow users to specify a custom/private, ie, non-Public-Good Sigstore Instance to use, including the TUF URLs (used to initialise cosign).

Sensible defaults use the Public-Good Sigstore Instance with their current URLs (known to cosign) and the buildkite-agent as the OIDC provider.

Documentation was amended to emphasise usage of image digest vs tag and to add more examples for both keyless and keyed signing using the Public-Good and a custom/private Sigstore instance.

Bonus:

fix BUILDKITE_PLUGIN_COSIGN variables names
remove the need to explicitly state the Public-Good Sigstore Instance default params (ie, the URLs) - those might change and cosign would know them
refactor code to group and then reuse the common logic, increase readability
remove the obsolete COSIGN_EXPERIMENTAL=1 (ref: https://blog.sigstore.dev/cosign-2-0-released/)
bump cosign default version to 2.2.4
bump plugin-tester to version 4.1.1
use specific plugin-linter version 2.1.0 instead of latest
fix tests/pre-checkout.bats to work with v2.1.0

This PR extends plugin schema to allow users to specify a custom, ie, non-Public-Good Sigstore Instance to use, including the TUF URLs (used to initialise cosign). Sensible defaults use the Public-Good Sigstore Instance with their current URLs (known to cosign) and the buildkite-agent as the OIDC provider. Documentation was amended to emphasise usage of image digest vs tag and to add more examples for both keyless and keyed signing using the Public-Good and a custom sigstore instance. Bonus: - fix BUILDKITE_PLUGIN_COSIGN variables names - remove the need to explicitly state the Public-Good Sigstore Instance default params (ie, the URLs) - those might change and cosign would know them - refactor code to group and then reuse the common logic, increase readability - bump cosign default version to 2.2.4 - bump plugin-tester to version 4.1.1 - use specific plugin-linter version 2.1.0 instead of latest - fix tests/pre-checkout.bats to work with v2.1.0

prezha · 2024-06-18T17:44:42Z

@JAORMX as you originally wrote the plugin, i would much appreciate if you can review as well

JAORMX · 2024-06-18T17:53:14Z

Will do!

tenyo

Just a question about the defaults, otherwise the script looks good to me

tenyo · 2024-06-18T19:09:01Z

hooks/post-command

+setup_keyless() {
+    echo "--- :key: Setup cosign keyless signing"
+
+    local rekor_url=${BUILDKITE_PLUGIN_COSIGN_KEYLESS_CONFIG_REKOR_URL}


Do you need to set a default URL here?

thanks @tenyo !

the default values for the public-good sigstore instance are baked into the cosign binary (that we download and use while plugin is run), and i think we should relay on those in cosign, rather than having our own hardcoded values, that might change at some point - eg, this comment:

// TODO: change this back to api.SigstorePublicServerURL after the v1 migration is complete.

so, for rekor, it's defined here then used as the flag's default value here

for eg, tuf, it's here, etc.

tenyo · 2024-06-18T19:13:09Z

hooks/post-command

+        sign_flags+=("--rekor-url" "${rekor_url}")
+    fi
+
+    local fulcio_url=${BUILDKITE_PLUGIN_COSIGN_KEYLESS_CONFIG_FULCIO_URL}


Same for this - maybe these are set automatically by cosign but figured I'd ask.

same as above - for fulcio, it's defined here

sfox-equinix · 2024-06-18T21:20:35Z

hooks/post-command

-    if [[ $status -ne 0 ]]; then
-        fail_with_message "cosign" "Failed to sign image"
+    # flags for the cosign initialize command
+    init_flags=()


Nitpick: Can this be made a local variable? (i.e., local init_flags=())

yeah, we could do that

sfox-equinix · 2024-06-18T21:21:20Z

hooks/post-command


-    rm out.sig || true
+        status=$?


Nitpick: Can status be made a local variable?

sfox-equinix · 2024-06-18T21:36:44Z

hooks/post-command

-\`\`\`
-EOF
+    if [ ${#init_flags[@]} -gt 0 ]; then
+        rm -rf ~/.sigstore


I am wondering if we need to delete the whole directory. It would be unfortunate if there are other important things in there. The documentation for cosign initialize seems to claim that it updates the tuf thing:

Any updated TUF repository will be written to $HOME/.sigstore/root/.
https://github.com/sigstore/cosign/blob/main/doc/cosign_initialize.md

It looks like it gets overwritten:

$ rm -rf ~/.sigstore/ $ cosign initialize --mirror https://foo.com --root https://foo.com/root.json (...) $ ls -l ~/.sigstore/root/ total 10 -rw------- 1 x x 56 Jun 18 21:31 remote.json drwx------ 2 x x 5 Jun 18 21:31 targets drwxr-xr-x 2 x x 8 Jun 18 21:31 tuf.db $ date Tue Jun 18 21:31:55 UTC 2024 $ date Tue Jun 18 21:32:02 UTC 2024 $ cosign initialize --mirror https://foo.com --root https://foo.com/root.json (...) $ ls -l ~/.sigstore/root/ total 10 -rw------- 1 x x 56 Jun 18 21:32 remote.json drwx------ 2 x x 5 Jun 18 21:31 targets drwxr-xr-x 2 x x 9 Jun 18 21:32 tuf.db $ ls -l ~/.sigstore/root/tuf.db/ total 16 -rw-r--r-- 1 x x 2955 Jun 18 21:32 000004.ldb -rw-r--r-- 1 x x 4928 Jun 18 21:32 000007.log -rw-r--r-- 1 x x 16 Jun 18 21:32 CURRENT -rw-r--r-- 1 x x 16 Jun 18 21:32 CURRENT.bak -rw-r--r-- 1 x x 0 Jun 18 21:31 LOCK -rw-r--r-- 1 x x 2264 Jun 18 21:32 LOG -rw-r--r-- 1 x x 87 Jun 18 21:32 MANIFEST-000008

it is not expected to find ~/.sigstore/ there - this is more to ensure that there isn't one

docs suggest removing the whole dir and so that's probably a good idea:
https://docs.sigstore.dev/system_config/public_deployment/#usage and https://docs.sigstore.dev/system_config/public_deployment/#revert-back-to-production

JAORMX · 2024-06-19T15:35:56Z

hooks/post-command

-\`\`\`
-EOF
+    if [ ${#init_flags[@]} -gt 0 ]; then
+        rm -rf ~/.sigstore


Does this mean this plugin can only run serially on a given host? Is there a variable one can set to control this directory? Would be ideal if it were unique per plugin run.

good point, thanks!

looks like that's not directly exposed/customisable by cosign initialize (via flags) but the sigstore tuf client reads the TUF_ROOT env var to figure out the location, so i'll try to use that:

// TufRootEnv is the name of the environment variable that locates an alternate local TUF root location.
TufRootEnv = "TUF_ROOT"

=> https://github.com/sigstore/sigstore/blob/b777e4be352ebf9394d534271f3dd888908e839a/pkg/tuf/client.go#L559

similarly, i made the out.sig filename "dynamic" (pseudo-random) for the same reasons (ie, to avoid potential race condition over the output file between multiple plugin runs on the same host)

waiting for the build (using this plugin) to complete and i'll share results back

yep, looks like that worked:

🔑 Init cosign Root status: { "local": "/var/lib/buildkite-agent/.sigstore-13508/root", "remote": "https://<REDACTED>", "metadata": { "root.json": { "version": 1, "len": 2178, "expiration": "01 Dec 24 19:08 UTC", "error": "" }, "snapshot.json": { "version": 1, "len": 618, "expiration": "01 Dec 24 19:08 UTC", "error": "" }, "targets.json": { "version": 1, "len": 1373, "expiration": "01 Dec 24 19:08 UTC", "error": "" }, "timestamp.json": { "version": 1, "len": 619, "expiration": "01 Dec 24 19:08 UTC", "error": "" } }, "targets": [ "ctfe.pub", "fulcio_v1.crt.pem", "rekor.pub" ] } Successfully initialised

perhaps we could pr to update cosign initialize cmd's docs and code so users are aware they can set tuf's root directory using the TUF_ROOT env var

fwiw, I used @prezha's TUF_ROOT environment variable finding in a separate project to run multiple instances of cosign simultaneously on the same computer. Prior to making that change, I was experiencing "resource busy" errors due to each cosign process modifying / writing files in ~/.sigstore. I think this emulates the same kind of situation we would see in a Buildkite CI runner.

In other words: @prezha's change appears to prevent cosign from stepping on other cosign processes.

thanks @sfox-equinix for checking and confirming!

@JAORMX is there anything additional you think should be addressed?

prezha · 2024-06-26T12:29:28Z

as per @stephen-fox's suggestion, i've amended the pr so that keyless now defaults to false, to make it a little bit "harder" to unintentionally leak sensitive data - reasoning is, people could eg, just specify an image that should be signed, but forget to add custom/private sigstore params, which would then use the public-good instance by default

i think this might be a reasonable "trade-off" - ie, we could add another config option (like use-public-instance or similar, set to false by default), that would require to be even more explicit, but not sure if that'd be justifiable atm, as i don't think there are that many private sigstore instances in the wild (yet): open to suggestions, if people think we should do it that ("proper") way now or improve reasonably later, when appropriate

UPDATE: i have practically reverted the last change described in this comment but kept the warnings i've added in the readme, and the reasons are:

@stephen-fox already made a separate pr for that, i simply missed it
i think we should re-consider it separately - ie, following the least surprise principle, we probably would not want to change the default behaviour (that was using keyless by default so far): while we will be able to check all the places we're using the plugin and accommodate as needed, there would be other users that migt relay on a behaviour that was set at the beginning
if there is a real need and we want to prevent users from unintentionally exposing sensitive data using the public-good sigstore instance, setting keyless to false by default might not be sufficient (ie, it's better but still not that obvious) and we could think about requiring an explicit action via (new) param (eg, use-public-instance or similar)

vipulagarwal

LGTM, thanks @prezha

vipulagarwal · 2024-07-02T11:34:59Z

UPDATE: i have practically reverted the last change described in this comment but kept the warnings i've added in the readme, and the reasons are:

1. @stephen-fox already made a separate [pr](https://github.com/equinixmetal-buildkite/cosign-buildkite-plugin/pull/10) for that, i simply missed it

2. i think we should re-consider it separately - ie, following the least surprise principle, we probably would not want to change the default behaviour (that was using `keyless` by default so far): while we will be able to check all the places we're using the plugin and accommodate as needed, there would be other users that migt relay on a behaviour that was set at the beginning

3. if there is a real need and we want to prevent users from unintentionally exposing sensitive data using the public-good sigstore instance, setting `keyless` to `false` by default might not be sufficient (ie, it's better but still not that obvious) and we could think about requiring an explicit action via (new) param (eg, use-public-instance or similar)

@prezha this makes sense as we could have downstream users which we are unaware of given the public nature of this project and it could be a big breaking change for them. Changing the defaults would not prevent user to make a mistake which degrades the original intention for the change. We can look into ways to deal with the mistake part internally with some tooling.

prezha · 2024-07-04T17:47:48Z

thanks for the review!
i think we're good to go, i'm going to merge this pr now

prezha force-pushed the custom-sigstore branch 9 times, most recently from 012129e to 685c014 Compare June 18, 2024 14:47

prezha force-pushed the custom-sigstore branch from 791b38f to f278f7b Compare June 18, 2024 17:25

prezha changed the title ~~Enable usage of a custom sigstore instance~~ Enable signing using a custom sigstore instance Jun 18, 2024

prezha self-assigned this Jun 18, 2024

prezha requested a review from a team June 18, 2024 17:43

tenyo reviewed Jun 18, 2024

View reviewed changes

sfox-equinix reviewed Jun 18, 2024

View reviewed changes

make status and init_flags local vars

8eae776

sfox-equinix approved these changes Jun 19, 2024

View reviewed changes

JAORMX reviewed Jun 19, 2024

View reviewed changes

enable safe concurrent plugin runs by using randomm locations

0eadbd6

prezha force-pushed the custom-sigstore branch from 73fe4ff to 0eadbd6 Compare June 20, 2024 16:56

sfox-equinix approved these changes Jun 25, 2024

View reviewed changes

default to keyed signing

d07e399

vipulagarwal previously approved these changes Jun 26, 2024

View reviewed changes

respect least surprise principle - keep default keyless and warnings

bb24bfa

prezha dismissed vipulagarwal’s stale review via bb24bfa July 1, 2024 13:11

vipulagarwal approved these changes Jul 2, 2024

View reviewed changes

vipulagarwal mentioned this pull request Jul 2, 2024

keyless: Disable by default. #10

Open

prezha merged commit 5fa8e35 into equinixmetal-buildkite:main Jul 4, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable signing using a custom sigstore instance #9

Enable signing using a custom sigstore instance #9

prezha commented Jun 14, 2024 •

edited

Loading

prezha commented Jun 18, 2024

JAORMX commented Jun 18, 2024

tenyo left a comment

tenyo Jun 18, 2024

prezha Jun 18, 2024 •

edited

Loading

tenyo Jun 18, 2024

prezha Jun 18, 2024 •

edited

Loading

sfox-equinix Jun 18, 2024

prezha Jun 19, 2024

sfox-equinix Jun 18, 2024

prezha Jun 19, 2024

sfox-equinix Jun 18, 2024

prezha Jun 19, 2024 •

edited

Loading

JAORMX Jun 19, 2024

prezha Jun 20, 2024 •

edited

Loading

prezha Jun 20, 2024

prezha Jun 20, 2024 •

edited

Loading

sfox-equinix Jun 25, 2024

prezha Jun 27, 2024

prezha commented Jun 26, 2024 •

edited

Loading

vipulagarwal left a comment

vipulagarwal commented Jul 2, 2024

prezha commented Jul 4, 2024

Enable signing using a custom sigstore instance #9

Enable signing using a custom sigstore instance #9

Conversation

prezha commented Jun 14, 2024 • edited Loading

prezha commented Jun 18, 2024

JAORMX commented Jun 18, 2024

tenyo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prezha Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prezha Jun 18, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prezha Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prezha Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prezha Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prezha commented Jun 26, 2024 • edited Loading

vipulagarwal left a comment

Choose a reason for hiding this comment

vipulagarwal commented Jul 2, 2024

prezha commented Jul 4, 2024

prezha commented Jun 14, 2024 •

edited

Loading

prezha Jun 18, 2024 •

edited

Loading

prezha Jun 18, 2024 •

edited

Loading

prezha Jun 19, 2024 •

edited

Loading

prezha Jun 20, 2024 •

edited

Loading

prezha Jun 20, 2024 •

edited

Loading

prezha commented Jun 26, 2024 •

edited

Loading