diff --git a/internal/credentials/kubernetes/database.go b/internal/credentials/kubernetes/database.go index 3a664dac4..8585a76d0 100644 --- a/internal/credentials/kubernetes/database.go +++ b/internal/credentials/kubernetes/database.go @@ -160,14 +160,20 @@ func (k *database) getCredentialsSecret( return strings.Compare(lhs.Name, rhs.Name) }) - // Normalize the repository URL. These normalizations should be safe even - // if not applicable to the URL type. - repoURL = helm.NormalizeChartRepositoryURL(git.NormalizeURL(repoURL)) + // Note: We formerly applied these normalizations to any URL, thinking them + // generally safe. We no longer do this as it was discovered that an image + // repository URL with a port number could be mistaken for an SCP-style URL of + // the form host.xz:path/to/repo + switch credType { + case credentials.TypeGit: + repoURL = git.NormalizeURL(repoURL) + case credentials.TypeHelm: + repoURL = helm.NormalizeChartRepositoryURL(repoURL) + } logger := logging.LoggerFromContext(ctx) // Search for a matching Secret. - var matchingSecret *corev1.Secret for _, secret := range secrets.Items { if secret.Data == nil { continue @@ -190,12 +196,22 @@ func (k *database) getCredentialsSecret( continue } if regex.MatchString(repoURL) { - matchingSecret = &secret - break + return &secret, nil } - } else if repoURL == helm.NormalizeChartRepositoryURL(git.NormalizeURL(string(urlBytes))) { + continue + } + + // Not a regex + secretURL := string(urlBytes) + switch credType { + case credentials.TypeGit: + secretURL = git.NormalizeURL(secretURL) + case credentials.TypeHelm: + secretURL = helm.NormalizeChartRepositoryURL(secretURL) + } + if secretURL == repoURL { return &secret, nil } } - return matchingSecret, nil + return nil, nil } diff --git a/internal/credentials/kubernetes/database_test.go b/internal/credentials/kubernetes/database_test.go index 1cbfc84b6..59a87fc0b 100644 --- a/internal/credentials/kubernetes/database_test.go +++ b/internal/credentials/kubernetes/database_test.go @@ -34,38 +34,45 @@ func TestGet(t *testing.T) { testProjectNamespace = "fake-namespace" testGlobalNamespace = "another-fake-namespace" - testCredType = credentials.TypeGit - // This deliberately omits the trailing .git to test normalization - testRepoURL = "https://github.com/akuity/kargo" - insecureTestURL = "http://github.com/akuity/bogus.git" + testGitRepoURL = "https://github.com/akuity/kargo" + testInsecureGitURL = "http://github.com/akuity/bogus.git" + + // This is deliberately an image URL that could be mistaken for an SCP-style + // Git URL to verify that Git URL normalization is not applied to image + // URLs. + testImageURL = "my-registry.io:5000/image" ) - testLabels := map[string]string{ - kargoapi.CredentialTypeLabelKey: testCredType.String(), + testGitLabels := map[string]string{ + kargoapi.CredentialTypeLabelKey: credentials.TypeGit.String(), + } + + testImageLabels := map[string]string{ + kargoapi.CredentialTypeLabelKey: credentials.TypeImage.String(), } - projectCredentialWithRepoURL := &corev1.Secret{ + projectGitCredentialWithRepoURL := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "project-credential-repo-url", + Name: "project-credential-git-repo-url", Namespace: testProjectNamespace, - Labels: testLabels, + Labels: testGitLabels, }, Data: map[string][]byte{ - credentials.FieldRepoURL: []byte(testRepoURL), + credentials.FieldRepoURL: []byte(testGitRepoURL), credentials.FieldUsername: []byte("project-exact"), credentials.FieldPassword: []byte("fake-password"), }, } - projectCredentialWithRepoURLPattern := &corev1.Secret{ + projectGitCredentialWithRepoURLPattern := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "project-credential-repo-url-pattern", + Name: "project-credential-git-repo-url-pattern", Namespace: testProjectNamespace, - Labels: testLabels, + Labels: testGitLabels, }, Data: map[string][]byte{ - credentials.FieldRepoURL: []byte(testRepoURL), + credentials.FieldRepoURL: []byte(testGitRepoURL), credentials.FieldRepoURLIsRegex: []byte("true"), credentials.FieldUsername: []byte("project-pattern"), credentials.FieldPassword: []byte("fake-password"), @@ -76,40 +83,94 @@ func TestGet(t *testing.T) { // Kargo will refuse to look for credentials for insecure URLs. However, // this is a secret that WOULD be matched if not for that check. This helps // us test that the check is working. - projectCredentialWithInsecureRepoURL := &corev1.Secret{ + projectGitCredentialWithInsecureRepoURL := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "project-credential-insecure-repo-url", + Name: "project-credential-git-insecure-repo-url", Namespace: testProjectNamespace, - Labels: testLabels, + Labels: testGitLabels, }, Data: map[string][]byte{ - credentials.FieldRepoURL: []byte(insecureTestURL), + credentials.FieldRepoURL: []byte(testInsecureGitURL), credentials.FieldUsername: []byte("project-insecure"), credentials.FieldPassword: []byte("fake-password"), }, } - globalCredentialWithRepoURL := &corev1.Secret{ + globalGitCredentialWithRepoURL := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "global-credential-git-repo-url", + Namespace: testGlobalNamespace, + Labels: testGitLabels, + }, + Data: map[string][]byte{ + credentials.FieldRepoURL: []byte(testGitRepoURL), + credentials.FieldUsername: []byte("global-exact"), + credentials.FieldPassword: []byte("fake-password"), + }, + } + + globalGitCredentialWithRepoURLPattern := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "global-credential-git-repo-url-pattern", + Namespace: testGlobalNamespace, + Labels: testGitLabels, + }, + Data: map[string][]byte{ + credentials.FieldRepoURL: []byte(testGitRepoURL), + credentials.FieldRepoURLIsRegex: []byte("true"), + credentials.FieldUsername: []byte("global-pattern"), + credentials.FieldPassword: []byte("fake-password"), + }, + } + + projectImageCredentialWithRepoURL := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "project-credential-image-repo-url", + Namespace: testProjectNamespace, + Labels: testImageLabels, + }, + Data: map[string][]byte{ + credentials.FieldRepoURL: []byte(testImageURL), + credentials.FieldUsername: []byte("project-exact"), + credentials.FieldPassword: []byte("fake-password"), + }, + } + + projectImageCredentialWithRepoURLPattern := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "global-credential-repo-url", + Name: "project-credential-image-repo-url-pattern", + Namespace: testProjectNamespace, + Labels: testImageLabels, + }, + Data: map[string][]byte{ + credentials.FieldRepoURL: []byte(testImageURL), + credentials.FieldRepoURLIsRegex: []byte("true"), + credentials.FieldUsername: []byte("project-pattern"), + credentials.FieldPassword: []byte("fake-password"), + }, + } + + globalImageCredentialWithRepoURL := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "global-credential-image-repo-url", Namespace: testGlobalNamespace, - Labels: testLabels, + Labels: testImageLabels, }, Data: map[string][]byte{ - credentials.FieldRepoURL: []byte(testRepoURL), + credentials.FieldRepoURL: []byte(testImageURL), credentials.FieldUsername: []byte("global-exact"), credentials.FieldPassword: []byte("fake-password"), }, } - globalCredentialWithRepoURLPattern := &corev1.Secret{ + globalImageCredentialWithRepoURLPattern := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "global-credential-repo-url-pattern", + Name: "global-credential-image-repo-url-pattern", Namespace: testGlobalNamespace, - Labels: testLabels, + Labels: testImageLabels, }, Data: map[string][]byte{ - credentials.FieldRepoURL: []byte(testRepoURL), + credentials.FieldRepoURL: []byte(testImageURL), credentials.FieldRepoURLIsRegex: []byte("true"), credentials.FieldUsername: []byte("global-pattern"), credentials.FieldPassword: []byte("fake-password"), @@ -119,76 +180,120 @@ func TestGet(t *testing.T) { testCases := []struct { name string secrets []client.Object + credType credentials.Type repoURL string expected *corev1.Secret }{ { - name: "exact match in project namespace", - secrets: []client.Object{projectCredentialWithRepoURL}, - repoURL: testRepoURL, - expected: projectCredentialWithRepoURL, + name: "git URL exact match in project namespace", + secrets: []client.Object{projectGitCredentialWithRepoURL}, + credType: credentials.TypeGit, + repoURL: testGitRepoURL, + expected: projectGitCredentialWithRepoURL, + }, + { + name: "git URL pattern match in project namespace", + secrets: []client.Object{projectGitCredentialWithRepoURLPattern}, + credType: credentials.TypeGit, + repoURL: testGitRepoURL, + expected: projectGitCredentialWithRepoURLPattern, + }, + { + name: "git URL exact match in global namespace", + secrets: []client.Object{globalGitCredentialWithRepoURL}, + credType: credentials.TypeGit, + repoURL: testGitRepoURL, + expected: globalGitCredentialWithRepoURL, + }, + { + name: "git URL pattern match in global namespace", + secrets: []client.Object{globalGitCredentialWithRepoURLPattern}, + credType: credentials.TypeGit, + repoURL: testGitRepoURL, + expected: globalGitCredentialWithRepoURLPattern, + }, + // Image URLs of the form host:port/image can be mistaken for SCP-style Git + // URLs. The next several test cases verify that Git URL normalization is + // not being applied to image URLs and incorrectly normalizing + // host:port/image as ssh://host:port/image. + { + name: "image URL exact match in project namespace", + secrets: []client.Object{projectImageCredentialWithRepoURL}, + credType: credentials.TypeImage, + repoURL: testImageURL, + expected: projectImageCredentialWithRepoURL, }, { - name: "pattern match in project namespace", - secrets: []client.Object{projectCredentialWithRepoURLPattern}, - repoURL: testRepoURL, - expected: projectCredentialWithRepoURLPattern, + name: "image URL pattern match in project namespace", + secrets: []client.Object{projectImageCredentialWithRepoURLPattern}, + credType: credentials.TypeImage, + repoURL: testImageURL, + expected: projectImageCredentialWithRepoURLPattern, }, { - name: "exact match in global namespace", - secrets: []client.Object{globalCredentialWithRepoURL}, - repoURL: testRepoURL, - expected: globalCredentialWithRepoURL, + name: "image URL exact match in global namespace", + secrets: []client.Object{globalImageCredentialWithRepoURL}, + credType: credentials.TypeImage, + repoURL: testImageURL, + expected: globalImageCredentialWithRepoURL, }, { - name: "pattern match in global namespace", - secrets: []client.Object{globalCredentialWithRepoURLPattern}, - repoURL: testRepoURL, - expected: globalCredentialWithRepoURLPattern, + name: "image URL pattern match in global namespace", + secrets: []client.Object{globalImageCredentialWithRepoURLPattern}, + credType: credentials.TypeImage, + repoURL: testImageURL, + expected: globalImageCredentialWithRepoURLPattern, }, + // The next several tests cases confirm the precedence rules for credential + // matching. { name: "precedence: exact match in project namespace over pattern match", secrets: []client.Object{ - projectCredentialWithRepoURL, - projectCredentialWithRepoURLPattern, + projectGitCredentialWithRepoURL, + projectGitCredentialWithRepoURLPattern, }, - repoURL: testRepoURL, - expected: projectCredentialWithRepoURL, + credType: credentials.TypeGit, + repoURL: testGitRepoURL, + expected: projectGitCredentialWithRepoURL, }, { name: "precedence: exact match in global namespace over pattern match", secrets: []client.Object{ - globalCredentialWithRepoURL, - globalCredentialWithRepoURLPattern, + globalGitCredentialWithRepoURL, + globalGitCredentialWithRepoURLPattern, }, - repoURL: testRepoURL, - expected: globalCredentialWithRepoURL, + credType: credentials.TypeGit, + repoURL: testGitRepoURL, + expected: globalGitCredentialWithRepoURL, }, { name: "precedence: match in project namespace over match in global namespace", secrets: []client.Object{ - projectCredentialWithRepoURL, - globalCredentialWithRepoURL, + projectGitCredentialWithRepoURL, + globalGitCredentialWithRepoURL, }, - repoURL: testRepoURL, - expected: projectCredentialWithRepoURL, + credType: credentials.TypeGit, + repoURL: testGitRepoURL, + expected: projectGitCredentialWithRepoURL, }, { name: "no match", secrets: []client.Object{ - projectCredentialWithRepoURL, - projectCredentialWithRepoURLPattern, - globalCredentialWithRepoURL, - globalCredentialWithRepoURLPattern, + projectGitCredentialWithRepoURL, + projectGitCredentialWithRepoURLPattern, + globalGitCredentialWithRepoURL, + globalGitCredentialWithRepoURLPattern, }, + credType: credentials.TypeGit, repoURL: "http://github.com/no/secrets/should/match/this.git", expected: nil, }, { name: "insecure HTTP endpoint", // Would match if not for the insecure URL check - secrets: []client.Object{projectCredentialWithInsecureRepoURL}, - repoURL: insecureTestURL, + secrets: []client.Object{projectGitCredentialWithInsecureRepoURL}, + credType: credentials.TypeGit, + repoURL: testInsecureGitURL, expected: nil, }, } @@ -204,7 +309,7 @@ func TestGet(t *testing.T) { ).Get( context.Background(), testProjectNamespace, - testCredType, + testCase.credType, testCase.repoURL, ) require.NoError(t, err)