Skip to content

Conversation

@wfurt
Copy link
Member

@wfurt wfurt commented Dec 20, 2021

This is follow-up on #57079.

When I did the original change, I run to problem that once ALPN callback is set it MUST return some valid value and that breaks use case when server does not wish to use ALPN. To keep the lookup simple I simply avoided resume with no ALPN. That of course bit me now in testing and I realized there is easy fix.

Each member of SslProtocols uses 2 bits starting with Ssl2 -> 12 -> 1100 and going up. The lover two bits are never used.
This change use the lowest bit to separate contexts with and without ALPN. With that, we can keep SslProtocols as only key for the hash lookup.

Second part I realized is that Server caching is on by default (even prior) #57079
https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_session_cache_mode.html
So even in cases we know resume will not work, we would calculate the ticket, send New Ticket message on wire and possibly cause burden on client to store session that will never be resumable.

So second part of this change is to turn caching off explicitly in case we use ephemeral context and resume will not be possible. Aside from resource saving it also makes it more obvious in packet capture if TLS resume is possible or not.

Note that client resume is coming soon. I wanted to split this part to make that change smaller and easier to review.

contributes to #22977

@wfurt wfurt added area-System.Net.Security os-linux Linux OS (any supported distro) labels Dec 20, 2021
@wfurt wfurt added this to the 7.0.0 milestone Dec 20, 2021
@wfurt wfurt requested review from a team, bartonjs and stephentoub December 20, 2021 23:53
@wfurt wfurt self-assigned this Dec 20, 2021
@ghost
Copy link

ghost commented Dec 20, 2021

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

This is follow-up on #57079.

When I did the original change, I run to problem that once ALPN callback is set it MUST return some valid value and that breaks use case when server does not wish to use ALPN. To keep the lookup simple I simply avoided resume with no ALPN. That of course bit me now in testing and I realized there is easy fix.

Each member of SslProtocols uses 2 bits starting with Ssl2 -> 12 -> 1100 and going up. The lover two bits are never used.
This change use the lowest bit to separate contexts with and without ALPN. With that, we can keep SslProtocols as only key for the hash lookup.

Second part I realized is that Server caching is on by default (even prior) #57079
https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_session_cache_mode.html
So even in cases we know resume will not work, we would calculate the ticket, send New Ticket message on wire and possibly cause burden on client to store session that will never be resumable.

So second part of this change is to turn caching off explicitly in case we use ephemeral context and resume will not be possible. Aside from resource saving it also makes it more obvious in packet capture if TLS resume is possible or not.

Note that client resume is coming soon. I wanted to split this part to make that change smaller and easier to review.

contributes to #22977

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Security, os-linux

Milestone: 7.0.0

@wfurt wfurt merged commit 5fdc58d into dotnet:main Jan 10, 2022
@wfurt wfurt deleted the resume branch January 10, 2022 21:58
@EgorBo
Copy link
Member

EgorBo commented Feb 2, 2022

@wfurt can this change be responsible for an improvement for YARP proxy?

image

@wfurt
Copy link
Member Author

wfurt commented Feb 2, 2022

I'm not sure how to read the graph. It seems like this is HTTP-HTTP -> no ssl involved. There may be some small improvement for SSL as the server will not calculate and send session tickets if resume is not possible but I'm not sure if that is applicable.
cc: @MihaZupan

@EgorBo
Copy link
Member

EgorBo commented Feb 2, 2022

Me neither actually 🙂 it's https://aka.ms/aspnet/benchmarks (16th page) it says something happened between 1/10 and 1/11 and this PR looked like the only suspect but it could also be a change in the YARP itself.

@MihaZupan
Copy link
Member

@EgorBo that change was a result of using non-validated headers in YARP: dotnet/yarp#1507

@EgorBo
Copy link
Member

EgorBo commented Feb 3, 2022

@EgorBo that change was a result of using non-validated headers in YARP: microsoft/reverse-proxy#1507

Nice! 🙂

@ghost ghost locked as resolved and limited conversation to collaborators Mar 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Net.Security os-linux Linux OS (any supported distro)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants