Improve handling of challenging OAuth clients

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
 }

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)
 	}
 
 }

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
+}

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
 
 		}

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:])
+		}
+	}
+}

pkg/auth/server/tokenrequest/endpoints.go

@@ -20,15 +20,16 @@ const (
 )
 
 type endpointDetails struct {
+	publicMasterURL   string
 	originOAuthClient *osincli.Client
 }
 
 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(`
   <pre>{{.OAuthJSON}}</pre>
 
   <h3>How do I use this token?</h3>
-  <pre>osc --token={{.AccessToken}} &hellip;</pre>
+  <pre>osc login --token={{.AccessToken}} --server={{.PublicMasterURL}}</pre>
   <pre>curl -H "Authorization: Bearer {{.AccessToken}}" &hellip;</pre>
 
   <h3>How do I delete this token when I'm done?</h3>

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)
+			}
 		},
 	}
 

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))
 		},