You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by shinrich <gi...@git.apache.org> on 2016/04/18 20:51:57 UTC
[GitHub] trafficserver pull request: TS-4180: Support for multiple intermed...
GitHub user shinrich opened a pull request:
https://github.com/apache/trafficserver/pull/578
TS-4180: Support for multiple intermediate cert chains
Previous fix parsed the comma separated file names, but would only load the first file.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shinrich/trafficserver ts-4180
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/578.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 #578
----
commit 25cde3c99f655781060639ca3038059622adbc77
Author: shinrich <sh...@yahoo-inc.com>
Date: 2016-04-18T18:50:29Z
TS-4180: Support for multiple intermediate cert chains
----
---
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: TS-4180: Support for multiple intermed...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/578#issuecomment-215163284
Yes, the current test is wrong. But it passed because the previous code was wrong. The correct behavior depends on what version of openssl the test is run against. Can we test for that and adjust the result accordingly?
---
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: TS-4180: Support for multiple intermed...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/578#issuecomment-216679391
I'll plan on working with @jacksontj at the summit next week to get the TSQA and openssl version issue figured out.
---
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: TS-4180: Support for multiple intermed...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/578#issuecomment-211685166
Seems reasonable to me 👍
---
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: TS-4180: Support for multiple intermed...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/578#issuecomment-215194735
Making the test more robust sounds like the right approach to me.
---
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: TS-4180: Support for multiple intermed...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafficserver/pull/578
---
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: TS-4180: Support for multiple intermed...
Posted by JamesPeach <gi...@git.apache.org>.
Github user JamesPeach commented on the pull request:
https://github.com/apache/trafficserver/pull/578#issuecomment-215151224
So this patch skips the only test we have for this stuff?
---
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: TS-4180: Support for multiple intermed...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/578#issuecomment-211695320
Can you please update https://github.com/apache/trafficserver/blob/master/ci/tsqa/tests/test_https.py to test this 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: TS-4180: Support for multiple intermed...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/578#issuecomment-212078480
Well looking at the existing tests anyway told me that my initial solution was wrong. We are moving into the area where behavior changes between 1.0.2 and 1.0.1. I just spent a couple hours with my 1.0.2 build and got this scenario working correctly in 1.0.2 (only return the cipher appropriate main cert (true in 1.0.1 and 1.0.2) and only return cipher appropriate intermediate cert (true only for 1.0.2 in 1.0.1 all intermediate certs will be returned regardless of cipher type because there is only one chain)).
The existing test checks for the number of certs returned in the chain and expects 2. This is true in the original code because we only ever added the first cert in the ssl_ca_name list. This series of code fixes changes this logic and the behavior changes depending on which openssl version is in use. For 1.0.1 three certs will be in the chain (base cert and both intermediates). For 1.0.2 two certs will be in the chain (base cert and intermediate of the appropriate type).
@jacksontj do we specify different versions of openssl to test against? Or should we make our test results conditional based on the version that happens to be present in the test environment.
Also, should these decisions be made compile time? Or should we try to be clever at run time and use the 1.0.2 support if we are linking against 1.0.2 and gracefully fallback to the 1.0.1 API otherwise.
For the moment, I'll add compile time checks and push my changes 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 pull request: TS-4180: Support for multiple intermed...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/578#issuecomment-215141283
I added SkipTest for the TestMix test case. We need to add logic to only run that test if openssl 1.0.2 is present. Perhaps we can hash that out in the ATS summit. In the meantime, I'd like to go ahead and get this pushed up if all are agreeable.
---
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.
---