From f926d741570a121d4e2fc94bd6ad2f98f1f65107 Mon Sep 17 00:00:00 2001 From: Eric Chiang Date: Mon, 9 Jan 2017 10:46:16 -0800 Subject: [PATCH] server: fixes for the implicit and hybrid flow Accept the following response_type for the implicit flow: id_token token id_token And the following for hybrid flow code id_token code token code token id_token This corrects the previous behavior of the implicit flow, which only accepted "token" (now correctly rejected). --- server/handlers.go | 95 ++++++++++++++++++++++----- server/oauth2.go | 129 +++++++++++++++++++++++------------- server/oauth2_test.go | 149 ++++++++++++++++++++++++++++++++++++++++++ server/rotation.go | 1 + server/server.go | 2 +- server/server_test.go | 4 +- 6 files changed, 318 insertions(+), 62 deletions(-) diff --git a/server/handlers.go b/server/handlers.go index c962265f..cda845f8 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -144,12 +144,23 @@ func (s *Server) discoveryHandler() (http.Handler, error) { // handleAuthorization handles the OAuth2 auth endpoint. func (s *Server) handleAuthorization(w http.ResponseWriter, r *http.Request) { - authReq, err := s.parseAuthorizationRequest(s.supportedResponseTypes, r) + authReq, err := s.parseAuthorizationRequest(r) if err != nil { s.logger.Errorf("Failed to parse authorization request: %v", err) - s.renderError(w, http.StatusInternalServerError, "Failed to connect to the database.") + if handler, ok := err.Handle(); ok { + // client_id and redirect_uri checked out and we can redirect back to + // the client with the error. + handler.ServeHTTP(w, r) + return + } + + // Otherwise render the error to the user. + // + // TODO(ericchiang): Should we just always render the error? + s.renderError(w, err.Status(), err.Error()) return } + authReq.Expiry = s.now().Add(time.Minute * 30) if err := s.storage.CreateAuthRequest(authReq); err != nil { s.logger.Errorf("Failed to create authorization request: %v", err) @@ -397,12 +408,25 @@ func (s *Server) sendCodeResponse(w http.ResponseWriter, r *http.Request, authRe s.renderError(w, http.StatusInternalServerError, "Invalid redirect URI.") return } - q := u.Query() + + var ( + // Was the initial request using the implicit or hybrid flow instead of + // the "normal" code flow? + implicitOrHybrid = false + + // Only present in hybrid or code flow. code.ID == "" if this is not set. + code storage.AuthCode + + // ID token returned immediately if the response_type includes "id_token". + // Only valid for implicit and hybrid flows. + idToken string + idTokenExpiry time.Time + ) for _, responseType := range authReq.ResponseTypes { switch responseType { case responseTypeCode: - code := storage.AuthCode{ + code = storage.AuthCode{ ID: storage.NewID(), ClientID: authReq.ClientID, ConnectorID: authReq.ConnectorID, @@ -419,32 +443,73 @@ func (s *Server) sendCodeResponse(w http.ResponseWriter, r *http.Request, authRe return } + // Implicit and hybrid flows that try to use the OOB redirect URI are + // rejected earlier. If we got here we're using the code flow. if authReq.RedirectURI == redirectURIOOB { if err := s.templates.oob(w, code.ID); err != nil { s.logger.Errorf("Server template error: %v", err) } return } - q.Set("code", code.ID) case responseTypeToken: - idToken, expiry, err := s.newIDToken(authReq.ClientID, authReq.Claims, authReq.Scopes, authReq.Nonce) + implicitOrHybrid = true + case responseTypeIDToken: + implicitOrHybrid = true + var err error + idToken, idTokenExpiry, err = s.newIDToken(authReq.ClientID, authReq.Claims, authReq.Scopes, authReq.Nonce) if err != nil { s.logger.Errorf("failed to create ID token: %v", err) s.tokenErrHelper(w, errServerError, "", http.StatusInternalServerError) return } - v := url.Values{} - v.Set("access_token", storage.NewID()) - v.Set("token_type", "bearer") - v.Set("id_token", idToken) - v.Set("state", authReq.State) - v.Set("expires_in", strconv.Itoa(int(expiry.Sub(s.now()).Seconds()))) - u.Fragment = v.Encode() } } - q.Set("state", authReq.State) - u.RawQuery = q.Encode() + if implicitOrHybrid { + v := url.Values{} + v.Set("access_token", storage.NewID()) + v.Set("token_type", "bearer") + v.Set("state", authReq.State) + if idToken != "" { + v.Set("id_token", idToken) + // The hybrid flow with only "code token" or "code id_token" doesn't return an + // "expires_in" value. If "code" wasn't provided, indicating the implicit flow, + // don't add it. + // + // https://openid.net/specs/openid-connect-core-1_0.html#HybridAuthResponse + if code.ID == "" { + v.Set("expires_in", strconv.Itoa(int(idTokenExpiry.Sub(s.now()).Seconds()))) + } + } + if code.ID != "" { + v.Set("code", code.ID) + } + + // Implicit and hybrid flows return their values as part of the fragment. + // + // HTTP/1.1 303 See Other + // Location: https://client.example.org/cb# + // access_token=SlAV32hkKG + // &token_type=bearer + // &id_token=eyJ0 ... NiJ9.eyJ1c ... I6IjIifX0.DeWt4Qu ... ZXso + // &expires_in=3600 + // &state=af0ifjsldkj + // + u.Fragment = v.Encode() + } else { + // The code flow add values to the URL query. + // + // HTTP/1.1 303 See Other + // Location: https://client.example.org/cb? + // code=SplxlOBeZQQYbYS6WxSbIA + // &state=af0ifjsldkj + // + q := u.Query() + q.Set("code", code.ID) + q.Set("state", authReq.State) + u.RawQuery = q.Encode() + } + http.Redirect(w, r, u.String(), http.StatusSeeOther) } diff --git a/server/oauth2.go b/server/oauth2.go index d8211e73..adbf1eed 100644 --- a/server/oauth2.go +++ b/server/oauth2.go @@ -24,20 +24,39 @@ type authErr struct { Description string } -func (err *authErr) ServeHTTP(w http.ResponseWriter, r *http.Request) { - v := url.Values{} - v.Add("state", err.State) - v.Add("error", err.Type) - if err.Description != "" { - v.Add("error_description", err.Description) +func (err *authErr) Status() int { + if err.State == errServerError { + return http.StatusInternalServerError } - var redirectURI string - if strings.Contains(err.RedirectURI, "?") { - redirectURI = err.RedirectURI + "&" + v.Encode() - } else { - redirectURI = err.RedirectURI + "?" + v.Encode() + return http.StatusBadRequest +} + +func (err *authErr) Error() string { + return err.Description +} + +func (err *authErr) Handle() (http.Handler, bool) { + // Didn't get a valid redirect URI. + if err.RedirectURI == "" { + return nil, false } - http.Redirect(w, r, redirectURI, http.StatusSeeOther) + + hf := func(w http.ResponseWriter, r *http.Request) { + v := url.Values{} + v.Add("state", err.State) + v.Add("error", err.Type) + if err.Description != "" { + v.Add("error_description", err.Description) + } + var redirectURI string + if strings.Contains(err.RedirectURI, "?") { + redirectURI = err.RedirectURI + "&" + v.Encode() + } else { + redirectURI = err.RedirectURI + "?" + v.Encode() + } + http.Redirect(w, r, redirectURI, http.StatusSeeOther) + } + return http.HandlerFunc(hf), true } func tokenErr(w http.ResponseWriter, typ, description string, statusCode int) error { @@ -192,20 +211,19 @@ func (s *Server) newIDToken(clientID string, claims storage.Claims, scopes []str } // parse the initial request from the OAuth2 client. -// -// For correctness the logic is largely copied from https://github.com/RangelReale/osin. -func (s *Server) parseAuthorizationRequest(supportedResponseTypes map[string]bool, r *http.Request) (req storage.AuthRequest, oauth2Err *authErr) { - if err := r.ParseForm(); err != nil { - return req, &authErr{"", "", errInvalidRequest, "Failed to parse request."} - } - - redirectURI, err := url.QueryUnescape(r.Form.Get("redirect_uri")) +func (s *Server) parseAuthorizationRequest(r *http.Request) (req storage.AuthRequest, oauth2Err *authErr) { + q := r.URL.Query() + redirectURI, err := url.QueryUnescape(q.Get("redirect_uri")) if err != nil { return req, &authErr{"", "", errInvalidRequest, "No redirect_uri provided."} } - state := r.FormValue("state") - clientID := r.Form.Get("client_id") + clientID := q.Get("client_id") + state := q.Get("state") + nonce := q.Get("nonce") + // Some clients, like the old go-oidc, provide extra whitespace. Tolerate this. + scopes := strings.Fields(q.Get("scope")) + responseTypes := strings.Fields(q.Get("response_type")) client, err := s.storage.GetClient(clientID) if err != nil { @@ -222,12 +240,11 @@ func (s *Server) parseAuthorizationRequest(supportedResponseTypes map[string]boo return req, &authErr{"", "", errInvalidRequest, description} } + // From here on out, we want to redirect back to the client with an error. newErr := func(typ, format string, a ...interface{}) *authErr { return &authErr{state, redirectURI, typ, fmt.Sprintf(format, a...)} } - scopes := strings.Fields(r.Form.Get("scope")) - var ( unrecognized []string invalidScopes []string @@ -247,7 +264,7 @@ func (s *Server) parseAuthorizationRequest(supportedResponseTypes map[string]boo isTrusted, err := s.validateCrossClientTrust(clientID, peerID) if err != nil { - return req, newErr(errServerError, "") + return req, newErr(errServerError, "Internal server error.") } if !isTrusted { invalidScopes = append(invalidScopes, scope) @@ -264,37 +281,61 @@ func (s *Server) parseAuthorizationRequest(supportedResponseTypes map[string]boo return req, newErr("invalid_scope", "Client can't request scope(s) %q", invalidScopes) } - nonce := r.Form.Get("nonce") - responseTypes := strings.Split(r.Form.Get("response_type"), " ") + var rt struct { + code bool + idToken bool + token bool + } + for _, responseType := range responseTypes { - if !supportedResponseTypes[responseType] { + switch responseType { + case responseTypeCode: + rt.code = true + case responseTypeIDToken: + rt.idToken = true + case responseTypeToken: + rt.token = true + default: return req, newErr("invalid_request", "Invalid response type %q", responseType) } - switch responseType { - case responseTypeCode: - case responseTypeToken: - // Implicit flow requires a nonce value. - // https://openid.net/specs/openid-connect-core-1_0.html#ImplicitAuthRequest - if nonce == "" { - return req, newErr("invalid_request", "Response type 'token' requires a 'nonce' value.") - } + if !s.supportedResponseTypes[responseType] { + return req, newErr(errUnsupportedResponseType, "Unsupported response type %q", responseType) + } + } - if redirectURI == redirectURIOOB { - err := fmt.Sprintf("Cannot use response type 'token' with redirect_uri '%s'.", redirectURIOOB) - return req, newErr("invalid_request", err) - } - default: - return req, newErr("invalid_request", "Invalid response type %q", responseType) + if len(responseTypes) == 0 { + return req, newErr("invalid_requests", "No response_type provided") + } + + if rt.token && !rt.code && !rt.idToken { + // "token" can't be provided by its own. + // + // https://openid.net/specs/openid-connect-core-1_0.html#Authentication + return req, newErr("invalid_request", "Response type 'token' must be provided with type 'id_token' and/or 'code'") + } + if !rt.code { + // Either "id_token code" or "id_token" has been provided which implies the + // implicit flow. Implicit flow requires a nonce value. + // + // https://openid.net/specs/openid-connect-core-1_0.html#ImplicitAuthRequest + if nonce == "" { + return req, newErr("invalid_request", "Response type 'token' requires a 'nonce' value.") + } + } + if rt.token { + if redirectURI == redirectURIOOB { + err := fmt.Sprintf("Cannot use response type 'token' with redirect_uri '%s'.", redirectURIOOB) + return req, newErr("invalid_request", err) } } return storage.AuthRequest{ ID: storage.NewID(), ClientID: client.ID, - State: r.Form.Get("state"), + State: state, Nonce: nonce, - ForceApprovalPrompt: r.Form.Get("approval_prompt") == "force", + ForceApprovalPrompt: q.Get("approval_prompt") == "force", Scopes: scopes, RedirectURI: redirectURI, ResponseTypes: responseTypes, diff --git a/server/oauth2_test.go b/server/oauth2_test.go index abb4e431..807de836 100644 --- a/server/oauth2_test.go +++ b/server/oauth2_test.go @@ -1 +1,150 @@ package server + +import ( + "context" + "net/http/httptest" + "net/url" + "testing" + + "github.com/coreos/dex/storage" +) + +func TestParseAuthorizationRequest(t *testing.T) { + tests := []struct { + name string + clients []storage.Client + supportedResponseTypes []string + + queryParams map[string]string + + wantErr bool + }{ + { + name: "normal request", + clients: []storage.Client{ + { + ID: "foo", + RedirectURIs: []string{"https://example.com/foo"}, + }, + }, + supportedResponseTypes: []string{"code"}, + queryParams: map[string]string{ + "client_id": "foo", + "redirect_uri": "https://example.com/foo", + "response_type": "code", + "scope": "openid email profile", + }, + }, + { + name: "invalid client id", + clients: []storage.Client{ + { + ID: "foo", + RedirectURIs: []string{"https://example.com/foo"}, + }, + }, + supportedResponseTypes: []string{"code"}, + queryParams: map[string]string{ + "client_id": "bar", + "redirect_uri": "https://example.com/foo", + "response_type": "code", + "scope": "openid email profile", + }, + wantErr: true, + }, + { + name: "invalid redirect uri", + clients: []storage.Client{ + { + ID: "bar", + RedirectURIs: []string{"https://example.com/bar"}, + }, + }, + supportedResponseTypes: []string{"code"}, + queryParams: map[string]string{ + "client_id": "bar", + "redirect_uri": "https://example.com/foo", + "response_type": "code", + "scope": "openid email profile", + }, + wantErr: true, + }, + { + name: "implicit flow", + clients: []storage.Client{ + { + ID: "bar", + RedirectURIs: []string{"https://example.com/bar"}, + }, + }, + supportedResponseTypes: []string{"code", "id_token", "token"}, + queryParams: map[string]string{ + "client_id": "bar", + "redirect_uri": "https://example.com/bar", + "response_type": "code id_token", + "scope": "openid email profile", + }, + }, + { + name: "unsupported response type", + clients: []storage.Client{ + { + ID: "bar", + RedirectURIs: []string{"https://example.com/bar"}, + }, + }, + supportedResponseTypes: []string{"code"}, + queryParams: map[string]string{ + "client_id": "bar", + "redirect_uri": "https://example.com/bar", + "response_type": "code id_token", + "scope": "openid email profile", + }, + wantErr: true, + }, + { + name: "only token response type", + clients: []storage.Client{ + { + ID: "bar", + RedirectURIs: []string{"https://example.com/bar"}, + }, + }, + supportedResponseTypes: []string{"code", "id_token", "token"}, + queryParams: map[string]string{ + "client_id": "bar", + "redirect_uri": "https://example.com/bar", + "response_type": "token", + "scope": "openid email profile", + }, + wantErr: true, + }, + } + + for _, tc := range tests { + func() { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + httpServer, server := newTestServer(ctx, t, func(c *Config) { + c.SupportedResponseTypes = tc.supportedResponseTypes + c.Storage = storage.WithStaticClients(c.Storage, tc.clients) + }) + defer httpServer.Close() + + params := url.Values{} + for k, v := range tc.queryParams { + params.Set(k, v) + } + + req := httptest.NewRequest("GET", httpServer.URL+"/auth?"+params.Encode(), nil) + _, err := server.parseAuthorizationRequest(req) + if err != nil && !tc.wantErr { + t.Errorf("%s: %v", tc.name, err) + } + if err == nil && tc.wantErr { + t.Errorf("%s: expected error", tc.name) + } + }() + } +} diff --git a/server/rotation.go b/server/rotation.go index 326e7597..8516e9bd 100644 --- a/server/rotation.go +++ b/server/rotation.go @@ -130,6 +130,7 @@ func (k keyRotater) rotate() error { // Remove expired verification keys. i := 0 + for _, key := range keys.VerificationKeys { if !key.Expiry.After(tNow) { keys.VerificationKeys[i] = key diff --git a/server/server.go b/server/server.go index 535e21be..c0b194a6 100644 --- a/server/server.go +++ b/server/server.go @@ -159,7 +159,7 @@ func newServer(ctx context.Context, c Config, rotationStrategy rotationStrategy) supported := make(map[string]bool) for _, respType := range c.SupportedResponseTypes { switch respType { - case responseTypeCode, responseTypeToken: + case responseTypeCode, responseTypeIDToken, responseTypeToken: default: return nil, fmt.Errorf("unsupported response_type %q", respType) } diff --git a/server/server_test.go b/server/server_test.go index 96786fc8..7c499c15 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -510,7 +510,7 @@ func TestOAuth2ImplicitFlow(t *testing.T) { httpServer, s := newTestServer(ctx, t, func(c *Config) { // Enable support for the implicit flow. - c.SupportedResponseTypes = []string{"code", "token"} + c.SupportedResponseTypes = []string{"code", "token", "id_token"} }) defer httpServer.Close() @@ -553,7 +553,7 @@ func TestOAuth2ImplicitFlow(t *testing.T) { w.WriteHeader(http.StatusOK) return } - u := oauth2Config.AuthCodeURL(state, oauth2.SetAuthURLParam("response_type", "token"), oidc.Nonce(nonce)) + u := oauth2Config.AuthCodeURL(state, oauth2.SetAuthURLParam("response_type", "id_token token"), oidc.Nonce(nonce)) http.Redirect(w, r, u, http.StatusSeeOther) }))