-
Couldn't load subscription status.
- Fork 1.8k
fix(#8940): token-authentication header typo in git resolver #8937
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,7 +47,7 @@ func (r remote) clone(ctx context.Context) (*repository, func(), error) { | |
| os.RemoveAll(tmpDir) | ||
| } | ||
|
|
||
| repo := repository{ | ||
| repo := &repository{ | ||
| url: r.url, | ||
| username: r.username, | ||
| password: r.password, | ||
|
|
@@ -62,7 +62,7 @@ func (r remote) clone(ctx context.Context) (*repository, func(), error) { | |
| } | ||
| return nil, cleanupFunc, err | ||
| } | ||
| return &repo, cleanupFunc, nil | ||
| return repo, cleanupFunc, nil | ||
| } | ||
|
|
||
| type repository struct { | ||
|
|
@@ -105,19 +105,18 @@ func (repo *repository) execGit(ctx context.Context, subCmd string, args ...stri | |
| // We need to configure which directory contains the cloned repository since `cd`ing | ||
| // into the repository directory is not concurrency-safe | ||
| configArgs := []string{"-C", repo.directory} | ||
|
|
||
| env := []string{"GIT_TERMINAL_PROMPT=false"} | ||
| if subCmd == "clone" { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was removed as some git commands like |
||
| // NOTE: Since this is only HTTP basic auth, authentication only supports http | ||
| // cloning, while unauthenticated cloning works for any other protocol supported | ||
| // by the git binary which doesn't require authentication. | ||
| if repo.username != "" && repo.password != "" { | ||
| token := base64.URLEncoding.EncodeToString([]byte(repo.username + ":" + repo.password)) | ||
| env = append( | ||
| env, | ||
| "GIT_AUTH_HEADER=Authorization=Basic "+token, | ||
| ) | ||
| configArgs = append(configArgs, "--config-env", "http.extraHeader=GIT_AUTH_HEADER") | ||
| } | ||
| // NOTE: Since this is only HTTP basic auth, authentication is only supported for http | ||
| // cloning, while unauthenticated cloning is supported for any other protocol supported | ||
| // by git which doesn't require authentication. | ||
| if repo.username != "" && repo.password != "" { | ||
| token := base64.URLEncoding.EncodeToString([]byte(repo.username + ":" + repo.password)) | ||
| env = append( | ||
| env, | ||
| "GIT_AUTH_HEADER=Authorization: Basic "+token, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| ) | ||
| configArgs = append(configArgs, "--config-env", "http.extraHeader=GIT_AUTH_HEADER") | ||
| } | ||
| cmd := repo.executor(ctx, "git", append(configArgs, args...)...) | ||
| cmd.Env = append(cmd.Environ(), env...) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -471,6 +471,59 @@ spec: | |
| } | ||
| } | ||
|
|
||
| func TestGitResolver_HTTPAuth(t *testing.T) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WIP regression test here |
||
| ctx := t.Context() | ||
| c, namespace := setup(ctx, t, gitFeatureFlags) | ||
|
|
||
| t.Parallel() | ||
|
|
||
| knativetest.CleanupOnInterrupt(func() { tearDown(ctx, t, c, namespace) }, t.Logf) | ||
| defer tearDown(ctx, t, c, namespace) | ||
|
|
||
| giteaClusterHostname, tokenSecretName := setupGitea(ctx, t, c, namespace) | ||
|
|
||
| requestUrl := fmt.Sprintf("http://%s/%s/%s", net.JoinHostPort(giteaClusterHostname, "3000"), scmRemoteOrg, scmRemoteRepo) | ||
|
|
||
| trName := helpers.ObjectNameForTest(t) | ||
| tr := parse.MustParseV1TaskRun(t, fmt.Sprintf(` | ||
| metadata: | ||
| name: %s | ||
| namespace: %s | ||
| spec: | ||
| taskRef: | ||
| resolver: git | ||
| params: | ||
| - name: url | ||
| value: %s | ||
| - name: revision | ||
| value: %s | ||
| - name: pathInRepo | ||
| value: %s | ||
| - name: gitToken | ||
| value: %s | ||
| - name: gitTokenKey | ||
| value: %s | ||
| `, | ||
| trName, | ||
| namespace, | ||
| requestUrl, | ||
| scmRemoteBranch, | ||
| scmRemoteTaskPath, | ||
| tokenSecretName, | ||
| scmTokenSecretKey, | ||
| )) | ||
|
|
||
| _, err := c.V1TaskRunClient.Create(ctx, tr, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| t.Fatalf("Failed to create TaskRun: %v", err) | ||
| } | ||
|
|
||
| t.Logf("Waiting for TaskRun %s in namespace %s to complete", trName, namespace) | ||
| if err := WaitForTaskRunState(ctx, c, trName, TaskRunSucceed(trName), "TaskRunSuccess", v1Version); err != nil { | ||
| t.Fatalf("Error waiting for TaskRun %s to finish: %s", trName, err) | ||
| } | ||
| } | ||
|
|
||
| func TestGitResolver_API(t *testing.T) { | ||
| ctx := t.Context() | ||
| c, namespace := setup(ctx, t, gitFeatureFlags) | ||
|
|
@@ -699,7 +752,7 @@ spec: | |
| }, | ||
| Type: corev1.SecretTypeOpaque, | ||
| Data: map[string][]byte{ | ||
| scmTokenSecretKey: []byte(base64.StdEncoding.Strict().EncodeToString([]byte(token))), | ||
| scmTokenSecretKey: []byte(token), | ||
|
Comment on lines
-702
to
+755
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure why this was base64 encoded on the application side. When I made the gitea repo private, API auth also broke until I removed this base64 encoding. I think it was mistakenly though to be required, however the base64 encoding looks to be happening server-side after the secret is created. |
||
| }, | ||
| }, metav1.CreateOptions{}) | ||
|
|
||
|
|
@@ -743,6 +796,7 @@ spec: | |
|
|
||
| _, _, err = giteaClient.CreateOrgRepo(scmRemoteOrg, gitea.CreateRepoOption{ | ||
| Name: scmRemoteRepo, | ||
| Private: true, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
| AutoInit: true, | ||
| DefaultBranch: scmRemoteBranch, | ||
| }) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
tokenandtokenKeyfields are allowed here, however they're used for API-based authentication not http auth. So they're semantically incorrect to include with theurlparameter.gitTokenandgitTokenKeyis correct.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure but people may have used it previously, (if it ever got shipped in a working state)