Fix infinite redirect loop when multiple cookies are sent

[jportner]

Summary

The existing session cookie implementation was brittle in terms of handling changes to configuration. Specifically, if a Kibana installation change its base path, or enabled secure session cookies, any existing cookies would still be sent by the browser. However, these outdated cookies would not ever be deleted because of the default behavior of the Hapi cookie authentication plugin.

I made the following changes:

• Add path and secure attributes to encrypted cookie session value contents
• Modify session cookie validation check to detect and invalidate cookies with outdated attributes that do not match the current server configuration

Note: cookies that do not contain these attributes are assumed to have been created with the current configuration. I.e., this will only resolve configuration mismatch issues for cookies that were created with this change in place.

Resolves #36825.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

• This was checked for cross-browser compatibility, including a check against IE11
• [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• [ ] Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios
• [ ] This was checked for keyboard-only and screenreader accessibility

For maintainers

• This was checked for breaking API changes and was labeled appropriately
• [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

[jportner]

Needed to add code to clear invalid cookies by specifying the path and isSecure attributes (using the Hapi toolkit).

Note, because of this, we no longer need Hapi to automatically clear invalid cookies (see below).

[jportner]

As mentioned above, we no longer need Hapi to automatically clear invalid cookies.

[jportner]

Author's notes for other reviewers.

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: 8eb4b9ae9975e4f8c2ebf6ed13403e1e0d6b4f20

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 70fcfe0a484cb6f4812a01229f7ba49d89219600

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: f74e598a42d8db8c52518ec8df30a478ca0f7411

[kobelb]

This fixes the issue when going from being hosted at a non-basepath to being hosted at a basepath. However, it doesn't address the issue when going from https to http. Others have run into the https to http issue, and https://www.petefreitag.com/item/857.cfm describes the same situation.

Let's address the https to http issue in a separate PR. I think using a different cookie name for https vs http would be a good solution here, but it's something we'd want to do in a major version upgrade. In light of this information, do you mind removing the "secure cookies" part of this PR, since it's not fixing the issue?

[jportner]

This fixes the issue when going from being hosted at a non-basepath to being hosted at a basepath. However, it doesn't address the issue when going from https to http. Others have run into the https to http issue, and https://www.petefreitag.com/item/857.cfm describes the same situation.

Ah, I tested with https enabled and toggling xpack.security.secureCookies between true and false (which may only happen if Kibana is sitting behind a reverse proxy). The current code does catch that difference and invalidate the cookie. However, we don't need to specify whether the cookie is Secure or not, because that attribute doesn't determine the scope of the cookie -- only Path and Domain determine scope. So the cookie would get overwritten anyway whenever the session is updated.

However, I didn't test like you mentioned above, going from https to http. I did a bit of reading and found this article: https://www.sjoerdlangkemper.nl/2017/10/25/strict-secure-cookies/
Apparently Google has proposed the "Strict Secure Cookies" specification, which is causing the issue you are describing. In this spec, a browser with an insecure connection (e.g., http) cannot overwrite or delete a Secure cookie.

It looks like this change was accepted in RFC 6265 version 01 (April 2017), and this shipped/enabled in Chrome 58 and Firefox 52.

Let's address the https to http issue in a separate PR. I think using a different cookie name for https vs http would be a good solution here, but it's something we'd want to do in a major version upgrade. In light of this information, do you mind removing the "secure cookies" part of this PR, since it's not fixing the issue?

Agree, and we should look at using Cookie Prefixes when that time comes.

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 60d7789

[elasticmachine]

Pinging @elastic/kibana-security (Team:Security)

[pgayvallet]

Is there a way to keep the functionality while calling clearInvalidCookie outside of validateFunc ? Adding behaviour in a validation method looks like introducing side effects to me, so I would prefer to avoid it if we can.

[jportner]

I don't love that it's there, but I couldn't think of a better place to put it.

I originally had it in the X-Pack authentication module in the Security plugin:

kibana/x-pack/plugins/security/server/authentication/index.ts

Lines 68 to 99 in 60d7789

	const isValid = (sessionValue: ProviderSession) => {
	// ensure that this cookie was created with the current Kibana configuration
	const { path, expires } = sessionValue;
	if (path !== undefined && path !== (http.basePath.serverBasePath || '/')) {
	authLogger.debug(`Outdated session value with path "${sessionValue.path}"`);
	return false;
	}
	// ensure that this cookie is not expired
	return !(expires && expires < Date.now());
	};
	const authenticator = new Authenticator({
	clusterClient,
	basePath: http.basePath,
	config: { sessionTimeout: config.sessionTimeout, authc: config.authc },
	isSystemAPIRequest: (request: KibanaRequest) => getLegacyAPI().isSystemAPIRequest(request),
	loggers,
	sessionStorageFactory: await http.createCookieSessionStorageFactory({
	encryptionKey: config.encryptionKey,
	isSecure: config.secureCookies,
	name: config.cookieName,
	validate: (session: ProviderSession) => {
	const array: ProviderSession[] = Array.isArray(session) ? session : [session];
	for (const sess of array) {
	if (!isValid(sess)) {
	return { isValid: false, path: sess.path };
	}
	}
	return { isValid: true };
	},
	}),
	});

However, I wanted to keep usage of the Hapi toolkit abstracted away from X-Pack, so I decided to move that code into Core in cookie_session_storage.ts.

If you have a better idea of where to put it, I'm all ears!

FWIW, Hapi's default behavior to clear the cookie (which I've overridden in this commit) already originates in its validation method. https://github.com/hapijs/cookie/blob/3a6e046e7adafafc382054000239c338a1a55d8c/lib/index.js#L197-L199

[pgayvallet]

I guess it's we are doing nothing that hapi was already doing, this should be alright. Unless someone has a better idea on where we could move this (don't have much experience on hapi lifecycle), maybe @joshdover or @restrry?

[mshustov]

What if the platform doesn't handle the removal, but the plugin controls it? Security plugin already has access to CookieStorage and can perform all setup/teardown logic.

[jportner]

I thought about that, but we are relying on the Hapi toolkit for the actual cookie removal -- right now we don't expose Hapi directly to plugins in the New Platform, we abstract it away. My impression was that we wanted to keep it that way.

[mshustov]

Correct, but we can extend API and provide methods to remove the cookie. This method will be a wrapper around hapi

[mshustov]

Ok, nvm. The core defines the auth strategy and sets the cookie in hapi. Probably, it's better to keep your current implementation with explicit removal in the core. Ok for me

[jportner]

Okay, thanks!

@pgayvallet any other concerns or nits?

[mshustov]

so, validate function returns on the very first invalid cookie and removes only it?

[jportner]

Yes, that's correct.

It's because we're using the Hapi toolkit to clear the cookie (which is the same method that the Hapi Cookie plugin uses under the hood in our current implementation -- we are just passing it an extra parameter). However, the Hapi toolkit can only clear one cookie at a time. This sounded like an acceptable approach to me because:

• The only time we'll ever see multiple cookies is if the client has stored several more than one cookie with the same name and different, overlapping paths (e.g., / and /foo)
• If multiple cookies are sent and they are all invalid (very unlikely), the client will make multiple round trips and delete each one; this will happen near-instantaneously and be imperceptible to the end user

The alternative was to try to reimplement that logic in a different way so we could delete multiple cookies in a single response, but that seemed unnecessary and risky -- we know the existing method works, and we want to avoid mucking around with Hapi under the hood as much as possible.

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: c7f9e12

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 16ad27c

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 85e51fd

[pgayvallet]

platform changes LGTM