Get ready to enable DropPrivileges by default#3061
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
h2zh
added
enhancement
h2zh
force-pushed
the
drop-privs-cont
branch
2 times, most recently
from
efb24ef to
7951e76
Compare
- LaunchBrokerListener() have to read the metadata, which is acquired through the advertisement
- In config.go, the Origin_GlobusConfigLocation directory (/run/pelican/xrootd/origin/globus) was being added to pelicanDirs and created with pelican user ownership. This caused all parent directories (/run/pelican, /run/pelican/xrootd, /run/pelican/xrootd/origin) to be created with pelican ownership before CheckXrootdEnv() ran, which couldn't fix them because config.MkdirAll() returns early for existing directories, causing the XRootD RunLocation is not owned by xrootd but pelican user and group - Origin.GlobusConfigLocation should be accessed by both pelican and xrootd user, to allow: 1. XRootD to read token files (*.tok, *.transfer.tok) at runtime, to authenticate requests to the Globus API 2. Pelican to write (create/update) token files, see persistToken() - How to let xrootd read the tokens after drop-privileges? The setgid bit is applied to Origin_GlobusConfigLocation/tokens to inherit the directory's group on files created in the directory
- If the default broker url is set, the federation discovery won't overwrite it with federation's broker url - Only Director run the broker. Setting it here would prevent federation discovery from populating the correct broker URL for caches/origins joining an existing federation.
- No matter in normal or drop-privileges mode, the TLS certificate file used by XRootD is always `runtimeXRootD/copied-tls-creds.crt`. The other file `runtimeXRootD/pelican/copied-tls-creds.crt` is just an intermediate product in drop privileges mode
- When drop privileges is enabled, check the pelican user can read TLS credentials. - If it doesn't, shut down the program with an error message to prompt the admin to manually change the perms. - Pelican won't automatically change them because these files are usually mounted from outside filesystem. Pelican can't change their perms. - Note that XRootD does not need direct access to these files as Pelican copies them to a runtime location. - Skip this check on Windows because few Pelican server is running on Windows. syscall.Stat_t only exists on Unix-like systems (Linux, macOS). On Windows, it doesn't exist and causes a compile-time error.
…mode - Note there's a counterpart PR in xrdhttp-pelican plugin repo
- Setup the permanent and temporary directory for the federation token file and sets proper perms. - The permanent directory is always owned by xrootd user. - In normal mode, the temporary directory is owned by the root. While in drop privileges mode, it is owned by the pelican user. - This commit is updated after the changes in PelicanPlatform#3081 to catch up the fixes in the set defaults process
- In drop-privileges mode the process runs as the unprivileged user after dropPrivileges() and can no longer write to RuntimeDir (e.g. /run/pelican), which caused "permission denied" when writing the address file and a fatal startup error. - This is safe because Server_ExternalWebUrl, Origin_Url, and Cache_Url are not set or changed after the drop in LaunchModules.
patrickbrophy
suggested changes
Feb 6, 2026
jhiemstrawisc
approved these changes
Feb 10, 2026
jhiemstrawisc
left a comment
jhiemstrawisc
left a comment
Member
There was a problem hiding this comment.
I eyeballed the changes to fed token stuff and I think things look okay here.
patrickbrophy
approved these changes
Feb 10, 2026
patrickbrophy
left a comment
patrickbrophy
left a comment
Contributor
There was a problem hiding this comment.
Thanks for this! LGTM
Member
|
I was a bit nervous about the failing Broker API test because this PR touches lines of code related to the Broker. However, I re-ran the tests and got the Broker API test to pass in one of the binaries, so I think we're actually dealing with the known flaky issues. I'm fine merging as is. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is structured as several self-contained commits. Each one fixes a particular permissions or drop-privileges bug in isolation. I recommend reviewing the PR commit-by-commit to follow the changes.
The federation token change requires a matching xrdhttp-pelican PR for fed-token handling (command 9 / atomic overwrite). You need to rebuild this plugin in your local environment to test it.
You can set
Server.DropPrivilegesto true to enable the feature. This PR leaves the default as false. I think a reasonable rollout plan would be first pilot it in a few servers running the 7.23, and eventually set the default to true in 7.24.You may find my note on function execution order useful: