You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by ericcarlschwartz <gi...@git.apache.org> on 2015/04/28 20:29:15 UTC
[GitHub] trafficserver pull request: Ts 3534
GitHub user ericcarlschwartz opened a pull request:
https://github.com/apache/trafficserver/pull/194
Ts 3534
Initial pull request for wiretracing v0. Please leave me your comments and I'll make changes as necessary.
Looking into this now: Making sure we turn tracing off for all connections when we return them to the pool Sudheer pointed this out to me.
Any other input would be greatly appreciated. Some of the feedback from the summit was passed on to me but would always be happy to hear more again. I know there was discussion of refactoring some of this. I also know there was discussion of outputting pcap files instead of traces to a log. <- this one might be hard because the traces are based on what's read from the buffers and not on the individual packets. if someone could point me to code where i could get that kind of granularity that would be a cool enhancement.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/yahoo/trafficserver TS-3534
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/194.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 #194
----
commit d1c4be46ce8ee725c9709b261e850491b00dea9d
Author: es <es...@yahoo-inc.com>
Date: 2014-10-02T19:27:56Z
TS-3534 Wiretracing SSL Connections
Initial Commit
commit 5a2e43cb79bef904b80a1736112c94c1451f0c53
Author: es <es...@yahoo-inc.com>
Date: 2015-04-27T23:04:37Z
TS-3534 Cleanup before PR
----
---
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 3534
Posted by sudheerv <gi...@git.apache.org>.
Github user sudheerv commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-97845539
I agree with @jpeach on the TCP_INFO usage. Are the TCP_INFO traces even part of the wire trace feature? Besides being non-portable, isn't there also (significant?) cost associated with having to call getsockopt on every read/write operation?
FWIR, they were at one point added to debug some post failure issue, and not originally meant to be part of the wire tracing?
---
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 3534
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-119679199
@ericcarlschwartz Nevermind. I just missed it the first time. Looks good.
---
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 3534
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz closed the pull request at:
https://github.com/apache/trafficserver/pull/194
---
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 3534
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-119546802
@ericcarlschwartz that sounds like a reasonable solution. I see that you've done half of it by adding the if !(TS_USE_TLS_SNI) in the handshake code.
You'll need to add the SNI case to ssl_servername_callback and the ssl_cert_callback to cover openssl 1.0.1 and 1.0.2+
---
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 3534
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-119295104
Ah yes I remember that part of things in terms of what's stored where, was just wondering how that worked exactly since the ssl_vc is in the faux transaction and (for spdy at least) the protocol selection is part of the handshake w/ NPN. I just ran a test w/ spdycat and see what's happening now a little better
---
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 3534
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-105570715
Well if you can configure logging on the ATS server, then it seems reasonable that you would have access to the SSL keys.
---
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 3534
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-97840771
I only looked quickly, but just a couple of points. The use of ``TCP_INFO`` is non-portable so this feature will need to be conditional. Why would we have our own tracing framework? There's already dtrace, systemtap and probably others. We should integrate with them rather than invent something that is less complete.
---
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 3534
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-119674972
@shinrich I added it to set_context_cert because it looks like both the ssl_servername_callback and ssl_cert_callback will call that and it saves some code duplication. is that ok? or should I move it to each?
---
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 3534
Posted by ushachar <gi...@git.apache.org>.
Github user ushachar commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-97664982
It would be better to move all the 'when to debug' logic into plugin space -- you can extend the TSHttpSsnDebugSet API, hook a plugin on the SNI callback and do whatever you want from there instead of adding more code to the already overcomplicated VConn layer.
This will also allow you to turn on debugging for specific requests (based on full URLs/headers/whatever) sort of like the xdebug plugin.
---
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 3534
Posted by RolandZink <gi...@git.apache.org>.
Github user RolandZink commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-105575681
With perfect forward secrecy getting the necessary keys becomes more difficult. I made some better experience by getting the session keys from the browsers. Some (Chrome and Firefox) support storing the session keys into a file and Wireshark can read the file. What needs to be done is setting the SSLKEYLOGFILE environment variable, see for example https://jimshaver.net/2015/02/11/decrypting-tls-browser-traffic-with-wireshark-the-easy-way/.
Maybe ATS can write a similar file?
---
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 3534
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-119973950
@ushachar I agree with your comment about making the computeSSLTrace decision accessible from the plugin. I read your earlier comment too quickly and thought you wanted to put all the trace logic into a plugin.
Having some simple controls in records.config seems reasonable (like the current single name, single IP, and percentage). But then we can give control to a plugin to make more elaborate decisions. Perhaps we could file another bug to track a new PluginAPI or an extension to the TSHttpSsnDebugSet API.
Also agree that a macro might eliminate some code redundancy.
---
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 3534
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-105575893
@sudheerv: the TCP_INFO usage can definitely be dropped because of the associated cost/its only situational usefulness. i will prepare a patch with it removed.
---
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 3534
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-105638387
I'm afraid I'm being a bit dense, but where is sslServerName ever set to a non-null value?
---
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 3534
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-119261713
The SNI name check looks better. A couple comments remaining.
computeSSLTrace() might be getting called more than once per transaction. You are probably better off pull the block that calls computeSSLTrace() and setSSLTrace() into the if(this->ssl == NULL) block and call that after the make_ssl_connection call. Save a bit of unnecessary work.
The other comment is about the client_vc and server_vc manipulations in HttpSM. Your casts are not going to work in the H2 and SPDY cases. The server_vc will be ok, but the client_vc is a PluginVC in those cases. Fortunately, you don't de-reference the client_vc only dynamic_cast it, so the code is safe, but your tracing will be off for SPDY/H2. I know that you are working on those issues for another project, so the SPDY/H2 wiretracing support can be updated later.
---
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 3534
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-119275969
@shinrich ah ok thanks for the comments. I'll make that first change today.
w.r.t. the second comment will have to think more about it. because we only do the cast on the client_vc which is where H2 and SPDY are I think there might be a way to refactor since PluginVC and SSLNetVC both share the NetVC base class. A fun project for later you're right.
Just for some clarification, if a PluginVC is not an SSLNetVC, how is the SSLHandshake handled in SPDY/H2 cases?
---
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 3534
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-120092663
@ushachar @shinrich I tend to agree that having some simple stuff available as records.config settings for those who don't want to write a new plugin each time they want to enable traces is a good call.
But being able to enable tracing via a plugin also seems valuable and it shouldn't be hard to extend or modify computeSSLTrace to that effect. If someone wants to open a JIRA for doing that and assign it to me, that's work I'd be happy to take on.
---
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 3534
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-97163497
Right now this PR is multiple commits. I'll probably be adding to it as people have suggestions for improvement. I can squash stuff down before anything gets merged :)
---
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 3534
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-105569519
Wireshark decrypts SSL, https://wiki.wireshark.org/SSL.
---
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 3534
Posted by sudheerv <gi...@git.apache.org>.
Github user sudheerv commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-105570499
Yes, but, you would need to know/use the cert to do that, which is not always straightforward - as I said, "the problem with existing tools is that they can not (easily) decrypt the SSL traffic. "
---
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 3534
Posted by ushachar <gi...@git.apache.org>.
Github user ushachar commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-119945084
I still think all the SSLNetVConnection::computeSSLTrace() logic should be moved into plugin space - even if not extending TSHttpSsnDebugSet and keeping this as a standalone option.
Also - how about a macro to unify lines like these:
"""
Debug("ssl.error", "SSLNetVConnection::sslClientHandShakeEvent, SSL_ERROR_WANT_READ");
TraceIn(trace, get_remote_addr(), get_remote_port(), "SSL client handshake ERROR_WANT_READ");
"""
---
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 3534
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-120100458
@shinrich rebased off apache/master and opened a new PR for this here. this one had some weirdness because of issues w/ apache/master and yahoo/master being out of sync
New PR is here for all to look at/comment on: https://github.com/apache/trafficserver/pull/246
---
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 3534
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-112873930
https://issues.apache.org/jira/browse/TS-3534
---
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 3534
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-120036935
I think @ushachar is suggesting that this whole feature should be in a plugin?
---
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 3534
Posted by ushachar <gi...@git.apache.org>.
Github user ushachar commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-120083030
I think moving the entire "when to debug" logic to plugin space would be a better idea -- yes.
Mostly in order to simplify the core code, but also to allow someone to easily extend it without treading into the 'here be dragons' area of the NetVCs :-)
There's already a hookpoint in the handshake phase -- all we'd need to do would be to add the plugin + API to set the flag....
---
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 3534
Posted by RolandZink <gi...@git.apache.org>.
Github user RolandZink commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-105592140
The log contains more than one key. When the browser writes it then there is only one client but more than one server and wireshark can decrypt all the connections. Not sure if wireshark will be happy to have all keys for all ATS clients in one file.
---
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 3534
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-119344998
@shinrich: did some testing there. the problem w/ putting it right in that particular spot is that the return value from SSL_get_servername call will always be null. Looks like we don't have that information from the SSL object yet.
I could move the call into SSLUtils where we're getting the cert for SNI anyway?
---
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 3534
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-105585945
I've dropped the tcp_info traces.
My understanding was in line with @sudheerv's that this was never intended as a total replacement to using wireshark, but rather an easier to use method that could cover many cases.
I'll look more into Roland's proposal. I haven't worked with chrome or firefox SSL key log files before. Do you only feed wireshark only the key for the corresponding client session or does the log contain all the symmetric keys generated for each client?
---
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 3534
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-105593729
@RolandZink, yeah that's what I was thinking. I will do some searching and see if something like that's been done before and how wireshark will behave with all the ATS clients' keys.
---
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 3534
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-119289670
For SPDY and H2 the underlying SSLNetVConnection is stored with the SPDYClient and the Http2Client respectively. Multiple faux Http1.1 transactions are created and sent via FetchSM/PluginVC. I think you were working this this issue with the logging changes you are 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: Ts 3534
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-115050303
@shinrich I think this is ready for another look if you or someone else has some time.
---
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 3534
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-119345316
and for the non SNI-enabled case I can put it in the equivalent function in the corresponding #else block
---
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 3534
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on the pull request:
https://github.com/apache/trafficserver/pull/194#issuecomment-119357789
@shinrich here's a proposed solution:
if TS_USE_TLS_SNI isn't set then we put the logic where you'd suggested. if it is, we set the trace right after we lookup the servername
---
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.
---