-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Preserve URL fragment during SAML handshake. #44513
Conversation
Pinging @elastic/kibana-security |
845b508
to
71f2980
Compare
@@ -100,7 +100,7 @@ xpack.security.authc.saml.realm: realm-name | |||
+ | |||
[source,yaml] | |||
-------------------------------------------------------------------------------- | |||
server.xsrf.whitelist: [/api/security/v1/saml] | |||
server.xsrf.whitelist: [/security/api/authc/saml/callback] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: not completely sure if we need authc
part. It may seem redundant, but at the same the security plugin API surface will expand significantly over time with various authz endpoints and such so it may be beneficial to separate them....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once #44855 is merged I'll change URLs from /security/api/*
to /api/security/*
. The question regarding authc
part in URL is still open though.
const xpackMainPlugin = server.plugins.xpack_main; | ||
const xpackInfo = xpackMainPlugin.info; | ||
securityPlugin.registerLegacyAPI({ | ||
xpackInfo, | ||
isSystemAPIRequest: server.plugins.kibana.systemApi.isSystemApiRequest.bind( | ||
server.plugins.kibana.systemApi | ||
), | ||
cspRules: createCSPRuleString(config.get('csp.rules')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: if we still need this we'll include that into NP blockers for Security plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Included as blocker, for now we agreed with Platform to provide this through legacy API shim and hopefully get support for "lightweight" apps by 8.0 so that we don't need to set CSP manually anymore.
@@ -12,6 +12,27 @@ import { KibanaRequest } from '../../../../../../../../src/core/server'; | |||
import { createCSPRuleString } from '../../../../../../../../src/legacy/server/csp'; | |||
|
|||
export function initAuthenticateApi({ authc: { login, logout }, config }, server) { | |||
const xsrfWhitelist = server.config().get('server.xsrf.whitelist'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: that the only BWC route deprecation approach I managed to find that wouldn't require NP plugin to expose SAML APIs directly (so that we can call them here). I don't like it, I'd rather be able to temporarily define routes with any URL for BWC reasons or have something like "URL alias" that we could define in NP for the new route. Will discuss with the Platform Team (discussing here #44620)
// HACK: we manually add a `RelayState` query string parameter with URL to redirect user to after | ||
// successful SAML handshake since Elasticsearch doesn't support it yet. We may break something | ||
// here eventually if we keep this workaround for too long. | ||
const redirect = `${redirectWithoutRelayState}&RelayState=${encodeURIComponent(nextURL)}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: there was the idea to somehow encrypt RelayState
so that it can't be altered, but I doubt it's a big a problem if we don't. Also keeping RelayState
as a plain path#fragment would allow administrators to specify any URL for IdP initiated login, e.g. like described in Auth0
docs.
stateRedirectURL || `${this.options.basePath.get(request)}/`, | ||
{ state: { accessToken, refreshToken } } | ||
); | ||
return AuthenticationResult.redirectTo(nextURL || `${this.options.basePath.get(request)}/`, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: we should probably validate if it's a relative path at least. What do you think?
@kobelb PR is ready for the preliminary feedback whenever you have time, thanks! |
window.location.replace( | ||
'${basePath.get( | ||
request | ||
)}/security/api/authc/saml/start?currentURL=' + encodeURIComponent('${currentPath}' + window.location.hash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currentPath
looks like it can be any user-controlled string, which would make this an XSS vector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(e.g. provide a currentPath
along the lines of ')); alert('hi!')//
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currentPath looks like it can be any user-controlled string, which would make this an XSS vector?
Yep, it's exactly what you see here, glaring XSS vulnerable code (with a bit tweaked payload) 🙂 We're just not sure where to place this currentPath
yet, initially we stored it in the intermediate encrypted cookie so that we don't need to do a "is it a relative path" check and care about proper escaping. If we still decide to keep it here I'll take care of properly handling this including the "is it a relative path" check.
Thank you for being on guard!
try { | ||
const authenticationResult = await authc.login(request, { | ||
provider: 'saml', | ||
value: { step: SAMLLoginStep.UserURLCaptured, currentURL: request.query.currentURL }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would catch currentURL
not being relative, but e.g. starting with https://attacker.com/...
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentioned it in #44513 (comment), but will have a check before we forward user to Kibaba after a successful authentication assuming we keep the current approach.
…cookie to store `redirectURLPath`.
@@ -100,7 +100,7 @@ xpack.security.authc.saml.realm: realm-name | |||
+ | |||
[source,yaml] | |||
-------------------------------------------------------------------------------- | |||
server.xsrf.whitelist: [/api/security/v1/saml] | |||
server.xsrf.whitelist: [/api/security/authc/saml/callback] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: previous comment was vanished with new commit. But essential I wasn't sure whether we need authc
part here. The more I think about it the more it seems unnecessary....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer we leave the authc
part out. I don't know if it gets us much, and not including it more closely mirrors the ES security routes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good 👍
` | ||
<!DOCTYPE html> | ||
<title>Kibana SAML Login</title> | ||
<link rel="icon" href="data:,"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: alternatively we can makefavicon.ico
or/and anything that ends with .ext
as special case in canRedirectRequest
....
This becomes a problem only if we store redirectPath
in the cookie when we render this custom HTML favicon.ico
request may cause us to create a new cookie :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For static resources like the favicon.ico
, we don't require auth. It looks like we're using the equivalent of http://localhost:5601/ui/favicons/favicon.ico
normally, should we be doing this here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For static resources like the favicon.ico, we don't require auth.
We don't, but we also run auth code before we figure out that the request is targeting non-existent resource (as it's done by browser automatically if we don't define favicon, somehost/favicon.ico
) and for security this request looks like any other non-resource request. But now I started wondering if it's expected that we call auth
hook for the 404 route, checking with Platform team....
It looks like we're using the equivalent of http://localhost:5601/ui/favicons/favicon.ico normally, should we be doing this here instead?
I considered this option, but this ui/favicons
path is maintained in another place and if someday they change the path this code will silently be broken (unless we try to simulate SAML IdP and write a functional test for that). Ideally this should be solved with #41964 and until then we can either use current workaround or switch to ui/favicons/favicon.ico
and hope that it won't change before #41964. Both options sound good to me, what would you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked to @restrry and there is a chance we'll treat it (i.e. that we call auth
hook for non-defined routes) as a bug, will link issue later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but this ui/favicons path is maintained in another place and if someday they change the path this code will silently be broken (unless we try to simulate SAML IdP and write a functional test for that)
That makes sense.
Talked to @restrry and there is a chance we'll treat it (i.e. that we call auth hook for non-defined routes) as a bug, will link issue later.
I don't mind us merging with the current work-around also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind us merging with the current work-around also.
We still have time, but good to know! :)
value: { | ||
step: SAMLLoginStep.SAMLResponseReceived, | ||
samlResponse: request.body.SAMLResponse, | ||
redirectURL: request.body.RelayState, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: we may validate that URL is relative here assuming we keep transmitting it as it's
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good! Outside of what was discussed over Slack this morning about no longer relying upon RelayState to store the full because of limitations on the max-size, I just have small nit-picks.
I don't see any issues with existing SAML logins when rolling out this new approach. Would you mind confirming this?
@@ -100,7 +100,7 @@ xpack.security.authc.saml.realm: realm-name | |||
+ | |||
[source,yaml] | |||
-------------------------------------------------------------------------------- | |||
server.xsrf.whitelist: [/api/security/v1/saml] | |||
server.xsrf.whitelist: [/api/security/authc/saml/callback] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer we leave the authc
part out. I don't know if it gets us much, and not including it more closely mirrors the ES security routes.
` | ||
<!DOCTYPE html> | ||
<title>Kibana SAML Login</title> | ||
<link rel="icon" href="data:,"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For static resources like the favicon.ico
, we don't require auth. It looks like we're using the equivalent of http://localhost:5601/ui/favicons/favicon.ico
normally, should we be doing this here instead?
<!DOCTYPE html> | ||
<title>Kibana SAML Login</title> | ||
<link rel="icon" href="data:,"> | ||
<script src="${currentBasePath}/api/security/authc/saml/capture-url-fragment.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This scares me from the XSS perspective. Currently, the spaces OnPreAuth handler isn't setting the basePath unless it matches this regex
const matchResult = pathToCheck.match(/^\/s\/([a-z0-9_\-]+)/); |
http://localhost:5601/s/${encodeURIComponent('"/><script>alert("xss");')}
Perhaps we should use the following:
ReactDOMServer.renderToString(React.createElement('script', { src: `${currentBasePath}/api/security/authc/saml/capture-url-fragment.js` }))
or some other method which does context specific escaping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, hmmmmm, I'm concerned that we say that we can't trust core base path service to provide us with non-malformed base path.... Let's discuss offline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, related discussion made me think that we don't need to bother with something that user can influence here (space ID) and rely on plain server base path instead.
this.logger.debug('Captured redirect URL.'); | ||
return this.authenticateViaHandshake( | ||
request, | ||
`${state.redirectURLPath}${attempt.redirectURLFragment}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're dropping the "basePath" here, so the space information is lost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, good catch! I should store URL with basepath (and space-id) into redirectURLPath
at the previous step and be more spaces aware in general :)
I think that's a reasonable mitigation.
I generally agree, and it's something we were willing to treat as an edge-case for the CSP changes to use |
Co-Authored-By: gchaps <33642766+gchaps@users.noreply.github.com>
💚 Build Succeeded |
{ body: { realm: 'test-realm' } } | ||
); | ||
|
||
expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: I know and agree in advance that testing things like this (logger.warn
) feels redundant, but it doesn't hurt and I can't think of any occasion when I suffered from anything like this during whole auth provider code life.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensuring a warn is called seems reasonable here to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice work!
{ body: { realm: 'test-realm' } } | ||
); | ||
|
||
expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensuring a warn is called seems reasonable here to me!
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Text LGTM
💚 Build Succeeded |
7.x/7.5.0: da745fa |
Preserve URL fragment during SAML handshake (based on the plan outlined in #18392 (comment), client-side page to extract fragment + RelayState).
Current SAML handshake flow
https://kibana/app/kibana#/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())...
/app/kibana
in the cookie and redirects user to IdP#/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())...
part of the initial URL)/app/kibana
from the cookie and redirects user to this URLProposed SAML handshake flow
https://kibana/app/kibana#/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())...
/app/kibana
, puts it into intermediate cookie and redirects user to/api/security/saml/capture-url-fragment
to capture URL fragment part/capture-url-fragment
page Kibana extracts#/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())...
part and redirects user to/api/security/saml/start
appending combined and encoded#/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())...
asredirectURLFragment
query string parameter/app/kibana
with#/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())...
, puts it into cookie together with SAML request ID and redirects user to IdP/api/security/saml/callback
/app/kibana/#/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())...
from the cookie and redirects user to this URLNote 1: if the path extracted at the step 2 is larger than the value specified in
xpack.security.authc.saml.maxRedirectURLSize
(2kb by default) then URL path isn't put into the cookie and user redirected directly to IdP. After successful authentication user will be redirected to the Kibana root/home page.Note 2: if URL fragment combined with the URL path at the step 4 is larger than the value specified in
xpack.security.authc.saml.maxRedirectURLSize
(2kb by default) then URL fragment is dropped and only URL path is stored in the cookie. After successful authentication user will be redirected to the Kibana path used to initiate SAML handshake (URL fragment isn't recovered).Proposed SAML handshake flow with `RelayState` (ABANDONED)
https://kibana/app/kibana#/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())...
/app/kibana
and redirects user tohttps://kibana/api/security/authc/saml/capture-url-fragment
EITHER appending encoded/app/kibana
asredirectURLPath
query string parameter OR puttingredirectURLPath
into the intermediate cookie/capture-url-fragment
page Kibana extracts#/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())...
part, EITHER combines it with theredirectURLPath
query string parameter OR not (if we chose to put it into cookie) and redirects user to/api/security/authc/saml/start
appending combined and encoded/app/kibana/#/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())...
asredirectURLFragment
query string parameter/app/kibana/#/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())...
to IdP redirect URL asRelayState
query string parameter and redirects user to IdPRelayState
body parameter Kibana sent to IdP at the previous step/app/kibana/#/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())...
from theRelayState
body parameter, validates that URL is relative to Kibana root and redirects user to this URLNote: with the proposed approach IdP initiated login can also be accompanied by a custom
RelayState
argument that will redirect user to any Kibana URL after login.Notes to myself:
Decide if benefits of having URL in RelayState in clear text over-weight possible risks, see 6.46 of(we'll tackle this separately, for now we'll store URL in encrypted cookie)Security and Privacy Considerations forthe OASIS SAML V2.0
. Should we encrypt it? Or just store a hash of it in the cookie together withrequestId
? That would disallow "targeted" IdP initiated logins.Blocked by: elastic/elasticsearch#46232, #44855Fixes: #18392
"Release Note: Kibana now fully preserves the URL user used to login with SAML."