Rename after redirection #2570

dynamic-entropy · 2025-08-06T12:50:03Z

In 8ce935f, the desired goal is to add the CGI parameter authz to the "Destination" of the MOVE operation before it is evaluated for access permissions.

After a redirection. the authorization header takes the form of a CGI parameter and is no longer present in the redirected request.

Optionally, one can instruct curl to send the authorization header on redirect with --location-trusted; however, this is not the default behaviour.
Note that the way things are implemented currently, the use of --location-trusted will not work either after redirection, (as tested here #2550 (comment) since the presence of authz in the resource url will prevent its copying into the header2cgi string.

xrootd/src/XrdHttp/XrdHttpReq.cc

Lines 211 to 218 in 8ac19b1

    
           auto it = std::find_if(prot->hdr2cgimap.begin(), prot->hdr2cgimap.end(),[key](const auto & item) { 
        
             return !strcasecmp(key,item.first.c_str()); 
        
           }); 
        
           if (it != prot->hdr2cgimap.end() && (opaque ? (0 == opaque->Get(it->second.c_str())) : true)) { 
        
             std::string s; 
        
             s.assign(val, line+len-val); 
        
             trim(s); 
        
             addCgi(it->second,s);

src/XrdHttp/XrdHttpReq.cc

dynamic-entropy

Do we really mean "anything" here or do we only need authz cgi parameter.

xrootd/src/XrdHttp/XrdHttpReq.cc

Lines 981 to 985 in 8ac19b1

    
           // We assume that anything appended to the CGI str should also 
        
           // apply to the destination in case of a MOVE. 
        
           if (strchr(destination.c_str(), '?')) destination.append("&"); 
        
           else destination.append("?"); 
        
           destination.append(hdr2cgistrEncoded.c_str());

tests/cluster/test.sh

src/XrdHttp/XrdHttpReq.cc

dynamic-entropy

@bbockelm made the requested changes for parsing.

Will defer to @amadio and @ccaffy for comments on options for handling multiple redirects through a manager node.

ccaffy

Is it actually OK that this goes to XrdHttpUtils so that unit tests are created with different cases? Including tests like authauthauthz= for example

tests/XrdHttpTests/XrdHttpTests.cc

src/XrdHttp/XrdHttpReq.cc

ccaffy

All good, think about squashing your commits please before merging :)

dynamic-entropy · 2025-08-14T13:06:35Z

Thankyou!

I have squashed the unit test and related changes in a single commit. The rest of the commits include conceptually separate changes, and I would like to keep them as is.

dynamic-entropy · 2025-08-14T13:10:24Z

Also rebased on master.

ccaffy · 2025-08-14T13:57:28Z

I think we can discuss with @ccaffy about the more thorough fix for the host check issue outlined in one of my comments -- but that seems clearly out of scope for this one.

Yes, feel free to open a new issue if that is not been done already ;)

dynamic-entropy · 2025-08-14T14:21:01Z

Reworded commits and opened the issue from Brian's comments: #2572

tests/CMakeLists.txt

src/XrdHttp/XrdHttpReq.hh

tests/cluster/test.sh

amadio · 2025-08-21T09:47:58Z

I discussed this with @abh3 yesterday. The conclusion is that the simplest fix should be applied here, i.e., just move the lines added by commit 6e01c0d to where moves are processed (shown below):

xrootd/src/XrdHttp/XrdHttpReq.cc

Lines 1723 to 1754 in d9af49d

    
           case XrdHttpReq::rtMOVE: 
        
           { 
        
             // --------- MOVE 
        
             memset(&xrdreq, 0, sizeof (ClientRequest)); 
        
             xrdreq.mv.requestid = htons(kXR_mv); 
        
             std::string s = resourceplusopaque.c_str(); 
        
             s += " "; 
        
             char buf[256]; 
        
             char *ppath; 
        
             int port = 0; 
        
             if (parseURL((char *) destination.c_str(), buf, port, &ppath)) { 
        
               prot->SendSimpleResp(501, NULL, NULL, (char *) "Cannot parse destination url.", 0, false); 
        
               return -1; 
        
             } 
        
             char buf2[256]; 
        
             strcpy(buf2, host.c_str()); 
        
             char *pos = strchr(buf2, ':'); 
        
             if (pos) *pos = '\0'; 
        
             // If we are a redirector we enforce that the host field is equal to 
        
             // whatever was written in the destination url 
        
             // 
        
             // If we are a data server instead we cannot enforce anything, we will 
        
             // just ignore the host part of the destination 
        
             if ((prot->myRole == kXR_isManager) && strcmp(buf, buf2)) { 
        
               prot->SendSimpleResp(501, NULL, NULL, (char *) "Only in-place renaming is supported for MOVE.", 0, false); 
        
               return -1; 
        
             }

Since we plan to support access_token as a synonym of authz in the future, the extra opaque from the source should be appended verbatim to the destination, as it's currently done, otherwise it will not work for access_token later.

src/XrdHttp/XrdHttpReq.cc

amadio · 2025-09-09T13:07:52Z

I think we agreed to have the tests added in a separate pull request (i.e. #2575), so could you please leave here just the two commits with fixes? Thank you.

dynamic-entropy · 2025-09-09T14:14:10Z

We did revise that and agreed that it is good for the tests to go in with this PR and if we ever revist them they can buid / modify them.
The reason being that these tests already take care of cleanup as required for subsequent runs.

On a different note: Actually I find this better than the XRootD tests as they leave no files but only last X=20 lines for one to debug after.

src/XrdHttp/XrdHttpExtHandler.cc

Move operations from a manager should succed after redirection

- Add tests for rename operation with HTTP - MOVE operation is currently fails after redirection - Thus the tests assert against http code 401; a fix would change it to 201 instead - For redirection from meta-manager; move failes for a manager node

…peration; this allows rename after redirections

dynamic-entropy requested review from bbockelm and ccaffy August 6, 2025 12:52

dynamic-entropy force-pushed the rename-after-redirection branch 4 times, most recently from c09ee62 to 27f4cb5 Compare August 11, 2025 10:41

dynamic-entropy commented Aug 11, 2025

View reviewed changes

src/XrdHttp/XrdHttpReq.cc Outdated Show resolved Hide resolved

dynamic-entropy commented Aug 11, 2025

View reviewed changes

bbockelm reviewed Aug 11, 2025

View reviewed changes

tests/cluster/test.sh Show resolved Hide resolved

dynamic-entropy force-pushed the rename-after-redirection branch from 27f4cb5 to ddd0688 Compare August 11, 2025 16:57

dynamic-entropy requested a review from bbockelm August 11, 2025 16:57

dynamic-entropy commented Aug 11, 2025

View reviewed changes

src/XrdHttp/XrdHttpReq.cc Outdated Show resolved Hide resolved

dynamic-entropy commented Aug 11, 2025

View reviewed changes

amadio added this to Release Planning Aug 12, 2025

dynamic-entropy force-pushed the rename-after-redirection branch from ddd0688 to 0427103 Compare August 13, 2025 09:17

ccaffy requested changes Aug 13, 2025

View reviewed changes

dynamic-entropy force-pushed the rename-after-redirection branch 2 times, most recently from 68db077 to 7a98d1d Compare August 14, 2025 11:02

dynamic-entropy commented Aug 14, 2025

View reviewed changes

tests/XrdHttpTests/XrdHttpTests.cc Outdated Show resolved Hide resolved

dynamic-entropy requested a review from ccaffy August 14, 2025 11:05

ccaffy approved these changes Aug 14, 2025

View reviewed changes

ccaffy requested changes Aug 14, 2025

View reviewed changes

src/XrdHttp/XrdHttpReq.cc Outdated Show resolved Hide resolved

src/XrdHttp/XrdHttpReq.cc Outdated Show resolved Hide resolved

src/XrdHttp/XrdHttpReq.cc Outdated Show resolved Hide resolved

dynamic-entropy force-pushed the rename-after-redirection branch from 7a98d1d to 602c395 Compare August 14, 2025 12:55

dynamic-entropy requested a review from ccaffy August 14, 2025 12:58

ccaffy approved these changes Aug 14, 2025

View reviewed changes

dynamic-entropy force-pushed the rename-after-redirection branch from 602c395 to 74db9e9 Compare August 14, 2025 13:03

dynamic-entropy requested a review from amadio August 14, 2025 13:03

dynamic-entropy force-pushed the rename-after-redirection branch from 74db9e9 to a91e055 Compare August 14, 2025 13:09

dynamic-entropy force-pushed the rename-after-redirection branch from a91e055 to c6c2ed3 Compare August 14, 2025 14:13

dynamic-entropy mentioned this pull request Aug 14, 2025

Resource Host and Destination Host mismatch handling for MOVE #2572

Open

amadio reviewed Aug 14, 2025

View reviewed changes

tests/CMakeLists.txt Show resolved Hide resolved

src/XrdHttp/XrdHttpReq.hh Outdated Show resolved Hide resolved

tests/cluster/test.sh Show resolved Hide resolved

amadio mentioned this pull request Aug 20, 2025

File rename over davs/https fails when using token authentication and redirection. #2550

Closed

amadio requested a review from abh3 August 20, 2025 18:11

dynamic-entropy force-pushed the rename-after-redirection branch from c6c2ed3 to bd575c1 Compare August 22, 2025 09:04

dynamic-entropy requested a review from amadio August 22, 2025 09:07

amadio reviewed Aug 22, 2025

View reviewed changes

src/XrdHttp/XrdHttpReq.cc Outdated Show resolved Hide resolved

amadio reviewed Aug 22, 2025

View reviewed changes

src/XrdHttp/XrdHttpReq.cc Outdated Show resolved Hide resolved

dynamic-entropy force-pushed the rename-after-redirection branch from bd575c1 to 58821f6 Compare August 25, 2025 17:37

dynamic-entropy requested a review from amadio August 25, 2025 17:40

dynamic-entropy force-pushed the rename-after-redirection branch from 58821f6 to b5ca86f Compare August 26, 2025 14:12

dynamic-entropy force-pushed the rename-after-redirection branch from b5ca86f to 3f71357 Compare September 9, 2025 12:53

amadio reviewed Sep 9, 2025

View reviewed changes

src/XrdHttp/XrdHttpExtHandler.cc Outdated Show resolved Hide resolved

dynamic-entropy force-pushed the rename-after-redirection branch from 3f71357 to 9d5ff87 Compare September 9, 2025 15:43

dynamic-entropy requested a review from amadio September 9, 2025 15:46

dynamic-entropy added 4 commits September 9, 2025 18:14

[Tests] Fix typo in TPC Tests xrootd config

0cb6b8d

[Tests] Add rename operations tests to cluster

5b76ac8

Move operations from a manager should succed after redirection

[XrdHttp] Append cgi parameters in resource to destination for MOVE o…

dde60fb

…peration; this allows rename after redirections

amadio force-pushed the rename-after-redirection branch from 9d5ff87 to dde60fb Compare September 9, 2025 16:14

amadio mentioned this pull request Sep 9, 2025

New tests for authenticated cluster #2575

Closed

amadio added this to the 5.9.0 milestone Sep 9, 2025

amadio merged commit f1de203 into xrootd:master Sep 9, 2025
11 checks passed

github-project-automation bot moved this to Done in Release Planning Sep 9, 2025

	auto it = std::find_if(prot->hdr2cgimap.begin(), prot->hdr2cgimap.end(),[key](const auto & item) {
	return !strcasecmp(key,item.first.c_str());
	});
	if (it != prot->hdr2cgimap.end() && (opaque ? (0 == opaque->Get(it->second.c_str())) : true)) {
	std::string s;
	s.assign(val, line+len-val);
	trim(s);
	addCgi(it->second,s);

	// We assume that anything appended to the CGI str should also
	// apply to the destination in case of a MOVE.
	if (strchr(destination.c_str(), '?')) destination.append("&");
	else destination.append("?");
	destination.append(hdr2cgistrEncoded.c_str());

Rename after redirection #2570

Rename after redirection #2570

Uh oh!

Conversation

dynamic-entropy commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dynamic-entropy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dynamic-entropy left a comment

Choose a reason for hiding this comment

Uh oh!

ccaffy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ccaffy left a comment

Choose a reason for hiding this comment

Uh oh!

dynamic-entropy commented Aug 14, 2025

Uh oh!

dynamic-entropy commented Aug 14, 2025

Uh oh!

ccaffy commented Aug 14, 2025

Uh oh!

dynamic-entropy commented Aug 14, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amadio commented Aug 21, 2025

Uh oh!

Uh oh!

Uh oh!

amadio commented Sep 9, 2025

Uh oh!

dynamic-entropy commented Sep 9, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dynamic-entropy commented Aug 6, 2025 •

edited

Loading

ccaffy left a comment •

edited

Loading