WIP: Implement OCI provider #2765

JAORMX · 2024-03-22T16:37:55Z

Summary

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

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

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.

coveralls · 2024-03-22T16:43:49Z

coverage: 47.046% (-0.5%) from 47.563%
when pulling f7a8851 on oci-prov
into c88dd18 on main.

jhrozek

I think this is looking as a good direction, I left a couple of comments just to understand how we're building providers and compose them, some were answered later in the patch series.

jhrozek · 2024-03-25T08:26:56Z

internal/providers/oci/oci.go

+	// join base name with contname
+	// TODO make this more robust
+	src := fmt.Sprintf("%s/%s", o.baseURL, contname)
+	repo, err := name.NewRepository(src)


Do you think we should default to using the StrictValidation option to avoid using default tags or the default registry?

jhrozek · 2024-03-25T08:29:06Z

internal/providers/oci/oci.go

+		}
+		for _, tag := range tags.Tags {
+			// Should we be ommiting digest tags?
+			if strings.HasPrefix(tag, "sha256-") {


If we want to use this provider in the sigstore provider we will not want to omit digest tags. I don't know which way makes the most sense as the default, it almost seems like something the caller should set.
There's also github.com/opencontainers/go-digest which specifies a constant for the canonical digest which currently defaults to sha-256.

Why does sigstore want the digest tags? (Just for my own understanding)

jhrozek · 2024-03-25T08:44:50Z

internal/providers/github/common.go

@@ -86,6 +88,7 @@ func NewGitHub(
 		client:   client,
 		cache:    cache,
 		delegate: delegate,
+		ghcrwrap: ghcr.FromGitHubClient(client, delegate.GetOwner()),


Instead of this wrap, wouldn't it be better to implement the GHCR provider as embedded interface in the github provider (maybe through the app/oauth2 delegate)? Or is the intent to allow using the GHCR provider independently of github?

that was the intent, that you could spawn a GHCR provider without needing the full blown github provider.

jhrozek · 2024-03-25T08:46:14Z

pkg/providers/v1/providers.go

@@ -114,6 +115,21 @@ type GitHub interface {
 	AddAuthToPushOptions(ctx context.Context, options *git.PushOptions) error
 }

+// GHCR is the interface for interacting with the GitHub Container Registry
+type GHCR interface {
+	Provider


Don't these also implement OCI?

jhrozek · 2024-03-25T08:47:17Z

internal/providers/providers.go

@@ -242,6 +245,51 @@ func (pb *ProviderBuilder) GetRepoLister() (provinfv1.RepoLister, error) {
 	return nil, fmt.Errorf("provider does not implement repo lister")
 }

+// GetGHCR returns a GHCR client for the provider.
+func (pb *ProviderBuilder) GetGHCR() (provinfv1.GHCR, error) {
+	if !pb.Implements(db.ProviderTypeGhcr) {


If we had the ghcr provider as part of the github interface we could do:

if pb.Implements(db.ProviderTypeGithub) { return pb.GetGithub }

right, that could be the case; but the intent was to be able to have an independent ghcr provider.

jhrozek · 2024-03-25T08:48:33Z

database/migrations/000036_oci_prov.up.sql

+-- limitations under the License.
+
+-- Add `ghcr` and `dockerhub` provider type enum
+ALTER TYPE provider_type ADD VALUE 'ghcr';


Can you remind me of the difference between type and class?

a type (or trait) is a claim of functionality. It denotes that you can do a certain subset of actions.

A class is like a template, it's a collection of types/traits alongside a collection of authentication flow. They allow us to aggregate these and build providers without having to always remember these magical combinations.

jhrozek · 2024-03-25T08:50:37Z

internal/providers/dockerhub/dockerhub.go

+
+	if resp.StatusCode != http.StatusOK {
+		if resp.StatusCode == http.StatusUnauthorized {
+			return nil, fmt.Errorf("unauthorized: %s", resp.Status)


These errors look like something we'll want to define our error types for so we can later do errors.Is in the callers

jhrozek · 2024-03-25T08:51:23Z

proto/minder/v1/minder.proto

@@ -2026,8 +2026,7 @@ enum ProviderType {
    PROVIDER_TYPE_GIT = 3 [(name) = "git"];
    PROVIDER_TYPE_OCI = 4 [(name) = "oci"];
    PROVIDER_TYPE_REPO_LISTER = 5 [(name) = "repo-lister"];
-    PROVIDER_TYPE_DOCKERHUB = 6 [(name) = "dockerhub"];
-    PROVIDER_TYPE_GHCR = 7 [(name) = "ghcr"];
+    PROVIDER_TYPE_CONTAINER_LISTER = 6 [(name) = "container-lister"];


Ah great I think this answers my earlier question about the type (reviewing patches one by one)

jhrozek · 2024-03-25T08:52:09Z

database/migrations/000036_oci_prov.up.sql

-- Add `ghcr` and `dockerhub` provider type enum
-ALTER TYPE provider_type ADD VALUE 'ghcr';
-ALTER TYPE provider_type ADD VALUE 'dockerhub';
+ALTER TYPE provider_type ADD VALUE 'container-lister';


answers my earlier question

Signed-off-by: Juan Antonio Osorio <[email protected]>

Use a general container lister and implement sample command to test it out Signed-off-by: Juan Antonio Osorio <[email protected]>

They might come from an OCI registry and not be immediately linked. Signed-off-by: Juan Antonio Osorio <[email protected]> Handle general artifact list Signed-off-by: Juan Antonio Osorio <[email protected]>

Signed-off-by: Juan Antonio Osorio <[email protected]>

evankanderson · 2024-03-27T08:57:25Z

internal/providers/github/ghcr/ghcr.go

+	pageNumber := 0
+	itemsPerPage := 100
+	pt := string(verifyif.ArtifactTypeContainer)
+	opt := &github.PackageListOptions{


This is because we can't use the /tags/list endpoint? (I'm just looking at go-containerregistry's implementation, which I understand to be fairly complete.)

Ah, this is because we want to check /v2/_catalog, which we don't have the correct token type for?

https://toddysm.com/2024/02/12/authenticating-with-oci-registries-github-container-registry-ghcr-implementation/ has some hints, and suggests that you can use a GitHub auth token (As Authorization: Bearer ...) with the correct scopes to get a GHCR-specific token from /v2/token and then use the token from /v2/token to call /v2/_catalog to get a list of repositories / images.

Note that the blog post suggests that this includes a list of all public repositories in the /v2/_catalog endpoint when called when human credentials. I don't know what the options for GitHub App credentials here are.

With the knowledge we have now of how the GitHub App works, we should probably revisit this implementation. I'd like GHCR to work in the following scenarios:

As part of the GitHub App provider

As part of the OAuth2 provider

As a standalone provider

JAORMX requested a review from a team as a code owner March 22, 2024 16:37

JAORMX changed the title ~~Implement OCI provider~~ WIP: Implement OCI provider Mar 22, 2024

JAORMX marked this pull request as draft March 22, 2024 16:38

JAORMX force-pushed the oci-prov branch 2 times, most recently from 1da7967 to 22acd31 Compare March 23, 2024 11:13

jhrozek reviewed Mar 25, 2024

View reviewed changes

JAORMX force-pushed the oci-prov branch 6 times, most recently from 971c805 to 9da9b28 Compare March 26, 2024 09:09

JAORMX added 8 commits March 26, 2024 12:35

Implement OCI provider

3967653

Signed-off-by: Juan Antonio Osorio <[email protected]>

Add dockerhub and ghcr providers

d8f3fab

Use a general container lister and implement sample command to test it out Signed-off-by: Juan Antonio Osorio <[email protected]>

Make artifacts be optionally linked to repositories

a3a99e3

They might come from an OCI registry and not be immediately linked. Signed-off-by: Juan Antonio Osorio <[email protected]> Handle general artifact list Signed-off-by: Juan Antonio Osorio <[email protected]>

Make signature validation take something other than github

170291a

Signed-off-by: Juan Antonio Osorio <[email protected]>

Add artifact register

a4665ec

Signed-off-by: Juan Antonio Osorio <[email protected]>

Fix register flow

645ad08

Signed-off-by: Juan Antonio Osorio <[email protected]>

fix EEA index

45a6890

Signed-off-by: Juan Antonio Osorio <[email protected]>

Pass in continer registry from provider

65a5955

Signed-off-by: Juan Antonio Osorio <[email protected]>

JAORMX force-pushed the oci-prov branch from 9da9b28 to 65a5955 Compare March 26, 2024 10:35

evankanderson reviewed Mar 27, 2024

View reviewed changes

JAORMX mentioned this pull request Apr 7, 2024

Add OCI providers + DockerHub and GHCR #2983

Merged

10 tasks

JAORMX closed this May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Implement OCI provider #2765

WIP: Implement OCI provider #2765

JAORMX commented Mar 22, 2024

coveralls commented Mar 22, 2024 •

edited

Loading

jhrozek left a comment

jhrozek Mar 25, 2024

danielCalbonero12345 Apr 4, 2024

jhrozek Mar 25, 2024

evankanderson Mar 27, 2024 •

edited

Loading

jhrozek Mar 25, 2024

JAORMX Mar 26, 2024

jhrozek Mar 25, 2024

jhrozek Mar 25, 2024

JAORMX Mar 26, 2024

jhrozek Mar 25, 2024

JAORMX Mar 26, 2024

jhrozek Mar 25, 2024

JAORMX Mar 26, 2024

jhrozek Mar 25, 2024

jhrozek Mar 25, 2024

evankanderson Mar 27, 2024

evankanderson Mar 27, 2024

JAORMX Mar 28, 2024

WIP: Implement OCI provider #2765

WIP: Implement OCI provider #2765

Conversation

JAORMX commented Mar 22, 2024

Summary

Change Type

Testing

Review Checklist:

coveralls commented Mar 22, 2024 • edited Loading

jhrozek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 22, 2024 •

edited

Loading

evankanderson Mar 27, 2024 •

edited

Loading