Skip to content

Separate xsrf handling and version checking#8151

Merged
epixa merged 1 commit intoelastic:masterfrom
epixa:headercheck
Sep 6, 2016
Merged

Separate xsrf handling and version checking#8151
epixa merged 1 commit intoelastic:masterfrom
epixa:headercheck

Conversation

@epixa
Copy link
Contributor

@epixa epixa commented Sep 2, 2016

Traditionally we've relied on the kbn-version header for csrf
protection, but that is also used for the client-side Kibana version
check that alerts users when their client is out of date and needs to be
refreshed, so it must match the version of Kibana exactly.

This poses a problem for any programmatic access that would only get set
up once but may run repeatedly throughout the future (e.g. watcher), so
there's now the additional option of specifying the kbn-xsrf header
instead. The value of the header does not matter, but its very presence
is all that is necessary to avoid xsrf attacks.

The xsrf protection just needs either one of these headers to exist.

Forward port of #8152

@ppisljar
Copy link
Contributor

ppisljar commented Sep 2, 2016

jenkins, test this

@ppisljar
Copy link
Contributor

ppisljar commented Sep 2, 2016

seems tests on master are broken. otherwise it LGTM

@epixa epixa removed the review label Sep 2, 2016
@epixa
Copy link
Contributor Author

epixa commented Sep 2, 2016

I need to do some changes to this, but I figured I'd get the 4.6 stuff done first: #8152

@epixa epixa added the blocker label Sep 2, 2016
@epixa
Copy link
Contributor Author

epixa commented Sep 2, 2016

I marked this as a blocker since it went into 4.6 already.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this? I realize previously we had conflated version checking and xsrf, but we always exposed it as an xsrf check. The version check is new, and users could legitimately want to skip xsfr check, but not the version check. This prevents them from choosing one or the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, don't review this PR right now. The 4.6 PR changed a bunch of this and was merged this morning: #8152

I'll forward port those changes to this PR before we review again.

@w33ble
Copy link
Contributor

w33ble commented Sep 6, 2016

@epixa you're still planning to update this PR, right?

@epixa
Copy link
Contributor Author

epixa commented Sep 6, 2016

Yep, once 4.6.1 goes out.

@epixa epixa removed the v4.6.1 label Sep 6, 2016
Traditionally we've relied on the kbn-version header for csrf
protection, but that is also used for the client-side Kibana version
check that alerts users when their client is out of date and needs to be
refreshed, so it must match the version of Kibana exactly.

This poses a problem for any programmatic access that would only get set
up once but may run repeatedly throughout the future (e.g. watcher), so
there's now the additional option of specifying the kbn-xsrf header
instead. The value of the header does not matter, but its very presence
is all that is necessary to avoid xsrf attacks.

The xsrf protection just needs either one of these headers to exist.
@epixa
Copy link
Contributor Author

epixa commented Sep 6, 2016

@w33ble This is good to go

@epixa epixa added the review label Sep 6, 2016
@jbudz jbudz self-assigned this Sep 6, 2016
@jbudz
Copy link
Contributor

jbudz commented Sep 6, 2016

LGTM

@epixa epixa merged commit ec3abde into elastic:master Sep 6, 2016
@epixa epixa deleted the headercheck branch September 6, 2016 21:50
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
Separate xsrf handling and version checking

Former-commit-id: ec3abde
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants