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/08 18:26:47 UTC
[GitHub] trafficserver pull request #992: TS-4480: Wildcards in certificates should o...
GitHub user shinrich opened a pull request:
https://github.com/apache/trafficserver/pull/992
TS-4480: Wildcards in certificates should only match one level.
As discussed in the bug, replaced the Trie with a second subdomain hash table which our matching options are limited.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shinrich/trafficserver ts-4480
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/992.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 #992
----
commit eea095712b2a70793a0116edf258713450106434
Author: Susan Hinrichs <sh...@ieee.org>
Date: 2016-09-08T18:23:28Z
TS-4480: Wildcards in certificates should only match one level.
----
---
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 #992: TS-4480: Wildcards in certificates should only mat...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/992
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/645/ 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 #992: TS-4480: Wildcards in certificates should o...
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/992#discussion_r78589196
--- Diff: iocore/net/test_certlookup.cc ---
@@ -88,7 +88,7 @@ REGRESSION_TEST(SSLCertificateLookup)(RegressionTest *t, int /* atype ATS_UNUSED
// Basic hostname cases.
box.check(lookup.find("www.foo.com")->ctx == foo, "host lookup for www.foo.com");
- box.check(lookup.find("www.bar.com")->ctx == all_com, "host lookup for www.bar.com");
+ box.check(lookup.find("www.bar.com") == NULL, "www.bar.com won't match *.com because we only match one level");
--- End diff --
Perhaps add some case bending to check that as well. Or is that a different bug?
```
box.check(lookup.find("B.WilD.cOm")->ctx == wild, "wildcard lookup for B.WilD.cOm");
```
---
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 #992: TS-4480: Wildcards in certificates should only mat...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/992
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/749/ 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 #992: TS-4480: Wildcards in certificates should o...
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/992#discussion_r78582673
--- Diff: iocore/net/SSLCertLookup.cc ---
@@ -346,48 +349,30 @@ int
SSLContextStorage::insert(const char *name, int idx)
{
ats_wildcard_matcher wildcard;
- bool inserted = false;
+ InkHashTableValue value;
+ char lower_case_name[TS_MAX_HOST_NAME_LEN + 1];
--- End diff --
Any reason to not just lower case `name` immediately?
---
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 #992: TS-4480: Wildcards in certificates should o...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/992#discussion_r78606649
--- Diff: iocore/net/SSLCertLookup.cc ---
@@ -346,48 +349,30 @@ int
SSLContextStorage::insert(const char *name, int idx)
{
ats_wildcard_matcher wildcard;
- bool inserted = false;
+ InkHashTableValue value;
+ char lower_case_name[TS_MAX_HOST_NAME_LEN + 1];
if (wildcard.match(name)) {
- // We turn wildcards into the reverse DNS form, then insert them into the trie
- // so that we can do a longest match lookup.
- char namebuf[TS_MAX_HOST_NAME_LEN + 1];
- char *reversed;
- ats_scoped_obj<ContextRef> ref;
-
- reversed = reverse_dns_name(name + 1, namebuf);
- if (!reversed) {
- Error("wildcard name '%s' is too long", name);
- return -1;
+ // Strip the wildcard and store the subdomain
+ const char *subdomain = index(name, '*');
+ if (subdomain) {
+ subdomain = index(subdomain, '.');
--- End diff --
I suppose. Can do the sanity check instead of the index.
---
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 #992: TS-4480: Wildcards in certificates should only mat...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the issue:
https://github.com/apache/trafficserver/pull/992
Pushed new version which addresses the comments.
---
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 #992: TS-4480: Wildcards in certificates should only mat...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/992
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/671/ 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 #992: TS-4480: Wildcards in certificates should only mat...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/992
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/794/ 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 #992: TS-4480: Wildcards in certificates should o...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich closed the pull request at:
https://github.com/apache/trafficserver/pull/992
---
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 #992: TS-4480: Wildcards in certificates should o...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/992#discussion_r78609493
--- Diff: iocore/net/SSLCertLookup.cc ---
@@ -346,48 +349,30 @@ int
SSLContextStorage::insert(const char *name, int idx)
{
ats_wildcard_matcher wildcard;
- bool inserted = false;
+ InkHashTableValue value;
+ char lower_case_name[TS_MAX_HOST_NAME_LEN + 1];
--- End diff --
No, agreed should move it up.
---
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 #992: TS-4480: Wildcards in certificates should only mat...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/992
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/670/ 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 #992: TS-4480: Wildcards in certificates should o...
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/992#discussion_r78580820
--- Diff: iocore/net/SSLCertLookup.cc ---
@@ -346,48 +349,30 @@ int
SSLContextStorage::insert(const char *name, int idx)
{
ats_wildcard_matcher wildcard;
- bool inserted = false;
+ InkHashTableValue value;
+ char lower_case_name[TS_MAX_HOST_NAME_LEN + 1];
if (wildcard.match(name)) {
- // We turn wildcards into the reverse DNS form, then insert them into the trie
- // so that we can do a longest match lookup.
- char namebuf[TS_MAX_HOST_NAME_LEN + 1];
- char *reversed;
- ats_scoped_obj<ContextRef> ref;
-
- reversed = reverse_dns_name(name + 1, namebuf);
- if (!reversed) {
- Error("wildcard name '%s' is too long", name);
- return -1;
+ // Strip the wildcard and store the subdomain
+ const char *subdomain = index(name, '*');
+ if (subdomain) {
+ subdomain = index(subdomain, '.');
--- End diff --
Isn't it required that `subdomain[1] == '.'`?
---
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 #992: TS-4480: Wildcards in certificates should only mat...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/992
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/690/ 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 #992: TS-4480: Wildcards in certificates should only mat...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/992
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/775/ 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 #992: TS-4480: Wildcards in certificates should o...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/992#discussion_r78609413
--- Diff: iocore/net/test_certlookup.cc ---
@@ -88,7 +88,7 @@ REGRESSION_TEST(SSLCertificateLookup)(RegressionTest *t, int /* atype ATS_UNUSED
// Basic hostname cases.
box.check(lookup.find("www.foo.com")->ctx == foo, "host lookup for www.foo.com");
- box.check(lookup.find("www.bar.com")->ctx == all_com, "host lookup for www.bar.com");
+ box.check(lookup.find("www.bar.com") == NULL, "www.bar.com won't match *.com because we only match one level");
--- End diff --
Yes, some mixed case look ups were added for the other branch.
---
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 #992: TS-4480: Wildcards in certificates should only mat...
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on the issue:
https://github.com/apache/trafficserver/pull/992
Ship it.
---
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 #992: TS-4480: Wildcards in certificates should o...
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/992#discussion_r78582875
--- Diff: iocore/net/SSLCertLookup.cc ---
@@ -404,6 +389,7 @@ SSLContextStorage::lookup(const char *name) const
{
InkHashTableValue value;
+ // First look for an exact name match
--- End diff --
Does the lookup do case insensitive search? Or is the input guaranteed to be lower case?
---
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 #992: TS-4480: Wildcards in certificates should o...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/992#discussion_r78609276
--- Diff: iocore/net/SSLCertLookup.cc ---
@@ -404,6 +389,7 @@ SSLContextStorage::lookup(const char *name) const
{
InkHashTableValue value;
+ // First look for an exact name match
--- End diff --
The case insensitive lookups came in a separate PR. Need to rebase this branch to include that.
---
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 #992: TS-4480: Wildcards in certificates should only mat...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/992
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/774/ 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.
---