Set session id context while creating SSLContext #2633

onlined · 2021-05-21T15:40:44Z

When using client certificates, session id context needs to be set. OpenSSL documentation covers it like this:

If the session id context is not set on an SSL/TLS server and client
certificates are used, stored sessions will not be reused but a fatal
error will be flagged and the handshake will fail.

Details: https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_session_id_context.html

Ruby OpenSSL wrapper conforms to this by setting the session id context to a random sequence of bytes whenever a context is created:
http://github.com/ruby/openssl/blob/master/lib/openssl/ssl.rb#L493

I am open to suggestions about generating random bytes. I feel like there can be a better way than this.

I have reviewed the guidelines for contributing to this repository.
I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
My pull request is 100 lines added/removed or less so that it can be easily reviewed.
If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
I have updated the documentation accordingly.
All new and existing tests passed, including Rubocop.

When using client certificates, session id context needs to be set. OpenSSL documentation covers it like this: If the session id context is not set on an SSL/TLS server and client certificates are used, stored sessions will not be reused but a fatal error will be flagged and the handshake will fail. Details: https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_session_id_context.html Ruby OpenSSL wrapper conforms to this by setting the session id context to a random sequence of bytes whenever a context is created: http://github.com/ruby/openssl/blob/master/lib/openssl/ssl.rb#L493 I am open to suggestions about generating random bytes. I feel like there can be a better way than this.

nateberkopec · 2021-05-27T13:20:11Z

Probably should just generate the randomness in C rather than get Ruby involved at all.

Fancy taking a stab at fixing this in the Java extension as well?

onlined · 2021-05-27T17:22:38Z

I had thought about generating the randomness in C, but using rand wouldn't be a good way IMO. rand uses a global seed, which can be set by srand (not setting the seed is equivalent to setting it 1). If I set the seed with srand, that's mutating global state of Ruby interpreter and can cause problems (now or in the future). If I don't set it, rand will always give the same random numbers, which is not that different than setting the bytes to a constant value.

A useful option could have been using RAND_bytes from OpenSSL, but success isn't guaranteed with it, so we'd need to handle the failure case.

I can see two ways after eliminating these:

Calling BCryptGenRandom on Windows and reading /dev/urandom on Linux or macOS. An alternative to using /dev/urandom on Linux or macOS is using POSIX (more general than Linux or macOS) rand_r which takes its own seed and modifies it (so no messing with global state). This solution will require maintenance when a new platform is introduced.
Using Ruby for generating random numbers, and that's what I did exactly.

onlined · 2021-05-28T00:17:02Z

For Java implementation, I checked jruby-openssl, but I saw that we cannot set session id context from Java: https://github.com/jruby/jruby-openssl/blob/master/src/main/java/org/jruby/ext/openssl/SSLContext.java#L447. I'm not sure, but I think it probably happens automatically with Java SSL library. Even if it's not, it should be fixed at Java SSL library or Puma should switch to another SSL implementation for JRuby. So, I think the Java part, which is either nothing or great effort, is out of scope of this PR.

evanphx

Could we clarify which versions of ruby Random::DEFAULT.bytes was first introduced?

MSP-Greg · 2021-05-28T03:54:01Z

Works in ruby 2.0.0p648 (2015-12-16) [x64-mingw32]

ext/puma_http11/mini_ssl.c

MSP-Greg · 2021-06-09T15:49:07Z

@onlined

This is messy. I recall that TLSv1.3 may handle session reuse differently that older protocols. I haven't had time to review docs to see if that is correct.

the handshake will fail

Have you seen that happen with Puma? I don't think there are any tests dealing with multiple requests from one client connection where 'client certificates are used'... Haven't had time to create one locally...

onlined · 2021-06-11T20:04:08Z

@MSP-Greg It happened with our backend using Puma and client cert authentication. This fixed solved the problem.

nateberkopec · 2021-06-11T20:32:13Z

It's in the docs on that ssl context id function @MSP-Greg - will hold on a release until you review though

MSP-Greg · 2021-06-11T21:26:33Z

@onlined Thanks. I suspected as much, as you wouldn't really have a reason for the PR...

@nateberkopec This is fine, but it will need to change to fix the deprecated warnings in Ruby 3.

I haven't had time to look at it, maybe set the value (instance variable) in the Ruby code in Puma::MiniSSL::Server#initialize, and then set it in the real Context in the c code in Puma::MiniSSL::SSLContext#initialize. That would leave the 'random' code as ruby, not c.

This only happens when the server sockets/listeners are created, so 'ruby vs c' probably isn't critical.

onlined · 2021-06-11T21:39:09Z

