Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add sigstore attest CLI subcommand to sign using DSSE envelopes #1115

Merged
merged 7 commits into from
Sep 13, 2024

Conversation

facutuesca
Copy link
Contributor

@facutuesca facutuesca commented Sep 10, 2024

Summary

Part of #1111.
Adds a sigstore attest CLI subcommand to sign using DSSE. The command is very similar to sigstore sign, but it takes two new options:

  • predicate-type: One of [ https://slsa.dev/provenance/v0.2, https://slsa.dev/provenance/v1 ]. The predicate type for the in-toto statement being signed over.
  • predicate: The path to the file containing the predicate JSON. The predicate's contents must match the predicate type specified in predicate-type.
sigstore attest --predicate-type https://slsa.dev/provenance/v0.2 --predicate ./path/to/predicate.json  FILE

Since the logic for both sigstore sign and sigstore attest is mostly the same, this PR extracts that shared logic into a separate function _sign_common(), which is then called by _sign() and _attest() in order to not duplicate that logic.

In order to test it, I have uploaded a SLSA v0.2 provenance predicate here, originally generated by GitHub for this sigstore-python release.
Once downloaded, it can be used to sign over a file with the following command:

sigstore attest --predicate-type https://slsa.dev/provenance/v0.2 --predicate ./path/to/predicate.json  FILE

Release Note

  • CLI: The sigstore attest subcommand has been added. This command is
    similar to cosign attest in that it signs over an artifact and a
    predicate using a DSSE envelope. This commands requires the user to pass
    a path to the file containing the predicate, and the predicate type.
    Currently only the SLSA Provenance v0.2 and v1.0 types are supported.

Documentation

Updated the README

cc @woodruffw

@woodruffw
Copy link
Member

/gcbrun

sigstore/_cli.py Outdated
dsse_options.add_argument(
"--predicate-type",
metavar="TYPE",
choices=PREDICATE_TYPES_CLI_MAP,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity: do other clients use these shorthand names? I'm wondering if we should just use the predicate URLs directly as choices here -- that makes the CLI slightly less ergonomic, but avoids adding one more custom convention 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I got these from cosign:

    --type='custom':
        specify a predicate type
        (slsaprovenance|slsaprovenance02|slsaprovenance1|link|spdx|spdxjson|cyclonedx|vuln|openvex|custom)
        or an URI

Copy link
Contributor Author

@facutuesca facutuesca Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah my bad, I forgot I had changed them. I changed them to slsaprovenance0_2 and slsaprovenance1_0 because slsaprovenance1 seems ambiguous. But yeah, these are not standard in any way. We can switch to the URLs

Copy link
Member

@woodruffw woodruffw Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Makes perfect sense to use them, then.

Edit: Or maybe not, given that the current names are somewhat ambiguous. Let's go with URLs only for now, and we can always move to support shorthand names once clients unify on them.

CC @loosebazooka @haydentherapper for visibility -- this is maybe too much of an implementation detail to standardize, but something for other clients to be aware of 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me to specify the predicate type as its full URL and not deal with shorthand.

sigstore/_cli.py Outdated
Comment on lines 283 to 312
output_options = attest.add_argument_group("Output options")
output_options.add_argument(
"--no-default-files",
action="store_true",
default=_boolify_env("SIGSTORE_NO_DEFAULT_FILES"),
help="Don't emit the default output files ({input}.sigstore.json)",
)
output_options.add_argument(
"--signature",
"--output-signature",
metavar="FILE",
type=Path,
default=os.getenv("SIGSTORE_OUTPUT_SIGNATURE"),
help=(
"Write a single signature to the given file; does not work with multiple input files"
),
)
output_options.add_argument(
"--certificate",
"--output-certificate",
metavar="FILE",
type=Path,
default=os.getenv("SIGSTORE_OUTPUT_CERTIFICATE"),
help=(
"Write a single certificate to the given file; does not work with multiple input files"
),
)
output_options.add_argument(
"--bundle",
metavar="FILE",
type=Path,
default=os.getenv("SIGSTORE_BUNDLE"),
help=(
"Write a single Sigstore bundle to the given file; does not work with multiple input "
"files"
),
)
output_options.add_argument(
"--output-directory",
metavar="DIR",
type=Path,
default=os.getenv("SIGSTORE_OUTPUT_DIRECTORY"),
help=(
"Write default outputs to the given directory (conflicts with --signature, --certificate"
", --bundle)"
),
)
output_options.add_argument(
"--overwrite",
action="store_true",
default=_boolify_env("SIGSTORE_OVERWRITE"),
help="Overwrite preexisting signature and certificate outputs, if present",
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this is a brand new command, I'm kind of tempted to remove/avoid the need to do anything except emit bundles 🙂 -- I think the only options we need here are --bundle and --overwrite.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The advantage of having these is that it allows us to reuse most of the code that was already written for sigstore sign. But I can remove them and refactor things around a bit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this still needs to be done -- IMO it's okay if end up being able to create a reusable _sign_common, since these extra flags are about the same size as the helper anyays (and we don't want to support them).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed! See last commit


uri: Optional[StrictStr]
digest: Optional[DigestSetSource]
entry_point: Optional[StrictStr] = Field(None, alias="entryPoint")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Rather than having to explicitly declare each alias here and below, maybe we can define a common base model with a model_config that tweaks alias generation?

Something like:

from pydantic.config import ConfigDict
from pydantic.alias_generators import to_camel

class _PredicateBase(BaseModel):
    model_config = ConfigDict(alias_generator=to_camel)

class ConfigSource(_PredicateBase):
    ...

...I think something like that will work, as will save us some manual typing 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! I also fixed a couple of bugs I found:

  • slsa-framework/slsa-github-generator incorrectly generates predicates using buildInvocationID (note the final uppercase D, should be buildInvocationId instead). In order to fix that, I added a validation alias that allows for both options. The serialization alias will use the camelCase one set by the config you suggested.
  • I was dumping the model without specifying it should output the camelCase aliases rather than our snake_case field names. Fixed by passing by_alias=True to model_dump()
  • Our model dump also included the optional fields that were set to None. However, that's how we model them, those fields are just absent in the source predicate. So now we pass exclude_none=True to model_dump, to exclude those fields from the dump.

Copy link
Contributor Author

@facutuesca facutuesca Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm given the last point, I wonder if instead of dumping our Predicate model, we should instead just validate it and then use the exact input the user gave us to build the in-toto statement.
Otherwise we might end up modifying it when doing the JSON -> Model -> JSON roundtrip (for example, the original JSON uses ID but the final JSON uses Id).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@woodruffw WDYT about this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@facutuesca I think that's a good idea! In principle the each JSON layout shouldn't need to be stable, but in practice users might expect that (and as you've pointed out it avoids a potential source of ser/de errors on our side).

@facutuesca facutuesca force-pushed the cli-attest-subcommand branch 2 times, most recently from 5684646 to 48051b9 Compare September 10, 2024 20:59
@woodruffw
Copy link
Member

/gcbrun

@woodruffw
Copy link
Member

/gcbrun

@woodruffw
Copy link
Member

/gcbrun

@@ -179,6 +180,58 @@ Output options:
```
<!-- @end-sigstore-sign-help@ -->


### Signing with DSSE envelopes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking out loud: we should probably add some documentation about when users might want to do this versus "normal" signing. But we can do that in a follow-up 🙂

Comment on lines +33 to +36
PREDICATE_TYPE_SLSA_v0_2 = "https://slsa.dev/provenance/v0.2"
PREDICATE_TYPE_SLSA_v1_0 = "https://slsa.dev/provenance/v1"

SUPPORTED_PREDICATE_TYPES = [PREDICATE_TYPE_SLSA_v0_2, PREDICATE_TYPE_SLSA_v1_0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flagging for a follow-up: maybe we can make this an enum.Enum instead? That will reduce the number of unique imports we need to do, and allow us to define "supported" as "present in the enum".

Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @facutuesca, LGTM! I've flagged two things for follow-ups 🙂

@woodruffw
Copy link
Member

/gcbrun

@woodruffw woodruffw enabled auto-merge (squash) September 13, 2024 15:19
@woodruffw woodruffw merged commit b704f82 into sigstore:main Sep 13, 2024
25 checks passed
@woodruffw woodruffw deleted the cli-attest-subcommand branch September 13, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants