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

[generic] fix attestation file creation when subject names are in subdirectories #1226

Merged
merged 3 commits into from
Nov 14, 2022

Conversation

asraa
Copy link
Collaborator

@asraa asraa commented Nov 9, 2022

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

Updates #1225

The generic SLSA 3 generator attests to subjects given as input and their digest. However, if the subject is in a sub-directory, then it will fail creating the attestation because the sub-directory doesn't exist.

This retrieves the Base for the subject name, instead of the full path name.
@ianlewis I could also have created the sub-directory, but that made a lot less sense to me.

Signed-off-by: Asra Ali <asraa@google.com>
@asraa asraa changed the title fix subdirectory attestation [generic] fix attestation file creation when subject names are in subdirectories Nov 9, 2022
Copy link
Member

@ianlewis ianlewis left a comment

Choose a reason for hiding this comment

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

Does the Go builder handle this ok? I guess it controls the subject's output location when it's compiled.

internal/builders/generic/attest.go Show resolved Hide resolved
Signed-off-by: Asra Ali <asraa@google.com>
@asraa
Copy link
Collaborator Author

asraa commented Nov 9, 2022

Does the Go builder handle this ok? I guess it controls the subject's output location when it's compiled.

Good call, checking on this. It doesn't currently enforce that even when setting the output from the config during dry run (it checks if it's under the current working directory, but even a sub-directory would count). Double-checking more.

@ianlewis
Copy link
Member

ianlewis commented Nov 9, 2022

Does the Go builder handle this ok? I guess it controls the subject's output location when it's compiled.

Good call, checking on this. It doesn't currently enforce that even when setting the output from the config during dry run (it checks if it's under the current working directory, but even a sub-directory would count). Double-checking more.

If it's an issue I can follow up later in my day today.

@asraa
Copy link
Collaborator Author

asraa commented Nov 9, 2022

OK, I don't think so. The config could end up setting

binary: ./subdirectory/binary-{{ .Os }}-{{ .Arch }}

and this would be allowed, and would fail on CreateNewFileUnderCurrentDirectory

@ianlewis
Copy link
Member

OK, I don't think so. The config could end up setting

binary: ./subdirectory/binary-{{ .Os }}-{{ .Arch }}

and this would be allowed, and would fail on CreateNewFileUnderCurrentDirectory

Yeah, lets go a separate PR for the Go builder.

@@ -20,6 +20,7 @@ import (
"encoding/json"
"fmt"
"os"
"path"
Copy link
Collaborator

@laurentsimon laurentsimon Nov 10, 2022

Choose a reason for hiding this comment

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

why not set a default value for the provenance file in https://github.com/slsa-framework/slsa-github-generator/blob/main/.github/workflows/generator_generic_slsa3.yml#L37?

We could also amend the doc to say it needs to be a top-level file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we may run into problems with the secure-upload-artifact Actions. They currently assume files are in the root folder, I think

Copy link
Collaborator

Choose a reason for hiding this comment

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

edit: we use the upload-artifact, not the secure one so my last comment is wrong

Copy link
Member

Choose a reason for hiding this comment

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

why not set a default value for the provenance file in https://github.com/slsa-framework/slsa-github-generator/blob/main/.github/workflows/generator_generic_slsa3.yml#L37?

I'd argue against this, if just for the fact that users would have to specify the input when the didn't before so it adds friction to upgrading and folks have complained previously about having to add new inputs.

@laurentsimon
Copy link
Collaborator

OK, I don't think so. The config could end up setting

binary: ./subdirectory/binary-{{ .Os }}-{{ .Arch }}

and this would be allowed, and would fail on CreateNewFileUnderCurrentDirectory

Yeah, lets go a separate PR for the Go builder.

Let's amend the doc to say the files don't support folders. I have not seen anyone need this feature when using Goreleaser.

@ianlewis
Copy link
Member

Let's amend the doc to say the files don't support folders. I have not seen anyone need this feature when using Goreleaser.

I'd say it's reasonable to support it if goreleaser supports it, but we can prioritize it when someone complains.

@asraa asraa enabled auto-merge (squash) November 14, 2022 15:25
@asraa asraa merged commit 5e57226 into slsa-framework:main Nov 14, 2022
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.

None yet

3 participants