From 88375ed39232e1eb964936bf33637e03054368c7 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Fri, 20 Feb 2015 15:44:36 -0500 Subject: [PATCH 1/3] Strip access_token param from requests in auth layer --- .../request/paramtoken/paramtoken.go | 17 +++++++++++++---- pkg/cmd/server/start.go | 3 +-- 2 files changed, 14 insertions(+), 6 deletions(-) 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 { From edb5a43ddbbe5c3b6f0e6495319e0f6928cac47c Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Fri, 20 Feb 2015 13:26:58 -0500 Subject: [PATCH 2/3] Strip auth headers before proxying, add auth headers on upgrade requests --- pkg/util/httpproxy/upgradeawareproxy.go | 78 ++++++++++++++++++++++--- 1 file changed, 70 insertions(+), 8 deletions(-) diff --git a/pkg/util/httpproxy/upgradeawareproxy.go b/pkg/util/httpproxy/upgradeawareproxy.go index c10f229db2b6..801685ce9902 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" @@ -54,6 +56,20 @@ func (p *UpgradeAwareSingleHostReverseProxy) RoundTrip(req *http.Request) (*http resp.Header.Del("Access-Control-Allow-Methods") resp.Header.Del("Access-Control-Allow-Origin") + 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? return resp, err @@ -88,7 +104,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 +173,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 +206,17 @@ 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) + 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 +224,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 +232,25 @@ 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") +} + +// 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) + } +} From 42aa52cb69bff3c0c2e84e88338cae57cf2c5b2d Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Fri, 20 Feb 2015 15:54:27 -0500 Subject: [PATCH 3/3] Remove cors headers from proxied connections --- pkg/util/httpproxy/upgradeawareproxy.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/pkg/util/httpproxy/upgradeawareproxy.go b/pkg/util/httpproxy/upgradeawareproxy.go index 801685ce9902..2d6a98e97371 100644 --- a/pkg/util/httpproxy/upgradeawareproxy.go +++ b/pkg/util/httpproxy/upgradeawareproxy.go @@ -50,12 +50,7 @@ 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) @@ -206,6 +201,8 @@ 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 + removeCORSHeaders(resp) + removeChallengeHeaders(resp) err = resp.Write(requestHijackedConn) if err != nil { glog.Errorf("Error writing backend response to client: %s", err) @@ -245,6 +242,15 @@ 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) {