fix: address gcb verifier comments and add gcb documentation #300

asraa · 2022-10-12T17:34:16Z

Signed-off-by: Asra Ali asraa@google.com

Addressing comments from https://github.com/slsa-framework/slsa-verifier/pull/292/files/dcb49d5bc9d6bc0119b3a41d64c529eda79a112c

Note: I reverted the behavior and clarified expectations in the README.md:

If the GCB provenance used a GCS bucket, we log a warning to Stderr to the user that the build was not configured with a GH trigger and therefore they cannot expect a version controlled source URI.
We still match on the previous behavior of the gs://[PROEJCT_ID]_cloudbuild/source/[BUCKET].tar.gz to preserve behavior.

Signed-off-by: Asra Ali <asraa@google.com>

verifiers/internal/gcb/provenance.go

asraa · 2022-10-12T17:35:07Z

verifiers/internal/gcb/provenance.go

+		// In v0.2, it uses the formats
+		// `https://github.com/laurentsimon/gcb-tests/commit/01ce393d04eb6df2a7b2b3e95d4126e687afb7ae`
+		// `gs://slsa-tooling_cloudbuild/source/1663616632.078353-fc7db143dcc64b5f9fe71d0497125ca1.tgz#1663616635134845`
+		if uri.Fragment == "" && !strings.HasPrefix(uri.Path, expectedURI.Path+"/commit/") {


it may be better to switch on scheme here.

Yeah, I think that's a more clear way to differentiate the URL types.

Though I wonder about the gs:// URLs. Do those show up when you submit a build via gcloud builds submit? or something else? Do we really want to verify builds that are built that way?

I guess users would have to pass a --source gs://slsa-tooling_cloudbuild/source argument to slsa-verifier? I think the source check mostly looses all meaning in this case. No?

TBC, I'm not sure we need to support source checks for builds submitted as gcloud builds submit. I also wonder how v0.3 handles builds that are submitted this way.

ianlewis · 2022-10-13T01:56:34Z

verifiers/internal/gcb/provenance.go

+		// In v0.2, it uses the formats
+		// `https://github.com/laurentsimon/gcb-tests/commit/01ce393d04eb6df2a7b2b3e95d4126e687afb7ae`
+		// `gs://slsa-tooling_cloudbuild/source/1663616632.078353-fc7db143dcc64b5f9fe71d0497125ca1.tgz#1663616635134845`
+		if uri.Fragment == "" && !strings.HasPrefix(uri.Path, expectedURI.Path+"/commit/") {


Yeah, I think that's a more clear way to differentiate the URL types.

Though I wonder about the gs:// URLs. Do those show up when you submit a build via gcloud builds submit? or something else? Do we really want to verify builds that are built that way?

I guess users would have to pass a --source gs://slsa-tooling_cloudbuild/source argument to slsa-verifier? I think the source check mostly looses all meaning in this case. No?

TBC, I'm not sure we need to support source checks for builds submitted as gcloud builds submit. I also wonder how v0.3 handles builds that are submitted this way.

verifiers/internal/gcb/provenance.go

asraa · 2022-10-13T14:11:07Z

TBC, I'm not sure we need to support source checks for builds submitted as gcloud builds submit. I also wonder how v0.3 handles builds that are submitted this way.

The v0.3 builds use the same gs://PROJECT_cloudbuild/source style bucket, with a separate digest field with the sha256.

v0.3 doesn't mean SLSA 3 unfortunately, also.

SOME gcloud builds submit, if configured with a github trigger, will appear with github.com/blah/blah, but otherwise, it pushes the source to that gs project bucket, and there's no identification of the original source. In that case, the only source pinning you can get is via the PROJECT_ID, essentially.

asraa · 2022-10-17T18:25:12Z

Tabling this for some feedback with Laurent and GCB team on dropping the source bucket requirement entirely.

ianlewis · 2022-10-18T02:57:57Z

TBC, I'm not sure we need to support source checks for builds submitted as gcloud builds submit. I also wonder how v0.3 handles builds that are submitted this way.

The v0.3 builds use the same gs://PROJECT_cloudbuild/source style bucket, with a separate digest field with the sha256.

v0.3 doesn't mean SLSA 3 unfortunately, also.

Yeah. The reason I asked was because you don't have any special logic for gs:// URLs in the v0.3 part of the switch statement. You only have that logic under v0.2

Signed-off-by: Asra Ali <asraa@google.com>

asraa · 2022-10-21T15:57:04Z

I updated this PR to mostly add documentation but preserve the original behavior (see updated PR description). I decided not to mess with allowing users to specify a short source bucket, and keep the original behavior.

laurentsimon · 2022-10-21T20:50:01Z

We still match on the previous behavior of the gs://[PROEJCT_ID]_cloudbuild/source/[BUCKET].tar.gz to preserve behavior.

will this include the trailing #<id>?

asraa · 2022-10-21T20:51:30Z

will this include the trailing #?

The current behavior when I made it did not: we match on everything up to .tar.gz, so I kept as is (I had thought it was "parity" with the /commit matching). Since we're v2-releasing this I can change that too

laurentsimon

LGTM. I left a question i a comment about the trailing #<id> in the matching. I recall you said there was one sometimes. Will it be required for the match? I think yes, but wanted confirmaiton.

laurentsimon · 2022-10-21T20:50:49Z

README.md

@@ -116,6 +116,8 @@ PASSED: Verified SLSA provenance

 The verified in-toto statement may be written to stdout with the `--print-provenance` flag to pipe into policy engines.

+Only GitHub URIs are supported with the `--source-uri` flag. A tag should not be specified, even if the provenance was built at some tag. If you intend to do source versioning validation, use `--print-provenance` and inspect the commit SHA of the config source or materials.


shall we create an issue for --source-sha1? Probably nor urgent...

laurentsimon · 2022-10-21T20:59:17Z

will this include the trailing #?

The current behavior when I made it did not: we match on everything up to .tar.gz, so I kept as is (I had thought it was "parity" with the /commit matching). Since we're v2-releasing this I can change that too

I see. The commit is omitted for the source-uri matching because it's not supposed to be there - it should be in a dedicated field sha according to SLSA specs. I don't know what the <id> means for bucket source. Does it make a difference? You may be right that it does not. Having sone clarification from GCB team may help validate we're not omitting an important parameter.

asraa · 2022-10-21T21:06:07Z

Does it make a difference? You may be right that it does not. Having sone clarification from GCB team may help validate we're not omitting an important parameter.

To some degree it may not matter at all until they expose source provenance FOR the bucket. That bucket just contains anything in the context of the gcloud builds submit. I know it is uploaded to the same bucket each time, with new versions which change the #blah fragment. I'll add some details to #309

* updates * updates * updates

address gcb verifier comments

9a4decd

Signed-off-by: Asra Ali <asraa@google.com>

asraa commented Oct 12, 2022

View reviewed changes

verifiers/internal/gcb/provenance.go Outdated Show resolved Hide resolved

asraa commented Oct 12, 2022

View reviewed changes

ianlewis reviewed Oct 13, 2022

View reviewed changes

asraa mentioned this pull request Oct 17, 2022

GCB: Does source pinning matter? #309

Open

asraa added 2 commits October 20, 2022 09:58

address comments and document the behavior

c050935

Signed-off-by: Asra Ali <asraa@google.com>

Merge branch 'main' into gcb-posterior-comments

be1c916

asraa requested a review from laurentsimon October 21, 2022 19:24

asraa changed the title ~~fix: address gcb verifier comments~~ fix: address gcb verifier comments and add gcb documentation Oct 21, 2022

laurentsimon approved these changes Oct 21, 2022

View reviewed changes

Merge branch 'main' into gcb-posterior-comments

e1d197a

asraa enabled auto-merge (squash) October 21, 2022 20:58

asraa mentioned this pull request Oct 21, 2022

Exposing options to verify against a commit SHA #315

Open

asraa merged commit e9cd6b7 into slsa-framework:main Oct 21, 2022

ramonpetgrave64 pushed a commit to ramonpetgrave64/slsa-verifier that referenced this pull request Apr 18, 2024

✨ Do not fetch all branches (slsa-framework#300)

aeb433e

* updates * updates * updates

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: address gcb verifier comments and add gcb documentation #300

fix: address gcb verifier comments and add gcb documentation #300

asraa commented Oct 12, 2022 •

edited

Loading

asraa Oct 12, 2022

ianlewis Oct 13, 2022

ianlewis Oct 13, 2022

asraa commented Oct 13, 2022

asraa commented Oct 17, 2022

ianlewis commented Oct 18, 2022

asraa commented Oct 21, 2022

laurentsimon commented Oct 21, 2022

asraa commented Oct 21, 2022 •

edited

Loading

laurentsimon left a comment

laurentsimon Oct 21, 2022

laurentsimon commented Oct 21, 2022

asraa commented Oct 21, 2022

		@@ -116,6 +116,8 @@ PASSED: Verified SLSA provenance

		The verified in-toto statement may be written to stdout with the `--print-provenance` flag to pipe into policy engines.

		Only GitHub URIs are supported with the `--source-uri` flag. A tag should not be specified, even if the provenance was built at some tag. If you intend to do source versioning validation, use `--print-provenance` and inspect the commit SHA of the config source or materials.

fix: address gcb verifier comments and add gcb documentation #300

fix: address gcb verifier comments and add gcb documentation #300

Conversation

asraa commented Oct 12, 2022 • edited Loading

asraa Oct 12, 2022

Choose a reason for hiding this comment

ianlewis Oct 13, 2022

Choose a reason for hiding this comment

ianlewis Oct 13, 2022

Choose a reason for hiding this comment

asraa commented Oct 13, 2022

asraa commented Oct 17, 2022

ianlewis commented Oct 18, 2022

asraa commented Oct 21, 2022

laurentsimon commented Oct 21, 2022

asraa commented Oct 21, 2022 • edited Loading

laurentsimon left a comment

Choose a reason for hiding this comment

laurentsimon Oct 21, 2022

Choose a reason for hiding this comment

laurentsimon commented Oct 21, 2022

asraa commented Oct 21, 2022

asraa commented Oct 12, 2022 •

edited

Loading

asraa commented Oct 21, 2022 •

edited

Loading