fix: resolve credentials for protected HTTP(S) repositories#3637
fix: resolve credentials for protected HTTP(S) repositories#3637Ankitsinghsisodya wants to merge 3 commits intoknative:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ankitsinghsisodya 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 |
knative-prow
Bot
added
the
size/L
|
Hi @Ankitsinghsisodya. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
knative-prow
Bot
added
the
needs-ok-to-test
Ankitsinghsisodya
force-pushed
the
fix/git-credential-helper
branch
2 times, most recently
from
ce89b19 to
f3791f9
Compare
There was a problem hiding this comment.
Pull request overview
Adds HTTP(S) credential resolution for protected template repositories by retrying go-git clones with BasicAuth sourced from ~/.git-credentials and ~/.netrc, addressing cases where go-git doesn’t consult system credential helpers.
Changes:
- Retry
git.Clone/git.PlainCloneontransport.ErrAuthenticationRequiredusing credentials resolved from~/.git-credentials, then~/.netrc. - Implement pure-Go parsers for git credential-store lines and a minimal netrc scanner.
- Add unit tests covering core
credentialsForURLbehavior andisAuthError.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
pkg/functions/repository.go |
Adds credential lookup + auth-retry logic for in-memory and disk clones; introduces git-credentials and netrc parsing helpers. |
pkg/functions/repository_credentials_test.go |
Adds tests for credential resolution and auth-error detection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // TestCredentialsForURL_HTTPS verifies that credentials stored in | ||
| // ~/.git-credentials are returned for a matching HTTPS URL. | ||
| func TestCredentialsForURL_HTTPS(t *testing.T) { |
Ankitsinghsisodya
force-pushed
the
fix/git-credential-helper
branch
from
f3791f9 to
20d0d3c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3637 +/- ##
==========================================
+ Coverage 56.61% 56.66% +0.05%
==========================================
Files 181 181
Lines 20853 20992 +139
==========================================
+ Hits 11806 11896 +90
- Misses 7833 7871 +38
- Partials 1214 1225 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Nice addition! One thing: it appears you are matching only host, not port. Port also should match (with possible exception of implicit ports 80 and 443). On the other hand scheme matching seems to be too restrictive. Credentials valid for http://localhost should match https://localhost in my opinion. Examples: |
Ankitsinghsisodya
force-pushed
the
fix/git-credential-helper
branch
from
20d0d3c to
ea9b27d
Compare
Done sir |
| // scanNetRC is a minimal netrc(5) token scanner. It returns the login and | ||
| // password for the first "machine" stanza whose name matches host. Falls back | ||
| // to the "default" stanza when no exact match is found. | ||
| func scanNetRC(content, host string) (login, password string) { |
There was a problem hiding this comment.
btw would it be worth simply adding a ignore case for line starting w #? AFAIK netrc does not officially support comments but apparently programs like curl do actually mention that its ok to use and they do ignore them. Python has some issues i found python/cpython#104511 but also supports them.
So maybe adding that would make this code a bit more robust. just a thought
cc @matejvasek
There was a problem hiding this comment.
ignore case for line starting w #
you mean skip lines starting with # ?
First time I read that I thought you wanted to turn down case sensitivity 😄
Yes we should skip lines starting with # IMO
There was a problem hiding this comment.
I have handled the problem. scanNetRC skips lines starting with # before tokenising... I have also added a test TestScanNetRC_CommentLinesIgnored to cover it...
Ankitsinghsisodya
force-pushed
the
fix/git-credential-helper
branch
from
ea9b27d to
8c8c32d
Compare
knative-prow
Bot
added
size/XL
When --repository points to a protected HTTP(S) remote, go-git's pure-Go clone had no access to the system credential stack, causing authentication failures even when the user had credentials configured via ~/.git-credentials or ~/.netrc. Adds credentialsForURL which reads credentials from, in order: 1. ~/.git-credentials (git credential store helper format) 2. ~/.netrc The retry follows git's own challenge/response model: an anonymous clone is attempted first; credentialsForURL is only called when the server responds with HTTP 401 (transport.ErrAuthenticationRequired). This avoids sending credentials to servers that do not require them. No subprocesses are spawned and no git binary on PATH is required. Fixes knative#3415 Signed-off-by: Ankitsinghsisodya <ankitsingh24012005@gmail.com>
Introduces new tests to verify credential matching behavior for different URL schemes and port configurations. The tests ensure that: - Credentials stored under `http://` are returned for `https://` requests and vice-versa. - Credentials with explicit non-standard ports do not match URLs on different ports. - Credentials with explicit ports match requests with the same port. - Implicit default ports (80 for http, 443 for https) are treated as equivalent to no port. These changes enhance the robustness of the credential handling logic in the repository.
Introduces new tests for the scanNetRC function to verify its behavior when parsing .netrc files. The tests ensure that: - The first matching machine stanza is returned when multiple stanzas match the same host. - Lines beginning with '#' are correctly ignored as comments during parsing. These additions enhance the reliability of the credential retrieval process from .netrc files.
Ankitsinghsisodya
force-pushed
the
fix/git-credential-helper
branch
from
8c8c32d to
dd8c16e
Compare
Closes #3415
Summary
ErrAuthenticationRequired, retry using credentials sourced from~/.git-credentials(git credential store format) and~/.netrc.FilesystemFromRepo(in-memory clone) andRepository.Write(disk clone) are covered.Problem
kn func create --repository <internal-gitlab-url>failed withauthentication requiredeven when the user hadcredential.helper = storeconfigured and~/.git-credentialspopulated, because go-git does not consult the system credential helpers on its own.Approach
ErrAuthenticationRequired, callcredentialsForURLwhich reads, in order:~/.git-credentials— matches on scheme + hostname~/.netrc— matches on hostname, falls back todefaultstanzaBasicAuth.Testing
credentialsForURL,credentialsFromGitStore,credentialsFromNetRC,scanNetRC, andisAuthErroradded inpkg/functions/repository_credentials_test.go.Notes
This fix targets
credential.helper = store(flat-file) users. SSH and system keychain helpers are out of scope for this change.