You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficserver.apache.org by shinrich <gi...@git.apache.org> on 2016/09/09 15:19:19 UTC
[GitHub] trafficserver pull request #1000: TS-4468: http.server_session_sharing.match...
GitHub user shinrich opened a pull request:
https://github.com/apache/trafficserver/pull/1000
TS-4468: http.server_session_sharing.match check SNI
Started with patch proposed by Jered Floyd on the bug. Tested the basic case of SNI name match/mismatch and reuse seem to work as expected.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shinrich/trafficserver ts-4468
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/1000.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1000
----
commit 262707c9ae1eb929fd1b91821569ff7aa9297f0e
Author: Susan Hinrichs <sh...@ieee.org>
Date: 2016-09-09T15:14:18Z
TS-4468: http.server_session_sharing.match check SNI
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1000: TS-4468: http.server_session_sharing.match check ...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1000
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/783/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1000: TS-4468: http.server_session_sharing.match check ...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1000
FreeBSD build *failed*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/791/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1000: TS-4468: http.server_session_sharing.match check ...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the issue:
https://github.com/apache/trafficserver/pull/1000
Pushed new version to address the case insensitive name comparison.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1000: TS-4468: http.server_session_sharing.match...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1000#discussion_r78414549
--- Diff: proxy/http/HttpSessionManager.cc ---
@@ -98,7 +113,10 @@ ServerSessionPool::acquireSession(sockaddr const *addr, INK_MD5 const &hostname_
// Otherwise we need to scan further matches to match the host name as well.
// Note we don't have to check the port because it's checked as part of the IP address key.
if (TS_SERVER_SESSION_SHARING_MATCH_IP != match_style) {
- while (loc && loc->hostname_hash != hostname_hash) {
+ while (loc) {
+ if (loc->hostname_hash == hostname_hash && match_sni(sm, loc->get_netvc())) {
--- End diff --
I don't think this is the right approach. ``match_sni`` should do exactly what it's name claims and we should check in the caller whether we are looking for a TLS session. I think that having ``match_sni`` make assumptions about the match is spooky action at a distance that makes this harder to understand than is necessary.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1000: TS-4468: http.server_session_sharing.match check ...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1000
Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/687/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1000: TS-4468: http.server_session_sharing.match check ...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the issue:
https://github.com/apache/trafficserver/pull/1000
Changed the name of match_sni to validat_sni. Added comments in valid_sni to clarify what is being compared and what constraint we are enforcing on connection reuse.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1000: TS-4468: http.server_session_sharing.match check ...
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on the issue:
https://github.com/apache/trafficserver/pull/1000
Looks good but needs better comments. The validity of the checking depends on subtle features and interactions that aren't mentioned (most importantly, that the SNI name is tweaked to the remapped FQDN when an origin session is originally created so the request FQDN and SNI are equivalent).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1000: TS-4468: http.server_session_sharing.match check ...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1000
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/661/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1000: TS-4468: http.server_session_sharing.match...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich closed the pull request at:
https://github.com/apache/trafficserver/pull/1000
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1000: TS-4468: http.server_session_sharing.match check ...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1000
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/688/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1000: TS-4468: http.server_session_sharing.match...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1000#discussion_r78414024
--- Diff: proxy/http/HttpSessionManager.cc ---
@@ -75,17 +75,32 @@ ServerSessionPool::match(HttpServerSession *ss, sockaddr const *addr, INK_MD5 co
(TS_SERVER_SESSION_SHARING_MATCH_HOST == match_style || ats_ip_addr_port_eq(ss->get_server_ip(), addr));
}
+bool
+ServerSessionPool::match_sni(HttpSM *sm, NetVConnection *netvc)
+{
+ // TS-4468: If the connection matches, make sure the SNI server
+ // name (if present) matches the request hostname
+ int len = 0;
+ const char *req_host = sm->t_state.hdr_info.server_request.host_get(&len);
+ const char *session_sni = netvc->options.sni_servername;
+
+ return ((sm->t_state.scheme != URL_WKSIDX_HTTPS) || !session_sni || !strncmp(session_sni, req_host, len));
--- End diff --
What is the scheme check doing?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1000: TS-4468: http.server_session_sharing.match...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1000#discussion_r78414689
--- Diff: proxy/http/HttpSessionManager.cc ---
@@ -75,17 +75,32 @@ ServerSessionPool::match(HttpServerSession *ss, sockaddr const *addr, INK_MD5 co
(TS_SERVER_SESSION_SHARING_MATCH_HOST == match_style || ats_ip_addr_port_eq(ss->get_server_ip(), addr));
}
+bool
+ServerSessionPool::match_sni(HttpSM *sm, NetVConnection *netvc)
+{
+ // TS-4468: If the connection matches, make sure the SNI server
+ // name (if present) matches the request hostname
+ int len = 0;
+ const char *req_host = sm->t_state.hdr_info.server_request.host_get(&len);
+ const char *session_sni = netvc->options.sni_servername;
+
+ return ((sm->t_state.scheme != URL_WKSIDX_HTTPS) || !session_sni || !strncmp(session_sni, req_host, len));
--- End diff --
Do you need to make a case-insensitive comparison here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1000: TS-4468: http.server_session_sharing.match...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1000#discussion_r78454974
--- Diff: proxy/http/HttpSessionManager.cc ---
@@ -75,17 +75,32 @@ ServerSessionPool::match(HttpServerSession *ss, sockaddr const *addr, INK_MD5 co
(TS_SERVER_SESSION_SHARING_MATCH_HOST == match_style || ats_ip_addr_port_eq(ss->get_server_ip(), addr));
}
+bool
+ServerSessionPool::match_sni(HttpSM *sm, NetVConnection *netvc)
+{
+ // TS-4468: If the connection matches, make sure the SNI server
+ // name (if present) matches the request hostname
+ int len = 0;
+ const char *req_host = sm->t_state.hdr_info.server_request.host_get(&len);
+ const char *session_sni = netvc->options.sni_servername;
+
+ return ((sm->t_state.scheme != URL_WKSIDX_HTTPS) || !session_sni || !strncmp(session_sni, req_host, len));
--- End diff --
If the scheme is not https, there is no need to do the SNI check.
Agreed, might as well do a case insensitive compare here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1000: TS-4468: http.server_session_sharing.match check ...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1000
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/765/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1000: TS-4468: http.server_session_sharing.match...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1000#discussion_r78455353
--- Diff: proxy/http/HttpSessionManager.cc ---
@@ -98,7 +113,10 @@ ServerSessionPool::acquireSession(sockaddr const *addr, INK_MD5 const &hostname_
// Otherwise we need to scan further matches to match the host name as well.
// Note we don't have to check the port because it's checked as part of the IP address key.
if (TS_SERVER_SESSION_SHARING_MATCH_IP != match_style) {
- while (loc && loc->hostname_hash != hostname_hash) {
+ while (loc) {
+ if (loc->hostname_hash == hostname_hash && match_sni(sm, loc->get_netvc())) {
--- End diff --
The original patch did not have match_sni. But the code was cut-n-pasted several places. I'd rather not pollute the caller with more details. Could change the function name to be less confusion. Like match_sni_if_needed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1000: TS-4468: http.server_session_sharing.match check ...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1000
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/792/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1000: TS-4468: http.server_session_sharing.match...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1000#discussion_r78414308
--- Diff: proxy/http/HttpSessionManager.cc ---
@@ -75,17 +75,32 @@ ServerSessionPool::match(HttpServerSession *ss, sockaddr const *addr, INK_MD5 co
(TS_SERVER_SESSION_SHARING_MATCH_HOST == match_style || ats_ip_addr_port_eq(ss->get_server_ip(), addr));
}
+bool
+ServerSessionPool::match_sni(HttpSM *sm, NetVConnection *netvc)
+{
+ // TS-4468: If the connection matches, make sure the SNI server
+ // name (if present) matches the request hostname
+ int len = 0;
+ const char *req_host = sm->t_state.hdr_info.server_request.host_get(&len);
+ const char *session_sni = netvc->options.sni_servername;
+
+ return ((sm->t_state.scheme != URL_WKSIDX_HTTPS) || !session_sni || !strncmp(session_sni, req_host, len));
--- End diff --
``strcmp(...) == 0`` is easier to read IMHO
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1000: TS-4468: http.server_session_sharing.match check ...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1000
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/679/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---