Skip to content

Commit

Permalink
Merge pull request #461 from spadgett/proxy
Browse files Browse the repository at this point in the history
Bug 1622372 - Require CSRF token on all proxied requests
  • Loading branch information
openshift-merge-robot authored Aug 27, 2018
2 parents ccd3666 + 09e85eb commit d566668
Show file tree
Hide file tree
Showing 8 changed files with 54 additions and 37 deletions.
32 changes: 25 additions & 7 deletions auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
const (
CSRFCookieName = "csrf-token"
CSRFHeader = "X-CSRFToken"
CSRFQueryParam = "x-csrf-token"
stateCookieName = "state-token"
errorOAuth = "oauth_error"
errorLoginState = "login_state_error"
Expand Down Expand Up @@ -356,24 +357,36 @@ func (a *Authenticator) redirectAuthError(w http.ResponseWriter, authErr string,
w.WriteHeader(http.StatusSeeOther)
}

func (a *Authenticator) VerifyReferer(r *http.Request) (err error) {
referer := r.Referer()
if len(referer) == 0 {
return fmt.Errorf("No referer!")
func (a *Authenticator) getSourceOrigin(r *http.Request) string {
origin := r.Header.Get("Origin")
if len(origin) != 0 {
return origin
}

u, err := url.Parse(referer)
return r.Referer()
}

// VerifySourceOrigin checks that the Origin request header, if present, matches the target origin. Otherwise, it checks the Referer request header.
// https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet#Identifying_Source_Origin
func (a *Authenticator) VerifySourceOrigin(r *http.Request) (err error) {
source := a.getSourceOrigin(r)
if len(source) == 0 {
return fmt.Errorf("no Origin or Referer header in request")
}

u, err := url.Parse(source)
if err != nil {
return err
}

isValid := a.refererURL.Hostname() == u.Hostname() &&
a.refererURL.Port() == u.Port() &&
a.refererURL.Scheme == u.Scheme &&
strings.HasPrefix(u.Path, a.refererURL.Path)
// The Origin header does not have a path
(u.Path == "" || strings.HasPrefix(u.Path, a.refererURL.Path))

if !isValid {
return fmt.Errorf("invalid referer: %v expected `%v`", referer, a.refererURL)
return fmt.Errorf("invalid Origin or Referer: %v expected `%v`", source, a.refererURL)
}
return nil
}
Expand All @@ -392,6 +405,11 @@ func (a *Authenticator) SetCSRFCookie(path string, w *http.ResponseWriter) {

func (a *Authenticator) VerifyCSRFToken(r *http.Request) (err error) {
CSRFToken := r.Header.Get(CSRFHeader)
if CSRFToken == "" {
// Fallback to a query parameter, which is needed for websockets
CSRFToken = r.URL.Query().Get(CSRFQueryParam)
}

CRSCookie, err := r.Cookie(CSRFCookieName)
if err != nil {
return fmt.Errorf("No CSRF Cookie!")
Expand Down
2 changes: 1 addition & 1 deletion auth/auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ func testReferer(t *testing.T, referer string, accept bool) {
r.Header.Set("Referer", referer)
}

err = a.VerifyReferer(r)
err = a.VerifySourceOrigin(r)

if err != nil && accept {
t.Errorf("Unexpected error for referer `%v`:\n%v", referer, err)
Expand Down
6 changes: 3 additions & 3 deletions cmd/bridge/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ func main() {

srv.K8sProxyConfig = &proxy.Config{
TLSClientConfig: tlsConfig,
HeaderBlacklist: []string{"Cookie"},
HeaderBlacklist: []string{"Cookie", "X-CSRFToken"},
Endpoint: k8sEndpoint,
}

Expand All @@ -268,7 +268,7 @@ func main() {
// Only proxy requests to the Prometheus API, not the UI.
srv.PrometheusProxyConfig = &proxy.Config{
TLSClientConfig: prometheusTLSConfig,
HeaderBlacklist: []string{"Cookie"},
HeaderBlacklist: []string{"Cookie", "X-CSRFToken"},
Endpoint: &url.URL{Scheme: "https", Host: openshiftPrometheusHost, Path: "/api"},
}
} else if !os.IsNotExist(err) {
Expand All @@ -283,7 +283,7 @@ func main() {
TLSClientConfig: &tls.Config{
InsecureSkipVerify: *fK8sModeOffClusterSkipVerifyTLS,
},
HeaderBlacklist: []string{"Cookie"},
HeaderBlacklist: []string{"Cookie", "X-CSRFToken"},
Endpoint: k8sEndpoint,
}
default:
Expand Down
6 changes: 2 additions & 4 deletions frontend/public/co-fetch.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,16 +84,14 @@ export class TimeoutError extends Error {
}

const cookiePrefix = 'csrf-token=';
const getCSRFToken = () => document && document.cookie && document.cookie.split(';')
export const getCSRFToken = () => document && document.cookie && document.cookie.split(';')
.map(c => _.trim(c))
.filter(c => c.startsWith(cookiePrefix))
.map(c => c.slice(cookiePrefix.length)).pop();

export const coFetch = (url, options = {}, timeout=20000) => {
const allOptions = _.defaultsDeep({}, initDefaults, options);
if (allOptions.method !== 'GET') {
allOptions.headers['X-CSRFToken'] = getCSRFToken();
}
allOptions.headers['X-CSRFToken'] = getCSRFToken();

// If the URL being requested is absolute (and therefore, not a local request),
// remove the authorization header to prevent credentials from leaking.
Expand Down
12 changes: 11 additions & 1 deletion frontend/public/module/ws-factory.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,15 @@
*/
/* eslint-disable no-console */

import { getCSRFToken } from '../co-fetch';

function addCSRFQueryParam(href) {
const url = new URL(href);
const csrfToken = getCSRFToken();
url.searchParams.set('x-csrf-token', csrfToken);
return url.href;
}

function createURL(host, path) {
let url;

Expand All @@ -22,7 +31,8 @@ function createURL(host, path) {
if (path) {
url += path;
}
return url;

return addCSRFQueryParam(url);
}

export function WSFactory(id, options) {
Expand Down
5 changes: 4 additions & 1 deletion pkg/proxy/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,12 @@ func decodeSubprotocol(encodedProtocol string) (string, error) {
return string(decodedProtocol), err
}

var headerBlacklist = []string{"Cookie"}
var headerBlacklist = []string{"Cookie", "X-CSRFToken"}

func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
// Block scripts from running in proxied content for browsers that support Content-Security-Policy.
w.Header().Set("Content-Security-Policy", "default-src 'none';")

isWebsocket := false
upgrades := r.Header["Upgrade"]

Expand Down
27 changes: 8 additions & 19 deletions server/middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,27 +27,16 @@ func authMiddlewareWithUser(a *auth.Authenticator, handlerFunc func(user *auth.U

r.Header.Set("Authorization", fmt.Sprintf("Bearer %s", user.Token))

safe := false
switch r.Method {
case
"GET",
"HEAD",
"OPTIONS",
"TRACE":
safe = true
if err := a.VerifySourceOrigin(r); err != nil {
plog.Infof("invalid source origin: %v", err)
w.WriteHeader(http.StatusForbidden)
return
}

if !safe {
if err := a.VerifyReferer(r); err != nil {
plog.Infof("Invalid referer %v", err)
w.WriteHeader(http.StatusForbidden)
return
}
if err := a.VerifyCSRFToken(r); err != nil {
plog.Infof("Invalid CSRFToken %v", err)
w.WriteHeader(http.StatusForbidden)
return
}
if err := a.VerifyCSRFToken(r); err != nil {
plog.Infof("invalid CSRFToken: %v", err)
w.WriteHeader(http.StatusForbidden)
return
}

handlerFunc(user, w, r)
Expand Down
1 change: 0 additions & 1 deletion server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ type jsGlobals struct {
Branding string `json:"branding"`
DocumentationBaseURL string `json:"documentationBaseURL"`
ClusterName string `json:"clusterName"`
CSRFToken string `json:"CSRFToken"`
GoogleTagManagerID string `json:"googleTagManagerID"`
LoadTestFactor int `json:"loadTestFactor"`
}
Expand Down

0 comments on commit d566668

Please sign in to comment.