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

Add storage permissions to UserDetail #511

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hannesdejager
Copy link
Collaborator

This extends the UserDetail trait with a storage_permissions method that allow storage back-ends to limit what a user can do.

@hannesdejager hannesdejager marked this pull request as draft May 15, 2024 20:53
@hannesdejager hannesdejager requested a review from robklg May 15, 2024 21:07
Copy link
Contributor

@asomers asomers left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me. BTW some of these permissions don't correspond 1:1 to Capsicum rights. For example, GET and LIST would both require CAP_READ but nothing else. So the storage backend itself will also have to validate rights. But it would have to do that anyway, if we want the rights to be enforced on !FreeBSD.

Do you want a permission like STAT that allows the SIZE and MDTM commands? Or should they come along with LIST?

@hannesdejager hannesdejager force-pushed the hannes/storage-perms branch from b71a2cb to aa56544 Compare May 16, 2024 21:50
@hannesdejager hannesdejager force-pushed the hannes/storage-perms branch from aa56544 to 709eb7b Compare June 24, 2024 19:39
@asomers
Copy link
Contributor

asomers commented Oct 22, 2024

ping. What does this PR still need?

@hannesdejager
Copy link
Collaborator Author

ping. What does this PR still need?

Hey sorry @asomers , I've been so focussed on the book I'm writing I've neglected and forgotten about this. Will try to attend to it in the next couple of days.

This extends the UserDetail trait with a storage_permissions
method that allow storage back-ends to limit what a user can
do.
@hannesdejager
Copy link
Collaborator Author

Related PR: #503

@hannesdejager
Copy link
Collaborator Author

hannesdejager commented Nov 17, 2024

The problem here is that this introduces a breaking change as such that all storage back-ends will need to be updated to make use of this, even the ones we don't have control over.

@asomers
Copy link
Contributor

asomers commented Nov 17, 2024

The problem here is that this introduces a breaking change as such that all storage back-ends will need to be updated to make use of this, even the ones we don't have control over.

Is it really a breaking change? Will the default implementation of UserDetail::storage_permissions not provide for backwards-compatibility?

@hannesdejager
Copy link
Collaborator Author

hannesdejager commented Nov 17, 2024

If a user implementation is passed in that returns something other than the default, full access implementation, then storage back-ends that don't handle this don't behave correctly.

I wonder if we can a UserDetail implementation with this functionality specifically in the filesystem back-end? I think you only rely on that right?

At least for now to unblock you.

How are you using this functionality, do you have your own FTP server that uses libunftp or do you want to eventually extend unFTP?

@asomers
Copy link
Contributor

asomers commented Nov 17, 2024

If a user implementation is passed in that returns something other than the default, full access implementation, then storage back-ends that don't handle this don't behave correctly.

Oh, I get it. Should we enforce these rights in libunftp itself then? Instead of the storage backend? That would fix the backwards-compatibility problem.

I wonder if we can a UserDetail implementation with this functionality specifically in the filesystem back-end? I think you only rely on that right?

The filesystem backend? Yes, I'm using that. Are you suggesting adding a UserDetail implementation just for purposes of testing the necessary backend changes?

At least for now to unblock you.

How are you using this functionality, do you have your own FTP server that uses libunftp or do you want to eventually extend unFTP?

I have my own FTP server based on crates/unftp-sbe-fs/examples/cap-ftpd.rs .

@hannesdejager
Copy link
Collaborator Author

Should we enforce these rights in libunftp itself then? Instead of the storage backend?
Most probably, and easiest is maybe to use a delegate pattern i.e. wrapping something like https://crates.io/crates/unftp-sbe-restrict around the given storage back-end by default and have to option to override it that behaviour and swap it out for e.g. capsicum in the storage back-end.

Are you suggesting adding a UserDetail implementation just for purposes of testing the necessary backend changes?
I wasn't thinking of testing but making it a feature of the storage back-end. But on second thought maybe thats not a great idea.

Would still be nice if this can be externally decided in a https://crates.io/crates/unftp-sbe-restrict way to keep libunftp core small

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

Successfully merging this pull request may close these issues.

2 participants