Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CSRF protection to deck #13323

Merged
merged 4 commits into from
Jul 16, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ require (
github.com/google/uuid v1.0.0
github.com/googleapis/gax-go/v2 v2.0.5 // indirect
github.com/gophercloud/gophercloud v0.0.0-20181215224939-bdd8b1ecd793 // indirect
github.com/gorilla/csrf v1.6.0
mirandachrist marked this conversation as resolved.
Show resolved Hide resolved
github.com/gorilla/securecookie v1.1.1
github.com/gorilla/sessions v1.1.3
github.com/gregjones/httpcache v0.0.0-20190212212710-3befbb6ad0cc
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,8 @@ github.com/gophercloud/gophercloud v0.0.0-20181215224939-bdd8b1ecd793 h1:rT82/6k
github.com/gophercloud/gophercloud v0.0.0-20181215224939-bdd8b1ecd793/go.mod h1:3WdhXV3rUYy9p6AUW8d94kr+HS62Y4VL9mBnFxsD8q4=
github.com/gorilla/context v1.1.1 h1:AWwleXJkX/nhcU9bZSnZoi3h/qGYqQAGhq6zZe/aQW8=
github.com/gorilla/context v1.1.1/go.mod h1:kBGZzfjB9CEq2AlWe17Uuf7NDRt0dE0s8S51q0aT7Yg=
github.com/gorilla/csrf v1.6.0 h1:60oN1cFdncCE8tjwQ3QEkFND5k37lQPcRjnlvm7CIJ0=
github.com/gorilla/csrf v1.6.0/go.mod h1:7tSf8kmjNYr7IWDCYhd3U8Ck34iQ/Yw5CJu7bAkHEGI=
github.com/gorilla/mux v1.6.2 h1:Pgr17XVTNXAk3q/r4CpKzC5xBM/qW1uVLV+IhRZpIIk=
github.com/gorilla/mux v1.6.2/go.mod h1:1lud6UwP+6orDFRuTfBEV8e9/aOM/c4fVVCaMa2zaAs=
github.com/gorilla/securecookie v1.1.1 h1:miw7JPhV+b/lAHSXz4qd/nN9jRiAFV5FwjeKyCS8BvQ=
Expand Down
1 change: 1 addition & 0 deletions prow/cmd/deck/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ go_library(
"//prow/tide/history:go_default_library",
"//vendor/cloud.google.com/go/storage:go_default_library",
"//vendor/github.com/NYTimes/gziphandler:go_default_library",
"//vendor/github.com/gorilla/csrf:go_default_library",
"//vendor/github.com/gorilla/sessions:go_default_library",
"//vendor/github.com/prometheus/client_golang/prometheus:go_default_library",
"//vendor/github.com/sirupsen/logrus:go_default_library",
Expand Down
34 changes: 34 additions & 0 deletions prow/cmd/deck/csrf.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# CSRF attacks
In Deck, we make a number of `POST` requests that require user authentication. These requests are susceptible
to [cross site request forgery (CSRF) attacks](https://en.wikipedia.org/wiki/Cross-site_request_forgery),
in which a malicious actor tricks an already authenticated user into submitting a form to one of these endpoints
and performing one of these protected actions on their behalf.

# Protection
If `--cookie-secret` is 32 or more bytes long, CSRF protection is automatically enabled.
If `--rerun-creates-job` is specified, CSRF protection is required, and accordingly,
`--cookie-secret` must be 32 bytes long.

We protect against CSRF attacks using the [gorilla CSRF](https://github.com/gorilla/csrf) library, implemented
in [#13323](https://github.com/kubernetes/test-infra/pull/13323). Broadly, this protection works by ensuring that
any `POST` request originates from our site, rather than from an outside link.
We do so by requiring that every `POST` request made to Deck includes a secret token either in the request header
or in the form itself as a hidden input.

We cryptographically generate the CSRF token using the `--cookie-secret` and a user session value and
include it as a header in every `POST` request made from Deck.
If you are adding a new `POST` request, you must include the CSRF token as described in the gorilla
[documentation](https://github.com/gorilla/csrf).

The gorilla library expects a 32-byte CSRF token. If `--cookie-secret` is sufficiently long,
direct job reruns will be enabled via the `/rerun` endpoint. Otherwise, if `--cookie-secret` is less
than 32 bytes and `--rerun-creates-job` is enabled, Deck will refuse to start. Longer values will
work but should be truncated.

By default, gorilla CSRF requires that all `POST` requests are made over HTTPS. If developing locally
over HTTP, you must specify `--allow-insecure` to Deck, which will configure both gorilla CSRF
and GitHub oauth to allow HTTP requests.

CSRF can also be executed by tricking a user into making a state-mutating `GET` request. All
state-mutating requests must therefore be `POST` requests, as gorilla CSRF does not secure `GET`
requests.
65 changes: 58 additions & 7 deletions prow/cmd/deck/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package main
import (
"bytes"
"context"
"crypto/sha256"
"encoding/base64"
"encoding/json"
"errors"
Expand All @@ -36,6 +37,7 @@ import (

"cloud.google.com/go/storage"
"github.com/NYTimes/gziphandler"
"github.com/gorilla/csrf"
"github.com/gorilla/sessions"
"github.com/prometheus/client_golang/prometheus"
"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -93,6 +95,7 @@ type options struct {
spyglassFilesLocation string
gcsCredentialsFile string
rerunCreatesJob bool
allowInsecure bool
}

func (o *options) Validate() error {
Expand Down Expand Up @@ -139,6 +142,7 @@ func gatherOptions(fs *flag.FlagSet, args ...string) options {
fs.StringVar(&o.templateFilesLocation, "template-files-location", "/template", "Path to the template files")
fs.StringVar(&o.gcsCredentialsFile, "gcs-credentials-file", "", "Path to the GCS credentials file")
fs.BoolVar(&o.rerunCreatesJob, "rerun-creates-job", false, "Change the re-run option in Deck to actually create the job. **WARNING:** Only use this with non-public deck instances, otherwise strangers can DOS your Prow instance")
fs.BoolVar(&o.allowInsecure, "allow-insecure", false, "Allows insecure requests for CSRF and GitHub oauth.")
o.kubernetes.AddFlags(fs)
fs.Parse(args)
o.configPath = config.ConfigPath(o.configPath)
Expand Down Expand Up @@ -272,8 +276,6 @@ func main() {
mux.Handle("/tide-history", gziphandler.GzipHandler(handleSimpleTemplate(o, cfg, "tide-history.html", nil)))
mux.Handle("/plugins", gziphandler.GzipHandler(handleSimpleTemplate(o, cfg, "plugins.html", nil)))

indexHandler := handleSimpleTemplate(o, cfg, "index.html", struct{ SpyglassEnabled, ReRunCreatesJob bool }{o.spyglass, o.rerunCreatesJob})

runLocal := o.pregeneratedData != ""

var fallbackHandler func(http.ResponseWriter, *http.Request)
Expand All @@ -289,6 +291,10 @@ func main() {
fallbackHandler(w, r)
return
}
indexHandler := handleSimpleTemplate(o, cfg, "index.html", struct {
SpyglassEnabled bool
ReRunCreatesJob bool
}{o.spyglass, o.rerunCreatesJob})
indexHandler(w, r)
})

Expand All @@ -301,8 +307,50 @@ func main() {
// signal to the world that we're ready
health.ServeReady()

// cookie secret will be used for CSRF protection and should be exactly 32 bytes
mirandachrist marked this conversation as resolved.
Show resolved Hide resolved
// we sometimes accept different lengths to stay backwards compatible
var csrfToken []byte
if o.cookieSecretFile != "" {
cookieSecretRaw, err := loadToken(o.cookieSecretFile)
if err != nil {
logrus.WithError(err).Fatal("Could not read cookie secret file")
}
decodedSecret, err := base64.StdEncoding.DecodeString(string(cookieSecretRaw))
if err != nil {
logrus.WithError(err).Fatal("Error decoding cookie secret")
}
if len(decodedSecret) == 32 {
mirandachrist marked this conversation as resolved.
Show resolved Hide resolved
csrfToken = decodedSecret
}
if len(decodedSecret) > 32 {
logrus.Warning("Cookie secret should be exactly 32 bytes. Consider truncating the existing cookie to that length")
hash := sha256.Sum256(decodedSecret)
csrfToken = hash[:]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the [:] here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sum256 returns a fixed length array, not a slice. It was a bit of a hassle making this work, and the [:] was the best solution I could find

}
if len(decodedSecret) < 32 {
if o.rerunCreatesJob {
logrus.Fatal("Cookie secret must be exactly 32 bytes")
return
}
logrus.Warning("Cookie secret should be exactly 32 bytes")
}
}

// if we allow direct reruns, we must protect against CSRF in all post requests using the cookie secret as a token
// for more information about CSRF, see https://github.com/kubernetes/test-infra/blob/master/prow/cmd/deck/csrf.md
if o.rerunCreatesJob && csrfToken == nil {
logrus.Fatal("Rerun creates job cannot be enabled without CSRF protection, which requires --cookie-secret to be exactly 32 bytes")
return
}

if csrfToken != nil {
CSRF := csrf.Protect(csrfToken, csrf.Path("/"), csrf.Secure(!o.allowInsecure))
logrus.WithError(http.ListenAndServe(":8080", CSRF(traceHandler(mux)))).Fatal("ListenAndServe returned.")
return
}
// setup done, actually start the server
logrus.WithError(http.ListenAndServe(":8080", traceHandler(mux))).Fatal("ListenAndServe returned.")

}

// localOnlyMain contains logic used only when running locally, and is mutually exclusive with
Expand Down Expand Up @@ -489,12 +537,14 @@ func prodOnlyMain(cfg config.Getter, o options, mux *http.ServeMux) *http.ServeM
&githubOAuthConfig,
logrus.WithField("client", "pr-status"))

secure := !o.allowInsecure

mux.Handle("/pr-data.js", handleNotCached(
prStatusAgent.HandlePrStatus(prStatusAgent)))
// Handles login request.
mux.Handle("/github-login", goa.HandleLogin(oauthClient))
mux.Handle("/github-login", goa.HandleLogin(oauthClient, secure))
// Handles redirect from GitHub OAuth server.
mux.Handle("/github-login/redirect", goa.HandleRedirect(oauthClient, githuboauth.NewGitHubClientGetter()))
mux.Handle("/github-login/redirect", goa.HandleRedirect(oauthClient, githuboauth.NewGitHubClientGetter(), secure))
}
mux.Handle("/rerun", gziphandler.GzipHandler(handleRerun(prowJobClient, o.rerunCreatesJob, cfgGetter, goa, githuboauth.NewGitHubClientGetter())))

Expand Down Expand Up @@ -725,7 +775,8 @@ func handleRequestJobViews(sg *spyglass.Spyglass, cfg config.Getter, o options)
setHeadersNoCaching(w)
src := strings.TrimPrefix(r.URL.Path, "/view/")

page, err := renderSpyglass(sg, cfg, src, o)
csrfToken := csrf.Token(r)
page, err := renderSpyglass(sg, cfg, src, o, csrfToken)
if err != nil {
logrus.WithError(err).Error("error rendering spyglass page")
message := fmt.Sprintf("error rendering spyglass page: %v", err)
Expand All @@ -744,7 +795,7 @@ func handleRequestJobViews(sg *spyglass.Spyglass, cfg config.Getter, o options)
}

// renderSpyglass returns a pre-rendered Spyglass page from the given source string
func renderSpyglass(sg *spyglass.Spyglass, cfg config.Getter, src string, o options) (string, error) {
func renderSpyglass(sg *spyglass.Spyglass, cfg config.Getter, src string, o options, csrfToken string) (string, error) {
renderStart := time.Now()

src = strings.TrimSuffix(src, "/")
Expand Down Expand Up @@ -913,7 +964,7 @@ lensesLoop:
}
t := template.New("spyglass.html")

if _, err := prepareBaseTemplate(o, cfg, t); err != nil {
if _, err := prepareBaseTemplate(o, cfg, csrfToken, t); err != nil {
return "", fmt.Errorf("error preparing base template: %v", err)
}
t, err = t.ParseFiles(path.Join(o.templateFilesLocation, "spyglass.html"))
Expand Down
2 changes: 2 additions & 0 deletions prow/cmd/deck/static/pr/pr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {getCookieByName, tidehistory} from '../common/common';

declare const tideData: TideData;
declare const allBuilds: Job[];
declare const csrfToken: string;

type UnifiedState = JobState | "expected" | "error" | "failure" | "pending" | "success";

Expand Down Expand Up @@ -115,6 +116,7 @@ function createXMLHTTPRequest(fulfillFn: (request: XMLHttpRequest) => any, error
request.withCredentials = true;
request.open("POST", url, true);
request.setRequestHeader("Content-type", "application/x-www-form-urlencoded");
request.setRequestHeader("X-CSRF-Token", csrfToken);

return request;
}
Expand Down
7 changes: 6 additions & 1 deletion prow/cmd/deck/static/prow/prow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {JobHistogram, JobSample} from './histogram';
declare const allBuilds: Job[];
declare const spyglass: boolean;
declare const rerunCreatesJob: boolean;
declare const csrfToken: string;
mirandachrist marked this conversation as resolved.
Show resolved Hide resolved

function shortenBuildRefs(buildRef: string): string {
return buildRef && buildRef.replace(/:[0-9a-f]*/g, '');
Expand Down Expand Up @@ -654,7 +655,6 @@ function redraw(fz: FuzzySearch): void {
modal.style.display = "block";
rerunCommand.innerHTML = "Nice try! The direct rerun feature hasn't been implemented yet, so that button does nothing.";
}

}

function createRerunCell(modal: HTMLElement, rerunElement: HTMLElement, prowjob: string): HTMLTableDataCellElement {
Expand All @@ -680,6 +680,11 @@ function createRerunCell(modal: HTMLElement, rerunElement: HTMLElement, prowjob:
const form = document.createElement('form');
form.method = 'POST';
form.action = `${url}`;
const tokenInput = document.createElement('input');
tokenInput.type = 'hidden';
tokenInput.name = 'gorilla.csrf.Token';
tokenInput.value = csrfToken;
form.append(tokenInput);
c.appendChild(form);
form.submit();
};
Expand Down
13 changes: 9 additions & 4 deletions prow/cmd/deck/static/spyglass/spyglass.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {isTransitMessage, serialiseHashes} from "./common";
declare const src: string;
declare const lensArtifacts: {[index: string]: string[]};
declare const lensIndexes: number[];
declare const csrfToken: string;

// Loads views for this job
function loadLenses(): void {
Expand Down Expand Up @@ -53,7 +54,7 @@ function updateHash(index: number, hash: string): void {

function parseHash(): {[index: string]: string} {
const parts = location.hash.substr(1).split(';');
const result: {[index: string]: string} = {};
const result: { [index: string]: string } = {};
for (const part of parts) {
if (part === '') {
continue;
Expand All @@ -64,6 +65,10 @@ function parseHash(): {[index: string]: string} {
return result;
}

function getLensRequestOptions(reqBody: string): RequestInit {
return {body: reqBody, method: 'POST', headers: {'X-CSRF-Token': csrfToken}, credentials: 'same-origin'};
}

window.addEventListener('message', async (e) => {
const data = e.data;
if (isTransitMessage(data)) {
Expand All @@ -88,13 +93,13 @@ window.addEventListener('message', async (e) => {
break;
case "request": {
const req = await fetch(urlForLensRequest(lens, index, 'callback'),
{body: message.data, method: 'POST'});
getLensRequestOptions(message.data));
respond(await req.text());
break;
}
case "requestPage": {
const req = await fetch(urlForLensRequest(lens, index, 'rerender'),
{body: message.data, method: 'POST'});
getLensRequestOptions(message.data));
respond(await req.text());
break;
}
Expand All @@ -103,7 +108,7 @@ window.addEventListener('message', async (e) => {
frame.style.visibility = 'visible';
spinner.style.display = 'block';
const req = await fetch(urlForLensRequest(lens, index, 'rerender'),
{body: message.data, method: 'POST'});
getLensRequestOptions(message.data));
respond(await req.text());
break;
}
Expand Down
3 changes: 3 additions & 0 deletions prow/cmd/deck/template/base.html
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
<html lang="en">
<head>
<meta charset="UTF-8">
<script type="text/javascript">
var csrfToken = {{csrfToken}};
</script>
{{if googleAnalytics}}
<!-- Global site tag (gtag.js) - Google Analytics -->
<script async src="https://www.googletagmanager.com/gtag/js?id={{googleAnalytics}}"></script>
Expand Down
7 changes: 5 additions & 2 deletions prow/cmd/deck/templates.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package main

import (
"github.com/gorilla/csrf"
"github.com/sirupsen/logrus"
"html/template"
"k8s.io/test-infra/prow/cmd/deck/version"
Expand Down Expand Up @@ -60,7 +61,7 @@ func getConcreteSectionFunction(o options) func() baseTemplateSections {
}
}

func prepareBaseTemplate(o options, cfg config.Getter, t *template.Template) (*template.Template, error) {
func prepareBaseTemplate(o options, cfg config.Getter, csrfToken string, t *template.Template) (*template.Template, error) {
return t.Funcs(map[string]interface{}{
"settings": makeBaseTemplateSettings,
"branding": getConcreteBrandingFunction(cfg),
Expand All @@ -71,13 +72,14 @@ func prepareBaseTemplate(o options, cfg config.Getter, t *template.Template) (*t
"lightMode": func() bool { return false },
"deckVersion": func() string { return version.Version },
"googleAnalytics": func() string { return cfg().Deck.GoogleAnalytics },
"csrfToken": func() string { return csrfToken },
}).ParseFiles(path.Join(o.templateFilesLocation, "base.html"))
}

func handleSimpleTemplate(o options, cfg config.Getter, templateName string, param interface{}) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
t := template.New(templateName) // the name matters, and must match the filename.
if _, err := prepareBaseTemplate(o, cfg, t); err != nil {
if _, err := prepareBaseTemplate(o, cfg, csrf.Token(r), t); err != nil {
logrus.WithError(err).Error("error preparing base template")
http.Error(w, "error preparing base template", http.StatusInternalServerError)
return
Expand All @@ -88,6 +90,7 @@ func handleSimpleTemplate(o options, cfg config.Getter, templateName string, par
http.Error(w, "error parsing template", http.StatusInternalServerError)
return
}

if err := t.Execute(w, param); err != nil {
logrus.WithError(err).Error("error executing template " + templateName)
http.Error(w, "error executing template", http.StatusInternalServerError)
Expand Down
10 changes: 5 additions & 5 deletions prow/githuboauth/githuboauth.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,13 +121,13 @@ func NewAgent(config *config.GitHubOAuthConfig, logger *logrus.Entry) *Agent {

// HandleLogin handles GitHub login request from front-end. It starts a new git oauth session and
// redirect user to GitHub OAuth end-point for authentication.
func (ga *Agent) HandleLogin(client OAuthClient) http.HandlerFunc {
func (ga *Agent) HandleLogin(client OAuthClient, secure bool) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
destPage := r.URL.Query().Get("dest")
stateToken := xsrftoken.Generate(ga.gc.ClientSecret, "", "")
state := hex.EncodeToString([]byte(stateToken))
oauthSession, err := ga.gc.CookieStore.New(r, oauthSessionCookie)
oauthSession.Options.Secure = true
oauthSession.Options.Secure = secure
oauthSession.Options.HttpOnly = true
if err != nil {
ga.serverError(w, "Creating new OAuth session", err)
Expand Down Expand Up @@ -200,7 +200,7 @@ func (ga *Agent) HandleLogout(client OAuthClient) http.HandlerFunc {
// HandleRedirect handles the redirection from GitHub. It exchanges the code from redirect URL for
// user access token. The access token is then saved to the cookie and the page is redirected to
// the final destination in the config, which should be the front-end.
func (ga *Agent) HandleRedirect(client OAuthClient, getter GitHubClientGetter) http.HandlerFunc {
func (ga *Agent) HandleRedirect(client OAuthClient, getter GitHubClientGetter, secure bool) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
finalRedirectURL, err := r.URL.Parse(r.URL.Query().Get("dest"))
//This check prevents someone from specifying a different host to redirect to.
Expand Down Expand Up @@ -260,7 +260,7 @@ func (ga *Agent) HandleRedirect(client OAuthClient, getter GitHubClientGetter) h

// New session that stores the token.
session, err := ga.gc.CookieStore.New(r, tokenSession)
session.Options.Secure = true
session.Options.Secure = secure
session.Options.HttpOnly = true
if err != nil {
ga.serverError(w, "Create new session", err)
Expand All @@ -283,7 +283,7 @@ func (ga *Agent) HandleRedirect(client OAuthClient, getter GitHubClientGetter) h
Value: *user.Login,
Path: "/",
Expires: time.Now().Add(time.Hour * 24 * 30),
Secure: true,
Secure: secure,
})
http.Redirect(w, r, finalRedirectURL.String(), http.StatusFound)
}
Expand Down
Loading