Replacing rb_const_get(rb_cRandom, rb_intern_const("DEFAULT")) with rb_funcall(rb_cRandom, rb_intern_const("new"), 0) can be a quick win (it works on all supported versions and is not deprecated on Ruby 3) instead of assigning a global variable in Ruby code and accessing it in C. BTW, I would have preferred (actually I tried it first) Random.bytes instead Random::DEFAULT.bytes but the former doesn't exist in former Ruby versions.

nateberkopec · 2021-06-11T22:40:21Z

Oh this will actually warn on Ruby 3? Didn't realize that... we do need to fix that.

MSP-Greg · 2021-06-11T22:43:17Z

@onlined & @nateberkopec

Please have a look at PR #2642, which removes the warnings...

Fixes from PR's: puma#2535 puma#2633 puma#2642

* Actions - add Ruby 3.0 and 3.1 * .gitignore - add entry for local use * test_puma_server_ssl.rb - backport fix 888b0213f11 * minissl.c and extconf.rb - backport fixes Fixes from PR's: #2535 #2633 #2642

When using client certificates, session id context needs to be set. OpenSSL documentation covers it like this: If the session id context is not set on an SSL/TLS server and client certificates are used, stored sessions will not be reused but a fatal error will be flagged and the handshake will fail. Details: https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_session_id_context.html Ruby OpenSSL wrapper conforms to this by setting the session id context to a random sequence of bytes whenever a context is created: http://github.com/ruby/openssl/blob/master/lib/openssl/ssl.rb#L493 I am open to suggestions about generating random bytes. I feel like there can be a better way than this.

nateberkopec added ssl waiting-for-review Waiting on review from anyone labels May 21, 2021

onlined force-pushed the set-session-id-context branch 3 times, most recently from c45bccf to ddc2115 Compare May 23, 2021 08:06

onlined force-pushed the set-session-id-context branch from ddc2115 to 6332949 Compare May 25, 2021 21:26

nateberkopec requested a review from evanphx May 26, 2021 14:18

nateberkopec added the c-ext label May 27, 2021

nateberkopec added waiting-for-changes Waiting on changes from the requestor and removed waiting-for-review Waiting on review from anyone labels May 27, 2021

evanphx reviewed May 28, 2021

View reviewed changes

nateberkopec added waiting-for-review Waiting on review from anyone and removed waiting-for-changes Waiting on changes from the requestor labels May 28, 2021

nateberkopec merged commit ef1a9f5 into puma:master Jun 4, 2021

eregon reviewed Jun 9, 2021

View reviewed changes

ext/puma_http11/mini_ssl.c Show resolved Hide resolved

MSP-Greg mentioned this pull request Jun 11, 2021

minissl.c - Use Random.bytes if available #2642

Merged

7 tasks

MSP-Greg added bug and removed waiting-for-review Waiting on review from anyone labels Jun 11, 2021

MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Feb 13, 2022

minissl.c and extconf.rb - backport fixes

db61529

Fixes from PR's: puma#2535 puma#2633 puma#2642

MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Feb 13, 2022

minissl.c and extconf.rb - backport fixes

22b4b61

Fixes from PR's: puma#2535 puma#2633 puma#2642

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set session id context while creating SSLContext #2633

Set session id context while creating SSLContext #2633

onlined commented May 21, 2021 •

edited

nateberkopec commented May 27, 2021

onlined commented May 27, 2021 •

edited

onlined commented May 28, 2021 •

edited

evanphx left a comment

MSP-Greg commented May 28, 2021

MSP-Greg commented Jun 9, 2021

onlined commented Jun 11, 2021

nateberkopec commented Jun 11, 2021

MSP-Greg commented Jun 11, 2021 •

edited

onlined commented Jun 11, 2021 •

edited

nateberkopec commented Jun 11, 2021

MSP-Greg commented Jun 11, 2021

Set session id context while creating SSLContext #2633

Set session id context while creating SSLContext #2633

Conversation

onlined commented May 21, 2021 • edited

nateberkopec commented May 27, 2021

onlined commented May 27, 2021 • edited

onlined commented May 28, 2021 • edited

evanphx left a comment

Choose a reason for hiding this comment

MSP-Greg commented May 28, 2021

MSP-Greg commented Jun 9, 2021

onlined commented Jun 11, 2021

nateberkopec commented Jun 11, 2021

MSP-Greg commented Jun 11, 2021 • edited

onlined commented Jun 11, 2021 • edited

nateberkopec commented Jun 11, 2021

MSP-Greg commented Jun 11, 2021

onlined commented May 21, 2021 •

edited

onlined commented May 27, 2021 •

edited

onlined commented May 28, 2021 •

edited

MSP-Greg commented Jun 11, 2021 •

edited

onlined commented Jun 11, 2021 •

edited