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 2015/07/18 15:40:57 UTC
[GitHub] trafficserver pull request: TS-3746: Make proxy.config.ssl.client....
GitHub user shinrich opened a pull request:
https://github.com/apache/trafficserver/pull/254
TS-3746: Make proxy.config.ssl.client.verify.server overridable
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shinrich/trafficserver ts-3746-new
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/254.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 #254
----
commit 5eac14db1397aec97ffeaf2186a295d3639bc010
Author: shinrich <sh...@yahoo-inc.com>
Date: 2015-07-18T13:36:34Z
TS-3746: Make proxy.config.ssl.client.verify.server overridable
----
---
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-3746: Make proxy.config.ssl.client....
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/254#issuecomment-122645818
I have a few concerns with the code here actually. In general, we want more configurations overridable, including cache configurations and network configurations. I tried this once before, and it got -1'd for other reasons (cache clustering). What I'm asking for is, is there some better way we can convey the entire "oride" object back to other areas of the system, such as the network layers and cache layers ? It wouldn't be particular efficient to do many of these "special cases" going forward.
Also, there's some legitimate concerns here re: this being a generally good idea, or a fix for an organizational issue. I'll have to noodle on that a bit more (my gut tells me, if you want to enable cert verification for one set of servers, is it that much more work to do it for all servers?).
That much said, a few comments on the patch itself:
1) I think ssl_client_verify_server should be a MgmtByte, moved up to the section of those (to avoid padding), and of course use HttpEstablishStaticConfigByte() to for loading.
2) I don't think this patch was run through clang-format, the indentation doesn't look right.
3) Similarly to 1), but on line 1062 in HttpSM, we introduce two "empty" lines, with 2 white spaces?
---
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-3746: Make proxy.config.ssl.client....
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/254#issuecomment-122602193
Ok, so we should not allow control of the proxy.config.ssl.client.verify.server feature in the plugin because the plugin (remap or otherwise) might do the wrong thing.
So one alternative would be to add another entry to records.config e.g. proxy.config.ssl.client.verify.serverlist which is a list of domain names and/or IP addresses. If set and if the origin's IP or requested SNI is in the list, the verify feature is enabled.
Perhaps @dcarlin would weigh in on this since he requested this feature.
---
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-3746: Make proxy.config.ssl.client....
Posted by sudheerv <gi...@git.apache.org>.
Github user sudheerv commented on the pull request:
https://github.com/apache/trafficserver/pull/254#issuecomment-122599209
+1 to @jpeach 's point - SSL Hostname verification should be associated with an origin and not per remap/transaction.
---
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-3746: Make proxy.config.ssl.client....
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich closed the pull request at:
https://github.com/apache/trafficserver/pull/254
---
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-3746: Make proxy.config.ssl.client....
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/254#issuecomment-122596851
But what you are saying there is that the config *might* do what it says if you have aligned the starts appropriately. I don't think that's good enough. Configurations should work, all the time. Mostly working isn't a desirable state of affairs. Implementing per-origin configuration as per-request and then hoping for the best isn't a precedent that we should be setting.
---
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-3746: Make proxy.config.ssl.client....
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/254#issuecomment-122588703
Yes, you could write a confusing policy via the remap rules. You could have remap rules for two different URLs on the same host with different override values. And as you indicate if session sharing is enabled, you could reuse a server connection that was verified which the matching remap rule indicated that it shouldn't have been (or visa versa).
That would be a fairly odd use case. Since this is a per-origin feature, one would think that you would set the override variables consistently across the origin. You could also write an arbitrary plugin to set the override variable however you like.
I'm open to suggestions for other configuration options to enable origin granularity when controlling the proxy.config.ssl.client.verify.sever feature.
---
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-3746: Make proxy.config.ssl.client....
Posted by ushachar <gi...@git.apache.org>.
Github user ushachar commented on the pull request:
https://github.com/apache/trafficserver/pull/254#issuecomment-122602663
I'm with @jpeach on this one - allowing this to be configurable per transaction doesn't really make sense to me (it's not really like keep-alive -- once you do the validation and allow the connection, that's it - subsequent changes to the config won't have any meaning unless you're planning to implement a 'revalidation' API for existing connections).
Why not just instruct the admin to add the specific server/CA cert to their trusted cert storage?
That's far more secure then adding a hostname/IP based exception, and doesn't require any code change....
---
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-3746: Make proxy.config.ssl.client....
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/254#issuecomment-123091079
Agreed with everyone's notion that the origin servers should just have good certs. And that is what we are working towards. However, we need a short term solution to at least do some checking. We will keep this change local, but since it is an interim solution to a situation ATS should not be promoting we will not try to push it back into the open source.
---
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-3746: Make proxy.config.ssl.client....
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on the pull request:
https://github.com/apache/trafficserver/pull/254#issuecomment-122602554
Whether the verification is per origin is up to the administrator, via his configuration. The remap issue is a distraction, since the underlying issue is plugin control of the verification on a per transaction basis. There seems to be some confusion that this is specific to remap, which is not the case. It is only the Yahoo! use case does this through remap.
I don't see how this argument doesn't apply to something like keep alive, since it is also overrideable and could be set inconsistently via remap configuration. Do you really want to rule out any such value that could potentially cause problems if set via remap?
---
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-3746: Make proxy.config.ssl.client....
Posted by sudheerv <gi...@git.apache.org>.
Github user sudheerv commented on the pull request:
https://github.com/apache/trafficserver/pull/254#issuecomment-122609479
Agree with @ushachar -
Transaction and Session/connection are not interchangeable (at least, not how I see it). Keep-Alive is a *transaction* level property (see more below), whereas, server validation is a *session* level property.
Keep-Alive is allowed to be overridden in ATS, as it is a *HTTP* level property, which is defined/meant-to-be-used per transaction and the corresponding status (via *Connection* HTTP header) exchanged even in every transaction (consequently, it makes perfect sense to be associated per transaction).
OTOH, server cert verification is not a *HTTP* level property, it is a TLS layer property and is applied at a session/connection level and should (can) not clearly be overridden per remap or even within a plugin per transaction.
I'm fine to let that override per origin connection, which obviously requires maintaining separate sessions (verified vs non-verified) if server session sharing is to be supported. To that extent, even if session sharing is not supported to allow to let this feature be overridden per transaction, it still can not be allowed to be overridden per transaction (otherwise, how's that going to work with multiplexed transactions in a given session, if each Txn (in the same session) wants something different)?
---
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.
---