-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix: Add useHttpPath to support multiple Git credentials on same host #9143
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
base: main
Are you sure you want to change the base?
Fix: Add useHttpPath to support multiple Git credentials on same host #9143
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@ab-ghosh: The label(s) DetailsIn response to this:
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. |
|
/kind bug |
d1e3d33 to
b5254f2
Compare
b5254f2 to
5788891
Compare
aThorp96
left a comment
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.
Thanks @ab-ghosh! The useHttpPath setting seems like an elegant fix, I like it. I think we need to consider further when it's used, but in general I like it. See my comment for details on the issue with using it in every case
| // IMPORTANT: Git credential contexts with useHttpPath=true are required | ||
| // for repository-specific credentials on the same host to work correctly. | ||
| // Without this, Git only matches by host and uses the first credential found. | ||
| return fmt.Sprintf("[credential %q]\n username = %s\n useHttpPath = true\n", u, be.escapedUsername()) |
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.
This works in the case where each repository has different secrets, but this would break the existing behavior if one secret is authorized on multiple repositories.
For example if I have the following gitconfig with , the credential will not be used if I'm cloning https://gitea.com/test/repo2.
# ~/.gitconfig
[credential]
helper = store
# password in ~/.git-credentials
[credential "https://gitea.com/test/repo1"]
username = user1
useHttpPath = true
We should only set useHttpPath = true when the basicEntry shares a hostname with other basicEntrys in the basicGitConfig. Something like this should work okay to ensure that when a credential entry references the same host as another entry both entries will have useHttpPath = true, but otherwise all credential entries would omit the useHttpPath setting entirely
multiRepoHosts := []string{}
seenHosts := []string{}
for k, v := range dc.entries {
if slices.Contains(seenHosts, v.authURL.Host) {
multiRepoHosts = append(multiRepoHosts, k)
} else {
seenHosts = append(seenHosts, v.authURL.Host)
}
}
for _, k := range dc.order {
v := dc.entries[k]
useHttpPath := slices.Contains(multiRepoHosts, k)
gitConfigs = append(gitConfigs, v.configBlurb(k, useHttpPath))
}That being said, there is another risk which is worth discussing: suppose I have a shared auth key for all my org's repos https://gitea.com/athorp/*, and a second key for some other private repo https://gitea.com/secret/repo. In this case I think the "correct" way to configure my gitConfig would be to
- Set
useHttpPath = trueonly for the standalone repo - Ensure the shared credential was last in the list.
[credential "https://gitea.com/secret/repo"]
username = foo
useHttpPath = true
[credential "https://gitea.com"]
username = athorp
useHttpPath = false
I don't know if we want to get into mixing up the order of the credentials or if we should simply document how the order matters. IMO all useHttpPath = true credentials should be ordered before any useHttpPath = false credentials, but I could see counterarguments. Either way, if we ensure useHttpPath is only set when there is a path on the provided URL, then there is at least a way for the user to configure a shared credential alongside a one-off credential using the secret order and annotations:
apiVersion: v1
kind: Secret
metadata:
name: mutli-repo-secret
annotations:
tekton.dev/git-0: https://gitea.com # <- No path to ensure useHttpPath is not set in the gitconfig
...apiVersion: v1
kind: Secret
metadata:
name: specific-repo-secret
annotations:
tekton.dev/git-0: https://gitea.com/secret/repo.git
...apiVersion: v1
kind: ServiceAccount
metadata:
name: build
secrets:
- name: specific-repo-secret
- name: multi-repo-secret # <- if we don't reorder the credentials, the multi-repo-secret must be listed after the specific-repo-recretThere 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.
Ah that's some very good point @aThorp96 and it feels a bit complicated to handle...
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.
We could split the difference?
If any git basic-auth credential has a path in its tekton.dev/git-x annotation, assume the path is specific and add the useHttpPath setting and leave the ordering and specificity up to the user.
We'll need to document that the (already required) ordering numbers impact credential selection (or does the secret-list order determine that?), and repo-specific credentials should be ordered before host-wide credentials (if both are used side by side.) If a user wants to use multiple credentials which are each authorized for multiple repos, each necessary would regretfully need to be enumerated.
What do you think about this approach, @vdemeester @ab-ghosh ?
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.
Thank you @aThorp96 for the detailed analysis.
The path-based approach looks good to me. It's straightforward as if the annotation URL has a path, we set useHttpPath=true, otherwise we don't. This keeps the implementation simple and gives users explicit control through their URL structure. We can document the ordering behavior and provide clear examples for both single-repo and multi-repo scenarios. I think this strikes the right balance between solving the bug and maintaining backwards compatibility.
Looking forward to hearing @vdemeester thoughts on this
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.
I think that can make sense indeed. From the official Git documentation, credential helpers match in this order:
- Most specific match wins when using credential contexts
- Git processes credential helpers in the order configured
- The last matching helper is used
So we need to be very careful with the order, but I think we can determine it ?
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.
From the https://git-scm.com/book/en/v2/Git-Tools-Credential-Storage:
"When looking for credentials for a particular host, Git will query them in order, and stop after the first answer is provided."
And from https://git-scm.com/docs/gitcredentials:
"If there are multiple instances of the credential.helper configuration variable, each helper will be tried in turn, and may provide a
I misread a bit 🙃 , order is important, and we need to be very clear about this in documentation, and heavily test this. Let's iterate on this 👼🏼
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.
It sounds then like we're in agreement with the next steps:
- if the secret's
tekton.dev/git-\dannotation has a path after the hostname, setuseHttpPathin its credentials entry - test and confirm the necessary order when multiple credentials (one repo-specific credential, one hostname-wide credential) are used:
- Does the order of the secrets in the SA matter, or does the number in the annotation matter? Do they both metter?
- Update the existing documentation to describe the behavior and how to properly order multiple credentials including examples
Changes
This PR fixes a bug where multiple Git repositories on the same host could not use different credentials. When users linked multiple secrets to a ServiceAccount for different repositories on the same Git server (e.g.,
github.com/org/repo1andgithub.com/org/repo2), Tekton would incorrectly use the first credential for all repositories, authentication failures. The root cause was that Git's credential helper only matched by hostname, ignoring the repository path. This fix addsuseHttpPath = trueto Git credential contexts, instructing Git to match credentials based on the full repository URL. The solution is backwards compatible, requires no configuration changes, and includes comprehensive test coverage including a new test specifically for this scenario.TestBasicFlagHandlingMultipleReposSameHost to verify the fix
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes