sigstore / fulcio

Sigstore OIDC PKI
Apache License 2.0
643 stars 136 forks source link

Signing from Codefresh fails when scm_repo_url claim is not provided #1770

Closed ilia-medvedev-codefresh closed 1 month ago

ilia-medvedev-codefresh commented 1 month ago

Description

After the migration of OIDC providers to the new configuration in https://github.com/sigstore/fulcio/pull/1743/files signing from Codefresh is failing when scm_repo_url is not provided.

This is the error received from Cosign - Error: signing [ilmedcodefresh/cosign-build-test@sha256:9a14f704ed630d8a73246acf9085e68c9971636fe88f459f408dbaa4669bcad8]: getting signer: getting key from Fulcio: retrieving cert: POST https://fulcio.sigstore.dev/api/v1/signingCert returned 400 Bad Request: "{\"code\":3,\"message\":\"value <scm_repo_url> not present in either claims or defaults. {\\n\\t\\\"ExtensionName\\\": \\\"SourceRepositoryURI\\\",\\n\\t\\\"Issuer\\\": \\\"https://oidc.codefresh.io\\\"\\n}\",\"details\":[]}"

Repository urls are not always present in the pipeline context in Codefresh - for example when a build is executed manually and not triggered from git.

When I added the Codefresh OIDC provider initially it was taken to consideration, but with the new configuration it seems like all claims are mandatory.

haydentherapper commented 1 month ago

@ilia-medvedev-codefresh Sorry about that! We had assumed that all claims would be present in the token, otherwise we configure a default value based on what was present in the original provider code. We didn't see any comments about any claims being optional.

To confirm, are there any other claims that are not always set?

@javanlacerda Can you take a look at this? I think it would be sufficient to add scm_repo_url: "" to default-template-values in the config, and then the extension is simply skipped over when rendering.

We should also document the new requirement that all claims be present, or else a default value needs to be set. Can you also take a look at the error message, I don't think the JSON printing works well for logging - maybe just output as a string?

ilia-medvedev-codefresh commented 1 month ago

All good, thank you for the quick response! If you take a look here: https://github.com/sigstore/fulcio/blob/77325fa27326bb1e6886811def8856f431f1998d/pkg/identity/codefresh/principal.go#L62 I stated that those claims are only present for git triggers Also below you can see what claims are mandatory: https://github.com/sigstore/fulcio/blob/77325fa27326bb1e6886811def8856f431f1998d/pkg/identity/codefresh/principal.go#L102-L113 all others are optional.

javanlacerda commented 1 month ago

@ilia-medvedev-codefresh Sorry about that! We had assumed that all claims would be present in the token, otherwise we configure a default value based on what was present in the original provider code. We didn't see any comments about any claims being optional.

To confirm, are there any other claims that are not always set?

@javanlacerda Can you take a look at this? I think it would be sufficient to add scm_repo_url: "" to default-template-values in the config, and then the extension is simply skipped over when rendering.

We should also document the new requirement that all claims be present, or else a default value needs to be set. Can you also take a look at the error message, I don't think the JSON printing works well for logging - maybe just output as a string?

Sure! on it

haydentherapper commented 1 month ago

@ilia-medvedev-codefresh This has been rolled out to staging, can you test against staging? https://docs.sigstore.dev/system_config/public_deployment/ has some details aobout our staging URLs.

ilia-medvedev-codefresh commented 1 month ago

@haydentherapper thanks! It works.

haydentherapper commented 1 month ago

@ilia-medvedev-codefresh i'm rolling out the change for prod now, give it about 5 minutes, then can you confirm it works?

ilia-medvedev-codefresh commented 1 month ago

Works! Thanks :)