diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index ce18b64..d614f94 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -12,7 +12,7 @@ jobs: runs-on: ubuntu-latest steps: - name: set env vars - run: echo "$(go env GOPATH)/bin" >> $GITHUB_PATH + run: echo "$(go env GOPATH)/bin" >> "$GITHUB_PATH" - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 with: fetch-depth: 0 diff --git a/authclient/authclient.go b/authclient/authclient.go index 010eeaa..11e55db 100644 --- a/authclient/authclient.go +++ b/authclient/authclient.go @@ -3,6 +3,8 @@ package authclient import ( "context" + "crypto/rand" + "encoding/base64" "fmt" "io" "net" @@ -10,6 +12,8 @@ import ( "net/url" "os" "strings" + "sync/atomic" + "time" "golang.org/x/sync/errgroup" @@ -60,19 +64,44 @@ func (client *AuthClient) GetJWT(ctx context.Context, serverURL *url.URL, onOpen return strings.TrimSpace(string(rawJWTBytes)), nil } + return client.getBrowserJWT(ctx, serverURL, nil, onOpenBrowser) +} + +// GetBrowserJWT retrieves a new JWT through the interactive browser flow. It +// deliberately ignores configured service accounts and does not consult a +// token cache. +func (client *AuthClient) GetBrowserJWT( + ctx context.Context, + serverURL *url.URL, + loginParams url.Values, + onOpenBrowser func(string), +) (rawJWT string, err error) { + return client.getBrowserJWT(ctx, serverURL, loginParams, onOpenBrowser) +} + +func (client *AuthClient) getBrowserJWT( + ctx context.Context, + serverURL *url.URL, + loginParams url.Values, + onOpenBrowser func(string), +) (rawJWT string, err error) { li, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { return "", fmt.Errorf("failed to start listener: %w", err) } defer li.Close() + callbackPath, err := newBrowserJWTCallbackPath() + if err != nil { + return "", fmt.Errorf("failed to create browser callback: %w", err) + } - incomingJWT := make(chan string) + incomingJWT := make(chan string, 1) eg, ctx := errgroup.WithContext(ctx) eg.Go(func() error { - return client.runHTTPServer(ctx, li, incomingJWT) + return client.runHTTPServer(ctx, li, callbackPath, incomingJWT) }) eg.Go(func() error { - return client.runOpenBrowser(ctx, li, serverURL, onOpenBrowser) + return client.runOpenBrowser(ctx, li, callbackPath, serverURL, loginParams, onOpenBrowser) }) eg.Go(func() error { select { @@ -90,30 +119,39 @@ func (client *AuthClient) GetJWT(ctx context.Context, serverURL *url.URL, onOpen return rawJWT, nil } -func (client *AuthClient) runHTTPServer(ctx context.Context, li net.Listener, incomingJWT chan string) error { - var srv *http.Server - srv = &http.Server{ +func newBrowserJWTCallbackPath() (string, error) { + var nonce [32]byte + if _, err := rand.Read(nonce[:]); err != nil { + return "", err + } + return "/callback/" + base64.RawURLEncoding.EncodeToString(nonce[:]), nil +} + +func (client *AuthClient) runHTTPServer( + ctx context.Context, + li net.Listener, + callbackPath string, + incomingJWT chan<- string, +) error { + srv := &http.Server{ + ReadHeaderTimeout: 5 * time.Second, BaseContext: func(_ net.Listener) context.Context { return ctx }, - Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - jwt := r.FormValue("pomerium_jwt") - if jwt == "" { - http.Error(w, "not found", http.StatusNotFound) - return - } - incomingJWT <- jwt - - w.Header().Set("Content-Type", "text/plain") - _, _ = io.WriteString(w, "login complete, you may close this page") - + } + srv.Handler = &browserJWTCallbackHandler{ + ctx: ctx, + expectedHost: li.Addr().String(), + expectedPath: callbackPath, + incomingJWT: incomingJWT, + onAccepted: func() { go func() { _ = srv.Shutdown(ctx) }() - }), + }, } // shutdown the server when ctx is done. go func() { <-ctx.Done() - _ = srv.Shutdown(ctx) + _ = srv.Close() }() err := srv.Serve(li) if err == http.ErrServerClosed { @@ -122,13 +160,73 @@ func (client *AuthClient) runHTTPServer(ctx context.Context, li net.Listener, in return err } -func (client *AuthClient) runOpenBrowser(ctx context.Context, li net.Listener, serverURL *url.URL, onOpenBrowser func(string)) error { +type browserJWTCallbackHandler struct { + ctx context.Context + expectedHost string + expectedPath string + incomingJWT chan<- string + onAccepted func() + accepted atomic.Bool +} + +func (h *browserJWTCallbackHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if r.Host != h.expectedHost || r.URL.EscapedPath() != h.expectedPath { + http.Error(w, "not found", http.StatusNotFound) + return + } + if r.Method != http.MethodGet { + w.Header().Set("Allow", http.MethodGet) + http.Error(w, "method not allowed", http.StatusMethodNotAllowed) + return + } + query, err := url.ParseQuery(r.URL.RawQuery) + jwtValues, ok := query["pomerium_jwt"] + if err != nil || len(query) != 1 || !ok || len(jwtValues) != 1 || jwtValues[0] == "" { + http.Error(w, "not found", http.StatusNotFound) + return + } + if !h.accepted.CompareAndSwap(false, true) { + http.Error(w, "callback already consumed", http.StatusConflict) + return + } + select { + case h.incomingJWT <- jwtValues[0]: + case <-h.ctx.Done(): + http.Error(w, "login canceled", http.StatusRequestTimeout) + return + } + + w.Header().Set("Content-Type", "text/plain") + _, _ = io.WriteString(w, "login complete, you may close this page") + if h.onAccepted != nil { + h.onAccepted() + } +} + +func (client *AuthClient) runOpenBrowser( + ctx context.Context, + li net.Listener, + callbackPath string, + serverURL *url.URL, + loginParams url.Values, + onOpenBrowser func(string), +) error { browserURL := getBrowserURL(serverURL) + query := make(url.Values, len(loginParams)+1) + for key, values := range loginParams { + query[key] = append([]string(nil), values...) + } + // The callback is owned by this client. Never permit caller-supplied + // parameters to redirect the freshly issued credential elsewhere. + callbackURL := &url.URL{ + Scheme: "http", + Host: li.Addr().String(), + Path: callbackPath, + } + query.Set("pomerium_redirect_uri", callbackURL.String()) dst := browserURL.ResolveReference(&url.URL{ - Path: "/.pomerium/api/v1/login", - RawQuery: url.Values{ - "pomerium_redirect_uri": {fmt.Sprintf("http://%s", li.Addr().String())}, - }.Encode(), + Path: "/.pomerium/api/v1/login", + RawQuery: query.Encode(), }) req, err := http.NewRequest("GET", dst.String(), nil) diff --git a/authclient/authclient_test.go b/authclient/authclient_test.go index ee82d7c..cb8c0f5 100644 --- a/authclient/authclient_test.go +++ b/authclient/authclient_test.go @@ -2,11 +2,16 @@ package authclient import ( "context" + "fmt" + "io" "net" "net/http" + "net/http/httptest" "net/url" "os" "path/filepath" + "strings" + "sync" "testing" "time" @@ -29,11 +34,19 @@ func TestAuthClient(t *testing.T) { return } t.Cleanup(func() { li.Close() }) + expectedLoginParams := make(chan url.Values, 2) + loginRequestResults := make(chan error, 2) go func() { h := chi.NewMux() h.Get("/.pomerium/api/v1/login", func(w http.ResponseWriter, r *http.Request) { - _, _ = w.Write([]byte(r.FormValue("pomerium_redirect_uri"))) + callback, err := validateBrowserLoginRequest(r.URL.Query(), <-expectedLoginParams) + loginRequestResults <- err + if err != nil { + http.Error(w, "invalid login request", http.StatusBadRequest) + return + } + _, _ = w.Write([]byte(callback.String())) }) srv := &http.Server{ BaseContext: func(_ net.Listener) context.Context { @@ -44,36 +57,105 @@ func TestAuthClient(t *testing.T) { _ = srv.Serve(li) }() - ac := New() - ac.cfg.open = func(input string) error { + ac := New(WithServiceAccount("MUST_NOT_BE_USED")) + openBrowser := func(input string) error { u, err := url.Parse(input) if err != nil { return err } - u = u.ResolveReference(&url.URL{ - RawQuery: url.Values{ - "pomerium_jwt": {"TEST"}, - }.Encode(), - }) + const injectedJWT = "CROSS-IDENTITY-JWT-CANARY" + doCallback := func(method string, callback *url.URL, host, jwt string) (int, string, error) { + candidate := *callback + query := candidate.Query() + query.Set("pomerium_jwt", jwt) + candidate.RawQuery = query.Encode() + req, err := http.NewRequestWithContext(ctx, method, candidate.String(), nil) + if err != nil { + return 0, "", err + } + if host != "" { + req.Host = host + } + res, err := http.DefaultClient.Do(req) + if err != nil { + return 0, "", err + } + defer res.Body.Close() + body, err := io.ReadAll(res.Body) + return res.StatusCode, string(body), err + } + + wrongPath := *u + wrongPath.Path = "/callback/not-the-issued-state" + status, body, err := doCallback(http.MethodGet, &wrongPath, "", injectedJWT) + if err != nil { + return err + } + if status != http.StatusNotFound || strings.Contains(body, injectedJWT) { + return fmt.Errorf("wrong-path callback: status=%d body=%q", status, body) + } + + status, body, err = doCallback(http.MethodPost, u, "", injectedJWT) + if err != nil { + return err + } + if status != http.StatusMethodNotAllowed || strings.Contains(body, injectedJWT) { + return fmt.Errorf("wrong-method callback: status=%d body=%q", status, body) + } - req, err := http.NewRequestWithContext(ctx, "GET", u.String(), nil) + status, body, err = doCallback(http.MethodGet, u, "localhost:"+u.Port(), injectedJWT) if err != nil { return err } + if status != http.StatusNotFound || strings.Contains(body, injectedJWT) { + return fmt.Errorf("wrong-host callback: status=%d body=%q", status, body) + } + + extraQuery := *u + extraQuery.RawQuery = url.Values{"unexpected": {"EXTRA-QUERY-CANARY"}}.Encode() + status, body, err = doCallback(http.MethodGet, &extraQuery, "", injectedJWT) + if err != nil { + return err + } + if status != http.StatusNotFound || + strings.Contains(body, injectedJWT) || + strings.Contains(body, "EXTRA-QUERY-CANARY") { + return fmt.Errorf("extra-query callback: status=%d body=%q", status, body) + } - res, err := http.DefaultClient.Do(req) + status, body, err = doCallback(http.MethodGet, u, "", "TEST") if err != nil { return err } - _ = res.Body.Close() + if status != http.StatusOK || strings.Contains(body, "TEST") { + return fmt.Errorf("valid callback: status=%d body=%q", status, body) + } return nil } + ac.cfg.open = openBrowser - rawJWT, err := ac.GetJWT(ctx, &url.URL{ + serverURL := &url.URL{ Scheme: "http", Host: li.Addr().String(), - }, func(_ string) {}) - assert.NoError(t, err) + } + loginParams := url.Values{ + "pomerium_route": {"route.example.com"}, + "pomerium_redirect_uri": {"https://attacker.example/capture"}, + } + originalLoginParams := cloneURLValues(loginParams) + expectedLoginParams <- url.Values{"pomerium_route": {"route.example.com"}} + rawJWT, err := ac.GetBrowserJWT(ctx, serverURL, loginParams, func(_ string) {}) + require.NoError(t, err) + require.NoError(t, <-loginRequestResults) + assert.Equal(t, "TEST", rawJWT) + assert.Equal(t, originalLoginParams, loginParams, "caller-owned login parameters were mutated") + + ac = New() + ac.cfg.open = openBrowser + expectedLoginParams <- url.Values{} + rawJWT, err = ac.GetJWT(ctx, serverURL, func(_ string) {}) + require.NoError(t, err) + require.NoError(t, <-loginRequestResults) assert.Equal(t, "TEST", rawJWT) }) @@ -106,3 +188,146 @@ func TestAuthClient(t *testing.T) { assert.Equal(t, "SERVICE_ACCOUNT", rawJWT) }) } + +func validateBrowserLoginRequest(query, expectedParams url.Values) (*url.URL, error) { + redirectValues := query["pomerium_redirect_uri"] + if len(redirectValues) != 1 { + return nil, fmt.Errorf("expected exactly one redirect URI, got %d", len(redirectValues)) + } + callback, err := url.Parse(redirectValues[0]) + if err != nil { + return nil, fmt.Errorf("parse redirect URI: %w", err) + } + if callback.Scheme != "http" || callback.Hostname() != "127.0.0.1" || callback.Port() == "" { + return nil, fmt.Errorf("redirect URI is not a loopback HTTP listener: %q", callback) + } + const callbackPrefix = "/callback/" + if !strings.HasPrefix(callback.EscapedPath(), callbackPrefix) { + return nil, fmt.Errorf("redirect URI has unexpected callback path: %q", callback.EscapedPath()) + } + if len(strings.TrimPrefix(callback.EscapedPath(), callbackPrefix)) < 43 { + return nil, fmt.Errorf("redirect URI callback state has insufficient entropy") + } + if callback.RawQuery != "" || callback.Fragment != "" || callback.User != nil { + return nil, fmt.Errorf("redirect URI contains unexpected components: %q", callback) + } + + actualParams := cloneURLValues(query) + actualParams.Del("pomerium_redirect_uri") + if actualParams.Encode() != expectedParams.Encode() { + return nil, fmt.Errorf("login parameters mismatch: got %q, want %q", actualParams.Encode(), expectedParams.Encode()) + } + return callback, nil +} + +func cloneURLValues(values url.Values) url.Values { + clone := make(url.Values, len(values)) + for key, entries := range values { + clone[key] = append([]string(nil), entries...) + } + return clone +} + +func TestBrowserJWTCallbackHandlerIsSingleUseAndNonblocking(t *testing.T) { + t.Parallel() + + const ( + host = "127.0.0.1:49152" + path = "/callback/issued-unpredictable-state" + ) + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + incomingJWT := make(chan string, 1) + handler := &browserJWTCallbackHandler{ + ctx: ctx, + expectedHost: host, + expectedPath: path, + incomingJWT: incomingJWT, + } + + type result struct { + status int + body string + } + start := make(chan struct{}) + results := make(chan result, 2) + var workers sync.WaitGroup + for _, rawJWT := range []string{"FIRST-JWT-CANARY", "SECOND-JWT-CANARY"} { + rawJWT := rawJWT + workers.Add(1) + go func() { + defer workers.Done() + <-start + requestURL := "http://" + host + path + "?" + url.Values{"pomerium_jwt": {rawJWT}}.Encode() + req := httptest.NewRequest(http.MethodGet, requestURL, nil) + req.Host = host + response := httptest.NewRecorder() + handler.ServeHTTP(response, req) + results <- result{response.Code, response.Body.String()} + }() + } + close(start) + done := make(chan struct{}) + go func() { + workers.Wait() + close(done) + }() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("concurrent callbacks did not complete") + } + + statusCounts := map[int]int{} + for range 2 { + result := <-results + statusCounts[result.status]++ + assert.NotContains(t, result.body, "FIRST-JWT-CANARY") + assert.NotContains(t, result.body, "SECOND-JWT-CANARY") + } + assert.Equal(t, 1, statusCounts[http.StatusOK]) + assert.Equal(t, 1, statusCounts[http.StatusConflict]) + + select { + case got := <-incomingJWT: + assert.Contains(t, []string{"FIRST-JWT-CANARY", "SECOND-JWT-CANARY"}, got) + default: + t.Fatal("winning callback did not deliver a JWT") + } + select { + case duplicate := <-incomingJWT: + t.Fatalf("more than one callback won: %q", duplicate) + default: + } +} + +func TestBrowserJWTCallbackHandlerRejectsDuplicateJWTValues(t *testing.T) { + t.Parallel() + + const ( + host = "127.0.0.1:49152" + path = "/callback/issued-unpredictable-state" + ) + incomingJWT := make(chan string, 1) + handler := &browserJWTCallbackHandler{ + ctx: t.Context(), + expectedHost: host, + expectedPath: path, + incomingJWT: incomingJWT, + } + + requestURL := "http://" + host + path + "?pomerium_jwt=FIRST-JWT-CANARY&pomerium_jwt=SECOND-JWT-CANARY" + req := httptest.NewRequest(http.MethodGet, requestURL, nil) + req.Host = host + response := httptest.NewRecorder() + handler.ServeHTTP(response, req) + + assert.Equal(t, http.StatusNotFound, response.Code) + assert.NotContains(t, response.Body.String(), "FIRST-JWT-CANARY") + assert.NotContains(t, response.Body.String(), "SECOND-JWT-CANARY") + select { + case jwt := <-incomingJWT: + t.Fatalf("duplicate query values delivered a JWT: %q", jwt) + default: + } +}