Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(controller): do not normalize urls when searching for image creds #3110

Merged
merged 1 commit into from
Dec 10, 2024
+190 −69
Merged

fix(controller): do not normalize urls when searching for image creds #3110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 24 additions & 8 deletions internal/credentials/kubernetes/database.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,14 +160,20 @@
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)

Check warning on line 171 in internal/credentials/kubernetes/database.go

View check run for this annotation

Codecov / codecov/patch

internal/credentials/kubernetes/database.go#L170-L171 

Added lines #L170 - L171 were not covered by tests
}

logger := logging.LoggerFromContext(ctx)

// Search for a matching Secret.
var matchingSecret *corev1.Secret
for _, secret := range secrets.Items {
if secret.Data == nil {
continue
Expand All @@ -190,12 +196,22 @@
continue
}
if regex.MatchString(repoURL) {
matchingSecret = &secret
break
return &secret, nil
}
} else if repoURL == helm.NormalizeChartRepositoryURL(git.NormalizeURL(string(urlBytes))) {
continue

Check warning on line 201 in internal/credentials/kubernetes/database.go

View check run for this annotation

Codecov / codecov/patch

internal/credentials/kubernetes/database.go#L201 

Added line #L201 was not covered by tests
}

// Not a regex
secretURL := string(urlBytes)
switch credType {
case credentials.TypeGit:
secretURL = git.NormalizeURL(secretURL)
case credentials.TypeHelm:
secretURL = helm.NormalizeChartRepositoryURL(secretURL)

Check warning on line 210 in internal/credentials/kubernetes/database.go

View check run for this annotation

Codecov / codecov/patch

internal/credentials/kubernetes/database.go#L209-L210 

Added lines #L209 - L210 were not covered by tests
}
if secretURL == repoURL {
return &secret, nil
}
}
return matchingSecret, nil
return nil, nil
}
227 changes: 166 additions & 61 deletions internal/credentials/kubernetes/database_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand All @@ -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"),
Expand All @@ -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,
},
}
Expand All @@ -204,7 +309,7 @@ func TestGet(t *testing.T) {
).Get(
context.Background(),
testProjectNamespace,
testCredType,
testCase.credType,
testCase.repoURL,
)
require.NoError(t, err)
Expand Down
Loading