You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2020/07/03 17:31:44 UTC
[GitHub] [trafficserver] bneradt opened a new pull request #6972: Traffic Dump: dump server-side protocol stack
bneradt opened a new pull request #6972:
URL: https://github.com/apache/trafficserver/pull/6972
This also adds some TLS information in the protocol stack, such as the
verify_mode applied to the SSL session and whether the server requested
a client certificate.
With this change, there are now two kinds of protocol stacks that are
dumped. There is a session-level one that describes the client-side
protocol stack. It can look something like this:
"sessions": [
{
"connection-time": 1593796698149511642,
"protocol": {
"h2": {},
"ipv4": {},
"tcp": {},
"tls": {
"proxy-provided-cert": true,
"proxy-verify-mode": 0,
"sni": "www.tls.com",
"version": "TLSv1.2"
}
},
"transactions": [
...
In addition, there is now a server-side protocol stack that will be printed in
every proxy-request node. It can look something like this:
"protocol": {
"http/1.1": {},
"ipv4": {},
"tcp": {},
"tls": {
"proxy-provided-cert": false,
"proxy-verify-mode": 1,
"sni": "www.tls.com",
"version": "TLSv1.2"
}
},
Note that the dump provides TLS information from the perspective of the proxy,
therefore the TLS nodes are prefixed by "proxy-". The schema is also updated to
anticipate verifier directives that dictate client and server TLS behavior
("verify-mode" and "request-certificate").
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #6972: Traffic Dump: dump server-side protocol stack
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #6972:
URL: https://github.com/apache/trafficserver/pull/6972#issuecomment-655099721
> Thanks for the update, but that's not my point unfortunately. I'm saying "protocol" should be an array.
> https://stackoverflow.com/questions/7214293/is-the-order-of-elements-in-a-json-list-preserved
Ah, that makes sense. Sorry, I was thinking that you were pointing out that the map elements weren't ordered according to the protocol layers.
I think you're absolutely right. Making the protocol an array is the right thing to do. I've done that, using the scheme you described, in the latest commit.
Thanks for your feedback, I think this improves things a lot.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #6972: Traffic Dump: dump server-side protocol stack
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #6972:
URL: https://github.com/apache/trafficserver/pull/6972#issuecomment-655183428
> > Thanks for the update, but that's not my point unfortunately. I'm saying "protocol" should be an array.
> > https://stackoverflow.com/questions/7214293/is-the-order-of-elements-in-a-json-list-preserved
>
> Ah, that makes sense. Sorry, I was thinking that you were pointing out that the map elements weren't ordered according to the protocol layers.
>
> I think you're absolutely right. Making the protocol an array is the right thing to do. I've done that, using the scheme you described, in the latest commit.
>
> Thanks for your feedback, I think this improves things a lot.
Note that I updated the PR comment with a description of what the protocol nodes look like now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #6972: Traffic Dump: dump server-side protocol stack
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #6972:
URL: https://github.com/apache/trafficserver/pull/6972#issuecomment-655250573
> The output looks good to me now. Thank you for updating. I haven't looked into the detail yet but I left a few comments.
>
> Since you're adding new TS APIs, please follow the API review process below.
> https://cwiki.apache.org/confluence/display/TS/API+Review+Process
Thank you Masakazu. I wasn't aware of this review process. I sent an email to the dev list concerning these functions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #6972: Traffic Dump: dump server-side protocol stack
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #6972:
URL: https://github.com/apache/trafficserver/pull/6972#discussion_r451871100
##########
File path: src/traffic_server/InkAPI.cc
##########
@@ -9670,6 +9696,27 @@ TSHttpSsnClientProtocolStackGet(TSHttpSsn ssnp, int n, const char **result, int
return TS_SUCCESS;
}
+// Return information about the protocols used by the server
+TSReturnCode
+TSHttpTxnServerProtocolStackGet(TSHttpTxn txnp, int n, const char **result, int *actual)
+{
+ sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
+ sdk_assert(n == 0 || result != nullptr);
+ HttpSM *sm = reinterpret_cast<HttpSM *>(txnp);
+ int count = 0;
+ if (sm && n > 0) {
+ auto mem = static_cast<std::string_view *>(alloca(sizeof(std::string_view) * n));
Review comment:
Aha, sorry I mistook alloca for calloc. I wasn't worried about use-after-free, but uninitialized string_view. I'm ok with it as it is for now since the logic is not new, but I think classes should not be used like this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #6972: Traffic Dump: dump server-side protocol stack
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #6972:
URL: https://github.com/apache/trafficserver/pull/6972#issuecomment-654397966
> > You say protocol stack but you don't care the order?
>
> Oh, good point. The JSON pretty printer I used changed the order around. I'll fix that.
>
> That said, the way I wrote the plugin, the "tls" node is handled separately and is always placed at the end. I'll see whether I can fix that.
With the latest commit, the stack now looks like the following (note that the protocols are in order, from higher to lower layers):
```
"sessions": [
{
"protocol": {
"h2": {},
"tls": {
"version": "TLSv1.2",
"sni": "www.tls.com",
"proxy-verify-mode": 0,
"proxy-provided-cert": true
},
"tcp": {},
"ipv4": {}
},
```
And the server side:
```
"proxy-request": {
"protocol": {
"http/1.1": {},
"tls": {
"version": "TLSv1.2",
"sni": "www.tls.com",
"proxy-verify-mode": 1,
"proxy-provided-cert": false
},
"tcp": {},
"ipv4": {}
},
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #6972: Traffic Dump: dump server-side protocol stack
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #6972:
URL: https://github.com/apache/trafficserver/pull/6972#issuecomment-654532239
Thanks for the update, but that's not my point unfortunately. I'm saying "protocol" should be an array.
https://stackoverflow.com/questions/7214293/is-the-order-of-elements-in-a-json-list-preserved
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #6972: Traffic Dump: dump server-side protocol stack
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #6972:
URL: https://github.com/apache/trafficserver/pull/6972#discussion_r451212168
##########
File path: src/traffic_server/InkAPI.cc
##########
@@ -9670,6 +9696,27 @@ TSHttpSsnClientProtocolStackGet(TSHttpSsn ssnp, int n, const char **result, int
return TS_SUCCESS;
}
+// Return information about the protocols used by the server
+TSReturnCode
+TSHttpTxnServerProtocolStackGet(TSHttpTxn txnp, int n, const char **result, int *actual)
+{
+ sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
+ sdk_assert(n == 0 || result != nullptr);
+ HttpSM *sm = reinterpret_cast<HttpSM *>(txnp);
+ int count = 0;
+ if (sm && n > 0) {
+ auto mem = static_cast<std::string_view *>(alloca(sizeof(std::string_view) * n));
Review comment:
This might work, but I think string_view's constructor is not called and some initialization in the constructor might be skipped.
Also, where is the allocated memory freed?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #6972: Traffic Dump: dump server-side protocol stack
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #6972:
URL: https://github.com/apache/trafficserver/pull/6972#discussion_r451245597
##########
File path: src/traffic_server/InkAPI.cc
##########
@@ -9670,6 +9696,27 @@ TSHttpSsnClientProtocolStackGet(TSHttpSsn ssnp, int n, const char **result, int
return TS_SUCCESS;
}
+// Return information about the protocols used by the server
+TSReturnCode
+TSHttpTxnServerProtocolStackGet(TSHttpTxn txnp, int n, const char **result, int *actual)
+{
+ sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
+ sdk_assert(n == 0 || result != nullptr);
+ HttpSM *sm = reinterpret_cast<HttpSM *>(txnp);
+ int count = 0;
+ if (sm && n > 0) {
+ auto mem = static_cast<std::string_view *>(alloca(sizeof(std::string_view) * n));
Review comment:
The memory from `alloca` is freed on function return automatically:
https://linux.die.net/man/3/alloca
That might look problematic on the surface, but note that `mem` is only ultimate used to get the data() members of the statically allocated global string_view tags (IP_PROTO_TAG_HTTP_2_0, etc.) which are assigned to the caller's result char**. Thus I don't see a use after free or leak here. Also, the string_view constructor isn't needed because these wind up all just pointing to those global string_view tags.
Of course, let me know if I'm missing something.
(In case this is helpful, I didn't come up with this on my own. This function's logic is borrowed from the existing TSHttpSsnClientProtocolStackGet implementation, with modifications to reference the state machine function.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #6972: Traffic Dump: dump server-side protocol stack
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #6972:
URL: https://github.com/apache/trafficserver/pull/6972#issuecomment-653971064
You say protocol stack but you don't care the order?
What's the key for protocol? ALPN id? Protocol name? Protocol name with version?
I'd define the format like this:
```json
"protocol": [
{
"name": "ip",
"version" : "4"
},
{
"name": "tcp"
},
{
"name": "tls",
"version" : "1.2",
"alpn": "h2",
"sni": "example.com"
},
{
"name": "http",
"version": "2"
},
]
```
Also, I'd be curious what the format for QUIC would be like. It could be "udp", "quic" and "tls", but technically "tls" is not on top of "quic".
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #6972: Traffic Dump: dump server-side protocol stack
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #6972:
URL: https://github.com/apache/trafficserver/pull/6972#discussion_r451209096
##########
File path: plugins/experimental/traffic_dump/session_data.cc
##########
@@ -34,6 +37,148 @@ namespace
{
/** The final string used to close a JSON session. */
char const constexpr *const json_closing = "]}]}";
+
+/**
+ * A mapping from IP_PROTO_TAG to the string describing the JSON protocol node.
+ */
+std::unordered_map<std::string, std::string> tag_to_node = {
+ {std::string(IP_PROTO_TAG_IPV4), R"("name":"ip","version":"4")"},
+ {std::string(IP_PROTO_TAG_IPV6), R"("name":"ip","version":"6")"},
+
+ {std::string(IP_PROTO_TAG_TCP), R"("name":"tcp")"},
+ {std::string(IP_PROTO_TAG_UDP), R"("name":"udp")"},
+
+ {std::string(IP_PROTO_TAG_QUIC), R"("name:":"quic")"},
+
+ {std::string(IP_PROTO_TAG_TLS_1_0), R"("name":"tls","version":"1.0")"},
+ {std::string(IP_PROTO_TAG_TLS_1_1), R"("name":"tls","version":"1.1")"},
+ {std::string(IP_PROTO_TAG_TLS_1_2), R"("name":"tls","version":"1.2")"},
+ {std::string(IP_PROTO_TAG_TLS_1_3), R"("name":"tls","version":"1.3")"},
+
+ {std::string(IP_PROTO_TAG_HTTP_0_9), R"("name":"http","version":"0.9")"},
+ {std::string(IP_PROTO_TAG_HTTP_1_0), R"("name":"http","version":"1.0")"},
+ {std::string(IP_PROTO_TAG_HTTP_1_1), R"("name":"http","version":"1.1")"},
+ {std::string(IP_PROTO_TAG_HTTP_2_0), R"("name":"http","version":"2")"},
+
+ {std::string(IP_PROTO_TAG_HTTP_QUIC), R"("name":"http","version":"0.9")"},
+ {std::string(IP_PROTO_TAG_HTTP_QUIC), R"("name":"http","version":"3")"},
Review comment:
IP_PROTO_TAG_HTTP_**3**
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #6972: Traffic Dump: dump server-side protocol stack
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #6972:
URL: https://github.com/apache/trafficserver/pull/6972#issuecomment-655206848
The output looks good to me now. Thank you for updating. I haven't looked into the detail yet but I left a few comments.
Since you're adding new TS APIs, please follow the API review process below.
https://cwiki.apache.org/confluence/display/TS/API+Review+Process
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #6972: Traffic Dump: dump server-side protocol stack
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #6972:
URL: https://github.com/apache/trafficserver/pull/6972#issuecomment-654275807
[approve ci]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall merged pull request #6972: Traffic Dump: dump server-side protocol stack
Posted by GitBox <gi...@apache.org>.
randall merged pull request #6972:
URL: https://github.com/apache/trafficserver/pull/6972
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #6972: Traffic Dump: dump server-side protocol stack
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #6972:
URL: https://github.com/apache/trafficserver/pull/6972#discussion_r451245597
##########
File path: src/traffic_server/InkAPI.cc
##########
@@ -9670,6 +9696,27 @@ TSHttpSsnClientProtocolStackGet(TSHttpSsn ssnp, int n, const char **result, int
return TS_SUCCESS;
}
+// Return information about the protocols used by the server
+TSReturnCode
+TSHttpTxnServerProtocolStackGet(TSHttpTxn txnp, int n, const char **result, int *actual)
+{
+ sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
+ sdk_assert(n == 0 || result != nullptr);
+ HttpSM *sm = reinterpret_cast<HttpSM *>(txnp);
+ int count = 0;
+ if (sm && n > 0) {
+ auto mem = static_cast<std::string_view *>(alloca(sizeof(std::string_view) * n));
Review comment:
The memory from `alloca` is freed on function return automatically:
https://linux.die.net/man/3/alloca
That might look problematic on the surface, but note that `mem` is only ultimate used to get the data() members of the statically allocated global string_view tags (IP_PROTO_TAG_HTTP_2_0, etc.) which are assigned to the caller's result char**. Thus I don't see a use after free or leak here. Also, the string_view constructor isn't needed because these wind up all just pointing to those global string_view tags.
(In case this is helpful, I didn't come up with this on my own. This function's logic is borrowed from the existing TSHttpSsnClientProtocolStackGet implementation, with modifications to reference the state machine function.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #6972: Traffic Dump: dump server-side protocol stack
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #6972:
URL: https://github.com/apache/trafficserver/pull/6972#issuecomment-654295382
> You say protocol stack but you don't care the order?
Oh, good point. The JSON pretty printer I used changed the order around. I'll fix that.
That said, the way I wrote the plugin, the "tls" node is handled separately and is always placed at the end. I'll see whether I can fix that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org