Repository: sigstore/sigstore-go (go)

Issue:
SigningConfig should select the highest API version when possible
**Description**

<!--Tell us about the bug. What went wrong? How can we reproduce it? What should the correct behavior be?
-->

**Version**

<!--What version are you using? e.g. `cosign version`...
-->

Right now, when selecting a service or set of services, the SigningConfig will select the first match given a validity period and API version. If the client supports two API versions concurrently (e.g. Rekor v1 and Rekor v2) and the distributed signing config has entries for multiple API versions (which it will for Rekor), then the client may select an older API version depending on the sorting of the services.

I will also update the language in protobuf-specs to be more prescriptive on how clients should select a service given support for multiple API versions.

Pull request description:
Select highest API version with multiple SigningConfig services
Right now, when selecting a service or set of services, the SigningConfig will select the first match given a validity period and API version. If the client supports two API versions concurrently (e.g. Rekor v1 and Rekor v2) and the distributed signing config has entries for multiple API versions (which it will for Rekor), then the client may select an older API version depending on the sorting of the services.

This change sorts the supported API versions to select the highest API version first.

Fixes #453

<!--
Thanks for opening a pull request! Please do not just delete this text.  The three fields below are mandatory.

Please remember to:
- This PR requires an issue. If it is a new feature, the issue should proceed the PR and will have allowed sufficient time for discussions to take place. Please use
issue tags such as "Closes #XYZ" or "Resolves sigstore/repo-name#XYZ".
  [documentation](https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword)
- ensure your commits are signed-off, as sigstore uses the [DCO](https://en.wikipedia.org/wiki/Developer_Certificate_of_Origin) using `git commit -s`, or `git commit -s --amend` if you want to amend already existing commits
- lastly, ensure there are no merge commits!

Thank you :)
-->

#### Summary
<!--
 Explain the **motivation** for making this change. What existing problem does the pull request solve? How can reviewers test this PR?
-->

#### Release Note
<!--
Add a release note for each of the following conditions:

* Config changes (additions, deletions, updates)
* API additions—new endpoint, new response fields, or newly accepted request parameters
* Database changes (any)
* Websocket additions or changes
* Anything noteworthy to an administrator running private sigstore instances (err on the side of over-communicating)
* New features and improvements, including behavioural changes, UI changes and CLI changes
* Bug fixes and fixes of previous known issues
* Deprecation warnings, breaking changes, or compatibility notes

If no release notes are required write NONE. Use past-tense.

-->

#### Documentation
<!--

Does this change require an update to documentation? How will users implement your new feature?

Please reference a PR within https://docs.sigstore.dev

-->


Patch:
diff --git a/pkg/root/signing_config.go b/pkg/root/signing_config.go
index 495afb0..1490e14 100644
--- a/pkg/root/signing_config.go
+++ b/pkg/root/signing_config.go
@@ -47,15 +47,27 @@ type ServiceConfiguration struct {
 }
 
 // SelectService returns which service endpoint should be used based on supported API versions
-// and current time. It will select the first service that matches the criteria. Services should
-// be sorted from newest to oldest validity period start time, to minimize how far clients
-// need to search to find a matching service.
+// and current time. It will select the first service with the highest API version that matches
+// the criteria. Services should be sorted from newest to oldest validity period start time, to
+// minimize how far clients need to search to find a matching service.
 func SelectService(services []Service, supportedAPIVersions []uint32, currentTime time.Time) (string, error) {
-	for _, s := range services {
-		if slices.Contains(supportedAPIVersions, s.MajorAPIVersion) && s.ValidAtTime(currentTime) {
-			return s.URL, nil
+	if len(supportedAPIVersions) == 0 {
+		return "", fmt.Errorf("no supported API versions")
+	}
+
+	sortedVersions := make([]uint32, len(supportedAPIVersions))
+	copy(sortedVersions, supportedAPIVersions)
+	slices.Sort(sortedVersions)
+	slices.Reverse(sortedVersions)
+
+	for _, version := range sortedVersions {
+		for _, s := range services {
+			if version == s.MajorAPIVersion && s.ValidAtTime(currentTime) {
+				return s.URL, nil
+			}
 		}
 	}
+
 	return "", fmt.Errorf("no matching service found for API versions %v and current time %v", supportedAPIVersions, currentTime)
 }
 
@@ -63,31 +75,49 @@ func SelectService(services []Service, supportedAPIVersions []uint32, currentTim
 // and current time. It will use the configuration's selector to pick a set of services.
 // ALL will return all service endpoints, ANY will return a random endpoint, and
 // EXACT will return a random selection of a specified number of endpoints.
+// It will select services from the highest supported API versions and will not select
+// services from different API versions.
 func SelectServices(services []Service, config ServiceConfiguration, supportedAPIVersions []uint32, currentTime time.Time) ([]string, error) {
-	var urls []string
+	if len(supportedAPIVersions) == 0 {
+		return nil, fmt.Errorf("no supported API versions")
+	}
+
+	urlsByVersion := make(map[uint32][]string)
 	for _, s := range services {
 		if slices.Contains(supportedAPIVersions, s.MajorAPIVersion) && s.ValidAtTime(currentTime) {
-			urls = append(urls, s.URL)
+			urlsByVersion[s.MajorAPIVersion] = append(urlsByVersion[s.MajorAPIVersion], s.URL)
 		}
 	}
-	if len(urls) == 0 {
-		return nil, fmt.Errorf("no matching services found for API versions %v and current time %v", supportedAPIVersions, currentTime)
-	}
-	switch config.Selector {
-	case prototrustroot.ServiceSelector_ALL:
-		return urls, nil
-	case prototrustroot.ServiceSelector_ANY:
-		i := rand.Intn(len(urls)) // #nosec G404
-		return []string{urls[i]}, nil
-	case prototrustroot.ServiceSelector_EXACT:
-		matchedUrls, err := selectExact(urls, config.Count)
-		if err != nil {
-			return nil, err
+
+	sortedVersions := make([]uint32, len(supportedAPIVersions))
+	copy(sortedVersions, supportedAPIVersions)
+	slices.Sort(sortedVersions)
+	slices.Reverse(sortedVersions)
+
+	// Select services from the highest supported API version
+	for _, version := range sortedVersions {
+		urls, ok := urlsByVersion[version]
+		if !ok {
+			continue
+		}
+		switch config.Selector {
+		case prototrustroot.ServiceSelector_ALL:
+			return urls, nil
+		case prototrustroot.ServiceSelector_ANY:
+			i := rand.Intn(len(urls)) // #nosec G404
+			return []string{urls[i]}, nil
+		case prototrustroot.ServiceSelector_EXACT:
+			matchedUrls, err := selectExact(urls, config.Count)
+			if err != nil {
+				return nil, err
+			}
+			return matchedUrls, nil
+		default:
+			return nil, fmt.Errorf("invalid service selector")
 		}
-		return matchedUrls, nil
-	default:
-		return nil, fmt.Errorf("invalid service selector")
 	}
+
+	return nil, fmt.Errorf("no matching services found for API versions %v and current time %v", supportedAPIVersions, currentTime)
 }
 
 func selectExact[T any](slice []T, count uint32) ([]T, error) {


Test patch:
diff --git a/pkg/root/signing_config_test.go b/pkg/root/signing_config_test.go
index 0c6913f..a339d67 100644
--- a/pkg/root/signing_config_test.go
+++ b/pkg/root/signing_config_test.go
@@ -86,7 +86,7 @@ func TestSelectService(t *testing.T) {
 	tests := []struct {
 		name               string
 		services           []Service
-		supportedVersions  uint32
+		supportedVersions  []uint32
 		currentTime        time.Time
 		expectedURL        string
 		expectedErr        bool
@@ -95,7 +95,7 @@ func TestSelectService(t *testing.T) {
 		{
 			name:              "single matching service",
 			services:          services,
-			supportedVersions: 1,
+			supportedVersions: []uint32{1},
 			currentTime:       now,
 			expectedURL:       "url1",
 			expectedErr:       false,
@@ -103,7 +103,7 @@ func TestSelectService(t *testing.T) {
 		{
 			name:              "multiple matching service, first selected",
 			services:          services,
-			supportedVersions: 2,
+			supportedVersions: []uint32{2},
 			currentTime:       now,
 			expectedURL:       "url2",
 			expectedErr:       false,
@@ -111,7 +111,7 @@ func TestSelectService(t *testing.T) {
 		{
 			name:               "no matching version",
 			services:           services,
-			supportedVersions:  3,
+			supportedVersions:  []uint32{3},
 			currentTime:        now,
 			expectedErr:        true,
 			expectedErrMessage: "no matching service found for API versions [3] and current time",
@@ -119,7 +119,7 @@ func TestSelectService(t *testing.T) {
 		{
 			name:              "valid with no end time",
 			services:          services,
-			supportedVersions: 2,
+			supportedVersions: []uint32{2},
 			currentTime:       farFuture,
 			expectedURL:       "url_no_end",
 			expectedErr:       false,
@@ -127,7 +127,7 @@ func TestSelectService(t *testing.T) {
 		{
 			name:               "no matching service at all",
 			services:           []Service{},
-			supportedVersions:  1,
+			supportedVersions:  []uint32{1},
 			currentTime:        now,
 			expectedErr:        true,
 			expectedErrMessage: "no matching service found for API versions [1] and current time",
@@ -135,16 +135,43 @@ func TestSelectService(t *testing.T) {
 		{
 			name:              "first service selected when multiple match",
 			services:          services,
-			supportedVersions: 2,
+			supportedVersions: []uint32{2},
 			currentTime:       now.Add(time.Minute), // In the validity period of url3_new
 			expectedURL:       "url2",
 			expectedErr:       false,
 		},
+		{
+			name:              "match to highest API version with multiple supported versions",
+			services:          services,
+			supportedVersions: []uint32{1, 2},
+			currentTime:       now,
+			expectedURL:       "url2",
+			expectedErr:       false,
+		},
+		{
+			name: "match to highest API version with multiple supported versions, lower API version",
+			services: []Service{{URL: "url1",
+				MajorAPIVersion:     1,
+				ValidityPeriodStart: past,
+				ValidityPeriodEnd:   future}},
+			supportedVersions: []uint32{2, 1},
+			currentTime:       now,
+			expectedURL:       "url1",
+			expectedErr:       false,
+		},
+		{
+			name:               "no supported versions",
+			services:           services,
+			supportedVersions:  []uint32{},
+			currentTime:        now,
+			expectedErr:        true,
+			expectedErrMessage: "no supported API versions",
+		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			url, err := SelectService(tt.services, []uint32{tt.supportedVersions}, tt.currentTime)
+			url, err := SelectService(tt.services, tt.supportedVersions, tt.currentTime)
 			if (err != nil) != tt.expectedErr {
 				t.Errorf("SelectService() error = %v, expectedErr %v", err, tt.expectedErr)
 				return
@@ -190,7 +217,7 @@ func TestSelectServices(t *testing.T) {
 		name               string
 		services           []Service
 		config             ServiceConfiguration
-		supportedVersions  uint32
+		supportedVersions  []uint32
 		currentTime        time.Time
 		expectedURLs       []string
 		possibleURLs       [][]string
@@ -203,7 +230,7 @@ func TestSelectServices(t *testing.T) {
 			config: ServiceConfiguration{
 				Selector: prototrustroot.ServiceSelector_ALL,
 			},
-			supportedVersions: 2,
+			supportedVersions: []uint32{2},
 			currentTime:       now,
 			expectedURLs:      []string{"url2", "url3"},
 			expectedErr:       false,
@@ -214,7 +241,7 @@ func TestSelectServices(t *testing.T) {
 			config: ServiceConfiguration{
 				Selector: prototrustroot.ServiceSelector_ALL,
 			},
-			supportedVersions: 1,
+			supportedVersions: []uint32{1},
 			currentTime:       now,
 			expectedURLs:      []string{"url1"},
 			expectedErr:       false,
@@ -225,7 +252,7 @@ func TestSelectServices(t *testing.T) {
 			config: ServiceConfiguration{
 				Selector: prototrustroot.ServiceSelector_ALL,
 			},
-			supportedVersions:  3,
+			supportedVersions:  []uint32{3},
 			currentTime:        now,
 			expectedErr:        true,
 			expectedErrMessage: "no matching services found for API versions [3] and current time",
@@ -236,7 +263,7 @@ func TestSelectServices(t *testing.T) {
 			config: ServiceConfiguration{
 				Selector: prototrustroot.ServiceSelector_ANY,
 			},
-			supportedVersions: 2,
+			supportedVersions: []uint32{2},
 			currentTime:       now,
 			possibleURLs:      [][]string{{"url2"}, {"url3"}},
 			expectedErr:       false,
@@ -247,7 +274,7 @@ func TestSelectServices(t *testing.T) {
 			config: ServiceConfiguration{
 				Selector: prototrustroot.ServiceSelector_ANY,
 			},
-			supportedVersions: 1,
+			supportedVersions: []uint32{1},
 			currentTime:       now,
 			expectedURLs:      []string{"url1"},
 			expectedErr:       false,
@@ -258,7 +285,7 @@ func TestSelectServices(t *testing.T) {
 			config: ServiceConfiguration{
 				Selector: prototrustroot.ServiceSelector_ANY,
 			},
-			supportedVersions:  3,
+			supportedVersions:  []uint32{3},
 			currentTime:        now,
 			expectedErr:        true,
 			expectedErrMessage: "no matching services found for API versions [3] and current time",
@@ -270,7 +297,7 @@ func TestSelectServices(t *testing.T) {
 				Selector: prototrustroot.ServiceSelector_EXACT,
 				Count:    1,
 			},
-			supportedVersions: 2,
+			supportedVersions: []uint32{2},
 			currentTime:       now,
 			possibleURLs:      [][]string{{"url2"}, {"url3"}},
 			expectedErr:       false,
@@ -282,7 +309,7 @@ func TestSelectServices(t *testing.T) {
 				Selector: prototrustroot.ServiceSelector_EXACT,
 				Count:    2,
 			},
-			supportedVersions: 2,
+			supportedVersions: []uint32{2},
 			currentTime:       now,
 			possibleURLs:      [][]string{{"url2", "url3"}, {"url3", "url2"}},
 			expectedErr:       false,
@@ -294,7 +321,7 @@ func TestSelectServices(t *testing.T) {
 				Selector: prototrustroot.ServiceSelector_EXACT,
 				Count:    1,
 			},
-			supportedVersions: 1,
+			supportedVersions: []uint32{1},
 			currentTime:       now,
 			expectedURLs:      []string{"url1"},
 			expectedErr:       false,
@@ -306,7 +333,7 @@ func TestSelectServices(t *testing.T) {
 				Selector: prototrustroot.ServiceSelector_EXACT,
 				Count:    0,
 			},
-			supportedVersions:  2,
+			supportedVersions:  []uint32{2},
 			currentTime:        now,
 			expectedErr:        true,
 			expectedErrMessage: "service selector count must be greater than 0",
@@ -318,7 +345,19 @@ func TestSelectServices(t *testing.T) {
 				Selector: prototrustroot.ServiceSelector_EXACT,
 				Count:    3,
 			},
-			supportedVersions:  2,
+			supportedVersions:  []uint32{2},
+			currentTime:        now,
+			expectedErr:        true,
+			expectedErrMessage: "service selector count 3 must be less than or equal to the slice length 2",
+		},
+		{
+			name:     "EXACT selector, count greater than matches, multiple supported versions",
+			services: services,
+			config: ServiceConfiguration{
+				Selector: prototrustroot.ServiceSelector_EXACT,
+				Count:    3,
+			},
+			supportedVersions:  []uint32{1, 2},
 			currentTime:        now,
 			expectedErr:        true,
 			expectedErrMessage: "service selector count 3 must be less than or equal to the slice length 2",
@@ -330,7 +369,7 @@ func TestSelectServices(t *testing.T) {
 				Selector: prototrustroot.ServiceSelector_EXACT,
 				Count:    100,
 			},
-			supportedVersions:  2,
+			supportedVersions:  []uint32{2},
 			currentTime:        now,
 			expectedErr:        true,
 			expectedErrMessage: "service selector count 100 must be less than or equal to the slice length 2",
@@ -342,7 +381,7 @@ func TestSelectServices(t *testing.T) {
 				Selector: prototrustroot.ServiceSelector_EXACT,
 				Count:    1,
 			},
-			supportedVersions:  3,
+			supportedVersions:  []uint32{3},
 			currentTime:        now,
 			expectedErr:        true,
 			expectedErrMessage: "no matching services found for API versions [3] and current time",
@@ -353,16 +392,52 @@ func TestSelectServices(t *testing.T) {
 			config: ServiceConfiguration{
 				Selector: 99, // Invalid
 			},
-			supportedVersions:  2,
+			supportedVersions:  []uint32{2},
 			currentTime:        now,
 			expectedErr:        true,
 			expectedErrMessage: "invalid service selector",
 		},
+		{
+			name:     "match to highest API version with multiple supported versions",
+			services: services,
+			config: ServiceConfiguration{
+				Selector: prototrustroot.ServiceSelector_ALL,
+			},
+			supportedVersions: []uint32{1, 2},
+			currentTime:       now,
+			expectedURLs:      []string{"url2", "url3"},
+			expectedErr:       false,
+		},
+		{
+			name: "match to highest API version with multiple supported versions, lower API version",
+			services: []Service{{URL: "url1",
+				MajorAPIVersion:     1,
+				ValidityPeriodStart: past,
+				ValidityPeriodEnd:   future}},
+			config: ServiceConfiguration{
+				Selector: prototrustroot.ServiceSelector_ALL,
+			},
+			supportedVersions: []uint32{2, 1},
+			currentTime:       now,
+			expectedURLs:      []string{"url1"},
+			expectedErr:       false,
+		},
+		{
+			name:     "no supported versions",
+			services: services,
+			config: ServiceConfiguration{
+				Selector: prototrustroot.ServiceSelector_ALL,
+			},
+			supportedVersions:  []uint32{},
+			currentTime:        now,
+			expectedErr:        true,
+			expectedErrMessage: "no supported API versions",
+		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			urls, err := SelectServices(tt.services, tt.config, []uint32{tt.supportedVersions}, tt.currentTime)
+			urls, err := SelectServices(tt.services, tt.config, tt.supportedVersions, tt.currentTime)
 			if (err != nil) != tt.expectedErr {
 				t.Errorf("SelectServices() error = %v, expectedErr %v", err, tt.expectedErr)
 				return