archiecobbs / s3backer

FUSE/NBD single file backing store via Amazon S3
Other
535 stars 77 forks source link

s3backer is silently accepting invalid command line parameters #179

Open Nikratio opened 2 years ago

Nikratio commented 2 years ago

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
archiecobbs commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

The following could be passed through from the user:

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.