Skip to content
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

Introduce SessionIdGenerationStrategy #2286

Conversation

marcusdacoregio
Copy link
Collaborator

Issue gh-11

@marcusdacoregio marcusdacoregio self-assigned this Apr 5, 2023
@marcusdacoregio marcusdacoregio force-pushed the gh-11-sessionidgenerationstrategy branch from 86fdeee to d154602 Compare April 5, 2023 14:13
Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I've commented inline, but there appear to be places where the SessionIdGenerationStrategy is not used (I've commented inline there).

It also seems that there are probably some additional tests that should be written.

return Mono.justOrEmpty(this.defaultMaxInactiveInterval.toSeconds())
.map((interval) -> new MongoSession(this.sessionIdGenerationStrategy.generate(), interval))
.doOnNext((mongoSession) -> publishEvent(new SessionCreatedEvent(this, mongoSession)))
.switchIfEmpty(Mono.just(new MongoSession(this.sessionIdGenerationStrategy.generate())));
Copy link
Member

Choose a reason for hiding this comment

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

Using a secure random number generator (eg generating a UUID) is a blocking operation so we may need a reactive equivalent of the SessionIdGenerationStrategy that does the generation on a different thread.

Copy link
Member

Choose a reason for hiding this comment

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

Does this logic take into account changeSessionId()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we do not need a reactive equivalent since now the strategy is inside the session implementation

Copy link
Member

Choose a reason for hiding this comment

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

The blocking operation is still happening on the event thread which should not occur

Choose a reason for hiding this comment

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

Is blocking caused by /dev/random? If so then fix the root cause, not the symptom.
=> Add an entropy generator in your VM

Caveat: If using Docker, put the generator in the host, and all containers will benefit from that shared kernel entropy pool.

Entropy generators for Linux:

  1. Linux kernel 5.4+ => A new JITTER entropy generator was added. Many LTS/EL kernels back-ported that enhancement.
  2. Linux kernel before 5.4 => Install haveged (JITTER), or rngd (JITTER, plus optional x86-only RDRAND)

Caveat: If you care about FIPS 140, disable all JITTER and only use a compliant entropy source (e.g. RDRAND on x86, or other third-party compliant entropy daemons/services).
Caveat: I have not seen SecureRandom blocking issues on macOS or Windows for years.

Copy link

Choose a reason for hiding this comment

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

RHEL/CentOS case study:

  • 9.x w/ kernel 5.14 => Built-in since 9.0 because kernel 5.4+ comes with JITTER
  • 8.x w/ kernel 4.18 => Back-ported in a later 8.x, so update to latest 8.x
  • 7.x w/ kernel 3.10 => Back-ported in a later 7.x, so update to latest 7.x
  • 6.x w/ kernel 2.6.32 => Not back-ported, manually install haveged or rng-tools
  • 5.x w/ kernel 2.6.18 => Same as above
  • 4.x w/ kernel 2.6.9 => Same as above

Choose a reason for hiding this comment

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

If you are on an old Linux kernel, and can't install haveged/rng-tools, fall back to this old Java hack:
3. JVM system property at startup:-Djava.security.egd=file:/dev/urandom

Caveat: This is a security concern. It will lead to weak random data, especially right after a reboot when the entropy pool is empty.
Caveat: It probably only works for the default Sun/Oracle providers. Third-party crypto providers (e.g. Bouncy Castle) may or may not offer a workaround.

@marcusdacoregio marcusdacoregio force-pushed the gh-11-sessionidgenerationstrategy branch from d154602 to aaa4c62 Compare April 18, 2023 17:49
@marcusdacoregio marcusdacoregio marked this pull request as ready for review April 18, 2023 17:59
@marcusdacoregio marcusdacoregio force-pushed the gh-11-sessionidgenerationstrategy branch from aaa4c62 to 8b68f26 Compare April 18, 2023 19:09
@marcusdacoregio marcusdacoregio force-pushed the gh-11-sessionidgenerationstrategy branch 2 times, most recently from ed84337 to 3e802f4 Compare April 20, 2023 12:52
@marcusdacoregio marcusdacoregio force-pushed the gh-11-sessionidgenerationstrategy branch from 3e802f4 to 0950932 Compare June 12, 2023 13:14
@marcusdacoregio marcusdacoregio force-pushed the gh-11-sessionidgenerationstrategy branch from 4b7cf6e to baba71b Compare July 3, 2023 17:36
@marcusdacoregio marcusdacoregio added this to the 3.2.0-M1 milestone Jul 11, 2023
@marcusdacoregio marcusdacoregio force-pushed the gh-11-sessionidgenerationstrategy branch 3 times, most recently from 880c17e to c1fbc5e Compare July 12, 2023 12:33
@marcusdacoregio marcusdacoregio force-pushed the gh-11-sessionidgenerationstrategy branch from c1fbc5e to a4e393e Compare July 12, 2023 13:16
@marcusdacoregio marcusdacoregio merged commit d547b33 into spring-projects:main Jul 13, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core type: enhancement A general enhancement
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants