Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XrdHttp parsing of the headers is "fragile" #1964

Open
esindril opened this issue Mar 15, 2023 · 8 comments
Open

XrdHttp parsing of the headers is "fragile" #1964

esindril opened this issue Mar 15, 2023 · 8 comments
Assignees
Milestone

Comments

@esindril
Copy link
Contributor

esindril commented Mar 15, 2023

This issue was discovered while debugging the CMS SAM tests which run against the eoscms.cern.ch instance using scitokens over HTTP. Basically the symptom was that the first PROPFIND request would succeed while the second one would fail. For subsequent requests this pattern would repeat itself.

The requests were submitted using gfal python bindings in the same context (TCP connection). Basically the problem came from the way the bearer token was formatted in the sense that it contained an extra new line character at the end \n (0xA in hex).

This completely messes up the parsing of the HTTP headers in the XrdHttp layer and causes predictable failures. Thanks to Joao from the FTS team, I attach a simple python reproducer using the gfal client to trigger this issue and the corresponding client logs. The important thing in this script is the reading of the token from the file which appends a 0xA char at the end of it.

The original issue is now being fixed in the CMS SAM tests but still, I think the XrdHttp parsing of the headers should be more robust and handle such cases better by for example failing the initial request which has "malformed" headers rather than cascading the error to subsequent requests.

gfal_log.txt
gfal_test.py.txt

@bbockelm
Copy link
Contributor

for example failing the initial request which has "malformed" headers rather than cascading the error to subsequent requests.

Is it clear that XRootD is / is not doing the right thing?

As far as the TCP connection is concerned, isn't the first request valid and the subsequent one invalid?

@esindril
Copy link
Contributor Author

Definitely the client is not doing the right thing, but also the XrdHttp is not putting any effort into it. According to the standard both CRLF and LF are acceptable: https://www.rfc-editor.org/rfc/rfc7230#section-3.5
The client sends two requests but the server due to the parsing sees actually 3. The second one is an empty one due to the extra new line characters in the token header - and this becomes the response to the client's second request.

@bbockelm
Copy link
Contributor

The client sends two requests but the server due to the parsing sees actually 3

Ah, now I understand!

Ok, yes, we can make this robust so the server parses 2 requests, one valid and one invalid, instead of 3 requests.

@amadio
Copy link
Member

amadio commented Mar 23, 2023

Fixed by #1971. Closing.

@amadio amadio closed this as completed Mar 23, 2023
@esindril
Copy link
Contributor Author

@amadio I'm a afraid the referenced fix does not address the current issue.

@esindril esindril reopened this Mar 24, 2023
@amadio
Copy link
Member

amadio commented Mar 24, 2023

Sorry, my bad, I had this impression when discussing with Andy yesterday. At least I didn't mark this as fixed in the release notes of 5.5.4, phew.

@ccaffy
Copy link
Contributor

ccaffy commented May 16, 2024

Closed by #2255

@ccaffy ccaffy closed this as completed May 16, 2024
@bbockelm
Copy link
Contributor

@amadio / @abh3 - please re-open. The fix in #2255 caused an immediate regression in my unit tests; see #2273.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants