Make artifacts ingester work with both GitHub and OCI providers #3309

JAORMX · 2024-05-12T14:52:35Z

Summary

This modifies the ingester to change its behavior depending on whether
the provider is an OCI provider or a github one.

Closes: #3322

Change Type

Mark the type of change your PR introduces:

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

Review Checklist:

Reviewed my own code for quality and clarity.
Added comments to complex or tricky code sections.
Updated any affected documentation.
Included tests that validate the fix or feature.
Checked that related changes are merged.

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Mixed Scripts Detected.

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Mixed Scripts Detected.

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Mixed Scripts Detected.

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Mixed Scripts Detected.

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Mixed Scripts Detected.

rdimitrov · 2024-05-13T07:43:49Z

I know you thought of it but just to remind that the artifact verifier also interacts with the registry so both of these changes should be merged as a pair 👍

jhrozek · 2024-05-13T08:50:13Z

internal/engine/ingester/artifact/versioner.go

+
+// in case of the GitHub provider, a package version may be
+// linked to multiple tags
+func (gv *githubVersioner) GetVersions(ctx context.Context) ([]*minderv1.ArtifactVersion, error) {


I wonder if this should be part of the provider interface?

I thought about that, but it seemed quite involved and I'm not even sure if we're going to continue using this artifact-builtin going forward. I'm leaning more towards minimal, individual checks instead. Gotta give it some thought.

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Mixed Scripts Detected.

coveralls · 2024-05-13T14:33:05Z

coverage: 49.135% (-0.2%) from 49.285%
when pulling f9251c1 on oci-attestations
into 1851f4c on main.

stacklokbot · 2024-05-14T06:46:21Z

Minder analyzed this PR with Trusty and found no dependencies scored lower than your profile threshold.

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Mixed Scripts Detected.

stacklokbot · 2024-05-14T06:49:29Z

Minder analyzed this PR with Trusty and found no dependencies scored lower than your profile threshold.

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Mixed Scripts Detected.

JAORMX · 2024-05-14T07:46:31Z

Note that this still doesn't fully work, but it sure gets us forward. I have to apply the following diff for it to actually work:

diff --git a/internal/verifier/sigstore/container/container.go b/internal/verifier/sigstore/container/container.go
index 201d1d10e..63d14b826 100644
--- a/internal/verifier/sigstore/container/container.go
+++ b/internal/verifier/sigstore/container/container.go
@@ -612,7 +612,7 @@ func getBundleMsgSignature(simpleSigningLayer v1.Descriptor) (*protobundle.Bundl
 
 // BuildImageRef returns the OCI image reference
 func BuildImageRef(registry, owner, artifact, version string) string {
-       return fmt.Sprintf("%s/%s/%s@%s", registry, owner, artifact, version)
+       return fmt.Sprintf("%s/%s/%s:%s", registry, owner, artifact, version)
 }
 
 type sigstoreBundle struct {

which indicates something off about the BuildImageRef function (at least I don't understand how it currently works).

It puts a @ in the reference, which is usually (according to the spec) needed for checksums, while we pass a tag. If we build the reference with a : instead, it works as expected as it's able to parse the tagged reference. We need to investigate why this works with github currently and what we could do about this.

anyway, for the sake of keeping iterations small, I suggest we merge as is if it works with GH as expected.

dmjb · 2024-05-14T11:14:52Z

internal/engine/ingester/artifact/versioner.go

+
+// in case of the GitHub provider, a package version may be
+// linked to multiple tags
+func (gv *githubVersioner) GetVersions(ctx context.Context) (map[string]*minderv1.ArtifactVersion, error) {


Can we put these methods behind the provider interface instead of building wrappers around the providers here?

(Specifically, I am thinking of a Versioner trait)

Mind if I do that in a separate PR? There's still a bunch of fixes I'm doing.

This modifies the ingester to change its behavior depending on whether the provider is an OCI provider or a github one. Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>

Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>

It's used differently than I thought Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>

Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>

stacklokbot · 2024-05-14T11:45:27Z

Minder analyzed this PR with Trusty and found no dependencies scored lower than your profile threshold.

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Mixed Scripts Detected.

The logic to refer to containers is now changed to using the `Sha` field from the ArtifactVersions since this is what the bebavior was. With this, we can now evaluate policies with the dockerhub provider!! The usage of the word version around the verifier was misleading, so this was fixed so the code and expectations are easier to follow. Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>

stacklokbot · 2024-05-14T12:07:43Z

Minder analyzed this PR with Trusty and found no dependencies scored lower than your profile threshold.

stacklokbot