Fixes #2397 Add shared session cache in SSL_CTX by tmshort · Pull Request #3761 · openssl/openssl

tmshort · 2017-06-23T19:02:41Z

Support for shared session cache via SSL_CTX_share_session_cache().

The SSL_CTX_sessions() API always returns NULL, as the lock for the shared
session cache is not exposed to library consumers.

Use this method to share the cache for the resumption tests.

Checklist

documentation is added or updated
tests are added or updated

kaduk · 2017-06-23T21:40:57Z

If I recall correctly @vdukhovni was interested in something like this.

dot-asm · 2017-06-24T08:43:21Z

ssl/ssl_lib.c

Indent removal is unjustified.

oops, will fix.

dot-asm · 2017-06-24T08:44:41Z

ssl/ssl_lib.c

If you have styling issue in near vicinity, fix it. I mean it should be return l;, right?

dot-asm · 2017-06-24T09:17:39Z

ssl/ssl_sess.c

I'd say this kind of calls for "philosophical" problem. Locking kind of implies that you "legitimate" "battle" of callback setters. I mean you kind of say "everybody is allowed to set/change callback to their liking at any time and I ensure that it's properly serialized among threads". But this is not true. It's more than likely that it would be inappropriate to change callbacks once object is instantiated. Or at least allow different threads to have different opinions on what callbacks are...

