Skip to content

Commit

Permalink
chore(backport release-1.1): fix(controller): do not normalize urls w…
Browse files Browse the repository at this point in the history 
…hen searching for image creds (#3114)

Co-authored-by: Kent Rancourt <[email protected]>
  • Loading branch information
@akuitybot @krancour
akuitybot and krancour authored Dec 10, 2024
1 parent d9932c7 commit 31d32d7
Show file tree
Hide file tree
Showing 2 changed files with 190 additions and 69 deletions.
32 changes: 24 additions & 8 deletions internal/credentials/kubernetes/database.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
227 changes: 166 additions & 61 deletions internal/credentials/kubernetes/database_test.go
View file
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

0 comments on commit 31d32d7

Please sign in to comment.