diff --git a/pkg/auth/authenticator/request/paramtoken/paramtoken.go b/pkg/auth/authenticator/request/paramtoken/paramtoken.go index 3ebf38d4cf08..2a44334dc0f6 100644 --- a/pkg/auth/authenticator/request/paramtoken/paramtoken.go +++ b/pkg/auth/authenticator/request/paramtoken/paramtoken.go @@ -13,18 +13,27 @@ import ( // For this authenticator to work, tokens will be part of the request URL, and are more likely to be logged or otherwise exposed. // Every effort should be made to filter tokens from being logged when using this authenticator. type Authenticator struct { + // Param is the query param to use as a token param string - auth authenticator.Token + // Auth is the token authenticator to use to validate the token + auth authenticator.Token + // Remove indicates whether the parameter should be stripped from the incoming request + remove bool } -func New(param string, auth authenticator.Token) *Authenticator { - return &Authenticator{param, auth} +func New(param string, auth authenticator.Token, remove bool) *Authenticator { + return &Authenticator{param, auth, remove} } func (a *Authenticator) AuthenticateRequest(req *http.Request) (user.Info, bool, error) { - token := strings.TrimSpace(req.FormValue(a.param)) + q := req.URL.Query() + token := strings.TrimSpace(q.Get(a.param)) if token == "" { return nil, false, nil } + if a.remove { + q.Del(a.param) + req.URL.RawQuery = q.Encode() + } return a.auth.AuthenticateToken(token) } diff --git a/pkg/cmd/server/start.go b/pkg/cmd/server/start.go index 8170a000d510..b0c8d9f575be 100644 --- a/pkg/cmd/server/start.go +++ b/pkg/cmd/server/start.go @@ -395,8 +395,7 @@ func start(cfg *config, args []string) error { // Allow token as access_token param for WebSockets // TODO: make the param name configurable // TODO: limit this authenticator to watch methods, if possible - // TODO: prevent access_token param from getting logged, if possible - authenticators = append(authenticators, paramtoken.New("access_token", tokenAuthenticator)) + authenticators = append(authenticators, paramtoken.New("access_token", tokenAuthenticator, true)) var roots *x509.CertPool if osmaster.TLS { diff --git a/pkg/util/httpproxy/upgradeawareproxy.go b/pkg/util/httpproxy/upgradeawareproxy.go index c10f229db2b6..2d6a98e97371 100644 --- a/pkg/util/httpproxy/upgradeawareproxy.go +++ b/pkg/util/httpproxy/upgradeawareproxy.go @@ -1,9 +1,11 @@ package httpproxy import ( + "bufio" "crypto/tls" "fmt" "io" + "io/ioutil" "net" "net/http" "net/http/httputil" @@ -48,11 +50,20 @@ func (p *UpgradeAwareSingleHostReverseProxy) RoundTrip(req *http.Request) (*http return resp, err } - // strip off CORS headers sent from the backend - resp.Header.Del("Access-Control-Allow-Credentials") - resp.Header.Del("Access-Control-Allow-Headers") - resp.Header.Del("Access-Control-Allow-Methods") - resp.Header.Del("Access-Control-Allow-Origin") + removeCORSHeaders(resp) + removeChallengeHeaders(resp) + if resp.StatusCode == http.StatusUnauthorized { + glog.Errorf("Got unauthorized error from backend for: %s %s", req.Method, req.URL) + // Internal error, backend didn't recognize proxy identity + // Surface as a server error to the client + // TODO do we need to do more than this? + resp = &http.Response{ + StatusCode: http.StatusInternalServerError, + Status: http.StatusText(http.StatusInternalServerError), + Body: ioutil.NopCloser(strings.NewReader("Internal Server Error")), + ContentLength: -1, + } + } // TODO do we need to strip off anything else? @@ -88,7 +99,10 @@ func (p *UpgradeAwareSingleHostReverseProxy) newProxyRequest(req *http.Request) } // TODO is this the right way to copy headers? newReq.Header = req.Header - // TODO do we need to exclude any headers? + + // TODO do we need to exclude any other headers? + removeAuthHeaders(newReq) + return newReq, nil } @@ -154,6 +168,29 @@ func (p *UpgradeAwareSingleHostReverseProxy) serveUpgrade(w http.ResponseWriter, } defer backendConn.Close() + addAuthHeaders(req, p.clientConfig) + + err = req.Write(backendConn) + if err != nil { + glog.Errorf("Error writing request to backend: %s", err) + return + } + + resp, err := http.ReadResponse(bufio.NewReader(backendConn), req) + if err != nil { + glog.Errorf("Error reading response from backend: %s", err) + w.WriteHeader(http.StatusInternalServerError) + w.Write([]byte("Internal Server Error")) + return + } + + if resp.StatusCode == http.StatusUnauthorized { + glog.Errorf("Got unauthorized error from backend for: %s %s", req.Method, req.URL) + w.WriteHeader(http.StatusInternalServerError) + w.Write([]byte("Internal Server Error")) + return + } + requestHijackedConn, _, err := w.(http.Hijacker).Hijack() if err != nil { glog.Errorf("Error hijacking request connection: %s", err) @@ -164,19 +201,19 @@ func (p *UpgradeAwareSingleHostReverseProxy) serveUpgrade(w http.ResponseWriter, // NOTE: from this point forward, we own the connection and we can't use // w.Header(), w.Write(), or w.WriteHeader any more - err = req.Write(backendConn) + removeCORSHeaders(resp) + removeChallengeHeaders(resp) + err = resp.Write(requestHijackedConn) if err != nil { - glog.Errorf("Error writing request to backend: %s", err) + glog.Errorf("Error writing backend response to client: %s", err) + return } done := make(chan struct{}, 2) go func() { _, err := io.Copy(backendConn, requestHijackedConn) - if err != nil { - // TODO I see this printed at least whenever the client goes away from the page. - // Should we check for different types of errors and only log certain ones? - // Should we make this V(2+) ? + if err != nil && !strings.Contains(err.Error(), "use of closed network connection") { glog.Errorf("Error proxying data from client to backend: %v", err) } done <- struct{}{} @@ -184,7 +221,7 @@ func (p *UpgradeAwareSingleHostReverseProxy) serveUpgrade(w http.ResponseWriter, go func() { _, err := io.Copy(requestHijackedConn, backendConn) - if err != nil { + if err != nil && !strings.Contains(err.Error(), "use of closed network connection") { glog.Errorf("Error proxying data from backend to client: %v", err) } done <- struct{}{} @@ -192,3 +229,34 @@ func (p *UpgradeAwareSingleHostReverseProxy) serveUpgrade(w http.ResponseWriter, <-done } + +// removeAuthHeaders strips authorization headers from an incoming client +// This should be called on all requests before proxying +func removeAuthHeaders(req *http.Request) { + req.Header.Del("Authorization") +} + +// removeChallengeHeaders strips WWW-Authenticate headers from backend responses +// This should be called on all responses before returning +func removeChallengeHeaders(resp *http.Response) { + resp.Header.Del("WWW-Authenticate") +} + +// removeCORSHeaders strip CORS headers sent from the backend +// This should be called on all responses before returning +func removeCORSHeaders(resp *http.Response) { + resp.Header.Del("Access-Control-Allow-Credentials") + resp.Header.Del("Access-Control-Allow-Headers") + resp.Header.Del("Access-Control-Allow-Methods") + resp.Header.Del("Access-Control-Allow-Origin") +} + +// addAuthHeaders adds basic/bearer auth from the given config (if specified) +// This should be run on any requests not handled by the transport returned from TransportFor(config) +func addAuthHeaders(req *http.Request, clientConfig *kclient.Config) { + if clientConfig.BearerToken != "" { + req.Header.Set("Authorization", "Bearer "+clientConfig.BearerToken) + } else if clientConfig.Username != "" || clientConfig.Password != "" { + req.SetBasicAuth(clientConfig.Username, clientConfig.Password) + } +}