-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Expose session invalidation API. #92376
Expose session invalidation API. #92376
Conversation
{ | ||
path: '/internal/security/session/_all', | ||
validate: { | ||
query: schema.maybe( |
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 was also considering POST
+ body
+ /internal/security/session/_invalidate
, but didn't find enough benefits comparing to DELETE
+ query
. I know some ES APIs use body
with DELETE
, but IIRC it's not widely recommended and some proxies/tools may not be happy about it (e.g. the REST tool I use 🙂 ).
Let me know what approach 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.
I'm learning towards POST + body + /internal/security/session/_invalidate
. This name gives us more flexibility to expand its functionality over time. The _all
suffix we're using above confused me initially -- at first glance, I assumed it would always invalidate all sessions, but we also accept a query parameter to optionally invalidate a subset of sessions.
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 see you've noted in the PR description that this is an internal route for now. Is there a reason not to document this as a public endpoint? We intend for this to be used externally, and marking it as public won't preclude us from making a breaking change in a minor release if we need to. In other words, it could be an experimental/beta public API, much like our SO, Space, and Role APIs.
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.
The _all suffix we're using above confused me initially -- at first glance, I assumed it would always invalidate all sessions, but we also accept a query parameter to optionally invalidate a subset of sessions.
Yeah, that's because I added filter at a later iteration 🙂 Agree it's confusing now.
and marking it as public won't preclude us from making a breaking change in a minor release if we need to. In other words, it could be an experimental/beta public API, much like our SO, Space, and Role APIs.
That's a good point, I completely forgot about the experimental APIs! The "no guarantees yet" was my main motivator behind keeping this API as internal.
private getLoggerForSID(sid: string) { | ||
return this.options.logger.get(sid?.slice(-10)); | ||
private getLoggerForSID(sid?: string) { | ||
return this.options.logger.get(sid?.slice(-10) ?? 'x'.repeat(10)); |
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 just use parent "context" if SID isn't provided.
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 it'd be nice to explicitly denote that this is a session-less request. What do you think about *no_session*
or similar?
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.
Yep, I like no_session
and it's exactly 10 chars, thanks! 🙂
) | ||
), | ||
}, | ||
options: { tags: ['access:sessionManagement'] }, |
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.
question: do we have any precedent\ability to relax superuser-only requirement and, for example, give access to kibana_admin
?
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 have the ability to grant access to any user with the "Global All" privilege (aka kibana_admin
), but I don't know that these users should have the ability to force-logout everyone else.
IMO this feels like something that only a superuser (or someone with manage_security
, which is effectively superuser) should be able to do. Are there benefits to relaxing this requirement that I'm overlooking?
I could see us eventually offering a way to "invalidate all of my sessions", but we aren't at a place to do that just yet, and adding this capability in would increase the scope of this PR quite a bit, I'd imagine.
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.
Are there benefits to relaxing this requirement that I'm overlooking?
Nothing concrete yet, I'm just not very happy that we require a "cluster superpower" to invalidate Kibana-specific sessions and Kibana Admin
sounded like a role that could be assumed to be responsible for all Kibana-specific things. I'm totally fine to keep it as is right now, but if we can eventually have a Kibana Operator role or something, I'd vote for it 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.
Are there benefits to relaxing this requirement that I'm overlooking?
Nothing concrete yet, I'm just not very happy that we require a "cluster superpower" to invalidate Kibana-specific sessions and
Kibana Admin
sounded like a role that could be assumed to be responsible for all Kibana-specific things. I'm totally fine to keep it as is right now, but if we can eventually have a Kibana Operator role or something, I'd vote for it instead.
Yeah what you're saying makes sense. My fear is that kibana_admin
would be granted to too many folks, especially without an "operator" role readily available. More than happy to revisit this in the future!
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.
My fear is that kibana_admin would be granted to too many folks, especially without an "operator" role readily available.
Yeah, that's a good point, let's see how it goes with superuser then.
Pinging @elastic/kibana-security (Team:Security) |
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.
Mostly questions and nits - thanks for putting this together so quickly!
Can we also add some functional API tests to verify this works as expected, and respects the superuser
restrictions?
) | ||
), | ||
}, | ||
options: { tags: ['access:sessionManagement'] }, |
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 have the ability to grant access to any user with the "Global All" privilege (aka kibana_admin
), but I don't know that these users should have the ability to force-logout everyone else.
IMO this feels like something that only a superuser (or someone with manage_security
, which is effectively superuser) should be able to do. Are there benefits to relaxing this requirement that I'm overlooking?
I could see us eventually offering a way to "invalidate all of my sessions", but we aren't at a place to do that just yet, and adding this capability in would increase the scope of this PR quite a bit, I'd imagine.
// we still want to log the SID if session is available. | ||
const sessionCookieValue = await this.options.sessionCookie.get(request); | ||
const sessionLogger = this.getLoggerForSID(sessionCookieValue?.sid); | ||
sessionLogger.debug('Invalidating sessions.'); |
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.
nit: it might be nice to include the filters used as part of this invalidate operation. I think even a simple JSON.stringify
would be good enough
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.
Yep, we can do that. I was a bit concerned about logging a username if it's provided as afaik we never logged it anywhere before. It's probably too paranoid though.
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.
That's a good point. Feel free to omit for now then
private getLoggerForSID(sid: string) { | ||
return this.options.logger.get(sid?.slice(-10)); | ||
private getLoggerForSID(sid?: string) { | ||
return this.options.logger.get(sid?.slice(-10) ?? 'x'.repeat(10)); |
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 it'd be nice to explicitly denote that this is a session-less request. What do you think about *no_session*
or similar?
{ | ||
path: '/internal/security/session/_all', | ||
validate: { | ||
query: schema.maybe( |
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'm learning towards POST + body + /internal/security/session/_invalidate
. This name gives us more flexibility to expand its functionality over time. The _all
suffix we're using above confused me initially -- at first glance, I assumed it would always invalidate all sessions, but we also accept a query parameter to optionally invalidate a subset of sessions.
async (_context, request, response) => { | ||
return response.ok({ | ||
body: { | ||
total: await getSession().clearAll( |
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.
Similar to my comment about _all
above, clearAll
is potentially misleading too, since it's not always clearing all sessions, but rather sessions matching a optional criteria. What do you think about aligning on invalidate
? The one thing I don't like about this is that it's not immediately clear that this will invalidate multiple sessions, but perhaps that could be mitigated by requiring criteria, even if the criteria is empty?
What do you think?
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 will become a bit confusing since we'd have a clear
and invalidate
at the same time. How do you feel about joining clear
and clearAll
to a single clear
or invalidate
method instead?
export type InvalidateSessionFilter =
| { type: 'all' }
| { type: 'by-sid'; sid: string }
| { type: 'by-query'; query: { provider: { type: string; name?: string }; usernameHash?: string } };
Or keep them separate, let me see if I can come up with non-confusing names for both.
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.
How do you feel about joining clear and clearAll to a single clear or invalidate method instead?
Or keep them separate, let me see if I can come up with non-confusing names for both.
I would be fine with either of these approaches!
{ | ||
path: '/internal/security/session/_all', | ||
validate: { | ||
query: schema.maybe( |
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 see you've noted in the PR description that this is an internal route for now. Is there a reason not to document this as a public endpoint? We intend for this to be used externally, and marking it as public won't preclude us from making a breaking change in a minor release if we need to. In other words, it could be an experimental/beta public API, much like our SO, Space, and Role APIs.
@legrego I've handled the feedback, and PR is ready for another review pass. Thanks! |
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.
Thanks for changing the API around, I think this is a lot easier to understand!
Other than my very minor nits, the only other thing I'd like to see is API docs added for this endpoint 🙂
...filter, | ||
query: { | ||
provider: filter.query.provider, | ||
usernameHash: createHash('sha3-256').update(filter.query.username).digest('hex'), |
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.
nit: this is the third place where we're calculating the username hash. This might be a good opportunity to consolidate the logic into a single function.
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 catch! Will move to a separate function.
this.options.sessionIndex.clear(sessionCookieValue.sid), | ||
]); | ||
sessionLogger.debug('Invalidating current session.'); | ||
await this.options.sessionCookie.clear(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.
question what benefits to we get by invalidating the session cookie?
Clearing the session cookie is very tightly coupled to the way that Hapi decides to handle this, so I worry about unintended side effects with hapi upgrades. If there's an error clearing the session cookie, then we won't perform the actual session invalidation, which is arguably the more important aspect.
If we really should clear the session cookie here, then I think we should do one of two things:
- Catch any errors thrown by
sessionCookie.clear
, so that this operation may continue - Clear the session cookie after we've invalidated the requested server-side sessions.
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.
question what benefits to we get by invalidating the session cookie?
No real benefits to be honest, cleared it just because we could 🙂
Clearing the session cookie is very tightly coupled to the way that Hapi decides to handle this, so I worry about unintended side effects with hapi upgrades. If there's an error clearing the session cookie, then we won't perform the actual session invalidation, which is arguably the more important aspect.
That's an interesting point, I didn't think about that. I'm not sure if the risk is high, but I agree that it still exists. Let me just drop this line then to eliminate the potential problem completely.
import { getSAMLRequestId, getSAMLResponse } from '../../fixtures/saml/saml_tools'; | ||
import { FtrProviderContext } from '../../ftr_provider_context'; | ||
|
||
export default function ({ getService }: FtrProviderContext) { |
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.
🏅 These tests are great!
Sure, will add the docs in this PR! I need to finally stop this bad habit of moving docs to follow-ups anyway 🙈 |
@@ -0,0 +1,11 @@ | |||
[role="xpack"] |
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: copy-pasting from the role-management APIs docs 🙈
@@ -397,6 +397,14 @@ NOTE: *Public URL* is available only when anonymous access is configured and you | |||
+ | |||
For more information, refer to <<embedding, Embed {kib} content in a web page>>. | |||
|
|||
[float] |
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 initially planned to document invalidate API in the follow-up and combine it with the slightly related small (:crossed_fingers:) change we were talking about here, sooo that's my excuse for this change in this PR 🙂
@legrego, it's ready for another round! Added doc preview link to the issue description. |
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, thanks @azasypkin! Let's get docs review prior to merging
@elasticmachine merge upstream |
@@ -0,0 +1,11 @@ | |||
[role="xpack"] | |||
[[session-management-api]] |
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.
Will you be adding more APIs under the "Kibana user session management" section? If not, it would be better to make the "Invalidate user sessions API" a standalone page similar to "Shorten URL" in the TOC.
Is Kibana needed? Or, can the title simply be "User session management APIs"?
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.
Will you be adding more APIs under the "Kibana user session management" section?
We don't have capacity yet, but I can see that we'll want to add user session enumeration APIs that will support session management UI and automation workflows for the admins in the future.
Is Kibana needed? Or, can the title simply be "User session management APIs"?
I'm not sure to be honest. User session is a Kibana-only thing and I basically used the same convention we used for Spaces that is also a Kibana-only thing (Kibana Spaces APIs
). I think Kibana
is redundant here since this is a section of the Kibana docs after all, but what do you think?
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 Kibana is not needed.
docs/api/session-management.asciidoc
Outdated
[[session-management-api]] | ||
== {kib} user session management APIs | ||
|
||
Allows managing {kib} <<xpack-security-session-management, user sessions>>. |
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.
The sentence "Allows managing Kibana user sessions" isn't needed. It's covered in the title and in the sentence that follows it.
==== Request body | ||
|
||
`match`:: | ||
(Required, string) Specifies how {kib} should determine which sessions should be invalidated. Can either be `all` to invalidate all existing sessions, or `query` to only invalidate sessions that match the query specified in the additional `query` parameter. |
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.
(Required, string) Specifies how {kib} should determine which sessions should be invalidated. Can either be `all` to invalidate all existing sessions, or `query` to only invalidate sessions that match the query specified in the additional `query` parameter. | |
(Required, string) Specifies how {kib} determines which sessions to invalidate. Can either be `all` to invalidate all existing sessions, or `query` to only invalidate sessions that match the query specified in the additional `query` parameter. |
(Required, string) Specifies how {kib} should determine which sessions should be invalidated. Can either be `all` to invalidate all existing sessions, or `query` to only invalidate sessions that match the query specified in the additional `query` parameter. | ||
|
||
`query`:: | ||
(Optional, object) Specifies the query that {kib} should use to match the sessions that should be invalidated when `match` parameter is set to `query`. This parameter is forbidden if `match` is set to `all`. |
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.
(Optional, object) Specifies the query that {kib} should use to match the sessions that should be invalidated when `match` parameter is set to `query`. This parameter is forbidden if `match` is set to `all`. | |
(Optional, object) Specifies the query that {kib} uses to match the sessions to invalidate when the `match` parameter is set to `query`. This parameter is forbidden if `match` is set to `all`. |
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.
Does the last sentence mean "You cannot use this parameter if match
is set to all
.
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.
Does the last sentence mean "You cannot use this parameter if match is set to all.
That's correct. Will use You cannot use this parameter if match is set to all.
instead, thanks!
[%collapsible%open] | ||
===== | ||
`provider` ::: | ||
(Required, object) Describes the <<authentication-security-settings, authentication provider(s)>> for which to invalidate sessions. |
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.
(Required, object) Describes the <<authentication-security-settings, authentication provider(s)>> for which to invalidate sessions. | |
(Required, object) Describes the <<authentication-security-settings, authentication providers>> for which to invalidate sessions. |
Indicates a successful call. | ||
|
||
`403`:: | ||
Indicates that the user may not be authorized to invalidate sessions for other users, refer to <<session-management-api-invalidate-prereqs, Prerequisite section>>. |
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.
Indicates that the user may not be authorized to invalidate sessions for other users, refer to <<session-management-api-invalidate-prereqs, Prerequisite section>>. | |
Indicates that the user may not be authorized to invalidate sessions for other users. Refer to <<session-management-api-invalidate-prereqs, prerequisites>>. |
|
||
{kib} maintains a separate <<xpack-security-session-management, session>> for every anonymous user, as it does for all other authentication mechanisms. | ||
|
||
You can configure both <<session-idle-timeout, session idle timeout>> and <<session-lifespan, session lifespan>> for the anonymous sessions as you'd do for any other session with the only exception that idle timeout is explicitly disabled for the anonymous sessions by default. That means that the global <<security-session-and-cookie-settings, `xpack.security.session.idleTimeout`>> setting won't affect anonymous sessions. If you want to change the idle timeout for the anonymous sessions, you must configure the provider-level <<anonymous-authentication-provider-settings, `xpack.security.authc.providers.anonymous.<provider-name>.session.idleTimeout`>> setting 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.
You can configure both <<session-idle-timeout, session idle timeout>> and <<session-lifespan, session lifespan>> for the anonymous sessions as you'd do for any other session with the only exception that idle timeout is explicitly disabled for the anonymous sessions by default. That means that the global <<security-session-and-cookie-settings, `xpack.security.session.idleTimeout`>> setting won't affect anonymous sessions. If you want to change the idle timeout for the anonymous sessions, you must configure the provider-level <<anonymous-authentication-provider-settings, `xpack.security.authc.providers.anonymous.<provider-name>.session.idleTimeout`>> setting instead. | |
You can configure <<session-idle-timeout, session idle timeout>> and <<session-lifespan, session lifespan>> for anonymous sessions the same as you do for any other session with the exception that idle timeout is explicitly disabled for anonymous sessions by default. The global <<security-session-and-cookie-settings, `xpack.security.session.idleTimeout`>> setting doesn't affect anonymous sessions. To change the idle timeout for anonymous sessions, you must configure the provider-level <<anonymous-authentication-provider-settings, `xpack.security.authc.providers.anonymous.<provider-name>.session.idleTimeout`>> setting. |
@@ -6,6 +6,8 @@ When you log in, {kib} creates a session that is used to authenticate subsequent | |||
|
|||
When your session expires, or you log out, {kib} will invalidate your cookie and remove session information from the index. {kib} also periodically invalidates and removes any expired sessions that weren't explicitly invalidated. | |||
|
|||
To manage user sessions programmatically, {kib} exposes a set of <<session-management-api, session management APIs>>. |
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.
To manage user sessions programmatically, {kib} exposes a set of <<session-management-api, session management APIs>>. | |
To manage user sessions programmatically, {kib} exposes <<session-management-api, session management APIs>>. |
@gchaps thanks for review! PR should be ready for another review round. |
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.
Docs LGTM
@@ -0,0 +1,11 @@ | |||
[role="xpack"] | |||
[[session-management-api]] |
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 Kibana is not needed.
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
Summary
We recently started to store part of the user session in the dedicated Elasticsearch index. The values from index are removed when user explicitly logs out or when the session expires. But if sessions are configured to never expire (default behavior currently) then session index may grow infinitely and admins might want to clean that up manually.
Additionally admins might want to remove sessions from the index if they need to log user(s) out if there is a need.
Currently it's possible to access/manipulate
.kibana_security_session_x
index directly, but as part of the system-indices project, end-users will no longer be able to interact with Kibana's system-indices using the traditional ES APIs unless they include a specific HTTP request header value pair.This PR introduces a high-level Kibana API that will allow admins to invalidate/remove either all sessions or only sessions for a specific authentication provider or user.
Note: The API is only available to superusers for now.
Release note
We are exposing a new experimental API endpoint to allow superusers to invalidate sessions of other users. Refer to
Kibana user session management APIs
section of the Kibana REST API documentation for more info.Docs Preview
Fixes: #81941