Skip to content

[4.6] Separate xsrf handling and version checking#8152

Merged
epixa merged 1 commit intoelastic:4.6from
epixa:4.6-headercheck
Sep 2, 2016
Merged

[4.6] Separate xsrf handling and version checking#8152
epixa merged 1 commit intoelastic:4.6from
epixa:4.6-headercheck

Conversation

@epixa
Copy link
Contributor

@epixa epixa commented Sep 2, 2016

Backport of #8151

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 epixa added backport This PR is a backport of another PR v4.6.1 labels Sep 2, 2016
@epixa epixa added the review label Sep 2, 2016
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should mark this with a backwards compatibility comment of some sort, maybe we should do that in more places.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would prefer that we implement this at the CLI, similarly to how we support legacy config values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the comment above it not suffice?

@ppisljar
Copy link
Contributor

ppisljar commented Sep 2, 2016

LGTM, makes complete sense to check for both version and the new header to maintain backward compatibility and the comment above the disabled definition makes it clear enough in my opinion.

@epixa
Copy link
Contributor Author

epixa commented Sep 2, 2016

@spalger Updated with your feedback from zoom

Copy link
Contributor

Choose a reason for hiding this comment

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

Would probably be easier to follow without all the !s

if (isUnsafeMethod && (missingVersionHeader || missingXsrfHeader)) {
  // ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The logic here is intentionally that both headers must be missing in order to error. So really it's a question of:

!isSafeMethod && !hasVersionHeader && !hasXsrfHeader
// or
isUnsafeMethod && missingVersionHeader && missingXsrfHeader

I'd rather those negations be applied to boolean values rather than wrapping entire multi-condition statements like req.method === 'get' || req.method === 'head'

@spalger
Copy link
Contributor

spalger commented Sep 2, 2016

one typo, and one option for extra credit, but LGTM!

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 2, 2016

Fixed the typo and responded to the rest

@epixa epixa merged commit 3ff0f9b into elastic:4.6 Sep 2, 2016
@epixa epixa deleted the 4.6-headercheck branch September 2, 2016 16:35
@epixa epixa restored the 4.6-headercheck branch September 2, 2016 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport This PR is a backport of another PR review v4.6.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants