From fac849f7bc77cc30b3bacae5a8f0b5dd4bf3879c Mon Sep 17 00:00:00 2001 From: Emanuel Besliu <32497562+emanuelbesliu@users.noreply.github.com> Date: Fri, 26 Jun 2026 11:57:39 +0300 Subject: [PATCH] feat(oauth): support generic OIDC/OAuth providers and fix misleading errors Fixes #1247. `step oauth --provider` accepted an HTTPS issuer URL in addition to `google`/`github`, but rejected plaintext `http://` providers, the validation error still claimed only "google or github" were valid, and endpoint discovery only ever tried the OpenID Connect `/.well-known/openid-configuration` path (a long-standing in-code TODO). This made `step oauth` awkward to use with a self-hosted or local identity provider such as Keycloak. Changes: - Validate(): accept an OIDC/OAuth issuer URL and report it in the error message instead of the misleading "google or github" text. - Allow plaintext `http://` providers (e.g. a local test IdP) when the `--insecure` flag is set, matching the existing implicit-flow gating. - disco(): resolve the TODO by falling back to the RFC 8414 OAuth 2.0 authorization server metadata document (`/.well-known/oauth-authorization-server`) when OIDC discovery fails, and honor a provider URL that already points at a `/.well-known/` document. - fetchDiscovery(): check the HTTP status code so a 404/HTML response yields a clear "unexpected status code" error instead of "unsupported format". - Update --provider usage text accordingly. - Add command/oauth/cmd_test.go covering Validate() and disco() (OIDC, RFC 8414 fallback, explicit well-known URL, non-200, invalid JSON). Signed-off-by: Emanuel Besliu <32497562+emanuelbesliu@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- command/oauth/cmd.go | 81 +++++++++++++++++----- command/oauth/cmd_test.go | 138 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 203 insertions(+), 16 deletions(-) create mode 100644 command/oauth/cmd_test.go diff --git a/command/oauth/cmd.go b/command/oauth/cmd.go index 655637c52..c7baa81f6 100644 --- a/command/oauth/cmd.go +++ b/command/oauth/cmd.go @@ -113,11 +113,18 @@ websites. Learn more at https://en.wikipedia.org/wiki/OAuth. This command by default performs the authorization flow with a preconfigured Google application, but a custom one can be set combining the flags **--client-id**, **--client-secret**, and **--provider**. The provider value -must be set to the OIDC discovery document (.well-known/openid-configuration) -endpoint. If Google is used this flag is not necessary, but the appropriate +must be set to the issuer URL of an OpenID Connect or OAuth 2.0 authorization +server. Its endpoints are discovered from the OpenID Connect discovery document +(.well-known/openid-configuration), falling back to the RFC 8414 OAuth 2.0 +authorization server metadata document (.well-known/oauth-authorization-server). +If Google is used this flag is not necessary, but the appropriate value would be be https://accounts.google.com or https://accounts.google.com/.well-known/openid-configuration +A plaintext **http://** provider URL (e.g. a local test instance of an identity +provider) is only accepted together with the **--insecure** flag, because the +discovery request and tokens would otherwise be sent without TLS. + ## EXAMPLES Do the OAuth 2.0 flow using the default client: @@ -328,8 +335,11 @@ type options struct { // Validate validates the options. func (o *options) Validate() error { - if o.Provider != "google" && o.Provider != "github" && !strings.HasPrefix(o.Provider, "https://") { - return errors.New("use a valid provider: google or github") + if o.Provider != "google" && o.Provider != "github" && + !strings.HasPrefix(o.Provider, "https://") && !strings.HasPrefix(o.Provider, "http://") { + return errors.Errorf("invalid value '%s' for flag '--provider'; "+ + "expected 'google', 'github', or an OIDC/OAuth issuer URL "+ + "(e.g. https://accounts.google.com)", o.Provider) } if o.CallbackListener != "" { if _, _, err := net.SplitHostPort(o.CallbackListener); err != nil { @@ -363,6 +373,12 @@ func oauthCmd(c *cli.Context) error { if err := opts.Validate(); err != nil { return err } + // A plaintext http:// provider transmits the discovery request and tokens + // without TLS, so it is only allowed behind the --insecure flag. This is + // primarily useful for local test instances of an identity provider. + if strings.HasPrefix(opts.Provider, "http://") && !c.Bool("insecure") { + return errs.RequiredInsecureFlag(c, "provider") + } if (opts.Provider != "google" || c.IsSet("authorization-endpoint")) && !c.IsSet("client-id") { return errors.New("flag '--client-id' required with '--provider'") } @@ -681,31 +697,64 @@ func newOauth(provider, clientID, clientSecret, authzEp, deviceAuthzEp, tokenEp, }, nil } +// discoveryPaths are the well-known metadata locations probed, in order, to +// resolve a provider's authorization-server endpoints. The OpenID Connect +// Discovery 1.0 path is tried first, falling back to the RFC 8414 OAuth 2.0 +// Authorization Server Metadata path. See +// https://openid.net/specs/openid-connect-discovery-1_0.html and +// https://tools.ietf.org/html/rfc8414#section-5 +var discoveryPaths = []string{ + "/.well-known/openid-configuration", + "/.well-known/oauth-authorization-server", +} + func disco(provider string) (map[string]interface{}, error) { u, err := url.Parse(provider) if err != nil { - return nil, err + return nil, errors.Wrapf(err, "error parsing provider '%s'", provider) } - // TODO: OIDC and OAuth specify two different ways of constructing this - // URL. This is the OIDC way. Probably want to try both. See - // https://tools.ietf.org/html/rfc8414#section-5 - if !strings.Contains(u.Path, "/.well-known/openid-configuration") { - u.Path = path.Join(u.Path, "/.well-known/openid-configuration") + + // If the provider already points directly at a well-known metadata + // document, fetch it as-is instead of appending another suffix. + if strings.Contains(u.Path, "/.well-known/") { + return fetchDiscovery(u.String()) } - resp, err := http.Get(u.String()) + + base := u.Path + var firstErr error + for _, p := range discoveryPaths { + u.Path = path.Join(base, p) + details, err := fetchDiscovery(u.String()) + if err == nil { + return details, nil + } + if firstErr == nil { + firstErr = err + } + } + return nil, firstErr +} + +// fetchDiscovery retrieves and parses an authorization-server metadata +// document from the given URL. +func fetchDiscovery(rawurl string) (map[string]interface{}, error) { + resp, err := http.Get(rawurl) // #nosec G704 -- request intentionally relies on user configuration if err != nil { - return nil, errors.Wrapf(err, "error retrieving %s", u.String()) + return nil, errors.Wrapf(err, "error retrieving %s", rawurl) } defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return nil, errors.Errorf("error retrieving %s: unexpected status code %d", rawurl, resp.StatusCode) + } b, err := io.ReadAll(resp.Body) if err != nil { - return nil, errors.Wrapf(err, "error retrieving %s", u.String()) + return nil, errors.Wrapf(err, "error retrieving %s", rawurl) } details := make(map[string]interface{}) - if err = json.Unmarshal(b, &details); err != nil { - return nil, errors.Wrapf(err, "error reading %s: unsupported format", u.String()) + if err := json.Unmarshal(b, &details); err != nil { + return nil, errors.Wrapf(err, "error reading %s: unsupported format", rawurl) } - return details, err + return details, nil } // postForm simulates http.PostForm but adds the header "Accept: diff --git a/command/oauth/cmd_test.go b/command/oauth/cmd_test.go new file mode 100644 index 000000000..3789f140b --- /dev/null +++ b/command/oauth/cmd_test.go @@ -0,0 +1,138 @@ +package oauth + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestOptions_Validate(t *testing.T) { + tests := []struct { + name string + provider string + wantErr bool + }{ + {"google", "google", false}, + {"github", "github", false}, + {"https issuer", "https://accounts.google.com", false}, + {"https issuer with path", "https://sso.example.org/realms/homelab", false}, + // http:// is accepted by Validate(); the --insecure gating is + // enforced separately in oauthCmd. + {"http issuer", "http://localhost:8080/realms/test", false}, + {"bare name", "keycloak", true}, + {"ftp issuer", "ftp://accounts.google.com", true}, + {"empty", "", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + o := &options{Provider: tt.provider} + err := o.Validate() + if tt.wantErr { + if err == nil { + t.Fatalf("Validate() error = nil, want error for provider %q", tt.provider) + } + return + } + if err != nil { + t.Fatalf("Validate() error = %v, want nil for provider %q", err, tt.provider) + } + }) + } +} + +const ( + oidcDoc = `{"authorization_endpoint":"https://idp.example/auth","token_endpoint":"https://idp.example/token"}` + oauthDoc = `{"authorization_endpoint":"https://idp.example/oauth/auth","token_endpoint":"https://idp.example/oauth/token"}` +) + +func TestDisco(t *testing.T) { + t.Run("oidc discovery", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/.well-known/openid-configuration" { + _, _ = w.Write([]byte(oidcDoc)) + return + } + http.NotFound(w, r) + })) + defer srv.Close() + + d, err := disco(srv.URL) + if err != nil { + t.Fatalf("disco() error = %v", err) + } + if got := d["token_endpoint"]; got != "https://idp.example/token" { + t.Fatalf("token_endpoint = %v, want https://idp.example/token", got) + } + }) + + t.Run("rfc8414 oauth fallback", func(t *testing.T) { + // Only the OAuth authorization server metadata path is served; the + // OIDC path 404s, exercising the RFC 8414 fallback. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/.well-known/oauth-authorization-server" { + _, _ = w.Write([]byte(oauthDoc)) + return + } + http.NotFound(w, r) + })) + defer srv.Close() + + d, err := disco(srv.URL) + if err != nil { + t.Fatalf("disco() error = %v", err) + } + if got := d["token_endpoint"]; got != "https://idp.example/oauth/token" { + t.Fatalf("token_endpoint = %v, want https://idp.example/oauth/token", got) + } + }) + + t.Run("explicit well-known url fetched as-is", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/.well-known/openid-configuration" { + _, _ = w.Write([]byte(oidcDoc)) + return + } + http.NotFound(w, r) + })) + defer srv.Close() + + d, err := disco(srv.URL + "/.well-known/openid-configuration") + if err != nil { + t.Fatalf("disco() error = %v", err) + } + if d["authorization_endpoint"] != "https://idp.example/auth" { + t.Fatalf("unexpected metadata: %v", d) + } + }) + + t.Run("non-200 reports status code", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "nope", http.StatusNotFound) + })) + defer srv.Close() + + _, err := disco(srv.URL) + if err == nil { + t.Fatal("disco() error = nil, want error") + } + if !strings.Contains(err.Error(), "404") { + t.Fatalf("disco() error = %v, want mention of status code 404", err) + } + }) + + t.Run("invalid json reports unsupported format", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("not json")) + })) + defer srv.Close() + + _, err := disco(srv.URL) + if err == nil { + t.Fatal("disco() error = nil, want error") + } + if !strings.Contains(err.Error(), "unsupported format") { + t.Fatalf("disco() error = %v, want unsupported format", err) + } + }) +}