From 7f896fe6b84ca03828578ffacc69d335e47367c2 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Fri, 10 Apr 2015 00:43:44 -0400 Subject: [PATCH 1/3] Protect browsers from basic-auth CSRF attacks Before sending a basic-auth challenge, require a custom header. --- .../password_auth_handler.go | 21 +++++- .../password_auth_handler_test.go | 44 +++++++++--- .../oauth/handlers/default_auth_handler.go | 47 ++++++++++++ .../handlers/default_auth_handler_test.go | 72 +++++++++++++++++++ pkg/cmd/experimental/tokens/request.go | 9 +-- pkg/cmd/experimental/tokens/tokens.go | 3 +- pkg/cmd/util/tokencmd/challenging_client.go | 10 +++ 7 files changed, 187 insertions(+), 19 deletions(-) diff --git a/pkg/auth/authenticator/challenger/passwordchallenger/password_auth_handler.go b/pkg/auth/authenticator/challenger/passwordchallenger/password_auth_handler.go index 19c18058610d..76f30f0780bf 100644 --- a/pkg/auth/authenticator/challenger/passwordchallenger/password_auth_handler.go +++ b/pkg/auth/authenticator/challenger/passwordchallenger/password_auth_handler.go @@ -1,6 +1,7 @@ package passwordchallenger import ( + "fmt" "net/http" oauthhandlers "github.com/openshift/origin/pkg/auth/oauth/handlers" @@ -10,6 +11,12 @@ type basicPasswordAuthHandler struct { realm string } +// CSRFTokenHeader must be passed when requesting a WWW-Authenticate Basic challenge to prevent CSRF attacks on browsers. +// The presence of this header indicates that a user agent intended to make a basic auth request (as opposed to a browser +// being tricked into requesting /oauth/authorize?response_type=token&client_id=openshift-challenging-client). +// Because multiple clients (osc, Java client, etc) are required to set this header, it probably should not be changed. +const CSRFTokenHeader = "X-CSRF-Token" + // NewBasicAuthChallenger returns a AuthenticationChallenger that responds with a basic auth challenge for the supplied realm func NewBasicAuthChallenger(realm string) oauthhandlers.AuthenticationChallenger { return &basicPasswordAuthHandler{realm} @@ -18,7 +25,19 @@ func NewBasicAuthChallenger(realm string) oauthhandlers.AuthenticationChallenger // AuthenticationChallenge returns a header that indicates a basic auth challenge for the supplied realm func (h *basicPasswordAuthHandler) AuthenticationChallenge(req *http.Request) (http.Header, error) { headers := http.Header{} - headers.Add("WWW-Authenticate", "Basic realm=\""+h.realm+"\"") + + if len(req.Header.Get(CSRFTokenHeader)) == 0 { + headers.Add("Warning", + fmt.Sprintf( + `%s %s "A non-empty %s header is required to receive basic-auth challenges"`, + oauthhandlers.WarningHeaderMiscCode, + oauthhandlers.WarningHeaderOpenShiftSource, + CSRFTokenHeader, + ), + ) + } else { + headers.Add("WWW-Authenticate", fmt.Sprintf(`Basic realm="%s"`, h.realm)) + } return headers, nil } diff --git a/pkg/auth/authenticator/challenger/passwordchallenger/password_auth_handler_test.go b/pkg/auth/authenticator/challenger/passwordchallenger/password_auth_handler_test.go index 2ae376244737..24bd71b96ffb 100644 --- a/pkg/auth/authenticator/challenger/passwordchallenger/password_auth_handler_test.go +++ b/pkg/auth/authenticator/challenger/passwordchallenger/password_auth_handler_test.go @@ -1,27 +1,49 @@ package passwordchallenger import ( + "fmt" "net/http" + "strings" "testing" ) func TestAuthChallengeNeeded(t *testing.T) { - handler := NewBasicAuthChallenger("testing-realm") - req := &http.Request{} - header, err := handler.AuthenticationChallenge(req) + realm := "testing-realm" + expectedChallenge := fmt.Sprintf(`Basic realm="%s"`, realm) + + handler := NewBasicAuthChallenger(realm) + req, _ := http.NewRequest("GET", "", nil) + req.Header.Set(CSRFTokenHeader, "1") + header, err := handler.AuthenticationChallenge(req) if err != nil { - t.Errorf("Unexepcted error: %v", err) + t.Errorf("Unexpected error: %v", err) } + if warning := header.Get("Warning"); warning != "" { + t.Errorf("Unexpected warning %v", warning) + } + if challenge := header.Get("WWW-Authenticate"); challenge != expectedChallenge { + t.Errorf("Expected %v, got %v", expectedChallenge, challenge) + } + +} + +func TestAuthChallengeWithoutCSRF(t *testing.T) { + realm := "testing-realm" + expectedWarning := CSRFTokenHeader - if value, ok := header["Www-Authenticate"]; ok { - expectedValue := "Basic realm=\"testing-realm\"" - if value[0] != expectedValue { - t.Errorf("Expected %v, got %v", expectedValue, value) - } - } else { - t.Error("Did not get back header") + handler := NewBasicAuthChallenger(realm) + req, _ := http.NewRequest("GET", "", nil) + header, err := handler.AuthenticationChallenge(req) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + if warning := header.Get("Warning"); !strings.Contains(warning, expectedWarning) { + t.Errorf("Expected warning containing %s, got %s", expectedWarning, warning) + } + if challenge := header.Get("WWW-Authenticate"); challenge != "" { + t.Errorf("Unexpected challenge %v", challenge) } } diff --git a/pkg/auth/oauth/handlers/default_auth_handler.go b/pkg/auth/oauth/handlers/default_auth_handler.go index 6b17e4661f2d..66829255527c 100644 --- a/pkg/auth/oauth/handlers/default_auth_handler.go +++ b/pkg/auth/oauth/handlers/default_auth_handler.go @@ -3,6 +3,8 @@ package handlers import ( "fmt" "net/http" + "regexp" + "strings" kerrors "github.com/GoogleCloudPlatform/kubernetes/pkg/util/errors" @@ -32,6 +34,40 @@ func NewUnionAuthenticationHandler(passedChallengers map[string]AuthenticationCh return &unionAuthenticationHandler{challengers, redirectors, errorHandler} } +const ( + // WarningHeaderMiscCode is the code for "Miscellaneous warning", which may be displayed to human users + WarningHeaderMiscCode = "199" + // The name of the agent adding the warning header + WarningHeaderOpenShiftSource = "OpenShift" + + warningHeaderCodeIndex = 1 + warningHeaderAgentIndex = 2 + warningHeaderTextIndex = 3 + warningHeaderDateIndex = 4 +) + +var ( + // http://tools.ietf.org/html/rfc2616#section-14.46 + warningRegex = regexp.MustCompile(strings.Join([]string{ + // Beginning of the string + `^`, + // Exactly 3 digits (captured in group 1) + `([0-9]{3})`, + // A single space + ` `, + // 1+ non-space characters (captured in group 2) + `([^ ]+)`, + // A single space + ` `, + // quoted-string (value inside quotes is captured in group 3) + `"((?:[^"\\]|\\.)*)"`, + // Optionally followed by quoted HTTP-Date + `(?: "([^"]+)")?`, + // End of the string + `$`, + }, "")) +) + // AuthenticationNeeded looks at the oauth Client to determine whether it wants try to authenticate with challenges or using a redirect path // If the client wants a challenge path, it muxes together all the different challenges from the challenge handlers // If (the client wants a redirect path) and ((there is one redirect handler) or (a redirect handler was requested via the "useRedirectHandler" parameter), @@ -60,6 +96,17 @@ func (authHandler *unionAuthenticationHandler) AuthenticationNeeded(apiClient au if len(headers) > 0 { mergeHeaders(w.Header(), headers) w.WriteHeader(http.StatusUnauthorized) + + // Print Misc Warning headers (code 199) to the body + if warnings, hasWarnings := w.Header()[http.CanonicalHeaderKey("Warning")]; hasWarnings { + for _, warning := range warnings { + warningParts := warningRegex.FindStringSubmatch(warning) + if len(warningParts) != 0 && warningParts[warningHeaderCodeIndex] == WarningHeaderMiscCode { + fmt.Fprintln(w, warningParts[warningHeaderTextIndex]) + } + } + } + return true, nil } diff --git a/pkg/auth/oauth/handlers/default_auth_handler_test.go b/pkg/auth/oauth/handlers/default_auth_handler_test.go index 3e9f1c81105c..1ce17c2c3901 100644 --- a/pkg/auth/oauth/handlers/default_auth_handler_test.go +++ b/pkg/auth/oauth/handlers/default_auth_handler_test.go @@ -185,3 +185,75 @@ func (w *badTestClient) GetRedirectUri() string { func (w *badTestClient) GetUserData() interface{} { return "w.client" } + +func TestWarningRegex(t *testing.T) { + testcases := map[string]struct { + Header string + Match bool + Parts []string + }{ + // Empty + "empty": {}, + + // Invalid code segment + "code 2 numbers": {Header: `19 OpenShift "Message goes here"`}, + "code 4 numbers": {Header: `1999 OpenShift "Message goes here"`}, + "code non-numbers": {Header: `ABC OpenShift "Message goes here"`}, + + // Invalid agent segment + "agent missing": {Header: `199 "Message goes here"`}, + "agent spaces": {Header: `199 Open Shift "Message goes here"`}, + + // Invalid text segment + "text missing": {Header: `199 OpenShift`}, + "text unquoted": {Header: `199 OpenShift Message`}, + "text single quotes": {Header: `199 OpenShift 'Message'`}, + "text bad quotes": {Header: `199 OpenShift "Mes"sage"`}, + "text bad escape": {Header: `199 OpenShift "Mes\\"sage"`}, + + // Invalid date segment + "date unquoted": {Header: `199 OpenShift "Message" Date`}, + "date single quoted": {Header: `199 OpenShift "Message" 'Date'`}, + "date empty": {Header: `199 OpenShift "Message" ""`}, + + // Valid segments + "valid no date": { + Header: `199 OpenShift "Message goes here"`, + Match: true, + Parts: []string{"199", "OpenShift", "Message goes here", ""}, + }, + + "valid with date": { + Header: `199 OpenShift "Message goes here" "date"`, + Match: true, + Parts: []string{"199", "OpenShift", "Message goes here", "date"}, + }, + + "valid with escaped quote": { + Header: `199 OpenShift "Message \" goes here" "date"`, + Match: true, + Parts: []string{"199", "OpenShift", `Message \" goes here`, "date"}, + }, + + "valid with escaped quote and slash": { + Header: `199 OpenShift "Message \\\" goes here" "date"`, + Match: true, + Parts: []string{"199", "OpenShift", `Message \\\" goes here`, "date"}, + }, + } + + for k, tc := range testcases { + parts := warningRegex.FindStringSubmatch(tc.Header) + match := len(parts) > 0 + if match != tc.Match { + t.Errorf("%s: Expected match %v, got %v", k, tc.Match, match) + continue + } + if !match { + continue + } + if !reflect.DeepEqual(parts[1:], tc.Parts) { + t.Errorf("%s: Expected\n\t%#v\n\tgot\n\t%#v", k, tc.Parts, parts[1:]) + } + } +} diff --git a/pkg/cmd/experimental/tokens/request.go b/pkg/cmd/experimental/tokens/request.go index bff0c460963b..7fa69dda8c9d 100644 --- a/pkg/cmd/experimental/tokens/request.go +++ b/pkg/cmd/experimental/tokens/request.go @@ -4,6 +4,7 @@ import ( "fmt" "os" + "github.com/GoogleCloudPlatform/kubernetes/pkg/kubectl/cmd/util" "github.com/spf13/cobra" "github.com/openshift/origin/pkg/cmd/util/clientcmd" @@ -17,14 +18,10 @@ func NewCmdRequestToken(f *clientcmd.Factory) *cobra.Command { Long: `request an access token`, Run: func(cmd *cobra.Command, args []string) { clientCfg, err := f.OpenShiftClientConfig.ClientConfig() - if err != nil { - fmt.Errorf("%v\n", err) - } + util.CheckErr(err) accessToken, err := tokencmd.RequestToken(clientCfg, os.Stdin, "", "") - if err != nil { - fmt.Errorf("%v\n", err) - } + util.CheckErr(err) fmt.Printf("%v\n", string(accessToken)) }, diff --git a/pkg/cmd/experimental/tokens/tokens.go b/pkg/cmd/experimental/tokens/tokens.go index cf571356594e..e39790acc6e3 100644 --- a/pkg/cmd/experimental/tokens/tokens.go +++ b/pkg/cmd/experimental/tokens/tokens.go @@ -2,6 +2,7 @@ package tokens import ( "os" + "path" "github.com/GoogleCloudPlatform/kubernetes/pkg/client" "github.com/golang/glog" @@ -44,5 +45,5 @@ func getFlagString(cmd *cobra.Command, flag string) string { } func getRequestTokenURL(clientCfg *client.Config) string { - return clientCfg.Host + origin.OpenShiftLoginPrefix + tokenrequest.RequestTokenEndpoint + return clientCfg.Host + path.Join(origin.OpenShiftOAuthAPIPrefix, tokenrequest.RequestTokenEndpoint) } diff --git a/pkg/cmd/util/tokencmd/challenging_client.go b/pkg/cmd/util/tokencmd/challenging_client.go index 12b191f926f5..39889cfd1e24 100644 --- a/pkg/cmd/util/tokencmd/challenging_client.go +++ b/pkg/cmd/util/tokencmd/challenging_client.go @@ -12,6 +12,10 @@ import ( "github.com/openshift/origin/pkg/cmd/util" ) +// CSRFTokenHeader is a marker header that indicates we are not a browser that got tricked into requesting basic auth +// Corresponds to the header expected by basic-auth challenging authenticators +const CSRFTokenHeader = "X-CSRF-Token" + // challengingClient conforms the kclient.HTTPClient interface. It introspects responses for auth challenges and // tries to response to those challenges in order to get a token back. type challengingClient struct { @@ -27,6 +31,12 @@ var basicAuthRegex = regexp.MustCompile(basicAuthPattern) // Do watches for unauthorized challenges. If we know to respond, we respond to the challenge func (client *challengingClient) Do(req *http.Request) (*http.Response, error) { + // Set custom header required by server to avoid CSRF attacks on browsers using basic auth + if req.Header == nil { + req.Header = http.Header{} + } + req.Header.Set(CSRFTokenHeader, "1") + resp, err := client.delegate.Do(req) if err != nil { return nil, err From c44f69bd2a6beac7cb9103283a93dfc6d1249d17 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Fri, 10 Apr 2015 01:20:36 -0400 Subject: [PATCH 2/3] Add placeholder challenger when login is only possible via browser --- .../placeholder_challenger.go | 33 +++++++++++++++++++ pkg/cmd/server/origin/auth.go | 9 +++++ 2 files changed, 42 insertions(+) create mode 100644 pkg/auth/authenticator/challenger/placeholderchallenger/placeholder_challenger.go diff --git a/pkg/auth/authenticator/challenger/placeholderchallenger/placeholder_challenger.go b/pkg/auth/authenticator/challenger/placeholderchallenger/placeholder_challenger.go new file mode 100644 index 000000000000..45347cdf7080 --- /dev/null +++ b/pkg/auth/authenticator/challenger/placeholderchallenger/placeholder_challenger.go @@ -0,0 +1,33 @@ +package placeholderchallenger + +import ( + "fmt" + "net/http" + + oauthhandlers "github.com/openshift/origin/pkg/auth/oauth/handlers" +) + +type placeholderChallenger struct { + tokenRequestURL string +} + +// New returns an AuthenticationChallenger that responds with a warning and link to the web UI for requesting a token +func New(url string) oauthhandlers.AuthenticationChallenger { + return placeholderChallenger{url} +} + +// AuthenticationChallenge returns a header that indicates a basic auth challenge for the supplied realm +func (c placeholderChallenger) AuthenticationChallenge(req *http.Request) (http.Header, error) { + headers := http.Header{} + headers.Add("Warning", + fmt.Sprintf( + `%s %s "You must obtain an API token by visiting %s"`, + oauthhandlers.WarningHeaderMiscCode, + oauthhandlers.WarningHeaderOpenShiftSource, + c.tokenRequestURL, + ), + ) + headers.Add("Link", fmt.Sprintf(`<%s>; rel="related"`, c.tokenRequestURL)) + + return headers, nil +} diff --git a/pkg/cmd/server/origin/auth.go b/pkg/cmd/server/origin/auth.go index 5de150d4bb6f..045810820b2e 100644 --- a/pkg/cmd/server/origin/auth.go +++ b/pkg/cmd/server/origin/auth.go @@ -23,6 +23,7 @@ import ( "github.com/openshift/origin/pkg/auth/authenticator" "github.com/openshift/origin/pkg/auth/authenticator/challenger/passwordchallenger" + "github.com/openshift/origin/pkg/auth/authenticator/challenger/placeholderchallenger" "github.com/openshift/origin/pkg/auth/authenticator/password/allowanypassword" "github.com/openshift/origin/pkg/auth/authenticator/password/basicauthpassword" "github.com/openshift/origin/pkg/auth/authenticator/password/denypassword" @@ -183,6 +184,9 @@ func OpenShiftOAuthAuthorizeURL(masterAddr string) string { func OpenShiftOAuthTokenURL(masterAddr string) string { return masterAddr + path.Join(OpenShiftOAuthAPIPrefix, osinserver.TokenPath) } +func OpenShiftOAuthTokenRequestURL(masterAddr string) string { + return masterAddr + path.Join(OpenShiftOAuthAPIPrefix, tokenrequest.RequestTokenEndpoint) +} func CreateOrUpdateDefaultOAuthClients(masterPublicAddr string, assetPublicAddresses []string, clientRegistry oauthclient.Registry) { clientsToEnsure := []*oauthapi.OAuthClient{ @@ -330,6 +334,11 @@ func (c *AuthConfig) getAuthenticationHandler(mux cmdutil.Mux, errorHandler hand } } + if len(redirectors) > 0 && len(challengers) == 0 { + // Add a default challenger that will warn and give a link to the web browser token-granting location + challengers["placeholder"] = placeholderchallenger.New(OpenShiftOAuthTokenRequestURL(c.Options.MasterPublicURL)) + } + authHandler := handlers.NewUnionAuthenticationHandler(challengers, redirectors, errorHandler) return authHandler, nil } From 7ecd59d7032c26df8884ee14981ecbe5e518cea7 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Fri, 10 Apr 2015 01:21:49 -0400 Subject: [PATCH 3/3] Display login failures in osc --- pkg/auth/server/tokenrequest/endpoints.go | 18 ++++++++++-------- pkg/cmd/cli/cmd/login.go | 21 ++++++++++++++++++++- pkg/cmd/server/origin/auth.go | 2 +- 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/pkg/auth/server/tokenrequest/endpoints.go b/pkg/auth/server/tokenrequest/endpoints.go index e310dd0c7bef..5a8f22bf84ab 100644 --- a/pkg/auth/server/tokenrequest/endpoints.go +++ b/pkg/auth/server/tokenrequest/endpoints.go @@ -20,6 +20,7 @@ const ( ) type endpointDetails struct { + publicMasterURL string originOAuthClient *osincli.Client } @@ -27,8 +28,8 @@ type Endpoints interface { Install(mux login.Mux, paths ...string) } -func NewEndpoints(originOAuthClient *osincli.Client) Endpoints { - return &endpointDetails{originOAuthClient} +func NewEndpoints(publicMasterURL string, originOAuthClient *osincli.Client) Endpoints { + return &endpointDetails{publicMasterURL, originOAuthClient} } // Install registers the request token endpoints into a mux. It is expected that the @@ -50,7 +51,7 @@ func (endpoints *endpointDetails) requestToken(w http.ResponseWriter, req *http. func (endpoints *endpointDetails) displayToken(w http.ResponseWriter, req *http.Request) { w.Header().Set("Content-Type", "text/html") - data := tokenData{RequestURL: "request"} + data := tokenData{RequestURL: "request", PublicMasterURL: endpoints.publicMasterURL} authorizeReq := endpoints.originOAuthClient.NewAuthorizeRequest(osincli.CODE) authorizeData, err := authorizeReq.HandleRequest(req) @@ -90,10 +91,11 @@ func renderToken(w io.Writer, data tokenData) { } type tokenData struct { - Error string - OAuthJSON string - AccessToken string - RequestURL string + Error string + OAuthJSON string + AccessToken string + RequestURL string + PublicMasterURL string } // TODO: allow template to be read from an external file @@ -112,7 +114,7 @@ var tokenTemplate = template.Must(template.New("tokenTemplate").Parse(` 
{{.OAuthJSON}}

How do I use this token?

- 
osc --token={{.AccessToken}} …
+ 
osc login --token={{.AccessToken}} --server={{.PublicMasterURL}}
curl -H "Authorization: Bearer {{.AccessToken}}" …

How do I delete this token when I'm done?

diff --git a/pkg/cmd/cli/cmd/login.go b/pkg/cmd/cli/cmd/login.go index c2e651a32418..b8a0dbad998b 100644 --- a/pkg/cmd/cli/cmd/login.go +++ b/pkg/cmd/cli/cmd/login.go @@ -3,10 +3,13 @@ package cmd import ( "fmt" "io" + "os" "github.com/spf13/cobra" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" cmdutil "github.com/GoogleCloudPlatform/kubernetes/pkg/kubectl/cmd/util" + "github.com/openshift/origin/pkg/cmd/cli/config" "github.com/openshift/origin/pkg/cmd/templates" osclientcmd "github.com/openshift/origin/pkg/cmd/util/clientcmd" @@ -37,7 +40,23 @@ func NewCmdLogin(f *osclientcmd.Factory, reader io.Reader, out io.Writer) *cobra Long: longDescription, Run: func(cmd *cobra.Command, args []string) { err := RunLogin(cmd, options) - cmdutil.CheckErr(err) + + if errors.IsUnauthorized(err) { + fmt.Fprintln(out, "Login failed (401 Unauthorized)") + + if err, isStatusErr := err.(*errors.StatusError); isStatusErr { + if details := err.Status().Details; details != nil { + for _, cause := range details.Causes { + fmt.Fprintln(out, cause.Message) + } + } + } + + os.Exit(1) + + } else { + cmdutil.CheckErr(err) + } }, } diff --git a/pkg/cmd/server/origin/auth.go b/pkg/cmd/server/origin/auth.go index 045810820b2e..5ce630231288 100644 --- a/pkg/cmd/server/origin/auth.go +++ b/pkg/cmd/server/origin/auth.go @@ -149,7 +149,7 @@ func (c *AuthConfig) InstallAPI(container *restful.Container) []string { osOAuthClient.Transport = &transport } - tokenRequestEndpoints := tokenrequest.NewEndpoints(osOAuthClient) + tokenRequestEndpoints := tokenrequest.NewEndpoints(c.Options.MasterPublicURL, osOAuthClient) tokenRequestEndpoints.Install(mux, OpenShiftOAuthAPIPrefix) // glog.Infof("oauth server configured as: %#v", server)