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

s3backer is silently accepting invalid command line parameters #179

Open
Nikratio opened this issue Apr 29, 2022 · 6 comments
Open

s3backer is silently accepting invalid command line parameters #179

Nikratio opened this issue Apr 29, 2022 · 6 comments

Comments

@Nikratio
Copy link
Contributor

I would have expected the following to give me an error about an unknown command line flag:

nikratio@vostro ~/i/s/build (nbd) [1]> ./s3backer --invalid
s3backer: no S3 bucket specified
nikratio@vostro ~/i/s/build (nbd) [1]> ./s3backer --invalid mybucket
s3backer: no mount point specified
nikratio@vostro ~/i/s/build (nbd) [1]> ./s3backer --invalid mybucket mnt
s3backer: auto-detecting block size and total file size...
s3backer: can't read data store meta-data: Operation not permitted
@Nikratio Nikratio changed the title s3backer is ignoring unknown command line parameters s3backer is silently accepting invalid command line parameters Apr 29, 2022
@archiecobbs
Copy link
Owner

Happy to hear ideas.

Unknown flags could be FUSE flags, so we need to pass them through to FUSE fromhandle_unknown_option() in s3b_config.c by returning 1.

Apparently FUSE doesn't complain about unknown flags either.

We can be stricter in NBD mode though; see 17eb4cc.

@Nikratio
Copy link
Contributor Author

Nikratio commented Apr 29, 2022

The short answer is (and I'm speaking with my libfuse maintainer hat here) is that FUSE filesystems should generally not blindly pass options from the user to libfuse. This is done in some of the example filesystems for simplicity, but is bad practice for anything else. Many of FUSE's options relate to specifics of the filesystem implementation and thus cannot be meaningfully chosen by the user - and may even result in data corruption if not compatible with the filesystem.

In other words, the "command line" parsed to FUSE should always be assembled by the file system from the FUSE options that it knows about and supports, not be a user-specified string that is passed through.

(Recent versions of libfuse are much stricter about this and do not accept unknown parameters, https://github.com/libfuse/libfuse/blob/master/include/fuse.h#L935)

@archiecobbs
Copy link
Owner

OK thanks for clarifying.

In other words, the "command line" parsed to FUSE should always be assembled by the file system from the FUSE options that it knows about and supports, not be a user-specified string that is passed through.

OK... but in order to do that, s3backer would have to have an internal list of FUSE flags that are acceptable. But that depends on the FUSE version (including future ones we don't know about), and the platform, and the OS version, etc. Seems like a brittle solution.

Not sure I see a clear answer as to what the right thing to do here is.

(Recent versions of libfuse are much stricter about this and do not accept unknown parameters, https://github.com/libfuse/libfuse/blob/master/include/fuse.h#L935)

That's good to know. It seems pretty clear to me that this was a FUSE bug.

@Nikratio
Copy link
Contributor Author

Nikratio commented Apr 29, 2022

OK thanks for clarifying.

In other words, the "command line" parsed to FUSE should always be assembled by the file system from the FUSE options that it knows about and supports, not be a user-specified string that is passed through.

OK... but in order to do that, s3backer would have to have an internal list of FUSE flags that are acceptable. But that depends on the FUSE version (including future ones we don't know about), and the platform, and the OS version, etc. Seems like a brittle solution.

I think this is exactly what makes it robust rather than brittle.

If s3backer does not know what a flag does, then it should not pass it to FUSE, because it may be incompatible with s3backer.

Other (known) flags fall in two categories:

(1) They need to have a specific value in order for s3backer to work. If these are then not supported by OS or libfuse, then s3backer failing to start is the right behavior.

(2) Multiple values are acceptable to s3backer. In this case they can be specified by the user (who can then take into account OS and libfuse version) and s3backer can pass them through after checking them against the allowlist.

@archiecobbs
Copy link
Owner

OK, since you're the FUSE expert :) would you like to propose a list of acceptable options?

FUSE is still somewhat of a mystery to me...

@Nikratio
Copy link
Contributor Author

With recent versions of FUSE, I think the following should be hardcoded in s3backer:

  • max_read=128k (but also set this in init callback)
  • fsname=s3backer

The following could be passed through from the user:

  • uid, gid, umask
  • allow_other, allow_root

With FUSE 2.9 (currently used by s3backer) there might be differences. But I'd strongly suggest to switch to FUSE 3 and the low-level API. For s3backer, this should not require that much work and you'll get much improved semantics.

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

No branches or pull requests

2 participants