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

dws: only allow owner to create persistent #215

Merged

Conversation

jameshcorbett
Copy link
Member

@jameshcorbett jameshcorbett commented Sep 19, 2024

Problem: until futher work is done to support various usage policies for persistent file systems, it is sometimes desirable to restrict their creation to the instance owner.

Reject jobs with create_persistent strings unless the job was submitted by the instance owner.

Add a flag to disable the check.

Fixes #205.

@behlendorf
Copy link
Collaborator

Who is meant by the "instance owner" here? I'm not sure I follow how this is intended to work.

@garlick
Copy link
Member

garlick commented Sep 19, 2024

It's the userid that is running the flux instance. For the system instance it's the flux user.
https://flux-framework.readthedocs.io/projects/flux-core/en/latest/guide/glossary.html#term-instance-owner

@jameshcorbett
Copy link
Member Author

In theory I could also restrict it to root but flux doesn't allow root to run jobs so that wouldn't work.

@jameshcorbett
Copy link
Member Author

@garlick do you have any tips for how I could submit a job as another user in the testsuite to make sure it's rejected?

@garlick
Copy link
Member

garlick commented Sep 19, 2024

It's unfortunately a bit complicated to fake that, but have a look at https://github.com/flux-framework/flux-core/blob/master/t/t2404-job-exec-multiuser.t

@grondo
Copy link
Contributor

grondo commented Sep 19, 2024

Note that if you are using test execution then it is a lot less complicated. Just take the t/scripts/sign-as.py script from flux-core, and use this function:

submit_as_alternate_user()
{
        FAKE_USERID=42
        flux run --dry-run "$@" | \
          flux python ${SHARNESS_TEST_SRCDIR}/scripts/sign-as.py $FAKE_USERID \
            >job.signed
        FLUX_HANDLE_USERID=$FAKE_USERID \
          flux job submit --flags=signed job.signed
}

See t2400-job-exec-test.t for example of usage. This requires a flux-core compiled with flux-security, so you'll want to use a prereq as in that test as well.

Copy link
Member

@wihobbs wihobbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this implementation tracks with our recent discussions in the Fluxion meetings. Approving.

@@ -284,6 +285,12 @@ def create_cb(handle, _t, msg, api_instance):
raise TypeError(
f"Malformed dw_directives, not list or string: {dw_directives!r}"
)
for directive in dw_directives:
if not unrestricted_persistent and "create_persistent" in directive:
if userid != os.getuid():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is running at the system instance level you could call getattr on security.owner. But up to you

Copy link
Member Author

@jameshcorbett jameshcorbett Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point! Probably more robust too. I force-pushed to make it do that instead. Setting MWP.

Problem: until futher work is done to support various usage policies
for persistent file systems, it is sometimes desirable to restrict
their creation to the instance owner.

Reject jobs with `create_persistent` strings unless the job was
submitted by the instance owner.

Add a flag to disable the check.
Problem: to test the UID enforcement of the create_persistent
restrictions, it would be helpful to submit a job as another user.

Copy over the sign-as.py script from flux-core.
Problem: there are no tests than non-owners' create_persistent
jobs are rejected.

Add a test.
@mergify mergify bot merged commit 0263e77 into flux-framework:master Sep 20, 2024
8 checks passed
@jameshcorbett jameshcorbett deleted the instance-owner-persistent branch September 20, 2024 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

restrict persistent rabbit storage to instance owners
5 participants