diff --git a/cmd/git-sync/main_test.go b/cmd/git-sync/main_test.go index 532c6f1e..d4d360d5 100644 --- a/cmd/git-sync/main_test.go +++ b/cmd/git-sync/main_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -14,6 +15,7 @@ import ( "testing" "time" + "entire.io/entire/git-sync/internal/auth" "entire.io/entire/git-sync/internal/syncertest" "entire.io/entire/git-sync/unstable" billy "github.com/go-git/go-billy/v6" @@ -29,6 +31,21 @@ import ( "github.com/go-git/go-git/v6/storage/memory" ) +// TestMain isolates the package's tests from the developer's local +// credential helper. Without this, `git credential fill` could find +// stored credentials for 127.0.0.1 (e.g. cached from an earlier test +// run) and turn EnsureAuthForService's would-be no-op into a real +// auth-probe POST, throwing off receive-pack POST counts. +// +// Tests that need to exercise helper behaviour explicitly should +// restore auth.GitCredentialCommand in their own setup. +func TestMain(m *testing.M) { + auth.GitCredentialCommand = func(_ context.Context, _ auth.CredentialOp, _ string) ([]byte, error) { + return nil, errors.New("no helper configured (test default)") + } + os.Exit(m.Run()) +} + const testBranch = "master" const modeReplicate = "replicate" diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 02778eda..aead1e09 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "net/url" + "os" "os/exec" "strings" @@ -29,15 +30,13 @@ type Endpoint struct { } // Resolve resolves the auth method for the given endpoint configuration. -// Order: explicit flags → Entire DB token → git credential helper → anonymous. +// Order: explicit flags → Entire DB token → anonymous (with the git credential +// helper deferred until the server returns 401, matching git's own behaviour). func Resolve(raw Endpoint, ep *url.URL) (Method, error) { if auth := explicitAuth(raw); auth != nil { return auth, nil } - if ep == nil { - return nil, nil //nolint:nilnil // nil signals no auth method found at this stage - } - if ep.Scheme != "http" && ep.Scheme != "https" { + if !isHTTPEndpoint(ep) { return nil, nil //nolint:nilnil // nil signals no auth method found at this stage } if username, password, ok, err := LookupEntireDBCredential(raw, ep); err != nil { @@ -45,9 +44,6 @@ func Resolve(raw Endpoint, ep *url.URL) (Method, error) { } else if ok { return &transporthttp.BasicAuth{Username: username, Password: password}, nil } - if username, password, ok := lookupGitCredential(ep); ok { - return &transporthttp.BasicAuth{Username: username, Password: password}, nil - } return nil, nil //nolint:nilnil // nil signals no auth method found at this stage } @@ -65,28 +61,61 @@ func explicitAuth(raw Endpoint) Method { return nil } -// GitCredentialFillCommand is replaceable for testing. -var GitCredentialFillCommand = func(ctx context.Context, input string) ([]byte, error) { - cmd := exec.CommandContext(ctx, "git", "credential", "fill") +// CredentialOp identifies a `git credential` subcommand. +type CredentialOp string + +const ( + CredentialOpFill CredentialOp = "fill" + CredentialOpApprove CredentialOp = "approve" + CredentialOpReject CredentialOp = "reject" +) + +// newGitCredentialCmd builds the `git credential ` invocation. Extracted +// so tests can inspect the command's environment without exec'ing git. +func newGitCredentialCmd(ctx context.Context, op CredentialOp, input string) *exec.Cmd { + cmd := exec.CommandContext(ctx, "git", "credential", string(op)) cmd.Stdin = strings.NewReader(input) - return cmd.Output() + // Suppress git's interactive username/password fallback. Without this, + // a host with no configured helper drops to a /dev/tty prompt and turns + // git-sync into an interactive command (issue #63). + cmd.Env = append(os.Environ(), "GIT_TERMINAL_PROMPT=0") + return cmd +} + +// GitCredentialCommand invokes `git credential ` with the given input +// (git-credential text format). Replaceable for testing. +var GitCredentialCommand = func(ctx context.Context, op CredentialOp, input string) ([]byte, error) { + return newGitCredentialCmd(ctx, op, input).Output() } -func lookupGitCredential(ep *url.URL) (string, string, bool) { - input := credentialFillInput(ep) +// GitCredentialHelper bridges Git's credential helper protocol to HTTP auth. +// Best-effort: a missing or misbehaving helper denies credentials rather +// than failing the surrounding sync. +type GitCredentialHelper struct{} + +// Lookup queries the git credential helper for credentials for ep. Returns +// ok=false if no credentials are available so the caller can surface a +// clean 401 rather than block. +// +//nolint:unparam // err is always nil today but kept for the CredentialHelper interface. +func (GitCredentialHelper) Lookup(ctx context.Context, ep *url.URL) (username, password string, ok bool, err error) { + if !isHTTPEndpoint(ep) { + return "", "", false, nil + } + input := credentialInput(ep, "", "") if input == "" { - return "", "", false + return "", "", false, nil } - output, err := GitCredentialFillCommand(context.Background(), input) - if err != nil { - return "", "", false + output, helperErr := GitCredentialCommand(ctx, CredentialOpFill, input) + if helperErr != nil { + return "", "", false, nil //nolint:nilerr // helper failure means "no credentials available" } values := parseCredentialOutput(output) - password := values["password"] + password = values["password"] if password == "" { - return "", "", false + return "", "", false, nil } - username := values["username"] + username = values["username"] if username == "" { if ep.User != nil && ep.User.Username() != "" { username = ep.User.Username() @@ -94,10 +123,37 @@ func lookupGitCredential(ep *url.URL) (string, string, bool) { username = defaultGitUsername } } - return username, password, true + return username, password, true, nil } -func credentialFillInput(ep *url.URL) string { +// Approve tells the helper the credentials worked. +func (h GitCredentialHelper) Approve(ctx context.Context, ep *url.URL, username, password string) { + h.signal(ctx, CredentialOpApprove, ep, username, password) +} + +// Reject tells the helper the credentials failed. +func (h GitCredentialHelper) Reject(ctx context.Context, ep *url.URL, username, password string) { + h.signal(ctx, CredentialOpReject, ep, username, password) +} + +func (GitCredentialHelper) signal(ctx context.Context, op CredentialOp, ep *url.URL, username, password string) { + input := credentialInput(ep, username, password) + if input == "" { + return + } + _, _ = GitCredentialCommand(ctx, op, input) //nolint:errcheck // advisory signal; helper failures swallowed +} + +func isHTTPEndpoint(ep *url.URL) bool { + return ep != nil && (ep.Scheme == "http" || ep.Scheme == "https") +} + +// credentialInput builds a git-credential format request body for the given +// endpoint. When username/password are set, they are appended (for use with +// `git credential approve`/`reject`). When both are empty, the result is a +// query body suitable for `git credential fill`. Explicit username overrides +// any user embedded in the endpoint URL. +func credentialInput(ep *url.URL, username, password string) string { if ep == nil || ep.Hostname() == "" { return "" } @@ -106,8 +162,15 @@ func credentialFillInput(ep *url.URL) string { if path := strings.TrimPrefix(ep.Path, "/"); path != "" { fmt.Fprintf(&b, "path=%s\n", path) } - if ep.User != nil && ep.User.Username() != "" { - fmt.Fprintf(&b, "username=%s\n", ep.User.Username()) + user := username + if user == "" && ep.User != nil { + user = ep.User.Username() + } + if user != "" { + fmt.Fprintf(&b, "username=%s\n", user) + } + if password != "" { + fmt.Fprintf(&b, "password=%s\n", password) } b.WriteString("\n") return b.String() diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 1fc69e7d..ede9aad3 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -119,7 +119,7 @@ func TestTokenExpiredOrExpiring(t *testing.T) { } } -func TestCredentialFillInput(t *testing.T) { +func TestCredentialInput_FillQueryWithEmbeddedUser(t *testing.T) { ep := &url.URL{ Scheme: "https", Host: "github.com", @@ -127,38 +127,66 @@ func TestCredentialFillInput(t *testing.T) { User: url.User("myuser"), } - got := credentialFillInput(ep) + got := credentialInput(ep, "", "") want := "protocol=https\nhost=github.com\npath=owner/repo.git\nusername=myuser\n\n" if got != want { - t.Errorf("credentialFillInput returned:\n%q\nwant:\n%q", got, want) + t.Errorf("credentialInput returned:\n%q\nwant:\n%q", got, want) } } -func TestCredentialFillInputNilEndpoint(t *testing.T) { - got := credentialFillInput(nil) +func TestCredentialInput_NilEndpoint(t *testing.T) { + got := credentialInput(nil, "", "") if got != "" { t.Errorf("expected empty string for nil endpoint, got %q", got) } } -func TestCredentialFillInputEmptyHost(t *testing.T) { +func TestCredentialInput_EmptyHost(t *testing.T) { ep := &url.URL{Scheme: "https"} - got := credentialFillInput(ep) + got := credentialInput(ep, "", "") if got != "" { t.Errorf("expected empty string for empty host, got %q", got) } } -func TestCredentialFillInputNoUser(t *testing.T) { +func TestCredentialInput_FillQueryNoUser(t *testing.T) { ep := &url.URL{ Scheme: "https", Host: "example.com", Path: "/repo.git", } - got := credentialFillInput(ep) + got := credentialInput(ep, "", "") want := "protocol=https\nhost=example.com\npath=repo.git\n\n" if got != want { - t.Errorf("credentialFillInput returned:\n%q\nwant:\n%q", got, want) + t.Errorf("credentialInput returned:\n%q\nwant:\n%q", got, want) + } +} + +func TestCredentialInput_ApproveRejectFormatIncludesUserAndPassword(t *testing.T) { + ep := &url.URL{ + Scheme: "https", + Host: "example.com", + Path: "/owner/repo.git", + } + got := credentialInput(ep, "alice", "s3cret") + want := "protocol=https\nhost=example.com\npath=owner/repo.git\nusername=alice\npassword=s3cret\n\n" + if got != want { + t.Errorf("credentialInput returned:\n%q\nwant:\n%q", got, want) + } +} + +func TestCredentialInput_ExplicitUserOverridesURLUser(t *testing.T) { + ep := &url.URL{ + Scheme: "https", + Host: "example.com", + User: url.User("from-url"), + } + got := credentialInput(ep, "explicit", "") + if !strings.Contains(got, "username=explicit\n") { + t.Errorf("expected explicit username to win, got:\n%q", got) + } + if strings.Contains(got, "username=from-url") { + t.Errorf("URL-embedded username should be overridden, got:\n%q", got) } } @@ -234,7 +262,6 @@ func TestResolve(t *testing.T) { name string raw Endpoint ep *url.URL - mockCred func(ctx context.Context, input string) ([]byte, error) wantType string // "token", "basic", "nil" wantUser string wantPass string @@ -270,29 +297,23 @@ func TestResolve(t *testing.T) { wantType: "nil", }, { - name: "nothing set HTTP endpoint no credential helper returns nil", - raw: Endpoint{}, - ep: ep, - mockCred: func(_ context.Context, _ string) ([]byte, error) { - return nil, errors.New("no helper") - }, + name: "nothing set HTTP endpoint returns nil (defer to helper on 401)", + raw: Endpoint{}, + ep: ep, wantType: "nil", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Save and restore GitCredentialFillCommand. - origCmd := GitCredentialFillCommand - defer func() { GitCredentialFillCommand = origCmd }() - - if tt.mockCred != nil { - GitCredentialFillCommand = tt.mockCred - } else { - // Default mock: no credential helper. - GitCredentialFillCommand = func(_ context.Context, _ string) ([]byte, error) { - return nil, errors.New("no helper") - } + // Resolve must never consult the git credential helper — + // that lookup is deferred to a 401 response. If anything + // here invokes the helper, fail loudly. + origCmd := GitCredentialCommand + defer func() { GitCredentialCommand = origCmd }() + GitCredentialCommand = func(_ context.Context, op CredentialOp, input string) ([]byte, error) { + t.Fatalf("unexpected GitCredentialCommand(%q, %q) call during Resolve", op, input) + return nil, nil } // Also ensure ENTIRE_CONFIG_DIR points nowhere so EntireDB lookup @@ -407,6 +428,178 @@ func TestExplicitAuth(t *testing.T) { } } +type recordedCredCall struct { + op CredentialOp + input string +} + +func withRecordingHelper(t *testing.T, calls *[]recordedCredCall, handler func(op CredentialOp, input string) ([]byte, error)) { + t.Helper() + orig := GitCredentialCommand + t.Cleanup(func() { GitCredentialCommand = orig }) + GitCredentialCommand = func(_ context.Context, op CredentialOp, input string) ([]byte, error) { + *calls = append(*calls, recordedCredCall{op: op, input: input}) + if handler == nil { + return nil, nil + } + return handler(op, input) + } +} + +func TestGitCredentialHelper_Lookup_ReturnsCredentials(t *testing.T) { + ep := &url.URL{Scheme: "https", Host: "example.com", Path: "/owner/repo.git"} + var calls []recordedCredCall + withRecordingHelper(t, &calls, func(op CredentialOp, _ string) ([]byte, error) { + if op != CredentialOpFill { + t.Fatalf("expected fill, got %q", op) + } + return []byte("username=alice\npassword=s3cret\n"), nil + }) + + user, pass, ok, err := GitCredentialHelper{}.Lookup(context.Background(), ep) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !ok { + t.Fatal("expected ok=true") + } + if user != "alice" || pass != "s3cret" { + t.Errorf("got user=%q pass=%q, want alice/s3cret", user, pass) + } + if len(calls) != 1 { + t.Fatalf("expected 1 helper call, got %d", len(calls)) + } + if !strings.Contains(calls[0].input, "protocol=https\nhost=example.com\n") { + t.Errorf("fill input missing host/protocol:\n%q", calls[0].input) + } +} + +func TestGitCredentialHelper_Lookup_HelperFailsReturnsNotFound(t *testing.T) { + ep := &url.URL{Scheme: "https", Host: "example.com"} + withRecordingHelper(t, new([]recordedCredCall), func(_ CredentialOp, _ string) ([]byte, error) { + return nil, errors.New("no helper") + }) + + _, _, ok, err := GitCredentialHelper{}.Lookup(context.Background(), ep) + if err != nil { + t.Errorf("expected no error when helper has no credentials, got %v", err) + } + if ok { + t.Error("expected ok=false when helper fails") + } +} + +func TestGitCredentialHelper_Lookup_EmptyPasswordReturnsNotFound(t *testing.T) { + ep := &url.URL{Scheme: "https", Host: "example.com"} + withRecordingHelper(t, new([]recordedCredCall), func(_ CredentialOp, _ string) ([]byte, error) { + return []byte("username=alice\n"), nil + }) + + _, _, ok, err := GitCredentialHelper{}.Lookup(context.Background(), ep) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if ok { + t.Error("expected ok=false when password is empty") + } +} + +func TestGitCredentialHelper_Lookup_UsernameFallsBackToGit(t *testing.T) { + ep := &url.URL{Scheme: "https", Host: "example.com"} + withRecordingHelper(t, new([]recordedCredCall), func(_ CredentialOp, _ string) ([]byte, error) { + return []byte("password=tok\n"), nil + }) + + user, _, ok, err := GitCredentialHelper{}.Lookup(context.Background(), ep) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !ok { + t.Fatal("expected ok=true") + } + if user != "git" { + t.Errorf("expected username fallback to 'git', got %q", user) + } +} + +func TestGitCredentialHelper_Lookup_NonHTTPEndpointReturnsNotFound(t *testing.T) { + ep := &url.URL{Scheme: "ssh", Host: "example.com"} + calls := new([]recordedCredCall) + withRecordingHelper(t, calls, func(_ CredentialOp, _ string) ([]byte, error) { + return []byte("password=tok\n"), nil + }) + + _, _, ok, err := GitCredentialHelper{}.Lookup(context.Background(), ep) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ok { + t.Error("expected ok=false for SSH endpoint — credential helper protocol is HTTP-only") + } + if len(*calls) != 0 { + t.Errorf("expected no helper call for SSH endpoint, got %d", len(*calls)) + } +} + +func TestGitCredentialHelper_Approve_SendsCredentialsToHelper(t *testing.T) { + ep := &url.URL{Scheme: "https", Host: "example.com", Path: "/owner/repo.git"} + var calls []recordedCredCall + withRecordingHelper(t, &calls, nil) + + GitCredentialHelper{}.Approve(context.Background(), ep, "alice", "s3cret") + + if len(calls) != 1 || calls[0].op != CredentialOpApprove { + t.Fatalf("expected one 'approve' call, got %+v", calls) + } + want := "protocol=https\nhost=example.com\npath=owner/repo.git\nusername=alice\npassword=s3cret\n\n" + if calls[0].input != want { + t.Errorf("approve input:\n%q\nwant:\n%q", calls[0].input, want) + } +} + +func TestGitCredentialHelper_Reject_SendsCredentialsToHelper(t *testing.T) { + ep := &url.URL{Scheme: "https", Host: "example.com"} + var calls []recordedCredCall + withRecordingHelper(t, &calls, nil) + + GitCredentialHelper{}.Reject(context.Background(), ep, "alice", "bad") + + if len(calls) != 1 || calls[0].op != CredentialOpReject { + t.Fatalf("expected one 'reject' call, got %+v", calls) + } + if !strings.Contains(calls[0].input, "username=alice\npassword=bad\n") { + t.Errorf("reject input missing creds:\n%q", calls[0].input) + } +} + +func TestGitCredentialHelper_ApproveRejectSwallowHelperErrors(t *testing.T) { + ep := &url.URL{Scheme: "https", Host: "example.com"} + withRecordingHelper(t, new([]recordedCredCall), func(_ CredentialOp, _ string) ([]byte, error) { + return nil, errors.New("helper unavailable") + }) + + // Approve/Reject must not panic when the helper is broken. + GitCredentialHelper{}.Approve(context.Background(), ep, "u", "p") + GitCredentialHelper{}.Reject(context.Background(), ep, "u", "p") +} + +// TestGitCredentialCmdDisablesTerminalPrompt is a regression test for issue +// #63 — without GIT_TERMINAL_PROMPT=0 git drops into an interactive prompt. +func TestGitCredentialCmdDisablesTerminalPrompt(t *testing.T) { + cmd := newGitCredentialCmd(context.Background(), CredentialOpFill, "protocol=https\nhost=example.com\n\n") + + var found bool + for _, kv := range cmd.Env { + if kv == "GIT_TERMINAL_PROMPT=0" { + found = true + break + } + } + if !found { + t.Errorf("expected GIT_TERMINAL_PROMPT=0 in cmd.Env, got %v", cmd.Env) + } +} + func TestEndpointBaseURL(t *testing.T) { tests := []struct { name string diff --git a/internal/gitproto/push.go b/internal/gitproto/push.go index 704601da..ae7d1e31 100644 --- a/internal/gitproto/push.go +++ b/internal/gitproto/push.go @@ -150,6 +150,13 @@ func sendReceivePack( if err := req.Encode(&header); err != nil { return fmt.Errorf("encode update-request: %w", err) } + // The push body is io.MultiReader(header, packData); packData comes + // from a live upload-pack pipe and isn't rewindable, so a mid-stream + // 401 can't trigger PostRPCStreamBody's normal helper retry. Probe + // for auth requirements with a same-shape POST first. + if hc, ok := conn.(*HTTPConn); ok { + hc.EnsureAuthForService(ctx, transport.ReceivePackService) + } body := io.Reader(bytes.NewReader(header.Bytes())) if packData != nil { body = io.MultiReader(body, packData) diff --git a/internal/gitproto/smarthttp.go b/internal/gitproto/smarthttp.go index 90c5fe45..89458d4d 100644 --- a/internal/gitproto/smarthttp.go +++ b/internal/gitproto/smarthttp.go @@ -12,6 +12,7 @@ import ( "strings" "github.com/go-git/go-git/v6/plumbing/protocol/capability" + transporthttp "github.com/go-git/go-git/v6/plumbing/transport/http" ) const maxHTTPErrorBody = 64 * 1024 @@ -70,6 +71,17 @@ type AuthMethod interface { Authorizer(req *http.Request) error } +// CredentialHelper provides on-demand credentials when an HTTP request is +// rejected with 401. Lookup must not block on user interaction — return +// ok=false instead, so the sync can surface a clean 401 rather than hang. +// Approve/Reject are advisory and intentionally have no error return: +// failures must not poison the outer request flow. +type CredentialHelper interface { + Lookup(ctx context.Context, ep *url.URL) (username, password string, ok bool, err error) + Approve(ctx context.Context, ep *url.URL, username, password string) + Reject(ctx context.Context, ep *url.URL, username, password string) +} + // HTTPConn represents a connection to a remote Git HTTP endpoint. type HTTPConn struct { Label string @@ -77,6 +89,13 @@ type HTTPConn struct { HTTP *http.Client Auth AuthMethod + // CredentialHelper, if set, is consulted on a 401 response when no + // initial Auth was configured. The retry happens once on /info/refs; + // on success the resolved credentials are stored in Auth for the + // remaining requests on this connection. Setting Auth up front + // disables the helper fallback — explicit auth wins. + CredentialHelper CredentialHelper + // FollowInfoRefsRedirect, when true, rewrites Endpoint.Scheme and // Endpoint.Host to the final URL returned by RequestInfoRefs after // HTTP redirects. Subsequent PostRPC* calls then target the @@ -94,6 +113,18 @@ type HTTPConn struct { // coordinated writer here so server-side progress lines don't // clobber the in-place ticker frame. ProgressOut io.Writer + + // pendingHelperCreds tracks credentials supplied by the helper via + // EnsureAuthForService but not yet validated against a real operation. + // The next RequestInfoRefs/PostRPCStreamBody approves on 2xx or rejects + // on 401/403, ensuring helper state reflects the actual outcome rather + // than an ambiguous probe response. + pendingHelperCreds *helperCreds +} + +type helperCreds struct { + user, pass string + url *url.URL } // NewHTTPConn creates a new connection to the given endpoint. @@ -161,23 +192,18 @@ func RequestInfoRefs(ctx context.Context, conn Conn, service string, gitProtocol // RequestInfoRefs fetches /info/refs for the given service. func (c *HTTPConn) RequestInfoRefs(ctx context.Context, service string, gitProtocol string) ([]byte, error) { - reqURL := fmt.Sprintf("%s/info/refs?service=%s", c.EndpointURL.String(), service) - req, err := http.NewRequestWithContext(ctx, http.MethodGet, reqURL, nil) + res, err := c.doInfoRefsRequest(ctx, service, gitProtocol, c.Auth) if err != nil { - return nil, fmt.Errorf("create info-refs request: %w", err) - } - req.Header.Set("Accept", "*/*") - req.Header.Set("User-Agent", capability.DefaultAgent()) - req.Header.Set(StatsPhaseHeader, service+" info-refs") - if gitProtocol != "" { - req.Header.Set("Git-Protocol", gitProtocol) + return nil, err } - ApplyAuth(req, c.Auth) - - res, err := c.HTTP.Do(req) + res, err = c.tryHelperRetry(ctx, res, func(auth AuthMethod) (*http.Response, error) { + return c.doInfoRefsRequest(ctx, service, gitProtocol, auth) + }) if err != nil { - return nil, fmt.Errorf("request info-refs: %w", err) + return nil, err } + c.resolvePendingHelperCreds(ctx, res) + defer res.Body.Close() if err := httpError(res); err != nil { return nil, err @@ -251,7 +277,38 @@ func PostRPCStreamBody(ctx context.Context, conn Conn, service string, body io.R // PostRPCStreamBody sends a POST to the given service using a streaming request body. // Caller must close the returned ReadCloser. +// +// On a 401 we consult the credential helper and retry, mirroring git's +// own behaviour for servers that allow anonymous /info/refs but gate the +// actual upload-pack/receive-pack POST behind auth. Retry is only possible +// when body is an io.Seeker (so we can rewind it); callers that pass a raw +// non-seekable Reader will see the 401 surface as-is. func (c *HTTPConn) PostRPCStreamBody(ctx context.Context, service string, body io.Reader, v2 bool, phase string) (io.ReadCloser, error) { + res, err := c.doPostRPCRequest(ctx, service, body, v2, phase, c.Auth) + if err != nil { + return nil, err + } + if seeker, ok := body.(io.Seeker); ok { + res, err = c.tryHelperRetry(ctx, res, func(auth AuthMethod) (*http.Response, error) { + if _, seekErr := seeker.Seek(0, io.SeekStart); seekErr != nil { + return nil, fmt.Errorf("rewind RPC body for credential-helper retry: %w", seekErr) + } + return c.doPostRPCRequest(ctx, service, body, v2, phase, auth) + }) + if err != nil { + return nil, err + } + } + c.resolvePendingHelperCreds(ctx, res) + if err := httpError(res); err != nil { + _ = res.Body.Close() + return nil, err + } + return res.Body, nil +} + +// doPostRPCRequest issues a single POST to /. Caller closes res.Body. +func (c *HTTPConn) doPostRPCRequest(ctx context.Context, service string, body io.Reader, v2 bool, phase string, auth AuthMethod) (*http.Response, error) { reqURL := fmt.Sprintf("%s/%s", c.EndpointURL.String(), service) req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, body) if err != nil { @@ -264,17 +321,12 @@ func (c *HTTPConn) PostRPCStreamBody(ctx context.Context, service string, body i if v2 { req.Header.Set("Git-Protocol", GitProtocolV2) } - ApplyAuth(req, c.Auth) - + ApplyAuth(req, auth) res, err := c.HTTP.Do(req) if err != nil { return nil, fmt.Errorf("post RPC: %w", err) } - if err := httpError(res); err != nil { - _ = res.Body.Close() - return nil, err - } - return res.Body, nil + return res, nil } // ApplyAuth applies the given auth method to an HTTP request. Errors from @@ -287,3 +339,179 @@ func ApplyAuth(req *http.Request, auth AuthMethod) { } _ = auth.Authorizer(req) //nolint:errcheck // BasicAuth and TokenAuth never error; future authorizers should surface 401s instead } + +// EnsureAuthForService tentatively attaches helper credentials before a +// non-rewindable request body is committed. It's a no-op when no helper +// is configured, when Auth is already set, or when the helper has no +// credentials to offer. +// +// Used from push.go (and other streaming-body POST paths) where the body +// is built from a live upstream stream (e.g. io.MultiReader over a pack +// reader) and can't be replayed on a mid-stream 401. +// +// The flow is: +// 1. Ask the helper if it has credentials for this endpoint. If not, +// bail — no point probing for an auth requirement we can't satisfy. +// (This also keeps anonymous syncs from doing a wasted no-op POST.) +// 2. Probe with a POST to / using the smart-HTTP flush packet +// "0000" as body — a valid no-op (zero ref updates, zero pack data) +// by spec. We probe with POST rather than GET because the auth layer +// may only gate the POST handler; a GET probe would slip past on +// servers that 404/405 GET while requiring auth on POST. +// 3. If the probe gets 401, attach the helper credentials tentatively. +// The next real operation (PostRPCStreamBody or RequestInfoRefs) +// calls resolvePendingHelperCreds, which Approves them on 2xx or +// Rejects them on 401/403 — helper state only changes based on the +// actual outcome, never on the probe response alone. +// +// If the probe doesn't 401 (200, 404, 405, etc.) we don't attach; the +// server either accepts anonymous POSTs here or returns ambiguously, +// and either way attaching unvalidated credentials could leak them. +func (c *HTTPConn) EnsureAuthForService(ctx context.Context, service string) { + if c.Auth != nil || c.CredentialHelper == nil { + return + } + user, pass, ok, lookupErr := c.CredentialHelper.Lookup(ctx, c.EndpointURL) + if lookupErr != nil || !ok { + return + } + res, err := c.doServiceProbe(ctx, service) + if err != nil { + return + } + defer res.Body.Close() + if res.StatusCode != http.StatusUnauthorized { + return + } + challengeURL := challengeURLFor(c.EndpointURL, res) + c.Auth = &transporthttp.BasicAuth{Username: user, Password: pass} + c.pendingHelperCreds = &helperCreds{user: user, pass: pass, url: challengeURL} +} + +// resolvePendingHelperCreds settles credentials that EnsureAuthForService +// attached tentatively, based on the outcome of a real operation. Called +// from RequestInfoRefs and PostRPCStreamBody. No-op if nothing pending. +func (c *HTTPConn) resolvePendingHelperCreds(ctx context.Context, res *http.Response) { + if c.pendingHelperCreds == nil || c.CredentialHelper == nil { + return + } + creds := c.pendingHelperCreds + switch { + case res.StatusCode >= http.StatusOK && res.StatusCode < http.StatusMultipleChoices: + c.pendingHelperCreds = nil + c.CredentialHelper.Approve(ctx, creds.url, creds.user, creds.pass) + case res.StatusCode == http.StatusUnauthorized || res.StatusCode == http.StatusForbidden: + c.pendingHelperCreds = nil + c.Auth = nil + c.CredentialHelper.Reject(ctx, creds.url, creds.user, creds.pass) + } + // Other status: leave pending. A later op on this conn may resolve; + // the conn is short-lived (one sync), so leftover pending state at + // end of life is harmless. +} + +// flushPacket is the smart-HTTP pkt-line "flush" marker. A request body +// containing only a flush packet is a valid no-op for both upload-pack +// (no wants/haves) and receive-pack (no ref updates, no pack data). +var flushPacket = []byte("0000") + +func (c *HTTPConn) doServiceProbe(ctx context.Context, service string) (*http.Response, error) { + reqURL := fmt.Sprintf("%s/%s", c.EndpointURL.String(), service) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, bytes.NewReader(flushPacket)) + if err != nil { + return nil, fmt.Errorf("create auth-probe request: %w", err) + } + req.Header.Set("Content-Type", fmt.Sprintf("application/x-%s-request", service)) + req.Header.Set("Accept", fmt.Sprintf("application/x-%s-result", service)) + req.Header.Set("User-Agent", capability.DefaultAgent()) + req.Header.Set(StatsPhaseHeader, service+" auth-probe") + res, err := c.HTTP.Do(req) + if err != nil { + return nil, fmt.Errorf("auth-probe request: %w", err) + } + return res, nil +} + +// tryHelperRetry handles the 401 → lookup → retry → approve/reject lifecycle +// when a CredentialHelper is configured and no explicit Auth was set up front +// (explicit auth must surface its own failures rather than be quietly papered +// over). retry attempts the same request with helper-supplied credentials. +// +// On retry success the credentials are stored on c.Auth so follow-up calls +// on the same connection reuse them. On retry failure (401, 403, or transport +// error) the helper is told to reject the credentials so a stale stored token +// self-heals on the next run. +// +// Caller is responsible for closing the returned response body. +func (c *HTTPConn) tryHelperRetry(ctx context.Context, res *http.Response, retry func(AuthMethod) (*http.Response, error)) (*http.Response, error) { + if res.StatusCode != http.StatusUnauthorized || c.Auth != nil || c.CredentialHelper == nil { + return res, nil + } + challengeURL := challengeURLFor(c.EndpointURL, res) + user, pass, ok, lookupErr := c.CredentialHelper.Lookup(ctx, challengeURL) + if lookupErr != nil { + _ = res.Body.Close() + return nil, fmt.Errorf("look up credentials: %w", lookupErr) + } + if !ok { + return res, nil + } + _ = res.Body.Close() + retryAuth := &transporthttp.BasicAuth{Username: user, Password: pass} + res, err := retry(retryAuth) + if err != nil { + c.CredentialHelper.Reject(ctx, challengeURL, user, pass) + return nil, err + } + switch { + case res.StatusCode == http.StatusUnauthorized || res.StatusCode == http.StatusForbidden: + // 403 included because some token services (e.g. Cloudflare) + // surface "Invalid or expired token" as 403 rather than 401. + c.CredentialHelper.Reject(ctx, challengeURL, user, pass) + case res.StatusCode >= http.StatusOK && res.StatusCode < http.StatusMultipleChoices: + c.Auth = retryAuth + c.CredentialHelper.Approve(ctx, challengeURL, user, pass) + } + return res, nil +} + +// challengeURLFor returns the URL key used to query the credential helper +// for an auth challenge. After a 3xx the actually-challenged host is in +// res.Request.URL, which may differ from c.EndpointURL — using the wrong +// one would query (and possibly approve/reject) credentials under the +// wrong helper key. The original repo path is preserved so the key still +// matches what the user configured. +func challengeURLFor(orig *url.URL, res *http.Response) *url.URL { + if res == nil || res.Request == nil || res.Request.URL == nil { + return orig + } + final := res.Request.URL + if final.Host == orig.Host && final.Scheme == orig.Scheme { + return orig + } + out := *orig + out.Scheme = final.Scheme + out.Host = final.Host + return &out +} + +// doInfoRefsRequest issues a single /info/refs GET. Caller closes res.Body. +func (c *HTTPConn) doInfoRefsRequest(ctx context.Context, service, gitProtocol string, auth AuthMethod) (*http.Response, error) { + reqURL := fmt.Sprintf("%s/info/refs?service=%s", c.EndpointURL.String(), service) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, reqURL, nil) + if err != nil { + return nil, fmt.Errorf("create info-refs request: %w", err) + } + req.Header.Set("Accept", "*/*") + req.Header.Set("User-Agent", capability.DefaultAgent()) + req.Header.Set(StatsPhaseHeader, service+" info-refs") + if gitProtocol != "" { + req.Header.Set("Git-Protocol", gitProtocol) + } + ApplyAuth(req, auth) + res, err := c.HTTP.Do(req) + if err != nil { + return nil, fmt.Errorf("request info-refs: %w", err) + } + return res, nil +} diff --git a/internal/gitproto/smarthttp_test.go b/internal/gitproto/smarthttp_test.go index 4ba264eb..f4eb3ec6 100644 --- a/internal/gitproto/smarthttp_test.go +++ b/internal/gitproto/smarthttp_test.go @@ -447,6 +447,699 @@ func (f roundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) } +func newAdvertisementResponse(req *http.Request) *http.Response { + res := &http.Response{ + StatusCode: http.StatusOK, + Request: req, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("0000")), + } + res.Header.Set("Content-Type", "application/x-git-upload-pack-advertisement") + return res +} + +func newUnauthorizedResponse(req *http.Request) *http.Response { + res := &http.Response{ + StatusCode: http.StatusUnauthorized, + Request: req, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("authentication required")), + } + res.Header.Set("WWW-Authenticate", `Basic realm="git"`) + return res +} + +func newTestConn(_ *testing.T, rt http.RoundTripper) *HTTPConn { + return NewHTTPConn( + &url.URL{Scheme: "https", Host: "example.com", Path: "/repo.git"}, + "src", nil, rt, + ) +} + +func TestRequestInfoRefs_AnonymousSucceedsWithoutConsultingHelper(t *testing.T) { + helper := &fakeCredentialHelper{user: "x", pass: "y", ok: true} + var authHeaders []string + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + authHeaders = append(authHeaders, req.Header.Get("Authorization")) + return newAdvertisementResponse(req), nil + })) + conn.CredentialHelper = helper + + if _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", ""); err != nil { + t.Fatalf("RequestInfoRefs: %v", err) + } + if helper.count("lookup") != 0 { + t.Errorf("expected 0 helper lookups on anonymous success, got %d", helper.count("lookup")) + } + if len(authHeaders) != 1 || authHeaders[0] != "" { + t.Errorf("expected exactly one anonymous request, got headers %v", authHeaders) + } +} + +func TestRequestInfoRefs_OnUnauthorizedRetriesWithHelperCredentials(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} + + var authHeaders []string + attempts := 0 + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + authHeaders = append(authHeaders, req.Header.Get("Authorization")) + attempts++ + if attempts == 1 { + return newUnauthorizedResponse(req), nil + } + return newAdvertisementResponse(req), nil + })) + conn.CredentialHelper = helper + + if _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", ""); err != nil { + t.Fatalf("RequestInfoRefs: %v", err) + } + + if got := helper.count("lookup"); got != 1 { + t.Errorf("expected 1 helper lookup, got %d", got) + } + if got := helper.count("approve"); got != 1 { + t.Errorf("expected 1 approve call, got %d", got) + } + if got := helper.count("reject"); got != 0 { + t.Errorf("expected 0 reject calls, got %d", got) + } + if last := helper.last("approve"); last == nil || last.user != "alice" || last.pass != "s3cret" { + t.Errorf("approve called with wrong creds: %+v", last) + } + if len(authHeaders) != 2 { + t.Fatalf("expected 2 requests (anon then auth retry), got %d: %v", len(authHeaders), authHeaders) + } + if authHeaders[0] != "" { + t.Errorf("first request should be anonymous, got Authorization=%q", authHeaders[0]) + } + if !strings.HasPrefix(authHeaders[1], "Basic ") { + t.Errorf("retry should have Basic auth header, got %q", authHeaders[1]) + } + if conn.Auth == nil { + t.Error("expected conn.Auth to be stored after successful auth retry") + } +} + +func TestRequestInfoRefs_OnUnauthorizedReusesStoredAuthOnNextCall(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} + + var authHeaders []string + attempts := 0 + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + authHeaders = append(authHeaders, req.Header.Get("Authorization")) + attempts++ + if attempts == 1 { + return newUnauthorizedResponse(req), nil + } + if req.Method == http.MethodGet { + return newAdvertisementResponse(req), nil + } + res := &http.Response{ + StatusCode: http.StatusOK, + Request: req, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + } + res.Header.Set("Content-Type", "application/x-git-upload-pack-result") + return res, nil + })) + conn.CredentialHelper = helper + + if _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", ""); err != nil { + t.Fatalf("RequestInfoRefs: %v", err) + } + if _, err := PostRPC(context.Background(), conn, "git-upload-pack", []byte("0000"), false, "phase"); err != nil { + t.Fatalf("PostRPC: %v", err) + } + + if got := helper.count("lookup"); got != 1 { + t.Errorf("expected only 1 helper lookup across both requests, got %d", got) + } + if len(authHeaders) != 3 { + t.Fatalf("expected 3 requests, got %d: %v", len(authHeaders), authHeaders) + } + if authHeaders[1] == "" || authHeaders[2] == "" { + t.Errorf("retry GET and follow-up POST should both carry auth: %v", authHeaders) + } +} + +func TestRequestInfoRefs_OnUnauthorizedSurfaces401WithoutHelper(t *testing.T) { + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return newUnauthorizedResponse(req), nil + })) + + _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", "") + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "401") { + t.Errorf("expected 401 error, got %v", err) + } +} + +func TestRequestInfoRefs_OnUnauthorizedSurfaces401WhenHelperHasNoCredentials(t *testing.T) { + helper := &fakeCredentialHelper{ok: false} + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return newUnauthorizedResponse(req), nil + })) + conn.CredentialHelper = helper + + _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", "") + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "401") { + t.Errorf("expected 401 error, got %v", err) + } + if got := helper.count("lookup"); got != 1 { + t.Errorf("expected 1 lookup attempt, got %d", got) + } + if got := helper.count("approve") + helper.count("reject"); got != 0 { + t.Errorf("expected no approve/reject when helper had no creds, got %d", got) + } +} + +func TestRequestInfoRefs_OnUnauthorizedRetryStill401CallsReject(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "bad", ok: true} + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return newUnauthorizedResponse(req), nil + })) + conn.CredentialHelper = helper + + _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", "") + if err == nil { + t.Fatal("expected error, got nil") + } + if got := helper.count("reject"); got != 1 { + t.Errorf("expected 1 reject call, got %d", got) + } + if got := helper.count("approve"); got != 0 { + t.Errorf("expected 0 approve calls, got %d", got) + } + if last := helper.last("reject"); last == nil || last.user != "alice" || last.pass != "bad" { + t.Errorf("reject called with wrong creds: %+v", last) + } +} + +// TestRequestInfoRefs_OnUnauthorizedRetry403CallsReject documents that some +// token services (notably Cloudflare) return 403 "Invalid or expired token" +// instead of 401 when stored credentials have expired. +func TestRequestInfoRefs_OnUnauthorizedRetry403CallsReject(t *testing.T) { + helper := &fakeCredentialHelper{user: "user", pass: "expired-token", ok: true} + attempts := 0 + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + attempts++ + if attempts == 1 { + return newUnauthorizedResponse(req), nil + } + res := &http.Response{ + StatusCode: http.StatusForbidden, + Request: req, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("Invalid or expired token")), + } + return res, nil + })) + conn.CredentialHelper = helper + + _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", "") + if err == nil { + t.Fatal("expected error, got nil") + } + if got := helper.count("reject"); got != 1 { + t.Errorf("expected 1 reject call on retry 403, got %d", got) + } + if got := helper.count("approve"); got != 0 { + t.Errorf("expected 0 approve calls, got %d", got) + } + if last := helper.last("reject"); last == nil || last.user != "user" || last.pass != "expired-token" { + t.Errorf("reject called with wrong creds: %+v", last) + } +} + +// TestRequestInfoRefs_DoesNotRetryWhenConnAlreadyAuthenticated: explicit auth +// must win over the helper, so users debugging a bad token they passed see +// the real error rather than a silent fallback. +func TestRequestInfoRefs_DoesNotRetryWhenConnAlreadyAuthenticated(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} + initialAuth := &transporthttp.BasicAuth{Username: "explicit", Password: "tok"} + + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return newUnauthorizedResponse(req), nil + })) + conn.Auth = initialAuth + conn.CredentialHelper = helper + + _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", "") + if err == nil { + t.Fatal("expected error, got nil") + } + if got := helper.count("lookup"); got != 0 { + t.Errorf("expected 0 helper lookups when auth was preconfigured, got %d", got) + } +} + +// TestRequestInfoRefs_OnUnauthorizedAfterRedirectKeysHelperOnFinalHost +// covers the case where /info/refs is 307'd to a different host and the +// replica returns 401: the helper must be queried for the host that +// actually challenged us, not the original endpoint. +func TestRequestInfoRefs_OnUnauthorizedAfterRedirectKeysHelperOnFinalHost(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} + attempts := 0 + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + attempts++ + if attempts == 1 { + // Simulate that Go's HTTP client followed a 3xx to replica.example + // before getting the 401 — res.Request.URL is the post-redirect URL. + res := newUnauthorizedResponse(req) + res.Request = &http.Request{URL: &url.URL{ + Scheme: "https", Host: "replica.example", Path: "/repo.git/info/refs", + }} + return res, nil + } + return newAdvertisementResponse(req), nil + })) + conn.CredentialHelper = helper + + if _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", ""); err != nil { + t.Fatalf("RequestInfoRefs: %v", err) + } + + lookup := helper.last("lookup") + if lookup == nil { + t.Fatal("expected helper lookup") + } + if !strings.Contains(lookup.url, "replica.example") { + t.Errorf("helper Lookup keyed on %q, want replica.example", lookup.url) + } + if strings.Contains(lookup.url, "/info/refs") { + t.Errorf("helper Lookup URL should carry the repo path, not /info/refs: %q", lookup.url) + } + approve := helper.last("approve") + if approve == nil || !strings.Contains(approve.url, "replica.example") { + t.Errorf("helper Approve keyed on wrong URL: %+v", approve) + } +} + +func TestPostRPC_OnUnauthorizedRetriesWithHelperCredentials(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} + var authHeaders []string + attempts := 0 + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + authHeaders = append(authHeaders, req.Header.Get("Authorization")) + attempts++ + if attempts == 1 { + return newUnauthorizedResponse(req), nil + } + res := &http.Response{ + StatusCode: http.StatusOK, + Request: req, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + } + res.Header.Set("Content-Type", "application/x-git-upload-pack-result") + return res, nil + })) + conn.CredentialHelper = helper + + if _, err := PostRPC(context.Background(), conn, "git-upload-pack", []byte("0000"), false, "phase"); err != nil { + t.Fatalf("PostRPC: %v", err) + } + + if got := helper.count("lookup"); got != 1 { + t.Errorf("expected 1 helper lookup, got %d", got) + } + if got := helper.count("approve"); got != 1 { + t.Errorf("expected 1 approve call, got %d", got) + } + if len(authHeaders) != 2 { + t.Fatalf("expected 2 requests (anon then auth), got %d: %v", len(authHeaders), authHeaders) + } + if authHeaders[0] != "" { + t.Errorf("first POST should be anonymous, got %q", authHeaders[0]) + } + if !strings.HasPrefix(authHeaders[1], "Basic ") { + t.Errorf("retry POST should have Basic auth, got %q", authHeaders[1]) + } + if conn.Auth == nil { + t.Error("expected conn.Auth to be stored after successful POST retry") + } +} + +func TestPostRPC_OnUnauthorizedSurfaces401WithoutHelper(t *testing.T) { + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return newUnauthorizedResponse(req), nil + })) + + _, err := PostRPC(context.Background(), conn, "git-upload-pack", []byte("0000"), false, "phase") + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "401") { + t.Errorf("expected 401 error, got %v", err) + } +} + +func TestPostRPC_OnUnauthorizedRetryStill401CallsReject(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "bad", ok: true} + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return newUnauthorizedResponse(req), nil + })) + conn.CredentialHelper = helper + + _, err := PostRPC(context.Background(), conn, "git-upload-pack", []byte("0000"), false, "phase") + if err == nil { + t.Fatal("expected error, got nil") + } + if got := helper.count("reject"); got != 1 { + t.Errorf("expected 1 reject call, got %d", got) + } + if got := helper.count("approve"); got != 0 { + t.Errorf("expected 0 approve calls, got %d", got) + } +} + +// TestEnsureAuthForService_TentativelyAttachesHelperCredsOnAnonymous401: +// the anonymous probe gets 401, the helper supplies credentials, and they +// are attached to the conn for the upcoming streaming POST — but NOT +// approved yet. Approval is deferred until the real operation validates +// them; otherwise a server that returns 405 to GET /git-receive-pack +// without checking auth would bless stale creds. +func TestEnsureAuthForService_TentativelyAttachesHelperCredsOnAnonymous401(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} + probes := 0 + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + probes++ + return newUnauthorizedResponse(req), nil + })) + conn.CredentialHelper = helper + + conn.EnsureAuthForService(context.Background(), "git-receive-pack") + + if conn.Auth == nil { + t.Fatal("expected conn.Auth to be tentatively set from helper") + } + if got := helper.count("lookup"); got != 1 { + t.Errorf("expected 1 helper lookup, got %d", got) + } + if got := helper.count("approve"); got != 0 { + t.Errorf("probe must not Approve — let the real operation validate. got %d", got) + } + if got := helper.count("reject"); got != 0 { + t.Errorf("probe must not Reject either. got %d", got) + } + if probes != 1 { + t.Errorf("expected exactly 1 probe (no retry), got %d", probes) + } +} + +func TestEnsureAuthForService_NoHelperIsNoOp(t *testing.T) { + called := false + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + called = true + return newAdvertisementResponse(req), nil + })) + + conn.EnsureAuthForService(context.Background(), "git-receive-pack") + + if called { + t.Error("expected EnsureAuthForService to be a no-op without a helper") + } + if conn.Auth != nil { + t.Error("expected conn.Auth to remain nil") + } +} + +func TestEnsureAuthForService_AnonymousServiceLeavesAuthNil(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusMethodNotAllowed, + Request: req, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + }, nil + })) + conn.CredentialHelper = helper + + conn.EnsureAuthForService(context.Background(), "git-receive-pack") + + if conn.Auth != nil { + t.Error("expected conn.Auth to remain nil when probe gets non-401") + } + if got := helper.count("approve"); got != 0 { + t.Errorf("must not Approve when probe didn't 401, got %d", got) + } +} + +// TestEnsureAuthForService_SkipsProbeWhenHelperHasNoCredentials avoids a +// wasted no-op POST when there are no credentials to attach anyway — the +// common shape for anonymous syncs and for syncs running in test/CI +// environments with no credential helper configured. +func TestEnsureAuthForService_SkipsProbeWhenHelperHasNoCredentials(t *testing.T) { + helper := &fakeCredentialHelper{ok: false} + called := false + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + called = true + return newUnauthorizedResponse(req), nil + })) + conn.CredentialHelper = helper + + conn.EnsureAuthForService(context.Background(), "git-receive-pack") + + if called { + t.Error("expected no probe when the helper has no credentials") + } + if conn.Auth != nil { + t.Error("expected conn.Auth to remain nil") + } +} + +// TestEnsureAuthForService_RealPostApprovesTentativeCreds covers the +// production push shape: probe attaches helper creds tentatively; the +// real POST succeeds, which is the actual proof creds are valid. Only +// then do we Approve in the helper. +func TestEnsureAuthForService_RealPostApprovesTentativeCreds(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} + var authHeaders []string + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + authHeaders = append(authHeaders, req.Header.Get("Authorization")) + if req.Header.Get("Authorization") == "" { + return newUnauthorizedResponse(req), nil + } + res := &http.Response{ + StatusCode: http.StatusOK, Request: req, Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + } + res.Header.Set("Content-Type", "application/x-git-receive-pack-result") + return res, nil + })) + conn.CredentialHelper = helper + + conn.EnsureAuthForService(context.Background(), "git-receive-pack") + body := io.MultiReader(strings.NewReader("0000"), strings.NewReader("")) + reader, err := PostRPCStreamBody(context.Background(), conn, "git-receive-pack", body, false, "phase") + if err != nil { + t.Fatalf("PostRPCStreamBody: %v", err) + } + _ = reader.Close() + + // 2 requests: probe-anon (401) + POST-authed (200). No second probe. + if len(authHeaders) != 2 { + t.Fatalf("expected 2 requests, got %d: %v", len(authHeaders), authHeaders) + } + if authHeaders[0] != "" { + t.Errorf("probe should be anonymous, got %q", authHeaders[0]) + } + if !strings.HasPrefix(authHeaders[1], "Basic ") { + t.Errorf("real POST should carry the helper creds, got %q", authHeaders[1]) + } + if got := helper.count("approve"); got != 1 { + t.Errorf("expected 1 Approve after the real POST succeeded, got %d", got) + } +} + +// TestEnsureAuthForService_RealPostRejectsTentativeCreds: helper supplied +// stale credentials, the real POST 401s, which is the definitive signal +// to reject them and clear c.Auth. +func TestEnsureAuthForService_RealPostRejectsTentativeCreds(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "stale", ok: true} + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + // Every request 401s (helper creds are stale). + return newUnauthorizedResponse(req), nil + })) + conn.CredentialHelper = helper + + conn.EnsureAuthForService(context.Background(), "git-receive-pack") + if conn.Auth == nil { + t.Fatal("expected conn.Auth to be tentatively set after probe 401") + } + body := io.MultiReader(strings.NewReader("0000"), strings.NewReader("")) + _, err := PostRPCStreamBody(context.Background(), conn, "git-receive-pack", body, false, "phase") + if err == nil { + t.Fatal("expected error from POST with stale creds") + } + if got := helper.count("reject"); got != 1 { + t.Errorf("expected 1 Reject after POST 401, got %d", got) + } + if got := helper.count("approve"); got != 0 { + t.Errorf("expected 0 Approve calls, got %d", got) + } + if conn.Auth != nil { + t.Error("expected conn.Auth to be cleared after rejecting bad creds") + } +} + +// TestEnsureAuthForService_ProbesWithPOSTAndFlushPacketBody verifies the +// probe uses the same HTTP method as the real operation (POST), with a +// minimal "0000" flush packet body — a valid no-op receive-pack push by +// the smart-HTTP spec. Probing with POST is essential: servers that +// gate only the POST handler (not GET) would otherwise slip past us. +func TestEnsureAuthForService_ProbesWithPOSTAndFlushPacketBody(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} + var probeMethod string + var probeBody []byte + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + probeMethod = req.Method + if req.Body != nil { + b, err := io.ReadAll(req.Body) + if err != nil { + t.Fatalf("read probe body: %v", err) + } + probeBody = b + } + return newUnauthorizedResponse(req), nil + })) + conn.CredentialHelper = helper + + conn.EnsureAuthForService(context.Background(), "git-receive-pack") + + if probeMethod != http.MethodPost { + t.Errorf("expected probe method POST, got %q", probeMethod) + } + if string(probeBody) != "0000" { + t.Errorf("expected probe body to be the flush packet '0000', got %q", probeBody) + } +} + +// TestEnsureAuthForService_DetectsAuthGatedPostEvenWhenGetIsAnonymous: +// the gap a GET-based probe would miss — server returns 404 to GET +// (the receive-pack endpoint isn't a GET resource) but 401 to POST. +// A POST probe correctly detects the auth requirement. +func TestEnsureAuthForService_DetectsAuthGatedPostEvenWhenGetIsAnonymous(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} + var methods []string + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + methods = append(methods, req.Method) + if req.Method == http.MethodGet { + return &http.Response{ + StatusCode: http.StatusNotFound, Request: req, Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + }, nil + } + // POST: server requires auth. + if req.Header.Get("Authorization") == "" { + return newUnauthorizedResponse(req), nil + } + return &http.Response{ + StatusCode: http.StatusOK, Request: req, Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + }, nil + })) + conn.CredentialHelper = helper + + conn.EnsureAuthForService(context.Background(), "git-receive-pack") + + if conn.Auth == nil { + t.Fatal("expected probe to detect POST auth requirement and attach helper creds") + } + if got := helper.count("lookup"); got != 1 { + t.Errorf("expected 1 helper lookup, got %d", got) + } + for _, m := range methods { + if m == http.MethodGet { + t.Errorf("probe should not use GET — server may serve GET differently than POST") + } + } +} + +// TestEnsureAuthForService_405ProbeWithCredsDoesNotPoisonHelper is the +// specific regression: previously, a 405 to the authenticated probe was +// interpreted as "creds accepted" and Approve was called — even though +// the server may have rejected the method before reading Authorization. +// The new contract never approves from a probe response at all. +func TestEnsureAuthForService_405ProbeWithCredsDoesNotPoisonHelper(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "stale", ok: true} + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + if req.Header.Get("Authorization") == "" { + return newUnauthorizedResponse(req), nil + } + // Server returns 405 without checking auth. + return &http.Response{ + StatusCode: http.StatusMethodNotAllowed, + Request: req, Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + }, nil + })) + conn.CredentialHelper = helper + + conn.EnsureAuthForService(context.Background(), "git-receive-pack") + + if got := helper.count("approve"); got != 0 { + t.Errorf("405 probe response must not Approve stale creds (got %d Approve calls)", got) + } +} + +type credCall struct { + op string // "lookup", "approve", "reject" + user string + pass string + url string // the *url.URL passed to the helper, stringified +} + +// fakeCredentialHelper is a test CredentialHelper. Set user/pass/ok/err to +// configure Lookup; inspect calls (via count/last) to assert lifecycle. +type fakeCredentialHelper struct { + user, pass string + ok bool + err error + + calls []credCall +} + +func (h *fakeCredentialHelper) Lookup(_ context.Context, ep *url.URL) (string, string, bool, error) { + h.calls = append(h.calls, credCall{op: "lookup", url: ep.String()}) + return h.user, h.pass, h.ok, h.err +} + +func (h *fakeCredentialHelper) Approve(_ context.Context, ep *url.URL, user, pass string) { + h.calls = append(h.calls, credCall{op: "approve", user: user, pass: pass, url: ep.String()}) +} + +func (h *fakeCredentialHelper) Reject(_ context.Context, ep *url.URL, user, pass string) { + h.calls = append(h.calls, credCall{op: "reject", user: user, pass: pass, url: ep.String()}) +} + +func (h *fakeCredentialHelper) count(op string) int { + n := 0 + for _, c := range h.calls { + if c.op == op { + n++ + } + } + return n +} + +func (h *fakeCredentialHelper) last(op string) *credCall { + for i := len(h.calls) - 1; i >= 0; i-- { + if h.calls[i].op == op { + return &h.calls[i] + } + } + return nil +} + type roundTripReader struct { remaining int } diff --git a/internal/syncer/auth_test.go b/internal/syncer/auth_test.go index 7e2019a6..d22a51ac 100644 --- a/internal/syncer/auth_test.go +++ b/internal/syncer/auth_test.go @@ -24,10 +24,10 @@ func TestResolveAuthMethodPrefersExplicitToken(t *testing.T) { t.Fatalf("new endpoint: %v", err) } - originalFill := auth.GitCredentialFillCommand - t.Cleanup(func() { auth.GitCredentialFillCommand = originalFill }) - auth.GitCredentialFillCommand = func(_ context.Context, input string) ([]byte, error) { - t.Fatalf("unexpected git credential fill call with input %q", input) + originalCred := auth.GitCredentialCommand + t.Cleanup(func() { auth.GitCredentialCommand = originalCred }) + auth.GitCredentialCommand = func(_ context.Context, op auth.CredentialOp, input string) ([]byte, error) { + t.Fatalf("unexpected git credential %s call with input %q", op, input) return nil, nil } @@ -118,10 +118,10 @@ func TestResolveAuthMethodUsesEntireDBStoredToken(t *testing.T) { t.Fatalf("write token: %v", err) } - originalFill := auth.GitCredentialFillCommand - t.Cleanup(func() { auth.GitCredentialFillCommand = originalFill }) - auth.GitCredentialFillCommand = func(_ context.Context, input string) ([]byte, error) { - t.Fatalf("unexpected git credential fill call with input %q", input) + originalCred := auth.GitCredentialCommand + t.Cleanup(func() { auth.GitCredentialCommand = originalCred }) + auth.GitCredentialCommand = func(_ context.Context, op auth.CredentialOp, input string) ([]byte, error) { + t.Fatalf("unexpected git credential %s call with input %q", op, input) return nil, nil } diff --git a/internal/syncer/integration_test.go b/internal/syncer/integration_test.go index 43da3598..35487c8e 100644 --- a/internal/syncer/integration_test.go +++ b/internal/syncer/integration_test.go @@ -1700,11 +1700,11 @@ func TestRun_IntegrationUsesGitCredentialHelperFallback(t *testing.T) { defer sourceServer.Close() defer targetServer.Close() - originalFill := auth.GitCredentialFillCommand + originalCred := auth.GitCredentialCommand t.Cleanup(func() { - auth.GitCredentialFillCommand = originalFill + auth.GitCredentialCommand = originalCred }) - auth.GitCredentialFillCommand = func(_ context.Context, input string) ([]byte, error) { + auth.GitCredentialCommand = func(_ context.Context, op auth.CredentialOp, input string) ([]byte, error) { if !strings.Contains(input, "protocol=http\n") { t.Fatalf("expected protocol in credential input, got %q", input) } @@ -1714,7 +1714,15 @@ func TestRun_IntegrationUsesGitCredentialHelperFallback(t *testing.T) { if !strings.Contains(input, "path=repo.git\n") { t.Fatalf("expected repo path in credential input, got %q", input) } - return []byte("username=" + username + "\npassword=" + password + "\n\n"), nil + switch op { + case auth.CredentialOpFill: + return []byte("username=" + username + "\npassword=" + password + "\n\n"), nil + case auth.CredentialOpApprove, auth.CredentialOpReject: + return nil, nil + default: + t.Fatalf("unexpected git credential op %q", op) + return nil, nil + } } result, err := Run(context.Background(), Config{ diff --git a/internal/syncer/syncer.go b/internal/syncer/syncer.go index 6f990235..8b6a9085 100644 --- a/internal/syncer/syncer.go +++ b/internal/syncer/syncer.go @@ -377,6 +377,9 @@ func newConn(raw Endpoint, label string, stats *statsCollector, httpClient *http client := instrumentHTTPClient(httpClient, raw.SkipTLSVerify, label, stats) conn := gitproto.NewHTTPConnWithClient(ep, label, authMethod, client) conn.FollowInfoRefsRedirect = raw.FollowInfoRefsRedirect + if authMethod == nil { + conn.CredentialHelper = auth.GitCredentialHelper{} + } return conn, nil }