Add CSRF protection to deck #13323

mirandachrist · 2019-07-04T00:42:25Z

This adds CSRF protection to deck (using the gorilla csrf library) if --rerun-creates-job is set to true and a file path for the CSRF token is specified. We will eventually require a CSRF file path to be specified if --rerun-creates-job is set to true, but for now we log a warning that this is insecure.
In preparation for removing the --rerun-creates-job flag, we will later be making an announcement that specifying the CSRF file path will be mandatory soon.

/assign @cjwagner
/cc @fejta
/cc @Katharine

Katharine

It works now!

I think this substantially improves our security here. My primary (non-security) concern is that the CSRF handling is done separately for every page. It would be preferable if it instead of everyone having to do this manually, it were handled in the base template. That would make it infrastructure that can mostly be ignored by everything else, and also gets rid of the need to have the defaulting behaviour for the params in handleSimpleTemplate.

Other than that, I'd like to see the ability to run with CSRF enabled in debug mode without needing to edit code, and I'm not a fan of us introducing new flags to do the same thing as the old ones - see the specific comments for details.

prow/cmd/deck/main.go

Katharine · 2019-07-08T20:36:32Z

prow/cmd/deck/main.go

@@ -139,6 +141,8 @@ 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")
+ // csrfTokenFile, if specified, must point to a file containing a 32 byte token that will be used to protect against CSRF in POST requests
+ fs.StringVar(&o.csrfTokenFile, "csrf-token", "", "Path to the file containing the CSRF token.")


Ideally we could reuse --cookie-secret for this - it serves the same purpose. Unfortunately, we didn't require 32-byte token (and it looks like on our deployment it's currently 64 bytes). We might be able to just truncate and complain if it's less than 32 bytes, but that presents a backwards compatibility concern. As far as I can tell, we never previously expressed guidance on how long --cookie-secret should be (or documented it at all?), which is itself bad.

We're eventually going to need to announce that we'll start requiring a CSRF token anyway. We could instead announce that --cookie-secret must be at least 32 bytes (or some more specific length) and truncate it. What do you think?

I think we should use --cookie-secret and do the following:

if the value is more than 32 bytes, emit a warning that the value should be exactly 32 bytes and suggest truncating the existing value, then continue

if the value is less than 32 bytes and --rerun-creates-job is not specified but --oauth-url is, emit a warning that it should be exactly 32 bytes and continue

if the value is less than 32 bytes and --rerun-creates-job is specified, emit an error and refuse to start

if neither --rerun-creates-job nor --oauth-url are specified, but --cookie-secret is, and --cookie-secret is not a 32-byte value, emit a warning and continue

if none of --rerun-creates-job, --oauth-url, nor --cookie-secret are specified, do nothing and proceed silently

Note that for some reason, --cookie-secret is base64-encoded (and then k8s internally base64-encodes it again...), so all of the above is after decoding. Unfortunately, changing this would not be backwards compatible, so let's not.

Are we sure that the entropy here is evenly distributed? If not then hashing the token might be preferable to truncating it. It would be unfortunate if the first 32 bytes tend to be the same, for example

No way of knowing since AFAICT we never documented how to generate this (in our case, probably yes, but hard to tell for anyone else). Hashing is probably a good idea.

Is there a specific library I should use for this?

I'd suggest the built-in crypto/sha256, which will give you the 32 bytes you want.

prow/cmd/deck/static/spyglass/spyglass.ts

prow/cmd/deck/static/prow/prow.ts

prow/cmd/deck/templates.go

Katharine

This looks pretty good to me! Mostly just nits at this point.

prow/cmd/deck/template/base.html

prow/cmd/deck/templates.go

prow/cmd/deck/main.go

fejta

Since none of us are experts (or maybe its just me) at CSRF a short explanation (and maybe including links to more authoritative ones) explaining how this is support to work in the PR description and/or a README in prow/githuboauth and/or deck would be appreciated.

go.mod

prow/cmd/deck/main.go

Katharine · 2019-07-12T21:05:06Z

prow/cmd/deck/main.go

+ }
+
+ // if we allow direct reruns, we must protect against CSRF in all post requests using the cookie secret as a token
+ if o.rerunCreatesJob {


This should always be enabled if it can be (i.e. if we have a sufficiently long secret). If we can't enable it, warn loudly and continue.

What is the use of the --rerun-creates-job flag now? It seems like we won't need it after this change

It still controls whether you can rerun jobs via the UI or have to copy a provided kubectl command.

Yes and we would very much like that flag to stay, because wer will eventually hopefully have two deck instances using the same backing Prow, a public and a private one. On the private one everyone should be allowed to use the rerun feature, on the public one obviously not. They will however use the same config.yaml

prow/cmd/deck/main.go

prow/cmd/deck/csrf.md

Adds documentation for why we need CSRF protection and how the protection works.

fejta · 2019-07-16T21:41:38Z

prow/cmd/deck/main.go

+ 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[:]


Why the [:] here?

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

k8s-ci-robot · 2019-07-16T21:42:29Z

LGTM label has been added.

Git tree hash: 4685b6ea5a342a3784c1841b15163c6c182c0a25

k8s-ci-robot · 2019-07-16T21:47:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fejta, Katharine, mirandachrist

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Katharine,fejta]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot assigned cjwagner Jul 4, 2019

k8s-ci-robot requested review from fejta and Katharine July 4, 2019 00:42

mirandachrist force-pushed the csrf branch 3 times, most recently from 76bb84e to 8415661 Compare July 8, 2019 20:31

Katharine suggested changes Jul 8, 2019

View reviewed changes

k8s-ci-robot assigned Katharine Jul 8, 2019

mirandachrist force-pushed the csrf branch 3 times, most recently from bd6e4fb to 701d31d Compare July 9, 2019 22:38

Katharine reviewed Jul 10, 2019

View reviewed changes

prow/cmd/deck/template/base.html Outdated Show resolved Hide resolved

prow/cmd/deck/templates.go Outdated Show resolved Hide resolved

prow/cmd/deck/main.go Show resolved Hide resolved

prow/cmd/deck/main.go Outdated Show resolved Hide resolved

fejta reviewed Jul 11, 2019

View reviewed changes

go.mod Show resolved Hide resolved

prow/cmd/deck/main.go Outdated Show resolved Hide resolved

prow/cmd/deck/main.go Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2019

mirandachrist mentioned this pull request Jul 12, 2019

Add documentation for why we need CSRF protection and how the protection works. #13430

Closed

Katharine reviewed Jul 12, 2019

View reviewed changes

mirandachrist force-pushed the csrf branch 2 times, most recently from d8a2175 to 7569599 Compare July 15, 2019 18:21

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2019

mirandachrist force-pushed the csrf branch from 7569599 to 90de3ce Compare July 15, 2019 18:52

mirandachrist commented Jul 15, 2019

View reviewed changes

prow/cmd/deck/main.go Show resolved Hide resolved

mirandachrist force-pushed the csrf branch 2 times, most recently from d2b91e7 to 4ef1605 Compare July 15, 2019 22:12

Katharine reviewed Jul 16, 2019

View reviewed changes

prow/cmd/deck/main.go Outdated Show resolved Hide resolved

prow/cmd/deck/main.go Outdated Show resolved Hide resolved

Katharine reviewed Jul 16, 2019

View reviewed changes

prow/cmd/deck/csrf.md Outdated Show resolved Hide resolved

mirandachrist force-pushed the csrf branch from 4ef1605 to 5db7297 Compare July 16, 2019 00:53

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2019

mirandachrist added 4 commits July 16, 2019 10:31

Add gorilla csrf as a dependency

c5bf10e

Add CSRF protection to deck

02cc078

Hash longer cookie secret

8326ae4

Create csrf.md

f5a3495

Adds documentation for why we need CSRF protection and how the protection works.

mirandachrist force-pushed the csrf branch from 5db7297 to f5a3495 Compare July 16, 2019 17:31

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2019

fejta approved these changes Jul 16, 2019

View reviewed changes

k8s-ci-robot assigned fejta Jul 16, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 16, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2019

Katharine approved these changes Jul 16, 2019

View reviewed changes

k8s-ci-robot merged commit bffe829 into kubernetes:master Jul 16, 2019

mirandachrist mentioned this pull request Jul 16, 2019

Allow direct triggering of reruns #13321

Closed

Katharine mentioned this pull request Jul 19, 2019

Partially remove the default for --cookie-secret-file. #13521

Merged

mirandachrist mentioned this pull request Jul 29, 2019

Support insecure 🍪s for testing Deck's GitHub oauth without SSL. #12989

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CSRF protection to deck #13323

Add CSRF protection to deck #13323

mirandachrist commented Jul 4, 2019

Katharine left a comment •

edited

Loading

Katharine Jul 8, 2019

mirandachrist Jul 9, 2019

Katharine Jul 9, 2019

mirandachrist Jul 9, 2019

fejta Jul 11, 2019 •

edited

Loading

Katharine Jul 11, 2019

mirandachrist Jul 15, 2019

Katharine Jul 15, 2019

Katharine left a comment

fejta left a comment

Katharine Jul 12, 2019

mirandachrist Jul 15, 2019

Katharine Jul 15, 2019

alvaroaleman Jul 16, 2019

fejta Jul 16, 2019

mirandachrist Jul 16, 2019

k8s-ci-robot commented Jul 16, 2019

k8s-ci-robot commented Jul 16, 2019

Add CSRF protection to deck #13323

Add CSRF protection to deck #13323

Conversation

mirandachrist commented Jul 4, 2019

Katharine left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fejta Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Katharine left a comment

Choose a reason for hiding this comment

fejta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 16, 2019

k8s-ci-robot commented Jul 16, 2019

Katharine left a comment •

edited

Loading

fejta Jul 11, 2019 •

edited

Loading