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

[feature] Update the provenance format in the docker-based generator #1566

Closed
rbehjati opened this issue Jan 24, 2023 · 3 comments · Fixed by #1615
Closed

[feature] Update the provenance format in the docker-based generator #1566

rbehjati opened this issue Jan 24, 2023 · 3 comments · Fixed by #1615
Labels
area:docker-based Docker based builder (supplying a builder image and command) type:feature New feature or request

Comments

@rbehjati
Copy link
Contributor

Describe the solution you'd like
The current provenance format does not quite comply with SLSA provenance v1.0. The following changes are needed.

  1. Move config parameters to the input mapValue (see fix: Unpack config file in BuildDefinition #1547 (comment))
  2. The command is encoded as a JSON string (see fix: Unpack config file in BuildDefinition #1547 (comment))
  3. Restructure external parameters according to the new format as in this example.

Additional context
See also

@rbehjati rbehjati added status:triage Issue that has not been triaged type:feature New feature or request labels Jan 24, 2023
@asraa
Copy link
Collaborator

asraa commented Jan 24, 2023

Thanks for this issue! I was just thinking about this. OK if I make this update?

I'm also thinking when in-toto is ready to make a PR we can probably move the definitions here upstream.

Only still confused about the oneof representation.

For generated proto, it uses interfaces -- so something like that can work, but I'd like to not introduce proto runtimes into the deps

@rbehjati
Copy link
Contributor Author

Thanks for this issue! I was just thinking about this. OK if I make this update?

Yes. Sure. Thanks :)

I'm also thinking when in-toto is ready to make a PR we can probably move the definitions here upstream.

Only still confused about the oneof representation.

For generated proto, it uses interfaces -- so something like that can work, but I'd like to not introduce proto runtimes into the deps

I don't have any experience with using protobufs in Go myself, but had a chat with @tiziano88, and his suggestion was that if interoperability with other languages and services is needed, then code generation from protobuf is preferable.

But I agree with you about runtime dependencies. Is it possible to start with a hand-written definition and replace it with auto-generated code in the future if there is need for it?

@rbehjati
Copy link
Contributor Author

rbehjati commented Jan 25, 2023

Continuing our discussion about input here :)

Looking at slsa-framework/slsa#582, one thing is unclear to me. Do we have to use "https://slsa.dev/github-actions-workflow/v0.1?draft", as the buildType, and use the external_parameters as described in the updated README in slsa-framework/slsa#582?

I assume not, since the proposed structure is missing important fields. With that assumption, the new BuildDefinition in slsa-framework/slsa#582 allows us to define our own struct for external_parameters and system_parameters. I suggest the following for external_parameters:

// DockerBasedBuilderParameters is the struct that we use as the type of system_parameters in the BuildDefinition.
type DockerBasedBuilderParameters struct {
	// The source GitHub repo
	Source ArtifactReference `json:"source"`

        // The Docker builder image
        BuilderImage ArtifactReference `json:"builderImage"`
 
        // The inputs to the workflow as described in https://docs.github.com/en/actions/learn-github-actions/contexts#inputs-context
        Inputs WorkFlowInputs  `json:"inputs,omitempty"`

	// Unpacked build config parameters
	Config BuildConfig `json:"buildConfig"`
}

type WorkFlowInputs struct {
	// The URI of the Docker builder image.
	BuilderImage string `json:"builderImage"`

        // The digest of the Docker builder image, of the form '<alg>:<digest>'.
        BuilderDigest string `json:"builderDigest"`
 
        // Path to a configuration file relative to the root of the repository.
        ConfigPath string `json:"configPath"`

	// Whether to build the builder from source.
	CompileBuilder bool `json:"compileBuilder"`

	// Whether the source repository is private.
	PrivateRepository bool `json:"privateRepository"`
}

// ArtifactReference is the same struct defined in https://github.com/slsa-framework/slsa/pull/582

// BuildConfig is defined in https://github.com/slsa-framework/slsa-github-generator/blob/77395259ff7f705e2578f4552b79d90c06f49554/internal/builders/docker/pkg/config.go#L30

From DockerBasedBuilderParameters we should be able to recreate the arguments to the command line tool, for dry-run and build.

The WorkFlowInputs captures the inputs that we currently have in the workflow.

  • Currently the workflow also contains builder-output-path which is unused, and I think it is actually redundant. The same path is included in the config file specified by config-path. I think builder-output-path can be removed from the workflow inputs.
  • CompileBuilder and PrivateRepository are not particularly relevant for re-running the build but I think for completeness we should keep them.
  • The builder tool, and in particular the dry-run command is not aware of the full list of WorkFlowInputs. In particular, it does not have CompileBuilder and PrivateRepository. The actual values for these fields must be added by the workflow.

@asraa This is my suggestion, and I hope it is helpful :) Please feel free to suggest changes or corrections. I am particularly not attached to the naming :)

In particular, I think we'd want to add fields for workflow path and github context as well. We might want to include these in system parameters though. The definition of systemParameters says that they are "set internally by the platform. In SLSA, these values are trusted because the platform is trusted; they are OPTIONAL and need not be verified downstream. They MAY be included to enable reproducible builds, debugging, or incident response."

@ianlewis ianlewis added area:docker-based Docker based builder (supplying a builder image and command) and removed status:triage Issue that has not been triaged labels Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:docker-based Docker based builder (supplying a builder image and command) type:feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants