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.

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

Jump to bottom

Merged

tekton-robot merged 2 commits into tektoncd:main from aThorp96:git-resolver-http-auth-fix

Aug 4, 2025

Member

aThorp96 commented Jul 31, 2025 •

edited

Loading

Changes

The header specified for the git resolver's http auth contained a typo
leading to authentication tokens not being properly sent to the remote.
This change fixes the typo in the header and also ensures the auth
setting is present for all git operations instead of just clone, since other
operations such as fetch may require remote authentication.

Additionally, this change fixes the outdated token-authenticated git
resolver example and adds an e2e regression test for authenticated git
cloning.

Resolves: #8940
Resolves: https://issues.redhat.com/browse/SRVKP-8260

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
pre-commit Passed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Bug fix: Before this change, there was a regression in which the git resolver was not authenticating with the provided `gitToken` and `gitTokenKey`, breaking the git resolver's http token-based auth. After this change, all git operations performed by the git resolver use the provided `gitToken` for remote authentication.

github-project-automation bot added this to Tekton Community Roadmap

github-project-automation bot moved this to Todo in Tekton Community Roadmap

tekton-robot added release-note-none do-not-merge/work-in-progress size/M labels

tekton-robot requested review from dibyom and vdemeester

July 31, 2025 20:27

aThorp96 commented

View reviewed changes

pkg/resolution/resolver/git/repository.go

    
              		token := base64.URLEncoding.EncodeToString([]byte(repo.username + ":" + repo.password))

              		env = append(

              			env,

              			"GIT_AUTH_HEADER=Authorization: Basic "+token,

Member Author

aThorp96 Jul 31, 2025

Authorization: Basic .... vs Authorization=Basic ... appears to have been the issue @vdemeester

aThorp96 commented

View reviewed changes

test/resolvers_test.go

    
              	}

              }

              func TestGitResolver_HTTPAuth(t *testing.T) {

Member Author

aThorp96 Jul 31, 2025

WIP regression test here

Collaborator

tekton-robot commented Jul 31, 2025

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/resolution/resolver/git/repository.go	84.9%	84.6%	-0.3

Member

waveywaves commented Jul 31, 2025

/kind bug

tekton-robot added the kind/bug label

aThorp96 force-pushed the git-resolver-http-auth-fix branch from acb78f0 to 7deb329 Compare

August 1, 2025 16:27

Collaborator

tekton-robot commented Aug 1, 2025

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/resolution/resolver/git/repository.go	84.9%	84.6%	-0.3

aThorp96 force-pushed the git-resolver-http-auth-fix branch from 7deb329 to 41fdbc9 Compare

August 1, 2025 18:40

Collaborator

tekton-robot commented Aug 1, 2025

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/resolution/resolver/git/repository.go	84.9%	84.6%	-0.3

aThorp96 force-pushed the git-resolver-http-auth-fix branch from 41fdbc9 to 217779c Compare

August 1, 2025 19:11

Collaborator

tekton-robot commented Aug 1, 2025

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/resolution/resolver/git/repository.go	84.9%	84.6%	-0.3

Collaborator

tekton-robot commented Aug 1, 2025

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/resolution/resolver/git/repository.go	84.9%	84.6%	-0.3

Collaborator

tekton-robot commented Aug 2, 2025

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/resolution/resolver/git/repository.go	84.9%	84.6%	-0.3

aThorp96 force-pushed the git-resolver-http-auth-fix branch from cb051af to 63472b1 Compare

August 2, 2025 17:14

aThorp96 added 2 commits

August 2, 2025 13:14


          fix(tektoncd#8940): Fix token-authentication header in git resolver

a657dd9

The header specified for the git resolver's http auth contained a typo
leading to authentication tokens not being properly sent to the remote.

Additionally, this change fixes the outdated token-authenticated git
resolver example and adds an e2e regression test for authenticated git
cloning.

Resolves: tektoncd#8940
Resolves: https://issues.redhat.com/browse/SRVKP-8260


          chore: fix unit test action name

e25050f

aThorp96 force-pushed the git-resolver-http-auth-fix branch from 63472b1 to e25050f Compare

August 2, 2025 17:18

aThorp96 changed the title ~~Fix http authentication for git resolver~~ fix(#8940): token-authentication header typo in git resolver

Collaborator

tekton-robot commented Aug 2, 2025

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/resolution/resolver/git/repository.go	84.9%	84.6%	-0.3

tekton-robot added release-note and removed release-note-none labels

aThorp96 marked this pull request as ready for review

August 2, 2025 17:26

tekton-robot removed the do-not-merge/work-in-progress label

aThorp96 commented

View reviewed changes

test/resolvers_test.go

Comment on lines -702 to +755

    
              			scmTokenSecretKey: []byte(base64.StdEncoding.Strict().EncodeToString([]byte(token))),

              			scmTokenSecretKey: []byte(token),

Member Author

aThorp96 Aug 2, 2025

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.

Member Author

aThorp96 commented Aug 2, 2025

/cc @waveywaves

tekton-robot requested a review from waveywaves

August 2, 2025 17:38

mbpavan commented Aug 4, 2025

/retest

1 similar comment

Member

waveywaves commented Aug 4, 2025

/retest

Member

chmouel commented Aug 4, 2025

/lgtm

but consider the renaming issue

Collaborator

tekton-robot commented Aug 4, 2025

@chmouel: changing LGTM is restricted to collaborators

In response to this:

/lgtm

but consider the renaming issue

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

waveywaves reviewed

View reviewed changes

test/resolvers_test.go

    
              		_, _, err = giteaClient.CreateOrgRepo(scmRemoteOrg, gitea.CreateRepoOption{

              			Name:          scmRemoteRepo,

              			Private:       true,

Member

waveywaves Aug 4, 2025

👍

Member

waveywaves commented Aug 4, 2025

/approve

tekton-robot added the approved label

Member

waveywaves commented Aug 4, 2025

@vdemeester, will wait for the last lgtm from you. Once this is in, we will make sure to cherry-pick it to 1.0.x.

/cherry-pick release-v1.0.x

Collaborator

tekton-robot commented Aug 4, 2025

@waveywaves: once the present PR merges, I will cherry-pick it on top of release-v1.0.x in a new PR and assign it to you.

In response to this:

@vdemeester, will wait for the last lgtm from you. Once this is in, we will make sure to cherry-pick it to 1.0.x.

/cherry-pick release-v1.0.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vdemeester approved these changes

View reviewed changes

Member

vdemeester left a comment

/lgtm

tekton-robot assigned vdemeester

tekton-robot added the lgtm label

Collaborator

tekton-robot commented Aug 4, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester, waveywaves

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vdemeester,waveywaves]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot merged commit cbc6c14 into tektoncd:main

48 of 50 checks passed

github-project-automation bot moved this from Todo to Done in Tekton Community Roadmap

Collaborator

tekton-robot commented Aug 4, 2025

@waveywaves: new pull request created: #8941

In response to this:

@vdemeester, will wait for the last lgtm from you. Once this is in, we will make sure to cherry-pick it to 1.0.x.

/cherry-pick release-v1.0.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot mentioned this pull request

[release-v1.0.x] fix(#8940): token-authentication header typo in git resolver #8941

Closed

Member

chmouel commented Aug 4, 2025 •

edited

Loading

I don't think my comment about renaming field without a migration plan was addressed before merging...

(and it doesn't encourage to do any reviews on tektoncd/pipeline tbh)

Member

waveywaves commented Aug 4, 2025

@chmouel yes, I didn't see it addressed as well. I will see it addressed and also have the necessary change cherry picked as well as required. Guess the comment got glossed over in the interest of getting this fix through.

Member Author

aThorp96 commented Aug 4, 2025 •

edited

Loading

I don't think my comment about renaming field without a migration plan was addressed before merging...

@chmouel Sorry for not addressing your comment properly. There is no user action or migration necessary and no fields have changed in this PR. The example's field was corrected, since it was using the api auth fields with a non-api git resolution request, but the way the git resolver is used remains the same in this change.

Member

waveywaves commented Aug 5, 2025

@aThorp96 we need to backport this to versions of tekton where this fix is needed. I have already created a cherry pick for 1.0.x, creating more for 1.1.x and 1.2.x and this is already in 1.3.x. Let me know if you could look into the cherry picks and see if it would be possible for you to review them. Would appreciate it. I can take a look at them later.

Member

waveywaves commented Aug 5, 2025

/cherry-pick release-v1.1.x

Member

waveywaves commented Aug 5, 2025

/cherry-pick release-v1.2.x

Collaborator

tekton-robot commented Aug 5, 2025

@waveywaves: new pull request created: #8946

In response to this:

/cherry-pick release-v1.1.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot mentioned this pull request

[release-v1.1.x] fix(#8940): token-authentication header typo in git resolver #8946

Closed

Collaborator

tekton-robot commented Aug 5, 2025

@waveywaves: #8937 failed to apply on top of branch "release-v1.2.x":

Patch is empty.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-v1.2.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

aThorp96 mentioned this pull request

[release-v1.0.x] fix(#8940): token-authentication header typo in git resolver #8947

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

waveywaves waveywaves left review comments

vdemeester vdemeester approved these changes

dibyom Awaiting requested review from dibyom

afrittoli Awaiting requested review from afrittoli

+1 more reviewer

chmouel chmouel left review comments

Labels

approved kind/bug lgtm release-note size/M