Yes. These callback were originally left in the SSL_CTX (in our original implementation), but were moved to the cache structure for some consistency. However; one only of these callbacks is invoked (regardless of the number of SSL_CTX's sharing the cache), depending on which SSL_CTX is "active" when an SSL_SESSION is added, removed, etc; since the SSL_CTX is passed into the callback. If a list of SSL_CTX's were kept in the cache, then a callback could be invoked for each of them, at which point, having SSL_CTX-specifc callbacks makes sense (but so does having a single callback).

So, I'm kinda "meh" on how the callbacks are stored.

What I'm trying to say is that this seems like case of excessive locking. Or on certain level meaningless locking. Don't callbacks kind of define how SSL_SESSION operates and should be set prior it becomes appropriate to share it? Because in such case, locking is not necessary...

I was thinking of a bigger picture; whether these should even be part of the SSL_SESSION_CACHE structure or remain in SSL_CTX. The locking may seem like overkill, but it's safe in all conditions.

bernd-edlinger · 2017-06-24T19:12:54Z

ssl/ssl_sess.c

this is not synchronized like SSL_CTX_sess_set_remove_cb ?

It's a read, not a write, so it's safe. If I were reading multiple things, or had to read multiple values to generate a return value, it would be locked. For example, |session_cache| is always non-NULL, if there was a chance it would be NULL, I would use a lock since I'd have to do a check.

The access to ctx->session_cache is not safe, if session_cache can be changed
simultaneously.
However I agree with @dot-asm that such use should be considered invalid.

A processor cannot do a simultaneous read and write to memory; so the pointer will never have an invalid value. If anything, you will get stale data, which may not matter. But @dot-asm's point still applies; and it's questionable if these functions should be in the SSL_SESSION or SSL_CTX; but I think that the functions associate closer with an SSL_SESSION than an SSL_CTX.

I mean, if you overwrite the the session_cache in one thread, and another thread executes something
that uses ctx->session_cache, it will load the pointer into a register and access the object creating
a race with the other thread deleting the old value of session_cache.
If the read/write locking in SSL_CTX_share_session_cache is good for anything then only
if the value is read with read lock at all times.
Have you tried this with thread sanitizer?

I see your point, but it only becomes a problem if the memory is unmapped when freed. The chance that this occurring is infinitely small, but non-zero.
But, the problem you are referring to is slightly bigger as the old session cache is freed before the new one is assigned, so there is an extremely small window where the old session_cache may be freed before the new one is assigned.
I have not had the chance to run it through thread-sanitizer.
But the bigger picture I'm interested in is if these functions belong with the SSL_SESSION_CACHE, or should stay with the SSL_CTX.

bernd-edlinger · 2017-06-24T19:16:01Z

ssl/ssl_lib.c

would this dead-lock if a == b ?

Yes, and that'd be bad mojo. I consider that a serious user error. But, from a different viewpoint, it should always succeed.

richsalz · 2017-06-26T13:33:47Z

Well it’s a “safe” read if the address isn’t being modified by another thread. We generally don’t care about that, but I just came across code (RAND_get_method) that does. Shrug. This is just a side-note.

tmshort · 2017-06-26T13:36:29Z

Well it’s a “safe” read if the address isn’t being modified by another thread. We generally don’t care about that, but I just came across code (RAND_get_method) that does. Shrug. This is just a side-note.

Yes, even if the pointer is being modified by another thread, a pointer modification is generally done in one memory operation, so this won't segfault or anything. You might get stale data, which would've been correct at the time the function was invoked.

tmshort · 2017-06-26T13:39:28Z

Yes, even if the pointer is being modified by another thread, a pointer modification is generally done in one memory operation, so this won't segfault or anything.

The only counter-example I can think of is far pointers in 16-bit x86 code; which we don't support anymore.

tmshort · 2017-06-26T13:41:58Z

And possibly 32-bit mode on a 80386SX (which has a 16-bit external bus, or any other processor where the memory data bus is less than the address size, such as 68008), but that would be two bus cycles, and still considered a single memory operation.

tmshort · 2017-06-26T14:07:54Z

Fixes #2397

kaduk · 2017-06-26T20:50:52Z

Yes, even if the pointer is being modified by another thread, a pointer modification is generally done in one memory operation, so this won't segfault or anything.

The only counter-example I can think of is far pointers in 16-bit x86 code; which we don't support anymore.

I think non-x86 platforms can have non-atomic accesses to word-sized variables, though don't have an example handy.

richsalz · 2017-06-26T20:52:12Z

Sorry for starting this sub-thread. My main point was that we’re not consistent.

FdaSilvaYY · 2017-06-26T21:47:38Z

ssl/ssl_lib.c

blank line after decl ...
You could merge declaration with his next assignment line too .

tmshort · 2017-06-27T19:54:15Z

I'm updating the get/set routines of SSL_SESSION_CACHE. These should resolve most of the issues raised.

Where it is just a get, there is a read lock for the SSL_CTX
Where it is just a set, there is a write lock for the SSL_CTX
Where it is a set that returns the old value, there is a write lock for both SSL_CTX and SSL_SESSION_CACHE
When getting the number of sessions, there is a read lock for both the SSL_CTX and SSL_SESSION_CACHE

Also updated the SSL_CTX_share_session_cache() to avoid the very small window where session_cache might point to a stale object.

bernd-edlinger · 2017-07-02T14:25:50Z

I wonder if it would be more straight forward to ask the application
to implement the shared session cache via new_session_cb,
remove_session_cb and get_session_cb, maybe together with
session_cache_mode == SSL_SESS_CACHE_NO_INTERNAL_LOOKUP,
or if you want add a completely new facility that uses these callbacks ?

tmshort · 2017-07-02T19:58:42Z

@bernd-edlinger, this does not replace those callbacks, but provides a simpler mechanism to share sessions among SSL_CTX without having to write a lot of code.

richsalz · 2017-08-16T15:15:55Z

I'm okay with this; are you, @bernd-edlinger ?

bernd-edlinger · 2017-08-16T18:01:07Z

ssl/ssl_lib.c

This lock might theoretically cause a dead-lock.
Suppose one thread calls
SSL_CTX_share_session_cache(a.b);
and the other thread calls
SSL_CTX_share_session_cache(b,a);

Yup, although I would except that SSL_CTX setup occur in an initialization thread, and that two threads wouldn't be modifying separate SSL_CTX's. Although, I did have a design where there was one SSL_CTX per thread that was affinity locked to a processor core... not that we used that.

bernd-edlinger · 2017-08-16T18:08:05Z

@richsalz actually I would be more happy to see an explicit SSL_SESSION_CACHE API object,
and assign that to SSL_CTX in the construction, and maybe keep some parts of the
SSL_CTX constant at runtime, if that makes less need for new locks.

richsalz · 2017-08-16T18:10:35Z

yes, that would be a better approach. Todd's on vacation, let's see what he says when he gets back.

kaduk · 2017-08-16T18:15:57Z

Hmm, would that have to wait for 1.2?
I guess the API considerations can be addressed by things like SSL_CTX_new_ex(SSL_METHOD *, SSL_SESSION_CACHE *), and absent this patch the per-SSL_CTX session cache has some immutable fields, so it seems like it could be plausibly done.
You'd still need a lock for the SSL_SESSION_CACHE, just no longer need to hold the SSL_CTX lock to access it as well.

tmshort · 2017-08-25T14:50:16Z

Ok, rebased, updated the locks and the documentation.

I'm still thinking of adding SSL_SESSION_CACHE_set/get APIs, but have yet to do that.

tmshort · 2018-01-10T20:03:13Z

Rebased

tmshort · 2018-01-19T16:21:17Z

Looks to be a doc-nits travis failure.

kaduk · 2018-02-28T16:24:44Z

It looks like at least both this and #4848 are trying to modify and/or extend the behavior of SSL_CTX_new() -- perhaps we should step back and think about what sorts of things we might want to do at the time of SSL_CTX creation and how to have a minimal number of APIs that supports all of those things in a reasonable fashion.

tmshort · 2018-04-13T16:02:59Z

Given that the SSL_SESSION_CACHE exists as a separate object, and there are potential changes (PR #5945) to add a different version of SSL_CTX_new_ex(), perhaps SSL_SESSION_CACHE can be assigned to a newly created SSL_CTX, i.e. one that has a reference count of 1, and/or no sessions attached to it?

Support for shared session cache via SSL_CTX_share_session_cache(). The SSL_CTX_sessions() API always returns NULL, as the lock for the shared session cache is not exposed to library consumers. Use this method to share the cache for the resumption tests.

tmshort · 2020-06-09T14:36:01Z

We are not going to attempt to get this in...

dot-asm reviewed Jun 24, 2017

View reviewed changes

bernd-edlinger reviewed Jun 24, 2017

View reviewed changes

tmshort force-pushed the master-shared-sessions branch from 37f6f16 to 7eff280 Compare June 26, 2017 14:04

tmshort changed the title ~~Add shared session cache in SSL_CTX~~ Fixes #2397 Add shared session cache in SSL_CTX Jun 26, 2017

FdaSilvaYY reviewed Jun 26, 2017

View reviewed changes

ssl/ssl_lib.c Outdated

Copy link

Contributor

FdaSilvaYY Jun 26, 2017

Choose a reason for hiding this comment

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

blank line after decl ...
You could merge declaration with his next assignment line too .

tmshort force-pushed the master-shared-sessions branch from 7eff280 to dfb11f3 Compare June 27, 2017 19:53

tmshort force-pushed the master-shared-sessions branch from dfb11f3 to c553771 Compare June 29, 2017 20:31

tmshort force-pushed the master-shared-sessions branch 3 times, most recently from 617a62b to b73195a Compare July 6, 2017 20:14

tmshort force-pushed the master-shared-sessions branch from b73195a to 22938f8 Compare July 27, 2017 19:46

bernd-edlinger reviewed Aug 16, 2017

View reviewed changes

tmshort force-pushed the master-shared-sessions branch from 8f95bfb to 12be35a Compare August 25, 2017 14:48

tmshort force-pushed the master-shared-sessions branch from 12be35a to 39630b1 Compare October 23, 2017 14:00

tmshort changed the title ~~WIP: Fixes #2397 Add shared session cache in SSL_CTX~~ Fixes #2397 Add shared session cache in SSL_CTX Oct 23, 2017

tmshort force-pushed the master-shared-sessions branch from 39630b1 to b56d956 Compare January 10, 2018 20:02

tmshort force-pushed the master-shared-sessions branch from b56d956 to 7e9cb34 Compare January 16, 2018 19:05

kaduk added this to the 1.1.1 milestone Jan 18, 2018

tmshort mentioned this pull request Jan 19, 2018

[rt.openssl.org #3869] [PATCH] Add shared session lists in SSL_CTX #2397

Closed

tmshort force-pushed the master-shared-sessions branch from 7e9cb34 to 7d47ef8 Compare January 19, 2018 16:49

mattcaswell modified the milestones: 1.1.1, Post 1.1.1 Jan 24, 2018

t8m mentioned this pull request Mar 16, 2018

Apply system_default configuration on SSL_CTX_new() #4848

Closed

2 tasks

tmshort force-pushed the master-shared-sessions branch 2 times, most recently from e0fff70 to 5aa4e1d Compare March 22, 2018 17:49

tmshort mentioned this pull request Apr 13, 2018

WIP: Mitigate regression errors when linking older apps with libssl 1.1.1 #5945

Closed

richsalz added after 1.1.1 and removed after 1.1.1 labels May 6, 2018

tmshort force-pushed the master-shared-sessions branch from de12e07 to 2a14408 Compare May 8, 2018 15:04

tmshort added 2 commits June 14, 2018 14:28

Remove SSL_CTX_new_ex() and replace with SSL_CTX_set1_session_cache()

957d143

tmshort force-pushed the master-shared-sessions branch from 2a14408 to 957d143 Compare June 14, 2018 18:39

tmshort mentioned this pull request Oct 3, 2019

Optimize session cache flushing #8687

Closed

1 task

mspncp removed the after 1.1.1 label Oct 25, 2019

tmshort closed this Jun 9, 2020

tmshort deleted the master-shared-sessions branch June 9, 2021 18:09

Uh oh!

Conversation

tmshort commented Jun 23, 2017

Checklist

Uh oh!

kaduk commented Jun 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richsalz commented Jun 26, 2017 via email

Uh oh!

tmshort commented Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmshort commented Jun 26, 2017

Uh oh!

tmshort commented Jun 26, 2017

Uh oh!

tmshort commented Jun 26, 2017

Uh oh!

kaduk commented Jun 26, 2017

Uh oh!

richsalz commented Jun 26, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmshort commented Jun 27, 2017

Uh oh!

bernd-edlinger commented Jul 2, 2017

Uh oh!

tmshort commented Jul 2, 2017

Uh oh!

richsalz commented Aug 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bernd-edlinger commented Aug 16, 2017

Uh oh!

richsalz commented Aug 16, 2017

Uh oh!

kaduk commented Aug 16, 2017

Uh oh!

tmshort commented Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmshort commented Jan 10, 2018

Uh oh!

tmshort commented Jan 19, 2018

tmshort commented Jun 26, 2017 •

edited

Loading

tmshort commented Aug 25, 2017 •

edited

Loading