Uses refresh=false for creating, updating, and invalidating user sessions #151800

jeramysoucy · 2023-02-21T22:17:04Z

Summary

Sets refresh parameter to false in session create, update, and invalidate. Previously refresh was set to 'wait_for' (or 'true' in the case of invalidating by query).

Tests

Several unit tests and functional tests have been updated to reflect the change in test snapshots and to manually refresh the session index in order to complete testing. The bulk of the test changes reside in the concurrent session limit suite.

Flaky Test Runner for relevant test suites: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1984

Documentation

Adds a note to the session-management ascii doc to document a known limitation of enforcing the concurrent sessions limit...

NOTE: Due to the rate at which session information is refreshed, there might be a few seconds where the concurrent session limit is not enforced.
This is something to consider for use cases where it is common to create multiple sessions simultaneously.

…Adds delays in tests to compensate for removing refresh wait_for.

jeramysoucy · 2023-02-21T22:51:13Z

x-pack/test/security_api_integration/tests/session_concurrent_limit/global_limit.ts

@@ -217,6 +223,9 @@ export default function ({ getService }: FtrProviderContext) {
          .set('kbn-xsrf', 'xxx')
          .set('Cookie', basicSessionCookie.cookieString());
        statusCodes.push(statusCode);
+        await setTimeoutAsync(1000);
+        await es.indices.refresh({ index: '.kibana_security_session*' });
+        await setTimeoutAsync(2000);


@azasypkin This was the only spot where just the index refresh was not enough. I had to put the static delays back in. Curious if you have any ideas on why the refresh wouldn't work here as it does elsewhere.

Hmmm, I have no idea to be honest, but let me think a bit.

By the way shouldn't you call this refresh before this for loop, right after you created sessions?

Right...that was a bit silly of me. I had moved the delays around previously and sort of lost the plot on this one. In the right spot it works 😅 . Thanks!

jeramysoucy · 2023-02-21T22:56:19Z

EDIT: not sure what happened to the link, but I fixed it.

@azasypkin We also use 'refresh: wait_for' (and 'refresh: true') in the invalidate method. Should we look at these cases as well?

// We don't specify primary term and sequence number as delete should always take precedence
// over any updates that could happen in the meantime.
const { statusCode } = await this.options.elasticsearchClient.delete(
  { id: filter.sid, index: this.aliasName, refresh: 'wait_for' },
  { ignore: [404], meta: true }
);

...

const response = await this.options.elasticsearchClient.deleteByQuery({
  index: this.aliasName,
  refresh: true,
  body: { query: deleteQuery }
});

azasypkin · 2023-02-22T13:44:05Z

@azasypkin We also use 'refresh: wait_for' (and 'refresh: true') in the invalidate method. Should we look at these cases as well?

Good point! These aren't in the hot path, but I think it'd still make sense to look at these as well.

jeramysoucy · 2023-02-22T13:48:30Z

x-pack/test/security_api_integration/tests/session_concurrent_limit/global_limit.ts

@@ -208,6 +213,8 @@ export default function ({ getService }: FtrProviderContext) {
        Array.from({ length: 10 }).map(() => loginWithBasic(testUser))
      );

+      await es.indices.refresh({ index: '.kibana_security_session*' });


The index needs to be refreshed in order for the session limit to be enforced. If we don't use 'wait_for', it is unclear that we can properly enforce session limits for concurrent sessions that are created at the same time. Is this a real use case? Do we want to support it?

The index needs to be refreshed in order for the session limit to be enforced. If we don't use 'wait_for', it is unclear that we can properly enforce session limits for concurrent sessions that are created at the same time

That's correct, but we have a 1s refresh interval configured for the index. That means that in the majority of the cases there will be just a small window (up to a few seconds) where the limit could be bypassed. Even though the problem might be more apparent for highly loaded clusters, I'd still say it's an acceptable tradeoff, at least for the initial release.

On the other hand, it's a relatively small and low-risk change to use refresh=wait_for only when concurrent session limit is configured, but I'm wondering if it'd make sense to make it only when/if we learn that it's not what our customers and users are comfortable with. What do you think @jeramysoucy @legrego @arisonl?

If we know of significant real life use cases where this will be a problem, then I would say let's default to using wait_for if concurrent session limits are configured. Otherwise, I like moving forward with refresh=false for now and potentially making wait_for a future option for flexibility should the need arise.

If we know of significant real life use cases where this will be a problem, then I would say let's default to using wait_for if concurrent session limits are configured. Otherwise, I like moving forward with refresh=false for now and potentially making wait_for a future option for flexibility should the need arise.

I'm comfortable with this, but I think we should call this out as a limitation in our docs for this feature. Otherwise, a security researcher could reasonably assert that this control is not working as intended.

I would like to better understand the details of the underlying technical issue (we'll discuss offline), but in principle I agree that a small (a few seconds) deviation from the expected behavior is not an issue for this use case, if documented. However, there is this reference: "in the majority of the cases there will be just a small window" which worries me a bit. Is there a chance that we our off by more than just a few seconds?

Just so I fully understand the problem. If we use refresh=wait_for, we are essentially synchronizing the response from the request Persist session in the diagram below, correct?

Yeah, it's close! The only difference is that we don't search for other sessions during login, but whenever the session is retrieved in the subsequent requests to make sure the session limitation isn't bypassed with tons of concurrent logins. But the idea is the same.

AFAIK, ES is looking at ways to make the data available for search even before the refresh interval has kicked in. Once that's complete, we may be able to completely migrate to refresh=wait_for. partying_face

Oh, that would be great, thanks for sharing! Is it something ES team has on the near-term roadmap (before Oct 2023)?

I'm not entirely sure about ES priorities in this regard.

@kobelb, @lukeelmers, Are you aware of an approx ETA for the Search after Write initiative?

I'm not sure of their current status on this, however the internal issue (ES-4076) seems to indicate May 2023.

Regarding augmenting our documentation to cover the known limitations of this change, do we want to make the note where we document the concurrent limit configuration option? https://www.elastic.co/guide/en/kibana/master/xpack-security-session-management.html#session-max-sessions

Regarding augmenting our documentation to cover the known limitations of this change, do we want to make the note where we document the concurrent limit configuration option?

Yeah, it's worth mentioning there.

…ctional tests.

jeramysoucy · 2023-02-22T22:28:48Z

@azasypkin We also use 'refresh: wait_for' (and 'refresh: true') in the invalidate method. Should we look at these cases as well?

Good point! These aren't in the hot path, but I think it'd still make sense to look at these as well.

I updated the invalidate method to use refresh=false as well, but had to move the manual index refresh in the concurrent session limit test suite. The call to get within the loop internally calls invalidate, so the index needed to be refreshed after each call.

elasticmachine · 2023-02-23T16:12:12Z

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

… the refresh changes present in this PR.

jeramysoucy · 2023-03-07T18:14:13Z

I've added a note to the session management docs. I think this is now ready for a final approval review @elastic/kibana-security.

gchaps

Approved with a small suggestion for the first sentence.

docs/user/security/session-management.asciidoc

Co-authored-by: gchaps <[email protected]>

azasypkin · 2023-03-09T10:53:14Z

ACK: Will review later today or tomorrow.

azasypkin

Looks great, thank you!

docs/user/security/session-management.asciidoc

x-pack/test/security_api_integration/tests/session_invalidate/invalidate.ts

x-pack/test/security_api_integration/tests/session_concurrent_limit/cleanup.ts

x-pack/test/security_api_integration/tests/session_concurrent_limit/global_limit.ts

jeramysoucy · 2023-03-10T17:03:16Z

docs/user/security/session-management.asciidoc

 --
 [source,yaml]
 --------------------------------------------------------------------------------
-xpack.security.session.сoncurrentSessions:
+xpack.security.session.concurrentSessions:


replaced cyrillic 'c' with latin 'c'

kibana-ci · 2023-03-10T18:15:55Z

💚 Build Succeeded

Buildkite Build
Commit: 53a130f

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`securitySolution`	434	437	+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	514	517	+3

History

💚 Build #112464 succeeded da9469e
💚 Build #109988 succeeded 3cca80b
💔 Build #109853 failed 66a43be
💚 Build #109637 succeeded 6201aa5
💔 Build #109613 failed f3ffb89
💔 Build #109466 failed 1f4340b

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…ions (elastic#151800) closes elastic#149338 ## Summary Sets refresh parameter to false in session create, update, and invalidate. Previously refresh was set to 'wait_for' (or 'true' in the case of invalidating by query). ### Tests Several unit tests and functional tests have been updated to reflect the change in test snapshots and to manually refresh the session index in order to complete testing. The bulk of the test changes reside in the [concurrent session limit suite](https://github.com/elastic/kibana/blob/66a43be28ca653809996b53198fc6fae2ee726f8/x-pack/test/security_api_integration/tests/session_concurrent_limit/global_limit.ts). Flaky Test Runner for relevant test suites: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1984 ### Documentation Adds a note to the session-management ascii doc to document a known limitation of enforcing the concurrent sessions limit... ``` NOTE: Due to the rate at which session information is refreshed, there might be a few seconds where the concurrent session limit is not enforced. This is something to consider for use cases where it is common to create multiple sessions simultaneously. ``` --------- Co-authored-by: gchaps <[email protected]>

## Summary Specifying a `refresh_interval` below 5s is no longer allowed with es serverless. This PR removes the explicit `refresh_interval` from the session index. Work done in #151800 makes specifying a `refresh_interval` unnecessary. ## Flaky Test Runner [Session Tests x50 ea](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2932) 🟢 Co-authored-by: Kibana Machine <[email protected]>

Changes refresh to false for writing and updating session documents. …

d3bba00

…Adds delays in tests to compensate for removing refresh wait_for.

jeramysoucy added Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:Security/Session Management Platform Security - Session Management v8.8.0 release_note:skip Skip the PR/issue when compiling release notes labels Feb 21, 2023

Replaces most delays with index refresh call.

1f4340b

jeramysoucy commented Feb 21, 2023

View reviewed changes

Moves index refresh for enforcing limit in concurrent logins.

f3ffb89

jeramysoucy commented Feb 22, 2023

View reviewed changes

jeramysoucy added 2 commits February 22, 2023 09:25

Adds index refresh to all applicable session functional tests.

6201aa5

Updates session invalidate to use refresh false. Updates unit and fun…

66a43be

…ctional tests.

Merge branch 'main' into use-refresh-false-for-user-sessions

3cca80b

jeramysoucy marked this pull request as ready for review February 23, 2023 16:12

jeramysoucy requested a review from a team as a code owner February 23, 2023 16:12

jeramysoucy changed the title ~~Uses refresh=false for creating and updating user sessions~~ Uses refresh=false for creating, updating, and invalidating user sessions Feb 23, 2023

jeramysoucy added 2 commits March 7, 2023 11:59

Adds documentation describing caveat of concurrent session limit with…

c99d2d6

… the refresh changes present in this PR.

Merge branch 'main' into use-refresh-false-for-user-sessions

da9469e

jeramysoucy requested a review from gchaps March 7, 2023 17:06

gchaps approved these changes Mar 7, 2023

View reviewed changes

docs/user/security/session-management.asciidoc Outdated Show resolved Hide resolved

Update docs/user/security/session-management.asciidoc

94adf9d

Co-authored-by: gchaps <[email protected]>

jeramysoucy requested a review from azasypkin March 8, 2023 21:41

azasypkin approved these changes Mar 10, 2023

View reviewed changes

Implements minor changes from code review

10beef1

jeramysoucy commented Mar 10, 2023

View reviewed changes

Merge branch 'main' into use-refresh-false-for-user-sessions

53a130f

jeramysoucy merged commit ba6058c into elastic:main Mar 10, 2023

kibanamachine added the backport:skip This commit does not require backporting label Mar 10, 2023

jeramysoucy deleted the use-refresh-false-for-user-sessions branch March 10, 2023 18:18

kc13greiner mentioned this pull request Aug 21, 2023

Removing refresh interval from session index #164328

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uses refresh=false for creating, updating, and invalidating user sessions #151800

Uses refresh=false for creating, updating, and invalidating user sessions #151800

jeramysoucy commented Feb 21, 2023 •

edited

Loading

jeramysoucy Feb 21, 2023

azasypkin Feb 22, 2023

azasypkin Feb 22, 2023

jeramysoucy Feb 22, 2023

jeramysoucy commented Feb 21, 2023 •

edited

Loading

azasypkin commented Feb 22, 2023 •

edited by jeramysoucy

Loading

jeramysoucy Feb 22, 2023

azasypkin Feb 22, 2023

jeramysoucy Feb 22, 2023

legrego Feb 22, 2023

arisonl Feb 22, 2023 •

edited

Loading

azasypkin Mar 1, 2023

afharo Mar 2, 2023

lukeelmers Mar 2, 2023

jeramysoucy Mar 6, 2023

azasypkin Mar 7, 2023

jeramysoucy commented Feb 22, 2023

elasticmachine commented Feb 23, 2023

jeramysoucy commented Mar 7, 2023

gchaps left a comment

azasypkin commented Mar 9, 2023

azasypkin left a comment

jeramysoucy Mar 10, 2023

kibana-ci commented Mar 10, 2023

ESLint disabled line counts

Total ESLint disabled count

Uses refresh=false for creating, updating, and invalidating user sessions #151800

Uses refresh=false for creating, updating, and invalidating user sessions #151800

Conversation

jeramysoucy commented Feb 21, 2023 • edited Loading

Summary

Tests

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeramysoucy commented Feb 21, 2023 • edited Loading

azasypkin commented Feb 22, 2023 • edited by jeramysoucy Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arisonl Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeramysoucy commented Feb 22, 2023

elasticmachine commented Feb 23, 2023

jeramysoucy commented Mar 7, 2023

gchaps left a comment

Choose a reason for hiding this comment

azasypkin commented Mar 9, 2023

azasypkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Mar 10, 2023

💚 Build Succeeded

Metrics [docs]

ESLint disabled line counts

Total ESLint disabled count

History

jeramysoucy commented Feb 21, 2023 •

edited

Loading

jeramysoucy commented Feb 21, 2023 •

edited

Loading

azasypkin commented Feb 22, 2023 •

edited by jeramysoucy

Loading

arisonl Feb 22, 2023 •

edited

Loading