From 1233de5dc1ff6012ac962c8de206a72783fee44a Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 13 Jul 2026 04:40:32 -0400 Subject: [PATCH 1/2] fix(provider): secure sidecar transport Require TLS for non-loopback listeners and reject redirect or hostname downgrade paths. Allow explicit private CA trust without disabling certificate verification. --- README.md | 9 + cmd/github-actions-runner-job/main.go | 53 ++++- cmd/github-actions-runner-job/main_test.go | 215 ++++++++++++++++++++- cmd/github-runner-provider/main.go | 49 ++++- cmd/github-runner-provider/main_test.go | 105 ++++++++++ 5 files changed, 426 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index f284db2..81fca2b 100644 --- a/README.md +++ b/README.md @@ -57,6 +57,15 @@ GITHUB_RUNNER_PROVIDER_STATE_DIR=/var/lib/workflow-github-runner-provider \ bin/github-runner-provider 127.0.0.1:8090 ``` +For a provider endpoint reachable outside host loopback, configure TLS with both +`GITHUB_RUNNER_PROVIDER_TLS_CERT_FILE` and +`GITHUB_RUNNER_PROVIDER_TLS_KEY_FILE`. The runner job continues to reject +plaintext bearer transport except for literal loopback IP endpoints and never +follows provider redirects. A private certificate authority can be supplied to +the runner job as base64-encoded PEM in +`COMPUTE_GITHUB_RUNNER_PROVIDER_CA_CERT_B64`; certificate verification and +hostname verification remain enabled. + workflow-compute should point at that service with `COMPUTE_GITHUB_RUNNER_PROVIDER_URL` and `COMPUTE_GITHUB_RUNNER_PROVIDER_TOKEN`; it should not receive `GITHUB_TOKEN`. diff --git a/cmd/github-actions-runner-job/main.go b/cmd/github-actions-runner-job/main.go index 0c1a79b..6fd9171 100644 --- a/cmd/github-actions-runner-job/main.go +++ b/cmd/github-actions-runner-job/main.go @@ -6,6 +6,9 @@ import ( "bytes" "compress/gzip" "context" + "crypto/tls" + "crypto/x509" + "encoding/base64" "encoding/json" "errors" "flag" @@ -389,10 +392,14 @@ func newSidecarClientFromEnv() (*providerSidecarClient, error) { return nil, fmt.Errorf("provider URL must be absolute and must not contain credentials, query, or fragment") } host := parsedURL.Hostname() - loopbackHTTP := parsedURL.Scheme == "http" && (strings.EqualFold(host, "localhost") || net.ParseIP(host).IsLoopback()) + loopbackHTTP := parsedURL.Scheme == "http" && net.ParseIP(host).IsLoopback() if parsedURL.Scheme != "https" && !loopbackHTTP { return nil, fmt.Errorf("provider URL must use HTTPS except for a loopback HTTP endpoint") } + httpClient, err := providerSidecarHTTPClient(parsedURL) + if err != nil { + return nil, err + } token := strings.TrimSpace(os.Getenv("COMPUTE_GITHUB_RUNNER_PROVIDER_TOKEN")) if token == "" { return nil, fmt.Errorf("COMPUTE_GITHUB_RUNNER_PROVIDER_TOKEN is required") @@ -406,10 +413,52 @@ func newSidecarClientFromEnv() (*providerSidecarClient, error) { return &providerSidecarClient{ baseURL: strings.TrimRight(parsedURL.String(), "/"), token: token, - http: &http.Client{Timeout: 30 * time.Second}, + http: httpClient, }, nil } +func providerSidecarHTTPClient(parsedURL *url.URL) (*http.Client, error) { + client := &http.Client{ + Timeout: 30 * time.Second, + CheckRedirect: func(*http.Request, []*http.Request) error { + return errors.New("provider sidecar redirects are forbidden") + }, + } + encodedCA := strings.TrimSpace(os.Getenv("COMPUTE_GITHUB_RUNNER_PROVIDER_CA_CERT_B64")) + if encodedCA == "" { + return client, nil + } + if parsedURL.Scheme != "https" { + return nil, errors.New("provider CA certificate requires an HTTPS provider URL") + } + caPEM, err := base64.StdEncoding.DecodeString(encodedCA) + if err != nil { + return nil, fmt.Errorf("decode provider CA certificate: %w", err) + } + roots, err := providerCertificatePool(caPEM, x509.SystemCertPool) + if err != nil { + return nil, err + } + transport := http.DefaultTransport.(*http.Transport).Clone() + transport.TLSClientConfig = &tls.Config{ + MinVersion: tls.VersionTLS12, + RootCAs: roots, + } + client.Transport = transport + return client, nil +} + +func providerCertificatePool(caPEM []byte, loadSystemRoots func() (*x509.CertPool, error)) (*x509.CertPool, error) { + roots, err := loadSystemRoots() + if err != nil || roots == nil { + roots = x509.NewCertPool() + } + if !roots.AppendCertsFromPEM(caPEM) { + return nil, errors.New("provider CA certificate must contain at least one PEM certificate") + } + return roots, nil +} + func (c *providerSidecarClient) orgJITConfig(ctx context.Context, req internal.EphemeralRunnerJobRequest, spec internal.EphemeralRunnerJobSpec) (internal.GitHubRunnerJITRegistration, error) { requestPath := "/v1/actions/orgs/" + url.PathEscape(req.Organization) + "/runners/jitconfig" body := struct { diff --git a/cmd/github-actions-runner-job/main_test.go b/cmd/github-actions-runner-job/main_test.go index b779ad0..8314866 100644 --- a/cmd/github-actions-runner-job/main_test.go +++ b/cmd/github-actions-runner-job/main_test.go @@ -5,10 +5,14 @@ import ( "bytes" "compress/gzip" "context" + "crypto/x509" + "encoding/base64" "encoding/json" + "encoding/pem" "errors" "fmt" "io" + "net" "net/http" "net/http/httptest" "os" @@ -2827,7 +2831,7 @@ func TestT915ProviderSidecarURLRejectsInsecureBearerTransport(t *testing.T) { }{ {name: "https", baseURL: "https://provider.example.invalid", valid: true}, {name: "loopback IPv4", baseURL: "http://127.0.0.1:8090", valid: true}, - {name: "loopback hostname", baseURL: "http://localhost:8090", valid: true}, + {name: "loopback hostname", baseURL: "http://localhost:8090", valid: false}, {name: "external plaintext", baseURL: "http://provider.example.invalid", valid: false}, {name: "credentials", baseURL: "https://user@provider.example.invalid", valid: false}, {name: "query", baseURL: "https://provider.example.invalid?token=unsafe", valid: false}, @@ -2846,6 +2850,215 @@ func TestT915ProviderSidecarURLRejectsInsecureBearerTransport(t *testing.T) { } } +func TestT915ProviderSidecarRejectsRedirects(t *testing.T) { + var redirected bool + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/redirected" { + redirected = true + _ = json.NewEncoder(w).Encode(map[string]string{"status": "ok"}) + return + } + http.Redirect(w, r, "/redirected", http.StatusFound) + })) + defer server.Close() + + caPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: server.Certificate().Raw}) + t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_URL", server.URL) + t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_TOKEN", "provider-token") + t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_CA_CERT_B64", base64.StdEncoding.EncodeToString(caPEM)) + + client, err := newSidecarClientFromEnv() + if err != nil { + t.Fatalf("create sidecar client: %v", err) + } + if err := client.do(context.Background(), http.MethodGet, "/healthz", nil, http.StatusOK, nil); err == nil || !strings.Contains(err.Error(), "redirect") { + t.Fatalf("redirect error = %v", err) + } + if redirected { + t.Fatal("provider client followed redirect") + } +} + +func TestT915ProviderSidecarHTTPSAcceptsExplicitCertificateAuthority(t *testing.T) { + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("Authorization"); got != "Bearer provider-token" { + t.Fatalf("authorization = %q", got) + } + _ = json.NewEncoder(w).Encode(map[string]string{"status": "ok"}) + })) + defer server.Close() + + caPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: server.Certificate().Raw}) + t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_URL", server.URL) + t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_TOKEN", "provider-token") + t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_CA_CERT_B64", base64.StdEncoding.EncodeToString(caPEM)) + + client, err := newSidecarClientFromEnv() + if err != nil { + t.Fatalf("create sidecar client with explicit CA: %v", err) + } + var health map[string]string + if err := client.do(context.Background(), http.MethodGet, "/healthz", nil, http.StatusOK, &health); err != nil { + t.Fatalf("call sidecar using explicit CA: %v", err) + } + if health["status"] != "ok" { + t.Fatalf("health = %+v", health) + } +} + +func TestT915ProviderCommandAndRunnerClientCompleteTLSHealthRoundTrip(t *testing.T) { + tlsFixture := httptest.NewTLSServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + certificate := tlsFixture.TLS.Certificates[0] + tlsFixture.Close() + + tlsDir := t.TempDir() + certFile := filepath.Join(tlsDir, "provider.crt") + keyFile := filepath.Join(tlsDir, "provider.key") + certPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certificate.Certificate[0]}) + keyDER, err := x509.MarshalPKCS8PrivateKey(certificate.PrivateKey) + if err != nil { + t.Fatalf("marshal provider private key: %v", err) + } + keyPEM := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: keyDER}) + if err := os.WriteFile(certFile, certPEM, 0o600); err != nil { + t.Fatalf("write provider certificate: %v", err) + } + if err := os.WriteFile(keyFile, keyPEM, 0o600); err != nil { + t.Fatalf("write provider key: %v", err) + } + + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("reserve provider address: %v", err) + } + providerAddress := listener.Addr().String() + if err := listener.Close(); err != nil { + t.Fatalf("release provider address: %v", err) + } + + providerBinary := filepath.Join(t.TempDir(), "github-runner-provider") + build := exec.Command("go", "build", "-o", providerBinary, "../github-runner-provider") + if output, err := build.CombinedOutput(); err != nil { + t.Fatalf("build real provider command: %v\n%s", err, output) + } + providerLogPath := filepath.Join(t.TempDir(), "provider.log") + providerLog, err := os.OpenFile(providerLogPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) + if err != nil { + t.Fatalf("open provider log: %v", err) + } + provider := exec.Command(providerBinary, providerAddress) + provider.Stdout = providerLog + provider.Stderr = providerLog + provider.Env = append(os.Environ(), + "GITHUB_RUNNER_PROVIDER_GITHUB_TOKEN=test-github-token", + "GITHUB_RUNNER_PROVIDER_TOKEN=test-provider-token", + "GITHUB_RUNNER_PROVIDER_REPOSITORIES=GoCodeAlone/workflow-compute", + "GITHUB_RUNNER_PROVIDER_ORGANIZATIONS=GoCodeAlone", + "GITHUB_RUNNER_PROVIDER_RUNNER_GROUPS=ephemeral", + "GITHUB_RUNNER_PROVIDER_STATE_DIR="+filepath.Join(tlsDir, "state"), + "GITHUB_RUNNER_PROVIDER_TLS_CERT_FILE="+certFile, + "GITHUB_RUNNER_PROVIDER_TLS_KEY_FILE="+keyFile, + ) + if err := provider.Start(); err != nil { + _ = providerLog.Close() + t.Fatalf("start real provider command: %v", err) + } + t.Cleanup(func() { + if provider.ProcessState == nil { + _ = provider.Process.Kill() + _ = provider.Wait() + } + _ = providerLog.Close() + }) + + t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_URL", "https://"+providerAddress) + t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_TOKEN", "test-provider-token") + t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_CA_CERT_B64", base64.StdEncoding.EncodeToString(certPEM)) + client, err := newSidecarClientFromEnv() + if err != nil { + t.Fatalf("create production sidecar client: %v", err) + } + var health map[string]string + for attempt := 0; attempt < 50; attempt++ { + err = client.do(context.Background(), http.MethodGet, "/healthz", nil, http.StatusOK, &health) + if err == nil { + break + } + time.Sleep(100 * time.Millisecond) + } + if err != nil { + _ = providerLog.Close() + output, _ := os.ReadFile(providerLogPath) + t.Fatalf("real provider TLS health request: %v\n%s", err, output) + } + if health["status"] != "ok" { + t.Fatalf("real provider health = %+v", health) + } +} + +func TestT915ProviderSidecarCertificateAuthorityKeepsHostnameVerification(t *testing.T) { + server := httptest.NewTLSServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + defer server.Close() + + caPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: server.Certificate().Raw}) + t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_URL", strings.Replace(server.URL, "127.0.0.1", "localhost", 1)) + t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_TOKEN", "provider-token") + t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_CA_CERT_B64", base64.StdEncoding.EncodeToString(caPEM)) + + client, err := newSidecarClientFromEnv() + if err != nil { + t.Fatalf("create sidecar client with explicit CA: %v", err) + } + if err := client.do(context.Background(), http.MethodGet, "/healthz", nil, http.StatusOK, nil); err == nil || !strings.Contains(err.Error(), "certificate") { + t.Fatalf("hostname mismatch error = %v", err) + } +} + +func TestT915ProviderSidecarCertificateAuthorityRequiresHTTPS(t *testing.T) { + t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_URL", "http://127.0.0.1:8090") + t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_TOKEN", "provider-token") + t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_CA_CERT_B64", base64.StdEncoding.EncodeToString([]byte("unused"))) + if _, err := newSidecarClientFromEnv(); err == nil || !strings.Contains(err.Error(), "requires an HTTPS provider URL") { + t.Fatalf("plaintext provider CA error = %v", err) + } +} + +func TestT915ProviderSidecarRejectsMalformedCertificateAuthority(t *testing.T) { + for _, encoded := range []string{ + "not-base64", + base64.StdEncoding.EncodeToString([]byte("not a PEM certificate")), + } { + t.Run(encoded, func(t *testing.T) { + t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_URL", "https://provider.example.invalid") + t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_TOKEN", "provider-token") + t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_CA_CERT_B64", encoded) + if _, err := newSidecarClientFromEnv(); err == nil || !strings.Contains(err.Error(), "provider CA certificate") { + t.Fatalf("malformed provider CA error = %v", err) + } + }) + } +} + +func TestT915ProviderCertificateAuthorityWorksWithoutSystemRoots(t *testing.T) { + server := httptest.NewTLSServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + defer server.Close() + caPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: server.Certificate().Raw}) + + pool, err := providerCertificatePool(caPEM, func() (*x509.CertPool, error) { + return nil, errors.New("system roots unavailable") + }) + if err != nil { + t.Fatalf("build explicit provider certificate pool: %v", err) + } + certificate, err := x509.ParseCertificate(server.Certificate().Raw) + if err != nil { + t.Fatalf("parse explicit provider certificate: %v", err) + } + if _, err := certificate.Verify(x509.VerifyOptions{Roots: pool}); err != nil { + t.Fatalf("verify explicit provider certificate: %v", err) + } +} + func writeExecutable(t *testing.T, path, content string) { t.Helper() if err := os.WriteFile(path, []byte(content), 0o755); err != nil { diff --git a/cmd/github-runner-provider/main.go b/cmd/github-runner-provider/main.go index dc73b61..c31b7c6 100644 --- a/cmd/github-runner-provider/main.go +++ b/cmd/github-runner-provider/main.go @@ -4,9 +4,11 @@ package main import ( "context" + "crypto/tls" "errors" "fmt" "log/slog" + "net" "net/http" "os" "os/signal" @@ -23,6 +25,11 @@ type providerHTTPShutdowner interface { Close() error } +type providerHTTPServer interface { + ListenAndServe() error + ListenAndServeTLS(certFile, keyFile string) error +} + type providerServiceStopper interface { Stop(context.Context) error } @@ -46,14 +53,21 @@ func run(ctx context.Context, logger *slog.Logger, args []string) error { if err != nil { return err } + tlsCertFile, tlsKeyFile, err := providerTLSFilesFromEnvironment() + if err != nil { + return err + } + if err := validateProviderTransport(addr, tlsCertFile, tlsKeyFile); err != nil { + return err + } service, err := internal.NewGitHubRunnerProviderHTTPService("github-runner-provider", config) if err != nil { return err } server := newProviderHTTPServer(addr, service.Handler()) - logger.InfoContext(ctx, "starting github-runner-provider", "addr", addr) + logger.InfoContext(ctx, "starting github-runner-provider", "addr", addr, "tls", tlsCertFile != "") serveDone := make(chan error, 1) - go func() { serveDone <- server.ListenAndServe() }() + go func() { serveDone <- serveProviderHTTP(server, tlsCertFile, tlsKeyFile) }() select { case serveErr := <-serveDone: return errors.Join(normalizeProviderServeError(serveErr), stopProvider(service, providerShutdownTimeout)) @@ -64,6 +78,36 @@ func run(ctx context.Context, logger *slog.Logger, args []string) error { } } +func validateProviderTransport(addr, certFile, keyFile string) error { + if certFile != "" && keyFile != "" { + return nil + } + host, _, err := net.SplitHostPort(addr) + if err != nil { + return fmt.Errorf("provider listen address must be host:port: %w", err) + } + if ip := net.ParseIP(host); ip == nil || !ip.IsLoopback() { + return errors.New("provider listener requires TLS outside a literal loopback address") + } + return nil +} + +func providerTLSFilesFromEnvironment() (string, string, error) { + certFile := strings.TrimSpace(os.Getenv("GITHUB_RUNNER_PROVIDER_TLS_CERT_FILE")) + keyFile := strings.TrimSpace(os.Getenv("GITHUB_RUNNER_PROVIDER_TLS_KEY_FILE")) + if (certFile == "") != (keyFile == "") { + return "", "", errors.New("GITHUB_RUNNER_PROVIDER_TLS_CERT_FILE and GITHUB_RUNNER_PROVIDER_TLS_KEY_FILE must be set together") + } + return certFile, keyFile, nil +} + +func serveProviderHTTP(server providerHTTPServer, certFile, keyFile string) error { + if certFile == "" { + return server.ListenAndServe() + } + return server.ListenAndServeTLS(certFile, keyFile) +} + func waitForProviderServeDone(serveDone <-chan error, timeout time.Duration) error { timer := time.NewTimer(timeout) defer timer.Stop() @@ -79,6 +123,7 @@ func newProviderHTTPServer(addr string, handler http.Handler) *http.Server { return &http.Server{ Addr: addr, Handler: handler, + TLSConfig: &tls.Config{MinVersion: tls.VersionTLS12}, ReadHeaderTimeout: 5 * time.Second, ReadTimeout: 10 * time.Second, WriteTimeout: 35 * time.Second, diff --git a/cmd/github-runner-provider/main_test.go b/cmd/github-runner-provider/main_test.go index a987679..d67eed7 100644 --- a/cmd/github-runner-provider/main_test.go +++ b/cmd/github-runner-provider/main_test.go @@ -2,6 +2,7 @@ package main import ( "context" + "crypto/tls" "errors" "io" "log/slog" @@ -35,6 +36,25 @@ type contextCheckingStopper struct { expired bool } +type recordingProviderHTTPServer struct { + plainCalls int + tlsCalls int + certFile string + keyFile string +} + +func (s *recordingProviderHTTPServer) ListenAndServe() error { + s.plainCalls++ + return http.ErrServerClosed +} + +func (s *recordingProviderHTTPServer) ListenAndServeTLS(certFile, keyFile string) error { + s.tlsCalls++ + s.certFile = certFile + s.keyFile = keyFile + return http.ErrServerClosed +} + func (s *contextCheckingStopper) Stop(ctx context.Context) error { s.called = true s.expired = ctx.Err() != nil @@ -46,6 +66,91 @@ func TestProviderHTTPServerHasBoundedConnectionTimeouts(t *testing.T) { if server.ReadHeaderTimeout <= 0 || server.ReadTimeout <= 0 || server.WriteTimeout <= 0 || server.IdleTimeout <= 0 { t.Fatalf("provider HTTP timeouts are not fully bounded: %+v", server) } + if server.TLSConfig == nil || server.TLSConfig.MinVersion < tls.VersionTLS12 { + t.Fatalf("provider TLS minimum is not pinned to TLS 1.2+: %+v", server.TLSConfig) + } +} + +func TestProviderTLSFilesRequireCertificateAndKeyTogether(t *testing.T) { + for _, tc := range []struct { + name string + certFile string + keyFile string + wantErr bool + }{ + {name: "disabled"}, + {name: "enabled", certFile: "/tls/provider.crt", keyFile: "/tls/provider.key"}, + {name: "certificate only", certFile: "/tls/provider.crt", wantErr: true}, + {name: "key only", keyFile: "/tls/provider.key", wantErr: true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("GITHUB_RUNNER_PROVIDER_TLS_CERT_FILE", tc.certFile) + t.Setenv("GITHUB_RUNNER_PROVIDER_TLS_KEY_FILE", tc.keyFile) + certFile, keyFile, err := providerTLSFilesFromEnvironment() + if tc.wantErr { + if err == nil || !strings.Contains(err.Error(), "must be set together") { + t.Fatalf("TLS file error = %v", err) + } + return + } + if err != nil { + t.Fatalf("TLS files: %v", err) + } + if certFile != tc.certFile || keyFile != tc.keyFile { + t.Fatalf("TLS files = %q, %q", certFile, keyFile) + } + }) + } +} + +func TestProviderTransportRequiresTLSOutsideLiteralLoopback(t *testing.T) { + for _, tc := range []struct { + name string + addr string + certFile string + keyFile string + wantErr bool + }{ + {name: "IPv4 loopback plaintext", addr: "127.0.0.1:8090"}, + {name: "IPv6 loopback plaintext", addr: "[::1]:8090"}, + {name: "hostname plaintext", addr: "localhost:8090", wantErr: true}, + {name: "wildcard plaintext", addr: "0.0.0.0:8090", wantErr: true}, + {name: "empty host plaintext", addr: ":8090", wantErr: true}, + {name: "external plaintext", addr: "192.0.2.10:8090", wantErr: true}, + {name: "wildcard TLS", addr: "0.0.0.0:8090", certFile: "/tls/provider.crt", keyFile: "/tls/provider.key"}, + {name: "malformed", addr: "not-an-address", wantErr: true}, + } { + t.Run(tc.name, func(t *testing.T) { + err := validateProviderTransport(tc.addr, tc.certFile, tc.keyFile) + if tc.wantErr { + if err == nil { + t.Fatal("unsafe provider transport accepted") + } + return + } + if err != nil { + t.Fatalf("safe provider transport rejected: %v", err) + } + }) + } +} + +func TestServeProviderHTTPUsesConfiguredTransport(t *testing.T) { + plain := &recordingProviderHTTPServer{} + if err := serveProviderHTTP(plain, "", ""); !errors.Is(err, http.ErrServerClosed) { + t.Fatalf("plain serve: %v", err) + } + if plain.plainCalls != 1 || plain.tlsCalls != 0 { + t.Fatalf("plain calls = %+v", plain) + } + + tlsServer := &recordingProviderHTTPServer{} + if err := serveProviderHTTP(tlsServer, "/tls/provider.crt", "/tls/provider.key"); !errors.Is(err, http.ErrServerClosed) { + t.Fatalf("TLS serve: %v", err) + } + if tlsServer.plainCalls != 0 || tlsServer.tlsCalls != 1 || tlsServer.certFile != "/tls/provider.crt" || tlsServer.keyFile != "/tls/provider.key" { + t.Fatalf("TLS calls = %+v", tlsServer) + } } func TestShutdownProviderGivesServiceIndependentCleanupDeadline(t *testing.T) { From 1f7d2e8a105981c9fdff38b45e33c9db55c5cc3f Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 13 Jul 2026 04:53:43 -0400 Subject: [PATCH 2/2] fix(provider): bind before reporting readiness Own and close the listener across TLS setup failures so readiness reports the actual bound address without a port-allocation race. --- cmd/github-actions-runner-job/main_test.go | 30 ++++++++++------- cmd/github-runner-provider/main.go | 19 +++++++---- cmd/github-runner-provider/main_test.go | 38 ++++++++++++++++++---- 3 files changed, 63 insertions(+), 24 deletions(-) diff --git a/cmd/github-actions-runner-job/main_test.go b/cmd/github-actions-runner-job/main_test.go index 8314866..a73bc44 100644 --- a/cmd/github-actions-runner-job/main_test.go +++ b/cmd/github-actions-runner-job/main_test.go @@ -12,13 +12,13 @@ import ( "errors" "fmt" "io" - "net" "net/http" "net/http/httptest" "os" "os/exec" "path" "path/filepath" + "regexp" "slices" "strconv" "strings" @@ -2927,15 +2927,6 @@ func TestT915ProviderCommandAndRunnerClientCompleteTLSHealthRoundTrip(t *testing t.Fatalf("write provider key: %v", err) } - listener, err := net.Listen("tcp", "127.0.0.1:0") - if err != nil { - t.Fatalf("reserve provider address: %v", err) - } - providerAddress := listener.Addr().String() - if err := listener.Close(); err != nil { - t.Fatalf("release provider address: %v", err) - } - providerBinary := filepath.Join(t.TempDir(), "github-runner-provider") build := exec.Command("go", "build", "-o", providerBinary, "../github-runner-provider") if output, err := build.CombinedOutput(); err != nil { @@ -2946,7 +2937,7 @@ func TestT915ProviderCommandAndRunnerClientCompleteTLSHealthRoundTrip(t *testing if err != nil { t.Fatalf("open provider log: %v", err) } - provider := exec.Command(providerBinary, providerAddress) + provider := exec.Command(providerBinary, "127.0.0.1:0") provider.Stdout = providerLog provider.Stderr = providerLog provider.Env = append(os.Environ(), @@ -2971,6 +2962,23 @@ func TestT915ProviderCommandAndRunnerClientCompleteTLSHealthRoundTrip(t *testing _ = providerLog.Close() }) + providerAddress := "" + addressPattern := regexp.MustCompile(`\baddr=([^ ]+)`) + for attempt := 0; attempt < 50; attempt++ { + output, _ := os.ReadFile(providerLogPath) + match := addressPattern.FindSubmatch(output) + if len(match) == 2 && string(match[1]) != "127.0.0.1:0" { + providerAddress = string(match[1]) + break + } + time.Sleep(100 * time.Millisecond) + } + if providerAddress == "" { + _ = providerLog.Close() + output, _ := os.ReadFile(providerLogPath) + t.Fatalf("provider did not report its bound address\n%s", output) + } + t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_URL", "https://"+providerAddress) t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_TOKEN", "test-provider-token") t.Setenv("COMPUTE_GITHUB_RUNNER_PROVIDER_CA_CERT_B64", base64.StdEncoding.EncodeToString(certPEM)) diff --git a/cmd/github-runner-provider/main.go b/cmd/github-runner-provider/main.go index c31b7c6..f01ad26 100644 --- a/cmd/github-runner-provider/main.go +++ b/cmd/github-runner-provider/main.go @@ -26,8 +26,8 @@ type providerHTTPShutdowner interface { } type providerHTTPServer interface { - ListenAndServe() error - ListenAndServeTLS(certFile, keyFile string) error + Serve(net.Listener) error + ServeTLS(net.Listener, string, string) error } type providerServiceStopper interface { @@ -65,9 +65,13 @@ func run(ctx context.Context, logger *slog.Logger, args []string) error { return err } server := newProviderHTTPServer(addr, service.Handler()) - logger.InfoContext(ctx, "starting github-runner-provider", "addr", addr, "tls", tlsCertFile != "") + listener, err := net.Listen("tcp", addr) + if err != nil { + return errors.Join(fmt.Errorf("listen on provider address: %w", err), stopProvider(service, providerShutdownTimeout)) + } + logger.InfoContext(ctx, "starting github-runner-provider", "addr", listener.Addr().String(), "tls", tlsCertFile != "") serveDone := make(chan error, 1) - go func() { serveDone <- serveProviderHTTP(server, tlsCertFile, tlsKeyFile) }() + go func() { serveDone <- serveProviderHTTP(server, listener, tlsCertFile, tlsKeyFile) }() select { case serveErr := <-serveDone: return errors.Join(normalizeProviderServeError(serveErr), stopProvider(service, providerShutdownTimeout)) @@ -101,11 +105,12 @@ func providerTLSFilesFromEnvironment() (string, string, error) { return certFile, keyFile, nil } -func serveProviderHTTP(server providerHTTPServer, certFile, keyFile string) error { +func serveProviderHTTP(server providerHTTPServer, listener net.Listener, certFile, keyFile string) error { + defer func() { _ = listener.Close() }() if certFile == "" { - return server.ListenAndServe() + return server.Serve(listener) } - return server.ListenAndServeTLS(certFile, keyFile) + return server.ServeTLS(listener, certFile, keyFile) } func waitForProviderServeDone(serveDone <-chan error, timeout time.Duration) error { diff --git a/cmd/github-runner-provider/main_test.go b/cmd/github-runner-provider/main_test.go index d67eed7..0f43956 100644 --- a/cmd/github-runner-provider/main_test.go +++ b/cmd/github-runner-provider/main_test.go @@ -6,6 +6,7 @@ import ( "errors" "io" "log/slog" + "net" "net/http" "os" "path/filepath" @@ -41,17 +42,30 @@ type recordingProviderHTTPServer struct { tlsCalls int certFile string keyFile string + address string } -func (s *recordingProviderHTTPServer) ListenAndServe() error { +type closeTrackingListener struct { + net.Listener + closed bool +} + +func (l *closeTrackingListener) Close() error { + l.closed = true + return l.Listener.Close() +} + +func (s *recordingProviderHTTPServer) Serve(listener net.Listener) error { s.plainCalls++ + s.address = listener.Addr().String() return http.ErrServerClosed } -func (s *recordingProviderHTTPServer) ListenAndServeTLS(certFile, keyFile string) error { +func (s *recordingProviderHTTPServer) ServeTLS(listener net.Listener, certFile, keyFile string) error { s.tlsCalls++ s.certFile = certFile s.keyFile = keyFile + s.address = listener.Addr().String() return http.ErrServerClosed } @@ -137,18 +151,30 @@ func TestProviderTransportRequiresTLSOutsideLiteralLoopback(t *testing.T) { func TestServeProviderHTTPUsesConfiguredTransport(t *testing.T) { plain := &recordingProviderHTTPServer{} - if err := serveProviderHTTP(plain, "", ""); !errors.Is(err, http.ErrServerClosed) { + plainSocket, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("plain listener: %v", err) + } + plainListener := &closeTrackingListener{Listener: plainSocket} + t.Cleanup(func() { _ = plainListener.Close() }) + if err := serveProviderHTTP(plain, plainListener, "", ""); !errors.Is(err, http.ErrServerClosed) { t.Fatalf("plain serve: %v", err) } - if plain.plainCalls != 1 || plain.tlsCalls != 0 { + if plain.plainCalls != 1 || plain.tlsCalls != 0 || plain.address != plainListener.Addr().String() || !plainListener.closed { t.Fatalf("plain calls = %+v", plain) } tlsServer := &recordingProviderHTTPServer{} - if err := serveProviderHTTP(tlsServer, "/tls/provider.crt", "/tls/provider.key"); !errors.Is(err, http.ErrServerClosed) { + tlsSocket, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("TLS listener: %v", err) + } + tlsListener := &closeTrackingListener{Listener: tlsSocket} + t.Cleanup(func() { _ = tlsListener.Close() }) + if err := serveProviderHTTP(tlsServer, tlsListener, "/tls/provider.crt", "/tls/provider.key"); !errors.Is(err, http.ErrServerClosed) { t.Fatalf("TLS serve: %v", err) } - if tlsServer.plainCalls != 0 || tlsServer.tlsCalls != 1 || tlsServer.certFile != "/tls/provider.crt" || tlsServer.keyFile != "/tls/provider.key" { + if tlsServer.plainCalls != 0 || tlsServer.tlsCalls != 1 || tlsServer.certFile != "/tls/provider.crt" || tlsServer.keyFile != "/tls/provider.key" || tlsServer.address != tlsListener.Addr().String() || !tlsListener.closed { t.Fatalf("TLS calls = %+v", tlsServer) } }