Don't use a non-resumable session if it is returned from the cache #18810
Conversation
mattcaswell
added
branch: master
|
This avoids the interoperability issue, but leaves a high probably of needless resumption failures when sessions are marked Please strongly consider updating the overflow code path to not do this in this PR. I also strongly feel that the expiration time order is a mistake, but fixing that would be a separate PR. |
Test sessions behave as we expect even in the case that an overflow occurs when adding a new session into the session cache.
We were incorrectly handling the case where the server side cache returns a non-resumable session. We ended up attempting to use the session for resumption but setting the session id to 0 length. This will cause the client to fail (typically with an unexpected message alert). Fixes openssl#18690
mattcaswell
force-pushed
the
sess-cache-overflow
branch
from
96387a8
to
228fa41
Compare
|
I've addressed the feedback from @t8m above and rebased this to resolve a conflict with master.
I do agree that changing this flag on sessions in the cache is unwise. However it is not quite clear to me what the implications of making this change are (in particular in a stable branch....and this PR is intended to be cherry-picked to 3.0). I think that particular issue is a lot less pressing given #18905. Therefore I do not agree that this needs to be dealt with in this PR. It is holding it up unnecessarily. Ping for review. |
|
CI is probably relevant. |
|
This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago |
|
Is this likely to land? |
paulidale
removed
the
approval: review pending
|
@mattcaswell please fix the CI failure |
If you mean to the release on Tuesday then no, I do not think it should go there. Unfortunately this PR needs work to fix the CI and according to our release policy we should already freeze the git repo before the release. |
t8m
removed
the
approval: otc review pending
|
Ping for the CI failure fix. |
|
ping @mattcaswell |
|
This PR is a year and a quarter old and solves an issue that we've encountered. We are going to have to pull in these patches manually. Is there some reason it hasn't been merged yet? |
|
I actually think this is the wrong fix. Co-incidentally I've been looking at this again recently. Updated fix coming soon. |
|
Let me know if there's anything we can do to help test it and get it out. |
We were incorrectly handling the case where the server side cache
returns a non-resumable session. We ended up attempting to use the session
for resumption but setting the session id to 0 length. This will cause the
client to fail (typically with an unexpected message alert).
Fixes #18690