fix(git): system-git fallback for Azure DevOps repositories (#288) - #292
fix(git): system-git fallback for Azure DevOps repositories (#288)#292consooo wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAzure DevOps remotes now route fetch, push, and repository checks through system git. SSH credentials expose material for subprocess use, the distroless image bundles git and dependencies, and configuration documentation describes ADO credentials and requirements. ChangesAzure DevOps system Git support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SmartFetch
participant systemGitSmartFetch
participant ADORepository
participant LocalRepository
SmartFetch->>systemGitSmartFetch: route ADO remote
systemGitSmartFetch->>ADORepository: ls-remote --symref
ADORepository-->>systemGitSmartFetch: refs and default branch
systemGitSmartFetch->>LocalRepository: git fetch --prune
LocalRepository-->>SmartFetch: updated repository state
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
internal/git/ado_system_git.go (3)
33-38: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSubstring matching can misroute non-ADO remotes.
strings.Containsmatches any URL whose path or hostname merely embeds these strings (e.g.https://git.example.com/mirrors/dev.azure.com-backup.git). Parsing the URL and matching the host suffix would be exact; thessh.dev.azure.comcheck is also redundant withdev.azure.com.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/git/ado_system_git.go` around lines 33 - 38, Update IsADOURL to parse rawURL and validate the parsed hostname rather than using substring checks. Match only hosts equal to dev.azure.com or visualstudio.com, or hosts ending with those domains, and remove the redundant ssh.dev.azure.com condition while preserving false results for embedded path or hostname text.
106-127: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd
-o IdentitiesOnly=yestoGIT_SSH_COMMAND.Without it ssh may still offer agent or default
~/.ssh/id_*identities before the provided key, which makes auth failures confusing and can authenticate with the wrong identity.🔒 Proposed fix
- sshCmd := "ssh -i '" + keyFile + "' -o BatchMode=yes" + sshCmd := "ssh -i '" + keyFile + "' -o IdentitiesOnly=yes -o BatchMode=yes"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/git/ado_system_git.go` around lines 106 - 127, Update the SSH command assembled in the *sshpkg.KeyAuth branch of the systemGitEnv setup to include the IdentitiesOnly=yes option alongside the provided key option, ensuring authentication uses only that key while preserving the existing known-hosts and cleanup behavior.
350-360: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse an empty lease for new branch creation.
When
remoteBranchHashis empty, branch creation runs without a concurrency guard and can clobber a remote branch created by another actor. Use--force-with-lease=<branchShort>:so Git rejects the push if the ref already exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/git/ado_system_git.go` around lines 350 - 360, Update the push argument construction around remoteBranchHash so the empty-hash path also appends --force-with-lease for branchShort with an empty expected remote value. Preserve the existing non-empty remoteBranchHash lease and ensure both new and existing branch pushes use concurrency protection.internal/ssh/auth.go (1)
83-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
crypto.PrivateKeyassertion is a no-op.
crypto.PrivateKeyis defined asany, so the type assertion on Line 94 can never fail — the real type filtering happens insidegossh.MarshalPrivateKey(which supports*rsa.PrivateKey,*ecdsa.PrivateKey,ed25519.PrivateKey, but not the DSA keysParseRawPrivateKeycan also return). Dropping the check keeps the marshal error as the single source of truth.♻️ Proposed simplification
- privKey, ok := rawKey.(crypto.PrivateKey) - if !ok { - return nil, errors.New("parsed SSH key does not implement crypto.PrivateKey") - } - block, err := gossh.MarshalPrivateKey(privKey, "") + block, err := gossh.MarshalPrivateKey(rawKey, "") if err != nil { - return nil, fmt.Errorf("marshal unencrypted SSH private key: %w", err) + return nil, fmt.Errorf("marshal unencrypted SSH private key (unsupported key type?): %w", err) }(
cryptoimport then becomes unused.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ssh/auth.go` around lines 83 - 103, Remove the no-op crypto.PrivateKey type assertion and its associated error handling from decryptPrivateKeyPEM, passing the parsed rawKey directly to gossh.MarshalPrivateKey. Remove the now-unused crypto import and retain MarshalPrivateKey’s error as the sole unsupported-key failure path.Dockerfile (1)
56-77: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a build-time sanity check so a missing shared library fails the build, not the cluster.
ldd ... 2>/dev/null || truesemantics mean a partially-harvested bundle produces an image whosegitonly fails at reconcile time. A quick verification in the same stage catches it during build.♻️ Proposed addition
esac; \ done; \ - done + done; \ + # Fail the build if anything is still unresolved once relocated. + for f in /bundle/bin/git /bundle/bin/ssh; do \ + ldd "$f" | grep -q 'not found' && { echo "missing deps for $f"; exit 1; }; \ + true; \ + done🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 56 - 77, Add a build-time verification step to the existing Dockerfile RUN block after harvesting dependencies, exercising the bundled /bundle/bin/git and /bundle/bin/ssh with the bundled library paths. Make the check fail the image build when either binary cannot start because a required shared library is missing, while preserving the current bundle layout and copy logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/configuration.md`:
- Around line 98-107: Update the recommended Azure DevOps PAT configuration in
the documentation to use Basic authentication with username and password
credentials, representing the PAT as the password. Remove bearerToken from this
PAT example and add or relocate it to a separate Entra-ID/OAuth token example,
while preserving the existing SSH guidance.
In `@internal/git/ado_system_git_test.go`:
- Around line 47-48: Replace the credential-like fixture values in
TestNewSystemGitEnv_BasicAuth and the related token fixture with clearly
non-secret test strings, updating every corresponding assertion including the
check near the BasicAuth password assertion. Keep the test behavior unchanged
while ensuring the GitGuardian scanner no longer classifies the literals as
credentials.
- Around line 240-254: Update assertSSHFilesNotWorldReadable to normalize shell
quoting from each sshCmd field before filtering and calling os.Stat, so
single-quoted /tmp/reverser-git paths resolve to actual files. Preserve the
existing path filtering and permission assertion, but do not silently skip stat
failures for candidate SSH files; report them through the test instead.
In `@internal/git/ado_system_git.go`:
- Around line 76-104: Update both the *http.BasicAuth and *http.TokenAuth
branches in the systemGitEnv construction to scope the git configuration’s
extraHeader to repoURL instead of using the global [http] section. Use the
already available repoURL when formatting each config, while preserving the
existing authorization values, file permissions, cleanup behavior, and
environment setup.
In `@internal/git/git_smart_fetch.go`:
- Around line 37-39: Guard all origin URL access against an empty URLs slice by
adding and reusing a helper such as originURL(repo) (string, error). In
internal/git/git_smart_fetch.go#L37-L39 and
internal/git/git_atomic_push.go#L208-L211, only perform ADO routing when an
origin URL exists; otherwise preserve the non-ADO/error path. In
internal/git/ado_system_git.go#L229-L233 within systemGitSmartFetch and
`#L309-L313` within systemGitPushAtomic, return a wrapped error when origin has no
URL instead of indexing the slice.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 56-77: Add a build-time verification step to the existing
Dockerfile RUN block after harvesting dependencies, exercising the bundled
/bundle/bin/git and /bundle/bin/ssh with the bundled library paths. Make the
check fail the image build when either binary cannot start because a required
shared library is missing, while preserving the current bundle layout and copy
logic.
In `@internal/git/ado_system_git.go`:
- Around line 33-38: Update IsADOURL to parse rawURL and validate the parsed
hostname rather than using substring checks. Match only hosts equal to
dev.azure.com or visualstudio.com, or hosts ending with those domains, and
remove the redundant ssh.dev.azure.com condition while preserving false results
for embedded path or hostname text.
- Around line 106-127: Update the SSH command assembled in the *sshpkg.KeyAuth
branch of the systemGitEnv setup to include the IdentitiesOnly=yes option
alongside the provided key option, ensuring authentication uses only that key
while preserving the existing known-hosts and cleanup behavior.
- Around line 350-360: Update the push argument construction around
remoteBranchHash so the empty-hash path also appends --force-with-lease for
branchShort with an empty expected remote value. Preserve the existing non-empty
remoteBranchHash lease and ensure both new and existing branch pushes use
concurrency protection.
In `@internal/ssh/auth.go`:
- Around line 83-103: Remove the no-op crypto.PrivateKey type assertion and its
associated error handling from decryptPrivateKeyPEM, passing the parsed rawKey
directly to gossh.MarshalPrivateKey. Remove the now-unused crypto import and
retain MarshalPrivateKey’s error as the sole unsupported-key failure path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5941da54-d743-49dc-9528-cbe7c6d1c604
📒 Files selected for processing (9)
Dockerfiledocs/configuration.mdinternal/git/ado_system_git.gointernal/git/ado_system_git_test.gointernal/git/credentials_test.gointernal/git/git.gointernal/git/git_atomic_push.gointernal/git/git_smart_fetch.gointernal/ssh/auth.go
4bc0aca to
2ac481c
Compare
|
Thanks @consooo for raising this! This is one of these things that you don't find out if you don't try it. It's a bit of a bummer tbh that it doesnt work with ADO so that really requires a fix. I'm going to be honest: I dislike calling git as external tool: go-git is so elegent as in that it's not going to require specials in the Dockerfile for example. Did you consider to live on the edge and try go-git version 6? I have so many tests that I would be willing to take the risk on working with a beta release of their side: especially if that would give a more clean approach. I'm not saying no directly: but it's a thing that I would like to weigh as well! And another though that pops up: I remember that I took the time to really make it effecient by diving a bit deeper in the git specifics, it has been a few months since then so I do need some time to see why that was. Perhaps we can also add a flag to bypass the advacned pushing: and just do a more simple out of the box push. One would hope that it then works as well: it simply can't be true that ADO doesnt work with go-git v5 can it? |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Thanks for the quick reply @sunib. Yeah, I was quite surprised as well. I tested with a private github repo and everything was fine. Switching to ADO, I encountered these weird 400s. This seems to be a well known annoying shortcoming, I would put the blame squarely on ADO. It's been there for 6 years!
I agree, this is not ideal. Most projects add a workaround either using system git or switch to or add git2go lib. I also considered just using go-git v6 or switching to git2go. But both require quite a bit of refactoring and go-git v6 has been in alpha for a while. There is no telling when it will be stable and it will lead to lots of work adapting to API changes. It might not be too bad if we simplified the git flow but with the current status, I think this would be quite the hassle.
Even a System git is the least painful way imho. Its only used in the ADO path, not for other repos. I noted that this is not the correct fix. The correct fix is to lift to v6 and this is only a temporary fix until it is released. Adding these binaries is not nice. But I think its preferable to doing a migration to go-git v6 and then keeping up with the changes until a stable version is released. This would be much more work. |
2ac481c to
59ed890
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
internal/git/ado_system_git_test.go (1)
92-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the negative case for the token guard, and consider coverage for the three ADO operations.
newSystemGitEnvrejects tokens containing\n/\r(ado_system_git.goLines 96-99) but no test exercises it, andsystemGitSmartFetch/systemGitPushAtomic/systemGitCheckRepoare entirely untested — a local bare repo asoriginplus a stubbedIsADOURL-matching URL would cover the ls-remote/refspec/force-with-lease logic.💚 Suggested test
func TestNewSystemGitEnv_TokenAuth_RejectsNewline(t *testing.T) { if _, err := newSystemGitEnv( "https://dev.azure.com/org/proj/_git/repo", &http.TokenAuth{Token: "test-token\nextraHeader: injected"}, ); err == nil { t.Fatal("expected error for token containing a newline") } }As per coding guidelines: "Cover new code with positive and negative tests, add integration tests for complex workflows".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/git/ado_system_git_test.go` around lines 92 - 130, The tests cover successful token authentication but not the newline-injection guard or the ADO Git operation workflows. Add a negative test alongside TestNewSystemGitEnv_TokenAuth that verifies newSystemGitEnv rejects tokens containing newline or carriage-return characters, and add integration coverage for systemGitSmartFetch, systemGitPushAtomic, and systemGitCheckRepo using a local bare repository as origin with an IsADOURL-matching URL to exercise ls-remote, refspec, and force-with-lease behavior.Source: Coding guidelines
internal/ssh/auth.go (1)
94-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead type assertion —
crypto.PrivateKeyisany.
rawKey.(crypto.PrivateKey)can never fail, so the error branch is unreachable and thecryptoimport exists only for it. Drop it and letMarshalPrivateKeyreport unsupported types.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ssh/auth.go` around lines 94 - 97, In the key handling flow, remove the `crypto.PrivateKey` type assertion and its unreachable error branch, along with the now-unused `crypto` import. Pass `rawKey` directly to `MarshalPrivateKey` so that function reports unsupported key types.Dockerfile (1)
58-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider
scanelf/apk infooverlddscraping, and note the attack-surface tradeoff.The
ldd-plus-awkharvest works but silently tolerates copy failures (|| true), so a missing dependency surfaces only at runtime.scanelf --needed --nobanneror lettingapkresolve the closure into a staging root is more deterministic. Separately, bundling git, ssh and a busybox shell removes most of the benefit of a distroless base — worth documenting the tradeoff (and the removal trigger once go-git v6 is stable) next to this stage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 58 - 77, Replace the ldd/awk dependency harvesting in the bundle stage with deterministic dependency resolution using scanelf --needed --nobanner or apk’s staging-root closure, and stop silently ignoring missing dependency copies. Add a concise note beside this stage documenting the expanded attack surface from bundling git, ssh, and a busybox shell, including the removal trigger when go-git v6 is stable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/git/ado_system_git.go`:
- Around line 59-60: Update the doc comment for the repoURL parameter near the
URL-scoped http section to remove the stale statement that it is not used by the
current implementation, while retaining the forward-compatibility and per-host
credential-scoping context.
- Around line 167-169: Sanitize credential-bearing URLs before constructing the
error in the cmd.Run failure path of the relevant git execution method. Redact
URL userinfo, including embedded ADO PATs, from both args and stderr before
passing them to fmt.Errorf, while preserving the existing error context and
wrapping behavior.
- Around line 34-38: Update IsADOURL to parse the input as a URL, require an
https or ssh scheme, and match dev.azure.com or visualstudio.com only against
the parsed hostname rather than arbitrary substrings; return false for malformed
URLs and unsafe values such as ext::sh and option-prefixed strings. Add the two
specified negative cases to TestIsADOURL, and ensure systemGitCheckRepo does not
pass rejected raw repoURL values to git.
In `@internal/ssh/auth.go`:
- Around line 68-71: Update the SSH authentication construction around
decryptPrivateKeyPEM so it does not re-serialize decrypted keys through
gossh.MarshalPrivateKey for every provider. Preserve the parsed key/signer for
go-git-based providers, and perform raw unencrypted PEM serialization only in
newSystemGitEnv or the system-git fallback that requires it, allowing supported
and unsupported key types to continue through other SSH providers.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 58-77: Replace the ldd/awk dependency harvesting in the bundle
stage with deterministic dependency resolution using scanelf --needed --nobanner
or apk’s staging-root closure, and stop silently ignoring missing dependency
copies. Add a concise note beside this stage documenting the expanded attack
surface from bundling git, ssh, and a busybox shell, including the removal
trigger when go-git v6 is stable.
In `@internal/git/ado_system_git_test.go`:
- Around line 92-130: The tests cover successful token authentication but not
the newline-injection guard or the ADO Git operation workflows. Add a negative
test alongside TestNewSystemGitEnv_TokenAuth that verifies newSystemGitEnv
rejects tokens containing newline or carriage-return characters, and add
integration coverage for systemGitSmartFetch, systemGitPushAtomic, and
systemGitCheckRepo using a local bare repository as origin with an
IsADOURL-matching URL to exercise ls-remote, refspec, and force-with-lease
behavior.
In `@internal/ssh/auth.go`:
- Around line 94-97: In the key handling flow, remove the `crypto.PrivateKey`
type assertion and its unreachable error branch, along with the now-unused
`crypto` import. Pass `rawKey` directly to `MarshalPrivateKey` so that function
reports unsupported key types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 85721021-e4d2-40eb-bb40-832cb3147c93
📒 Files selected for processing (9)
Dockerfiledocs/configuration.mdinternal/git/ado_system_git.gointernal/git/ado_system_git_test.gointernal/git/credentials_test.gointernal/git/git.gointernal/git/git_atomic_push.gointernal/git/git_smart_fetch.gointernal/ssh/auth.go
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/configuration.md
| unencryptedPEM, err := decryptPrivateKeyPEM([]byte(privateKey), []byte(password)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to prepare SSH private key for system-git: %w", err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm which key types x/crypto/ssh MarshalPrivateKey accepts in the pinned version.
ver=$(rg -N 'golang.org/x/crypto' go.mod | head -1)
echo "go.mod: $ver"
fd -t f 'go.sum' -x rg -N 'golang.org/x/crypto v' {} \; | head -3Repository: ConfigButler/gitops-reverser
Length of output: 472
🌐 Web query:
golang.org/x/crypto/ssh MarshalPrivateKey supported key types ed25519.PrivateKey value pointer
💡 Result:
The golang.org/x/crypto/ssh package supports marshaling ed25519.PrivateKey to the OpenSSH format using the MarshalPrivateKey function [1][2][3]. Regarding the ed25519.PrivateKey value pointer: The MarshalPrivateKey function accepts an argument of type crypto.PrivateKey, which is an interface [1][2]. While the standard representation of an Ed25519 private key in Go's crypto/ed25519 package is the type ed25519.PrivateKey (which is a byte slice), the implementation of MarshalPrivateKey in golang.org/x/crypto/ssh is designed to handle this type [4][5]. Specifically, the implementation internally handles Ed25519 keys by checking for the ed25519.PrivateKey type [5]. Some versions of the code explicitly handle pointers to these keys—for instance, by dereferencing them if provided as *ed25519.PrivateKey—to ensure compatibility with the required structure [5]. You can pass an ed25519.PrivateKey (the underlying byte slice) directly to MarshalPrivateKey, and the library will successfully marshal it into the OpenSSH format [4][5]. In summary: - MarshalPrivateKey supports ed25519.PrivateKey [4][5]. - You do not need to manually wrap the key in a pointer if you already have the standard ed25519.PrivateKey type, but if you do provide a pointer (*ed25519.PrivateKey), the library's marshaling logic is equipped to handle it correctly [5].
Citations:
- 1: https://go.googlesource.com/crypto/+/master/ssh/keys.go
- 2: https://godocs.io/golang.org/x/crypto/ssh
- 3: https://go.googlesource.com/crypto/+/abbc44d451a6f9236a2bbd26cbcd4d0fec473da3/ssh/keys.go
- 4: https://github.com/golang/crypto/blob/master/ssh/keys_test.go
- 5: openshift/golang-crypto@d359caa
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- locate auth.go and related files ---\n'
fd -t f 'auth\.go' .
printf '\n--- outline internal/ssh/auth.go ---\n'
ast-grep outline internal/ssh/auth.go --view expanded || true
printf '\n--- relevant auth.go sections ---\n'
sed -n '1,180p' internal/ssh/auth.go
printf '\n--- search NewSSHKeyAuth and decryptPrivateKeyPEM usages ---\n'
rg -n "NewSSHKeyAuth|decryptPrivateKeyPEM|PrivateKeyPEM|newSystemGitEnv" internal .Repository: ConfigButler/gitops-reverser
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate auth.go and related files ---'
fd -t f 'auth\.go' .
echo
echo '--- outline internal/ssh/auth.go ---'
ast-grep outline internal/ssh/auth.go --view expanded || true
echo
echo '--- relevant auth.go sections ---'
sed -n '1,220p' internal/ssh/auth.go
echo
echo '--- search NewSSHKeyAuth and decryptPrivateKeyPEM usages ---'
rg -n "NewSSHKeyAuth|decryptPrivateKeyPEM|PrivateKeyPEM|newSystemGitEnv" internal .Repository: ConfigButler/gitops-reverser
Length of output: 14026
🌐 Web query:
golang.org/x/crypto v0.54.0 ssh keys.go MarshalPrivateKey ed25519.PrivateKey dsa PrivateKey
💡 Result:
In the golang.org/x/crypto/ssh package (version v0.54.0), the MarshalPrivateKey function is used to serialize private keys into the OpenSSH format [1][2][3]. This function supports various key types, including ed25519.PrivateKey [1][4]. Regarding DSA (*dsa.PrivateKey): DSA is considered an obsolete and insecure algorithm [5]. In modern versions of the golang.org/x/crypto/ssh package, support for DSA has been significantly restricted [6][5]. While ParseDSAPrivateKey remains available for legacy purposes (marked as deprecated), general support for DSA keys in the package has been phased out or made opt-in to improve security [6][5]. Specifically, ParseRawPrivateKey and ParseRawPrivateKeyWithPassphrase no longer support DSA keys directly for automated parsing [6]. To use DSA, a developer must now explicitly use ParseDSAPrivateKey and then use NewSignerFromKey [6]. Furthermore, DSA is no longer included in default algorithm sets, meaning its use must be explicitly configured on both client and server sides if required [6]. In summary, while Ed25519 is fully supported for marshalling, DSA support is deprecated and requires explicit handling due to its security status [6][5].
Citations:
- 1: https://go.googlesource.com/crypto/+/master/ssh/keys.go
- 2: https://go.googlesource.com/crypto.git/+/refs/tags/v0.26.0/ssh/keys.go
- 3: https://pkg.go.dev/golang.org/x/crypto/ssh
- 4: https://go.googlesource.com/crypto/+/e052873987615dc96fe67607a9a6adb76311344f/ssh/keys_test.go
- 5: crypto/dsa: deprecate and remove from crypto/x509 and x/crypto/ssh golang/go#40337
- 6: x/crypto/ssh: ParseDSAPrivateKey lacks validation for DSA parameters golang/go#80418
Move unencrypted key re-serialization out of every SSH auth construction.
decryptPrivateKeyPEM runs for all remote providers and calls gossh.MarshalPrivateKey, which drops *rsa.PrivateKey, *ecdsa.PrivateKey, ed25519.PrivateKey, and rejects unsupported key types such as *dsa.PrivateKey. Since only the ADO system-git fallback needs raw unencrypted PEM bytes, defer this failure to newSystemGitEnv/system-git so go-git-based SSH providers keep working.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/ssh/auth.go` around lines 68 - 71, Update the SSH authentication
construction around decryptPrivateKeyPEM so it does not re-serialize decrypted
keys through gossh.MarshalPrivateKey for every provider. Preserve the parsed
key/signer for go-git-based providers, and perform raw unencrypted PEM
serialization only in newSystemGitEnv or the system-git fallback that requires
it, allowing supported and unsupported key types to continue through other SSH
providers.
…tler#288) go-git v5 strips MultiACK/MultiACKDetailed from its capability advertisement. ADO rejects any upload-pack request that omits multi_ack with HTTP 400 (TF401041: "Clients must support multi-ack."). This change adds a per-URL system-git fallback: any remote whose URL contains dev.azure.com, visualstudio.com, or ssh.dev.azure.com is routed through exec.Command("git", ...) instead of go-git. All other providers continue to use the existing go-git path unchanged. Three entry points now delegate to system git for ADO URLs: - SmartFetch → systemGitSmartFetch (git ls-remote + git fetch) - PushAtomic → systemGitPushAtomic (git ls-remote + git push --force-with-lease) - CheckRepo → systemGitCheckRepo (git ls-remote --symref) Credentials are never passed via argv: - BasicAuth: GIT_CONFIG_GLOBAL with http.extraHeader = Authorization: Basic <base64> - TokenAuth: GIT_CONFIG_GLOBAL with http.extraHeader = Authorization: Bearer <token> - SSH: GIT_SSH_COMMAND with -i <keyfile> and optional known_hosts Dockerfile: adds a git-bundle Alpine stage that collects git and all its musl shared-library dependencies via ldd, then copies them into the distroless final image. The distroless/static:debug base already includes /bin/sh (busybox), which is required to execute the GIT_ASKPASS script. Proper fix: go-git PR #1204 (full multi_ack implementation) is targeted for v6.0.0, which is still in alpha as of 2026-07-28. Drop this fallback and upgrade to go-git v6 once a stable release lands.
59ed890 to
2c5d321
Compare
Thanks for your quick replys and the attention/time that you are putting in this (also for the previous discussion on watches!). I'm running some tests in the background and actually found some interesting material on this, I'm working on a seperate branch to test if v6 is easy to do: if it is then I will take the guess to do some potenial rework on new v6 releases. I've just taken a closer look at their API changes and they are actually pretty carefull. -> and that version just fully suppors ADO out of the box. Normally I would just merge your PR btw, it's great work: but taking these extra Dockerfile deps isn't funny. The image size is 4x bigger, and the security implications are also annoying. Do you have problems with that route? You clearly have already spend time on this: and I might be overseeing things. I will create a PR in a minute, that will also include the more elaborate explanation. |
Sure, you know your repo much better then I do so you can better estimate the impact switching to go-git v6 has. I'd be fine with this too. You're right, that these additional binaries aren't nice, so if you can get go-git v6 to work, it'd be great. I just didn't want to be the one opening that bottle 😄 |
…address the review
Four review findings, all real:
- PrepareBranch pinned the signing policy only on repositories it CREATED.
Worker clones live on a volume across restarts, and a repository made
before the pin existed is the common case on upgrade -- either would hit
go-git v6's "cannot auto-sign commit" the first time an ambient
commit.gpgSign is true. The pin now covers the reuse path too, which is
where it matters most.
- getPushSession indexed remote.Config().URLs[0] unguarded. go-git rejects
a URL-less remote in its own validation, but a hand-edited .git/config
can still present one, and indexing it panics rather than fails.
- The pinned-known_hosts subtest could not tell a pin hit from the default
fallback, because the default set also carries the RSA algorithms. It now
asserts the list is narrower than the default and excludes ed25519, which
only the pin can produce.
- docs/INDEX.md still said "decision needed" after the design record moved
to decided-and-built, and undercounted its own list by one.
Separately, and found by porting #292's Azure DevOps examples: the credential
those examples document did not work. ADO sends a PAT as HTTP basic auth with
the token as the password and ignores the username, so its documented Secret
carries an empty username -- and firstSecretValue treats an empty value as an
absent key, so the basic-auth branch never fired and the Secret was refused
with "does not contain valid authentication data". Pre-existing on v5 too.
The password is what carries the credential, so it is what we branch on now. A
username with no password stays an error, because that one is a real mistake.
Verified against a real Azure DevOps repository, which is what the new opt-in
tests are for. Both skip themselves without a credential, so CI is unchanged:
- internal/git/ado_live_test.go walks the branch-resolution contract against
the real remote in the order the code implements it -- empty repository
resolves to nothing, an absent target falls back to the default, a present
target wins while the default is still fetched as a safety net -- and ends
on the negotiating fetch, the one request go-git v5 cannot make.
- test/e2e/ado_e2e_test.go proves the operator mirrors a live ConfigMap into
a real ADO repository. It reads the result back with canonical git rather
than our own library, so the assertion does not depend on the code under
test.
docs/azure-devops-getting-started.md is written from that e2e recipe, and says
plainly why the connectivity check can pass on a release where every fetch
fails: the advertisement is a different request that never needed multi_ack.
Refs #288
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* feat(git): go-git v6, so Azure DevOps repositories can be fetched at all Azure DevOps rejects any protocol-v0 upload-pack request whose capability list omits multi_ack, with HTTP 400 "TF401041: Clients must support multi-ack." go-git v5 keeps MultiACK and MultiACKDetailed in transport.UnsupportedCapabilities and deletes them from the server's advertisement as it parses, so the capability is never requested and every fetch against ADO fails (#288). v6 implements the capability (go-git#1204), and upstream then deleted their own ADO workaround example saying it "works out of the box". This takes v6.0.0-alpha.5 -- the latest tag, and identical to upstream main -- rather than PR #292's fallback to a bundled system git binary, which measured at +723 MB of image and left the CRITICAL image-scan gate blind to git, OpenSSH and OpenSSL because they arrive as loose files with no package database. The reasoning, the measurements and the four options are in docs/design/azure-devops-multi-ack.md. The blast radius of the ADO problem is one call, repo.Fetch. CheckRepo and listRemoteRefs read only the ref advertisement, and PushAtomic speaks receive-pack, which has no multi_ack at all. The tests assert exactly that, and they passed on v5. Red-first, and it needs no Azure DevOps tenant: canonical git's own upload-pack advertises multi_ack, so git-http-backend behind a proxy that enforces ADO's rule is a faithful simulator for both halves -- the proxy reproduces the rejected request, the real backend reproduces the multi-ACK response v5 also cannot parse. The v2 opt-in header is stripped so a v2-capable client cannot sidestep the capability under test. TestADO_SmartFetch_RequiresMultiAck fails on v5 with ADO's exact 400 and passes on v6. The API migration: - transport.AuthMethod is gone; auth is functional options. A credential now travels as []gitclient.Option, and git.Credential keeps the concrete value alongside so the Secret-key-to-auth-field mapping stays assertable -- the options are closures and cannot be inspected. - transport.NewEndpoint + client.NewClient + NewReceivePackSession become transport.ParseURL + gitclient.New(opts).Handshake. - AdvertisedReferences becomes GetRemoteRefs, returning a slice. - ReceivePack becomes Session.Push. The atomic push keeps its guarantee unchanged: one session serves both the advertisement and the push, and PushRequest.Commands takes the same *packp.Command, so the server-side Old/New compare-and-swap is verbatim. v6 negotiates report-status itself and returns a rejected command as the error from Push, so the separate status inspection collapses into one check, and Atomic is now a first-class field. Two settings v6 reads from the environment that v5 ignored, both failing closed, and neither visible to a unit test: - commit.gpgSign, merged across system, global and local scope, is consulted whenever CommitOptions.Signer is nil, and refuses the commit when set with no signer registered. Any host or image with it set would break every commit we make. PinExplicitSigningPolicy writes the local value false at init: our signing policy comes from the GitProvider, not from ambient config. - HostKeyAlgorithms is derived by reading ~/.ssh/known_hosts and /etc/ssh/ssh_known_hosts whenever ClientConfig returns it empty, even when a HostKeyCallback was supplied, and hard-fails when neither file exists. The controller image is distroless with neither, so every SSH remote would have failed in production regardless of the credential. ssh.KeyAuth now always populates the list, from the pinned known_hosts when there is one and a modern default set otherwise. Found by the e2e suite against a real Gitea; no unit test can reach it, because the fallback lives in the transport's connect rather than in ClientConfig. TestBranchWorker_ConcurrentOperations moves to a real git server. v5's file:// transport spawned the real git-receive-pack; v6 runs go-git's in-process one, whose updateReferences never compares cmd.Old and just sets the reference, so over file:// every racing push wins and the test was passing vacuously with 2 commits instead of 4. Any future test of the compare-and-swap must avoid file://. Closes #288 Refs #292 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(git): accept the Secret Azure DevOps tells people to create, and address the review Four review findings, all real: - PrepareBranch pinned the signing policy only on repositories it CREATED. Worker clones live on a volume across restarts, and a repository made before the pin existed is the common case on upgrade -- either would hit go-git v6's "cannot auto-sign commit" the first time an ambient commit.gpgSign is true. The pin now covers the reuse path too, which is where it matters most. - getPushSession indexed remote.Config().URLs[0] unguarded. go-git rejects a URL-less remote in its own validation, but a hand-edited .git/config can still present one, and indexing it panics rather than fails. - The pinned-known_hosts subtest could not tell a pin hit from the default fallback, because the default set also carries the RSA algorithms. It now asserts the list is narrower than the default and excludes ed25519, which only the pin can produce. - docs/INDEX.md still said "decision needed" after the design record moved to decided-and-built, and undercounted its own list by one. Separately, and found by porting #292's Azure DevOps examples: the credential those examples document did not work. ADO sends a PAT as HTTP basic auth with the token as the password and ignores the username, so its documented Secret carries an empty username -- and firstSecretValue treats an empty value as an absent key, so the basic-auth branch never fired and the Secret was refused with "does not contain valid authentication data". Pre-existing on v5 too. The password is what carries the credential, so it is what we branch on now. A username with no password stays an error, because that one is a real mistake. Verified against a real Azure DevOps repository, which is what the new opt-in tests are for. Both skip themselves without a credential, so CI is unchanged: - internal/git/ado_live_test.go walks the branch-resolution contract against the real remote in the order the code implements it -- empty repository resolves to nothing, an absent target falls back to the default, a present target wins while the default is still fetched as a safety net -- and ends on the negotiating fetch, the one request go-git v5 cannot make. - test/e2e/ado_e2e_test.go proves the operator mirrors a live ConfigMap into a real ADO repository. It reads the result back with canonical git rather than our own library, so the assertion does not depend on the code under test. docs/azure-devops-getting-started.md is written from that e2e recipe, and says plainly why the connectivity check can pass on a release where every fetch fails: the advertisement is a different request that never needed multi_ack. Refs #288 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(facts): what Azure DevOps actually rejects, measured one request at a time I probed ADO with a bare `want <sha>` carrying no capability list, got HTTP 200, and concluded that the six-year-old bug reports were wrong. They are not. That shape is the one row ADO accepts, and no real client sends it. Running go-git v5.19.1 against the same repository reproduced the reported failure immediately -- and not on a fetch, on the CLONE: CLONE FAILED: unexpected client error: unexpected requesting ".../git-upload-pack" status code: 400 The rule, measured one request shape at a time against a real repository: want <sha> -> 200 want <sha> side-band-64k ofs-delta agent=... -> 400 TF401041 want <sha> side-band-64k -> 400 TF401041 want <sha> agent=git/2.39.5 -> 400 TF401041 want <sha> multi_ack side-band-64k ofs-delta -> 200 want <sha> multi_ack_detailed side-band-64k -> 200 So the trigger is a capability list that omits multi_ack, not the absence of negotiation, and either capability satisfies it. v5 filters both out of the advertisement before deciding what to ask for, so every request it sends is the rejected shape. The doc keeps the wrong turn rather than quietly correcting it, because the failure mode is worth naming: probing with a hand-rolled request tests the request you built, not the behaviour you are attributing to it, and contradicting a long-standing external bug report needs a reproduction with the real client rather than a curl that disagrees. Also records what canonical git advertises (which is why it never notices this at all, and why git-http-backend is a usable stand-in), that receive-pack has no multi_ack whatsoever so pushing was never affected on any version, and the sources behind each claim. TestADOLive_StillRequiresMultiAck turns the premise into a canary: it asserts ADO STILL rejects the capability-list-without-multi_ack shape, and fails loudly with "GOOD NEWS, NOT A BUG" if Microsoft ever fixes it. Verified against the real server, which answers: TF401041: The Git protocol sent is not as expected (Clients must support multi-ack.). The public-fixture idea is dropped -- Azure DevOps no longer allows new public projects -- so the live tests stay opt-in and PAT-gated, and never run in CI. E2E_ADO_EMPTY_REPO_URL names a repository that stays empty, which is the only way to cover the empty-repository contract more than once: the main fixture seeds itself on first run. Refs #288 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: Azure DevOps setup gets its own page, like GitHub's configuration.md had grown a full Azure DevOps walkthrough inline -- the Secret, the GitProvider, Entra, SSH, and the multi_ack background -- duplicating azure-devops-getting-started.md a screen below the GitProvider example. github-setup-guide.md already establishes the pattern: per-provider setup is its own page, and configuration.md points at it. So the section goes, and what stays is the single thing that surprises people, placed where it bites: a note under the credentials-Secret auth-keys table, which is exactly where a reader learns HTTP basic means username + password and needs to know Azure DevOps is the exception that carries only a password. Also wires E2E_ADO_EMPTY_REPO_URL through the getting-started guide, and names TestADOLive_StillRequiresMultiAck as a canary there, so someone hitting a red build knows a failure means Microsoft fixed their end rather than that something broke. Verified against the real fixtures at dev.azure.com/configbutler/tests: all four live tests pass, including the empty-repository case against a repository that stays empty and the canary, which reports TF401041: The Git protocol sent is not as expected (Clients must support multi-ack.). and the operator-level e2e spec mirrors a live ConfigMap into the repository (1 passed, 0 failed). Refs #288 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: bound the canary's request, and two doc statements that contradicted themselves Three review findings, all valid: - The canary had no deadline on either half: context.Background() and http.DefaultClient's zero Timeout. An unreachable or mid-response Azure DevOps would hang the test rather than reaching the skip its error path already intends. Bounded at 30s. - "E2E_ADO_EMPTY_REPO_URL must stay empty" reads as an instruction to unset the variable, two lines below the command that sets it. It is the REPOSITORY it names that must stay empty. - The facts page claimed the simulator "rejects any upload-pack POST without multi_ack", which contradicted its own measured table two screens above: the check matches the multi_ack PREFIX, so multi_ack_detailed satisfies it, exactly as ADO does. Verified rather than assumed -- "multi_ack" is a substring of "multi_ack_detailed", and the simulator's test is bytes.Contains. The third is the one worth noticing: a page whose entire purpose is to record what was measured had a summary sentence disagreeing with its own table. Refs #288 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(azure-devops): PAT-only, and shorter Review feedback. The guide read as a tour rather than a path a newcomer can follow, and advertised two credential types nobody has tested against Azure DevOps. - SSH and Entra bearer tokens are no longer presented as setup steps. They use the same Secret keys as any other provider and probably work exactly as they do for GitHub, but nothing here has exercised them against ADO, so they are named once and labelled untested rather than walked through. - `kubectl create namespace` is now in the flow. The Secret command failed without it. - The branch is a `<branch>` placeholder used in both resources, instead of a hard-coded `main` the reader had no reason to think was a choice. - The PAT step says which organization, to set an expiry, to copy the token when shown, and that it inherits its user's repository permissions. - security-model.md claimed HTTP basic auth requires `username`, which contradicts the Azure PAT contract this branch introduced. It now says the password is what selects basic auth, and that ADO omits the username. Both pages are shorter: 155 -> 125 lines for the guide, and the facts page loses the essay while keeping the measurements and sources. One review point is not applied, because the evidence contradicts it: audit delivery is NOT a prerequisite for the final ConfigMap to produce a commit. Attribution is what needs audit; writes do not. The chart defaults to configured-author with attribution disabled, and test/e2e/ado_e2e_test.go sets up no audit at all yet passes by reading the committed manifest back out of Azure DevOps. The guide now says this explicitly, since the reviewer is unlikely to be the last person to assume otherwise. Refs #288 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: the username is optional for every provider, not an Azure DevOps carve-out The docs insisted on leaving `username` out, which overstates what the code does and framed a general rule as provider-specific. Read back: nothing in CredentialFromSecretData mentions Azure DevOps. It branches on `password`, and passes whatever `username` it found straight through. So all three shapes work -- `password` alone, `username: ""` with a password, and a real username with a password -- and only a username WITHOUT a password is an error. Measured against a real ADO repository, `CheckRepo` is accepted with the username set to "", "pat", "anything-at-all", and the account's own name. ADO genuinely ignores it, so "do not add a username" was advice for a problem that does not exist. configuration.md now states the rule where the auth-keys table is, since that table said HTTP basic needs `username` + `password` and thereby contradicted it: the password is what selects basic auth, the username is optional, and an empty value is the same as an absent key to the credential reader. That last point is the only genuinely surprising part, and it is a property of firstSecretValue rather than of Azure DevOps. The getting-started guide drops the warning and says the username is optional and ignored, which is both shorter and true. A test now covers the case that had none: a supplied username must survive into the credential. The three ADO-shaped cases were already covered; the ordinary one was not, which is how the docs drifted from the code unnoticed. Refs #288 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
It looks like that go-git 6 upgrade is holding up pretty nicely (#297): so I'm closing this. Thanks for opening this and your work, appreciated! |
2 participants
Pull Request
go-git v5 strips MultiACK/MultiACKDetailed from its capability advertisement. ADO rejects any upload-pack request that omits multi_ack with HTTP 400 (TF401041: "Clients must support multi-ack.").
Proper fix: go-git PR #1204 (full multi_ack implementation) is targeted for v6.0.0, which is still in alpha as of 2026-07-28. Drop this fallback and upgrade to go-git v6 once a stable release lands.
Description
This change adds a per-URL system-git fallback: any remote whose URL contains dev.azure.com, visualstudio.com, or ssh.dev.azure.com is routed through exec.Command("git", ...) instead of go-git. All other providers continue to use the existing go-git path unchanged.
Three entry points now delegate to system git for ADO URLs:
Credentials are never passed via argv:
Dockerfile: adds a git-bundle Alpine stage that collects git and all its musl shared-library dependencies via ldd, then copies them into the distroless final image. The distroless/static:debug base already includes /bin/sh (busybox), which is required to execute the GIT_ASKPASS script.
Type of Change
Please delete options that are not relevant.
Testing
Checklist
Related Issues
Closes #288
Summary by CodeRabbit