fix: use canonical names inside TUF fetcher (#144)

* fix: use canonical names inside TUF fetcher
* keep hold of reference to Config

pkg/tuf/registry.go

@@ -10,12 +10,14 @@ import (
 	"strings"
 	"time"
 
+	"github.com/distribution/reference"
 	"github.com/docker/attest/pkg/oci"
 	"github.com/google/go-containerregistry/pkg/authn"
 	"github.com/google/go-containerregistry/pkg/crane"
 	v1 "github.com/google/go-containerregistry/pkg/v1"
 	"github.com/google/go-containerregistry/pkg/v1/types"
 	"github.com/theupdateframework/go-tuf/v2/metadata"
+	"github.com/theupdateframework/go-tuf/v2/metadata/config"
 )
 
 const (
@@ -34,6 +36,7 @@ type RegistryFetcher struct {
 	targetsRepo   string
 	cache         *ImageCache
 	timeout       time.Duration
+	cfg           *config.UpdaterConfig
 }
 
 type ImageCache struct {
@@ -67,13 +70,31 @@ type Layers struct {
 	MediaType string  `json:"mediaType"`
 }
 
-func NewRegistryFetcher(metadataRepo, metadataTag, targetsRepo string) *RegistryFetcher {
+func NewRegistryFetcher(cfg *config.UpdaterConfig) (*RegistryFetcher, error) {
+	ref, err := reference.ParseNormalizedNamed(cfg.RemoteMetadataURL)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse metadata repo: %w", err)
+	}
+	// add latest tag
+	metadataTag := LatestTag
+	if tag, ok := ref.(reference.Tagged); ok {
+		metadataTag = tag.Tag()
+	}
+	metadataRepo := ref.Name()
+
+	targetsRef, err := reference.ParseNormalizedNamed(cfg.RemoteTargetsURL)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse targets repo: %w", err)
+	}
+	targetsRepo := targetsRef.Name()
 	return &RegistryFetcher{
+		// we need to keep these reference so that we can unmangle the URL paths when downloading files
+		cfg:          cfg,
 		metadataRepo: metadataRepo,
 		metadataTag:  metadataTag,
 		targetsRepo:  targetsRepo,
 		cache:        NewImageCache(),
-	}
+	}, nil
 }
 
 // DownloadFile downloads a file from an OCI registry, errors out if it failed,
@@ -188,17 +209,17 @@ func getDataFromLayer(fileLayer v1.Layer, maxLength int64) ([]byte, error) {
 // parseImgRef maintains the Fetcher interface by parsing a URL path to an image reference and file name.
 func (d *RegistryFetcher) parseImgRef(urlPath string) (imgRef, fileName string, err error) {
 	// Check if repo is target or metadata
-	if strings.Contains(urlPath, d.targetsRepo) {
+	if strings.HasPrefix(urlPath, d.cfg.RemoteTargetsURL) {
 		// determine if the target path contains subdirectories and set image name accordingly
 		// <repo>/<filename>          -> image = <repo>:<filename>, layer = <filename>
 		// <repo>/<subdir>/<filename> -> index = <repo>:<subdir>  , image = <filename> -> layer = <filename>
-		target := strings.TrimPrefix(urlPath, d.targetsRepo+"/")
+		target := strings.TrimPrefix(urlPath, d.cfg.RemoteTargetsURL+"/")
 		subdir, name, found := strings.Cut(target, "/")
 		if found {
 			return fmt.Sprintf("%s:%s", d.targetsRepo, subdir), fmt.Sprintf("%s/%s", subdir, name), nil
 		}
 		return fmt.Sprintf("%s:%s", d.targetsRepo, target), target, nil
-	} else if strings.Contains(urlPath, d.metadataRepo) {
+	} else if strings.HasPrefix(urlPath, d.cfg.RemoteMetadataURL) {
 		// build the metadata image name
 		// determine if role is a delegated role and set the tag accordingly
 		fileName = path.Base(urlPath)

pkg/tuf/registry_test.go

@@ -48,7 +48,6 @@ func TestRegistryFetcher(t *testing.T) {
 	LoadRegistryTestData(t, regAddr, OCITUFTestDataPath)
 
 	metadataRepo := regAddr.Host + metadataPath
-	metadataImgTag := LatestTag
 	targetsRepo := regAddr.Host + targetsPath
 	targetFile := "test.txt"
 	delegatedRole := testRole
@@ -59,12 +58,12 @@ func TestRegistryFetcher(t *testing.T) {
 	// note - url is ignored here - needed to make http url parsing happy even when using oci
 	cfg, err := config.New("", DockerTUFRootDev.Data)
 	require.NoError(t, err)
-
-	cfg.Fetcher = NewRegistryFetcher(metadataRepo, metadataImgTag, targetsRepo)
 	cfg.LocalMetadataDir = dir
 	cfg.LocalTargetsDir = dir
 	cfg.RemoteTargetsURL = targetsRepo
 	cfg.RemoteMetadataURL = metadataRepo
+	cfg.Fetcher, err = NewRegistryFetcher(cfg)
+	require.NoError(t, err)
 
 	// create a new Updater instance
 	up, err := updater.New(cfg)
@@ -190,28 +189,59 @@ func TestParseImgRef(t *testing.T) {
 	metadataRepo := "test" + metadataPath
 	metadataTag := LatestTag
 	delegatedRole := testRole
+	validRef := fmt.Sprintf("%s/2.root.json", metadataRepo)
+	expectedRef := fmt.Sprintf("docker.io/%s:%s", metadataRepo, metadataTag)
 	testCases := []struct {
-		name         string
+		name                     string
-		ref          string
+		ref                      string
-		expectedRef  string
+		expectedRef              string
-		expectedFile string
+		expectedFile             string
+		metadataRepo             string
+		metadataTag              string
+		expectedRefError         string
+		expectedConstructorError string
+		targetsRepo              string
 	}{
-		{"top-level metadata", fmt.Sprintf("%s/2.root.json", metadataRepo), fmt.Sprintf("%s:%s", metadataRepo, metadataTag), "2.root.json"},
+		{name: "top-level metadata", ref: validRef, expectedRef: expectedRef, expectedFile: "2.root.json"},
-		{"delegated metadata", fmt.Sprintf("%s/%s/5.test-role.json", metadataRepo, delegatedRole), fmt.Sprintf("%s:%s", metadataRepo, delegatedRole), "5.test-role.json"},
+		{name: "short metdata repo", ref: validRef, metadataRepo: "test" + metadataPath, expectedRef: expectedRef, expectedFile: "2.root.json"},
-		{"top-level target", fmt.Sprintf("%s/policy.yaml", targetsRepo), fmt.Sprintf("%s:policy.yaml", targetsRepo), "policy.yaml"},
+		{name: "library path", ref: fmt.Sprintf("test%s/2.root.json", metadataPath), metadataRepo: "test" + metadataPath, expectedRef: "docker.io/test/tuf-metadata:latest", expectedFile: "2.root.json"},
-		{"delegated target", fmt.Sprintf("%s/%s/policy.yaml", targetsRepo, delegatedRole), fmt.Sprintf("%s:%s", targetsRepo, delegatedRole), fmt.Sprintf("%s/policy.yaml", delegatedRole)},
+		{name: "short targets repo", ref: validRef, targetsRepo: "test" + targetsPath, expectedRef: expectedRef, expectedFile: "2.root.json"},
+		{name: "delegated metadata", ref: fmt.Sprintf("%s/%s/5.test-role.json", metadataRepo, delegatedRole), expectedRef: fmt.Sprintf("docker.io/%s:%s", metadataRepo, delegatedRole), expectedFile: "5.test-role.json"},
+		{name: "top-level target", ref: fmt.Sprintf("%s/policy.yaml", targetsRepo), expectedRef: fmt.Sprintf("docker.io/%s:policy.yaml", targetsRepo), expectedFile: "policy.yaml"},
+		{name: "delegated target", ref: fmt.Sprintf("%s/%s/policy.yaml", targetsRepo, delegatedRole), expectedRef: fmt.Sprintf("docker.io/%s:%s", targetsRepo, delegatedRole), expectedFile: fmt.Sprintf("%s/policy.yaml", delegatedRole)},
+		{name: "docker/targets", ref: fmt.Sprintf("%s/2.root.json", "docker.io/docker/targets"), expectedRef: "docker.io/docker/targets:latest", expectedFile: "2.root.json", metadataRepo: "docker.io/docker/targets"},
+		{name: "malformed ref", ref: fmt.Sprintf("%s/2.root.json", "@broken"), expectedRefError: "urlPath: @broken/2.root.json must be in metadata or targets repo"},
+		{name: "malformed metadataRepo", ref: validRef, metadataRepo: "@broken", expectedConstructorError: "failed to parse metadata repo: invalid reference format"},
+		{name: "malformed targetsRepo", ref: validRef, targetsRepo: "@broken", expectedConstructorError: "failed to parse targets repo: invalid reference format"},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			d := &RegistryFetcher{
+			repo := metadataRepo
-				metadataRepo: metadataRepo,
+			if tc.metadataRepo != "" {
-				metadataTag:  LatestTag,
+				repo = tc.metadataRepo
-				targetsRepo:  targetsRepo,
+			}
+			targets := targetsRepo
+			if tc.targetsRepo != "" {
+				targets = tc.targetsRepo
+			}
+			cfg := &config.UpdaterConfig{
+				RemoteMetadataURL: repo,
+				RemoteTargetsURL:  targets,
+			}
+			d, err := NewRegistryFetcher(cfg)
+			if tc.expectedConstructorError != "" {
+				assert.ErrorContains(t, err, tc.expectedConstructorError)
+			} else {
+				require.NoError(t, err)
+				imgRef, file, err := d.parseImgRef(tc.ref)
+				if tc.expectedRefError != "" {
+					assert.ErrorContains(t, err, tc.expectedRefError)
+				} else {
+					require.NoError(t, err)
+					assert.Equal(t, tc.expectedRef, imgRef, "ref mismatch")
+					assert.Equal(t, tc.expectedFile, file, "file mismatch")
+				}
 			}
-			imgRef, file, err := d.parseImgRef(tc.ref)
-			assert.NoError(t, err)
-			assert.Equal(t, tc.expectedRef, imgRef)
-			assert.Equal(t, tc.expectedFile, file)
 		})
 	}
 }

pkg/tuf/tuf.go

@@ -11,7 +11,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/distribution/reference"
 	"github.com/docker/attest/internal/embed"
 	"github.com/docker/attest/internal/util"
 	"github.com/theupdateframework/go-tuf/v2/metadata"
@@ -120,17 +119,10 @@ func NewClient(opts *ClientOptions) (*Client, error) {
 	cfg.RemoteTargetsURL = opts.TargetsSource
 
 	if tufSource == OCISource {
-		ref, err := reference.ParseNormalizedNamed(opts.MetadataSource)
+		cfg.Fetcher, err = NewRegistryFetcher(cfg)
 		if err != nil {
-			return nil, fmt.Errorf("failed to parse metadata source: %w", err)
+			return nil, fmt.Errorf("failed to create registry fetcher: %w", err)
 		}
-		// add latest tag
-		metadataTag := LatestTag
-		if tag, ok := ref.(reference.Tagged); ok {
-			metadataTag = tag.Tag()
-		}
-		metadataRepo := ref.Name()
-		cfg.Fetcher = NewRegistryFetcher(metadataRepo, metadataTag, opts.TargetsSource)
 	}
 
 	// create a new Updater instance

pkg/tuf/tuf_test.go

@@ -112,27 +112,29 @@ func TestDownloadTarget(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		tufClient, err := NewClient(&ClientOptions{DockerTUFRootDev.Data, tufPath, tc.metadataSource, tc.targetsSource, alwaysGoodVersionChecker})
+		t.Run(tc.name, func(t *testing.T) {
-		require.NoErrorf(t, err, "Failed to create TUF client: %v", err)
+			tufClient, err := NewClient(&ClientOptions{DockerTUFRootDev.Data, tufPath, tc.metadataSource, tc.targetsSource, alwaysGoodVersionChecker})
-		require.NotNil(t, tufClient.updater, "Failed to create updater")
+			require.NoErrorf(t, err, "Failed to create TUF client: %v", err)
+			require.NotNil(t, tufClient.updater, "Failed to create updater")
 
-		// get trusted tuf metadata
+			// get trusted tuf metadata
-		trustedMetadata := tufClient.updater.GetTrustedMetadataSet()
+			trustedMetadata := tufClient.updater.GetTrustedMetadataSet()
-		assert.NotNil(t, trustedMetadata, "Failed to get trusted metadata")
+			assert.NotNil(t, trustedMetadata, "Failed to get trusted metadata")
 
-		// download top-level target files
+			// download top-level target files
-		targets := trustedMetadata.Targets[metadata.TARGETS].Signed.Targets
+			targets := trustedMetadata.Targets[metadata.TARGETS].Signed.Targets
-		for _, target := range targets {
+			for _, target := range targets {
-			// download target files
+				// download target files
-			_, err := tufClient.DownloadTarget(target.Path, filepath.Join(tufPath, "download"))
+				_, err := tufClient.DownloadTarget(target.Path, filepath.Join(tufPath, "download"))
-			assert.NoErrorf(t, err, "Failed to download target: %v", err)
+				assert.NoErrorf(t, err, "Failed to download target: %v", err)
-		}
+			}
 
-		// download delegated target
+			// download delegated target
-		targetInfo, err := tufClient.updater.GetTargetInfo(delegatedTargetFile)
+			targetInfo, err := tufClient.updater.GetTargetInfo(delegatedTargetFile)
-		require.NoError(t, err)
+			require.NoError(t, err)
-		_, err = tufClient.DownloadTarget(targetInfo.Path, filepath.Join(tufPath, targetInfo.Path))
+			_, err = tufClient.DownloadTarget(targetInfo.Path, filepath.Join(tufPath, targetInfo.Path))
-		assert.NoError(t, err)
+			assert.NoError(t, err)
+		})
 	}
 }