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 2022/09/21 02:12:35 UTC
[GitHub] [trafficserver] maskit opened a new pull request, #9101: Fix handling of OPTIONS request
maskit opened a new pull request, #9101:
URL: https://github.com/apache/trafficserver/pull/9101
When I worked on #9094, I found that OPTIONS request (asterisk-form) is not handled correctly either.
HTTPHdr treats anything between request method and "HTTP/1.x" as URL, and it tries to parse the surrounded part by `url_parse`. When a request is asterisk-form (i.e. `OPTIONS * HTTP/1.1`) The parsing fails because "*" is not a valid URL.
This PR adds a special handling of asterisk-form. The change is small but it's a bit hacky, so I'm not going to strongly push this change and any feedback is welcome, but this allows users to proxy OPTIONS requests.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9101: Fix handling of OPTIONS request
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #9101:
URL: https://github.com/apache/trafficserver/pull/9101#discussion_r982022715
##########
proxy/hdrs/URL.cc:
##########
@@ -31,6 +31,8 @@
#include "HTTP.h"
#include "tscore/Diags.h"
+const char URLImpl::ASTERISK = '\xff';
Review Comment:
See the reply to Chris's comment above.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9101: Fix handling of OPTIONS request
Posted by GitBox <gi...@apache.org>.
maskit commented on PR #9101:
URL: https://github.com/apache/trafficserver/pull/9101#issuecomment-1281771997
> But I don't think ATS knows if it's making a forward-proxy request. You can configure it to make an absolute-uri request, but that could be a forward proxy or direct origin request, there's no way to know that from the config.
Isn't that a protocol violation? If ATS is making an absolute-form request, that must be a request for a proxy.
[3.2.1](https://datatracker.ietf.org/doc/html/rfc9112#section-3.2.1). origin-form
> When making a request directly to an origin server, other than a
CONNECT or server-wide OPTIONS request (as detailed below), a client
MUST send only the absolute path and query components of the target
URI as the request-target.
[3.2.2](https://datatracker.ietf.org/doc/html/rfc9112#section-3.2.2). absolute-form
> When making a request to a proxy, other than a CONNECT or server-wide
OPTIONS request (as detailed below), a client MUST send the target
URI in "absolute-form" as the request-target.
I'm not familiar with parent.config/strategies.yaml so I'm unsure if ATS can tell a request is for a proxy or an origin server, but we should make it possible to be compliant with the spec.
> To be clear, I wasn't objecting, just looking for input. This PR is an improvement, regardless of RFC9112§3.2.4.
Ok, I'm going to update the code as per @SolidWallOfCode 's suggestion if we are fine with the hacky special character.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9101: Fix handling of OPTIONS request
Posted by GitBox <gi...@apache.org>.
maskit commented on PR #9101:
URL: https://github.com/apache/trafficserver/pull/9101#issuecomment-1282041841
> The absolute-form is permitted to origins for normal requests.
Hmm, I wouldn't say it's permitted. The paragraph you quoted is for server side, and I'd read it as "a server must be permissive". It just defines what to do for the case an origin server receives an absolute-form request from a misbehaving client. Like the paragraph I quoted says, a client must use origin-form for normal requests for origin servers.
> I don't think it's possible to always make ATS automatically detect and do the right thing. Because we don't have the context to know if a parent is an origin or another proxy.
I'm trying to say let's make ATS a well-behaving client. As a server, it's impossible to know whether a client sent an absolute-form request to the server on purpose or not. We can't do anything for that. But as a client, ATS should always use an appropriate form. If an ATS operator configures ATS to send an absolute-form request to an origin server, the configuration is simply wrong even if the target server kindly accepts the request. In other words, a target server is not a proxy server if ATS is sending origin-form request to the server and the ATS is not a forward proxy in the middle of a chain (it could be the last one or a reverse proxy), unless the configuration is wrong.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on pull request #9101: Fix handling of OPTIONS request
Posted by GitBox <gi...@apache.org>.
rob05c commented on PR #9101:
URL: https://github.com/apache/trafficserver/pull/9101#issuecomment-1258776843
Curious, what do you think about https://datatracker.ietf.org/doc/html/rfc9112#section-3.2.4
```
If a proxy receives an OPTIONS request with an absolute-form of
request-target in which the URI has an empty path and no query
component, then the last proxy on the request chain MUST send a
request-target of "*" when it forwards the request to the indicated
origin server.
For example, the request
OPTIONS http://www.example.org:8001/ HTTP/1.1
would be forwarded by the final proxy as
OPTIONS * HTTP/1.1
Host: www.example.org:8001
after connecting to port 8001 of host "www.example.org".
```
?
It seems to me that we're violating the spec there. I think we're just not doing that replacement. And it's a `MUST`, so it's a strict spec violation.
But how do we even know if we're the last proxy in the chain? Is there any way in the protocol or request or remap line to determine that? I'm not seeing one. Which means there's no way we can actually fulfill the spec? Unless we add some kind of remap or parent line or plugin or something to configure "remap is last proxy", that users would just have to manually set? Seems kind of terrible
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9101: Fix handling of OPTIONS request
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #9101:
URL: https://github.com/apache/trafficserver/pull/9101#discussion_r982019562
##########
proxy/hdrs/HTTP.cc:
##########
@@ -1133,7 +1133,12 @@ http_parser_parse_req(HTTPParser *parser, HdrHeap *heap, HTTPHdrImpl *hh, const
ink_assert(hh->u.req.m_url_impl != nullptr);
url = hh->u.req.m_url_impl;
- err = ::url_parse(heap, url, &url_start, url_end, must_copy_strings, strict_uri_parsing);
+ if (method_wks_idx == HTTP_WKSIDX_OPTIONS && url_end - url_start == 1 && url_start[0] == '*') {
Review Comment:
In URLImpl, having '*'in the path means the URL is '/*'. The magic value is needed to distinguish the two.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9101: Fix handling of OPTIONS request
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #9101:
URL: https://github.com/apache/trafficserver/pull/9101#discussion_r982019562
##########
proxy/hdrs/HTTP.cc:
##########
@@ -1133,7 +1133,12 @@ http_parser_parse_req(HTTPParser *parser, HdrHeap *heap, HTTPHdrImpl *hh, const
ink_assert(hh->u.req.m_url_impl != nullptr);
url = hh->u.req.m_url_impl;
- err = ::url_parse(heap, url, &url_start, url_end, must_copy_strings, strict_uri_parsing);
+ if (method_wks_idx == HTTP_WKSIDX_OPTIONS && url_end - url_start == 1 && url_start[0] == '*') {
Review Comment:
In URLImpl, having "\*" in the path means the URL is '/\*'. The magic value is needed to distinguish the two.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9101: Fix handling of OPTIONS request
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9101:
URL: https://github.com/apache/trafficserver/pull/9101#discussion_r980594163
##########
proxy/hdrs/HTTP.cc:
##########
@@ -1133,7 +1133,12 @@ http_parser_parse_req(HTTPParser *parser, HdrHeap *heap, HTTPHdrImpl *hh, const
ink_assert(hh->u.req.m_url_impl != nullptr);
url = hh->u.req.m_url_impl;
- err = ::url_parse(heap, url, &url_start, url_end, must_copy_strings, strict_uri_parsing);
+ if (method_wks_idx == HTTP_WKSIDX_OPTIONS && url_end - url_start == 1 && url_start[0] == '*') {
+ url->set_path(heap, &URLImpl::ASTERISK, 1, true);
+ err = PARSE_RESULT_DONE;
Review Comment:
Why not `return PARSE_RESULT_DONE;`?
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9101: Fix handling of OPTIONS request
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #9101:
URL: https://github.com/apache/trafficserver/pull/9101#discussion_r982009209
##########
proxy/hdrs/HTTP.cc:
##########
@@ -1133,7 +1133,12 @@ http_parser_parse_req(HTTPParser *parser, HdrHeap *heap, HTTPHdrImpl *hh, const
ink_assert(hh->u.req.m_url_impl != nullptr);
url = hh->u.req.m_url_impl;
- err = ::url_parse(heap, url, &url_start, url_end, must_copy_strings, strict_uri_parsing);
+ if (method_wks_idx == HTTP_WKSIDX_OPTIONS && url_end - url_start == 1 && url_start[0] == '*') {
+ url->set_path(heap, &URLImpl::ASTERISK, 1, true);
+ err = PARSE_RESULT_DONE;
Review Comment:
The original code doesn't do it either.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9101: Fix handling of OPTIONS request
Posted by GitBox <gi...@apache.org>.
maskit commented on PR #9101:
URL: https://github.com/apache/trafficserver/pull/9101#issuecomment-1263039167
@rob05c I think that's for forward proxy. A forward proxy should always know whether it uses another forward proxy. The spec says below on section 3.2.2, and if it applies to revers proxies, all major browsers are violating the spec.
> When making a request to a proxy, other than a CONNECT or server-wide
OPTIONS request (as detailed below), a client MUST send the target
URI in "absolute-form" as the request-target.
I'm not sure if ATS can use another forward proxy to serve a request, but if it's impossible, ATS is always the last (forward) proxy in the chain and we need to do the replacement.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] cmcfarlen commented on a diff in pull request #9101: Fix handling of OPTIONS request
Posted by GitBox <gi...@apache.org>.
cmcfarlen commented on code in PR #9101:
URL: https://github.com/apache/trafficserver/pull/9101#discussion_r981654188
##########
proxy/hdrs/HTTP.cc:
##########
@@ -1133,7 +1133,12 @@ http_parser_parse_req(HTTPParser *parser, HdrHeap *heap, HTTPHdrImpl *hh, const
ink_assert(hh->u.req.m_url_impl != nullptr);
url = hh->u.req.m_url_impl;
- err = ::url_parse(heap, url, &url_start, url_end, must_copy_strings, strict_uri_parsing);
+ if (method_wks_idx == HTTP_WKSIDX_OPTIONS && url_end - url_start == 1 && url_start[0] == '*') {
Review Comment:
Why do you use the magic `ASTERISK` value rather than just check for `*`?
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9101: Fix handling of OPTIONS request
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9101:
URL: https://github.com/apache/trafficserver/pull/9101#discussion_r980592286
##########
proxy/hdrs/URL.cc:
##########
@@ -31,6 +31,8 @@
#include "HTTP.h"
#include "tscore/Diags.h"
+const char URLImpl::ASTERISK = '\xff';
Review Comment:
'*' is 0xff?
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9101: Fix handling of OPTIONS request
Posted by GitBox <gi...@apache.org>.
maskit commented on PR #9101:
URL: https://github.com/apache/trafficserver/pull/9101#issuecomment-1263030713
@rob05c I'd
> When making a request to a proxy, other than a CONNECT or server-wide
OPTIONS request (as detailed below), a client MUST send the target
URI in "absolute-form" as the request-target.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9101: Fix handling of OPTIONS request
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #9101:
URL: https://github.com/apache/trafficserver/pull/9101#discussion_r982019562
##########
proxy/hdrs/HTTP.cc:
##########
@@ -1133,7 +1133,12 @@ http_parser_parse_req(HTTPParser *parser, HdrHeap *heap, HTTPHdrImpl *hh, const
ink_assert(hh->u.req.m_url_impl != nullptr);
url = hh->u.req.m_url_impl;
- err = ::url_parse(heap, url, &url_start, url_end, must_copy_strings, strict_uri_parsing);
+ if (method_wks_idx == HTTP_WKSIDX_OPTIONS && url_end - url_start == 1 && url_start[0] == '*') {
Review Comment:
In URLImpl, having "\*" in the path means the URL is "/\*". The magic value is needed to distinguish the two.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9101: Fix handling of OPTIONS request
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #9101:
URL: https://github.com/apache/trafficserver/pull/9101#discussion_r982019562
##########
proxy/hdrs/HTTP.cc:
##########
@@ -1133,7 +1133,12 @@ http_parser_parse_req(HTTPParser *parser, HdrHeap *heap, HTTPHdrImpl *hh, const
ink_assert(hh->u.req.m_url_impl != nullptr);
url = hh->u.req.m_url_impl;
- err = ::url_parse(heap, url, &url_start, url_end, must_copy_strings, strict_uri_parsing);
+ if (method_wks_idx == HTTP_WKSIDX_OPTIONS && url_end - url_start == 1 && url_start[0] == '*') {
Review Comment:
In URLImpl, having "*" in the path means the URL is "/*". The magic value is needed to distinguish the two.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on pull request #9101: Fix handling of OPTIONS request
Posted by GitBox <gi...@apache.org>.
rob05c commented on PR #9101:
URL: https://github.com/apache/trafficserver/pull/9101#issuecomment-1281619482
> I'm not sure if ATS can use another forward proxy to serve a request
It can, via parent.config/strategies.yaml. We use multiple chained ATS forward-proxies in our CDN.
> I think that's for forward proxy. A forward proxy should always know whether it uses another forward proxy
Ahh, I think you're right. But I don't think ATS knows if it's making a forward-proxy request. You can configure it to make an absolute-uri request, but that could be a forward proxy or direct origin request, there's no way to know that from the config.
I think I'm convinced it isn't possible to fulfill the spec automatically. And users can always use something like Header Rewrite to rewrite the asterisk-form, if they need to.
To be clear, I wasn't objecting, just looking for input. This PR is an improvement, regardless of RFC9112§3.2.4.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9101: Fix handling of OPTIONS request
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9101:
URL: https://github.com/apache/trafficserver/pull/9101#discussion_r980593789
##########
proxy/hdrs/URL.h:
##########
@@ -43,6 +43,8 @@ enum URLType {
class URLImpl : public HdrHeapObjImpl
{
public:
+ static const char ASTERISK;
Review Comment:
Better as
```
static constexpr char ASTERISK = '*';
```
then there is no dfeinition needed.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] rob05c commented on pull request #9101: Fix handling of OPTIONS request
Posted by GitBox <gi...@apache.org>.
rob05c commented on PR #9101:
URL: https://github.com/apache/trafficserver/pull/9101#issuecomment-1281854215
> If ATS is making an absolute-form request, that must be a request for a proxy.
The absolute-form is permitted to origins for normal requests. There's no way to be sure from the config. The ATS operator might know, but ATS doesn't.
https://datatracker.ietf.org/doc/html/rfc9112#section-3.2.2
```
When an origin server receives a request with an absolute-form of
request-target, the origin server MUST ignore the received Host
header field (if any) and instead use the host information of the
request-target. Note that if the request-target does not have an
authority component, an empty Host header field will be sent in this
case.
A server MUST accept the absolute-form in requests even though most
HTTP/1.1 clients will only send the absolute-form to a proxy.
```
> we should make it possible to be compliant with the spec.
It's possible for an ATS operator to configure it, via a Header Rewrite or other plugin (e.g. txn_box).
I don't think it's possible to always make ATS automatically detect and do the right thing. Because we don't have the context to know if a parent is an origin or another proxy. And likewise per RFC9112§3.2.2, a client can send the absolute-form to a Forward Proxy or as a normal request (wherein ATS is acting as a Reverse Proxy, and shouldn't do the asterisk-form replacement). So without context, we have no way to know if clients are doing Forward Proxy or normal requests either.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] github-actions[bot] commented on pull request #9101: Fix handling of OPTIONS request
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #9101:
URL: https://github.com/apache/trafficserver/pull/9101#issuecomment-1384742628
This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org