improve ssh filter btrbk.sh

[calestyo]

The goal of this branch is to overhaul ssh_filter_btrbk.sh, especially far more hardening respectively tightening what is allowed to be executed, but also to add some mode for btrbk’s raw target mode.

[calestyo]

@digint This branch is still far from being ready... and I guess I'll do some big overhaul of the command matching as well.

Three questions that popped up from my side are:

• Is there a list of all possible exact commands, that btrbk uses (especially in the current version)[0]?
• Would you mind if we drop the functionality of having more than one -p option? Or why is that even necessary in the first place? Wouldn't a user normally need just one pathname, and all his stuff goes below that? And even if there is a use-case, couldn't that be done by the user using more than one SSH key, and via that, selecting different -p directories?
  I ask, because right now, the path is blindly added into the pattern (even if it contains meta-characters). If a user would use a very stupid path like -p ')))|.*|(((' any command would be accepted.
  Allowing multiple paths, requires me to add some ERE quoting, which is actually much more tricky than one might think.
• With respect to [0]. I'm not so much in favour of keeping the backwards compatible stuff in the code. Makes it just more complex. Wouldn't it be enough to refer users of old btrbk simply to using an old (compatible) ssh_filter_btrbk.sh?

[calestyo]

Hey @digint.

Have you had time to think about the above questions? :-)

[digint]

Is there a list of all possible exact commands, that btrbk uses (especially in the current version)[0]?

No. Closest would be the list in ssh_filter_btrbk(1). Not all features are supported, like btrbk extents. I think it's completely ok to support only the "core" features here.

Would you mind if we drop the functionality of having more than one -p option? Or why is that even necessary in the first place? Wouldn't a user normally need just one pathname, and all his stuff goes below that? And even if there is a use-case, couldn't that be done by the user using more than one SSH key, and via that, selecting different -p directories?

Yes, it's very common to send to multiple paths, e.g. one host is only allowed to send to "/backup/my-host" and "/backup/my-alias", but not "/backup/my-other-secret-host".

I ask, because right now, the path is blindly added into the pattern (even if it contains meta-characters). If a user would use a very stupid path like -p ')))|.*|(((' any command would be accepted.

See my comments in the review, for me it's ok to allow any regex for the , it's the users static configuration after all.

Allowing multiple paths, requires me to add some ERE quoting, which is actually much more tricky than one might think.

Nooo, some ERE quoting would make it completely unreadable. So either leave it as it is (sleek, straight-forward), or find another solution to check the paths (e.g. loop through list, probably adding much complexity as well).

With respect to [0]. I'm not so much in favour of keeping the backwards compatible stuff in the code. Makes it just more complex. Wouldn't it be enough to refer users of old btrbk simply to using an old (compatible) ssh_filter_btrbk.sh?

There are valid use cases to run same script against multiple versions of btrbk, we have to support older versions as well: it's not uncommon to have several clients with different versions of btrbk connecting to same ssh endpoint.

I don't want to keep compatibility forever, but at least it should be compatible to "current" btrbk versions of major distros.

[calestyo]

I'll reply to the rest later, need to catch a train.

What I'm trying to say: This script has already grown too complex for a simple "allow"-script, and I'm not sure if I should still recommend it. Every line of code here adds possible exploits, and restricting the users capabilities by either btrfs-progs-btrbk or btrfs-progs-sudo is enough for most users (e.g. restrict backup server to read-only progs, or restrict client to write-only).

My idea would be to completely revise the current matching/rejection handling.

It's IMO far too complex and thus error prone, to have the overall regexp assembled like this.

My idea is more that there should be one "line" that sets up the regexp for exactly one command... and this also in much stricter fashion, e.g. not allowing any option patterns (like it's done now via option_match) but really just exactly those options that are needed.

Which is also why it would help, to have a list of what may actually occur.

Maybe we could have a phone conference at some point, would perhaps be easier to discuss the more general things.

[digint]

My idea would be to completely revise the current matching/rejection handling. It's IMO far too complex and thus error prone, to have the overall regexp assembled like this.

I agree, the regex part is horrible, and has grown along with btrbk. Note that there are quite some subtleties in it, btrbk can assemble pretty complex command structures.

My idea is more that there should be one "line" that sets up the regexp for exactly one command

Which we basically have already with allow_cmd

and this also in much stricter fashion, e.g. not allowing any option patterns (like it's done now > via option_match) but really just exactly those options that are needed.

This is a tradeoff between complexity / maintainability / compatibility and security. As long as the allowed command can not do harm on the filtered path (even with all options mis-used), it should be fine. For others, the options should be well defined, but then tends to get forgotten on a quick fixes.

Which is also why it would help, to have a list of what may actually occur.

Well, the list is in the code ;-) The goal was, or should be that ssh_filter_btrbk.sh can be read as such a list.

[digint]

I pushed improve-ssh_filter_btrbk.sh branch with the commits which I can merge to master for 0.32.6. Please have a quick look at it, I will have to run some basic tests as well before merging.

The issue left then is how to improve the handling of path name restrictions, without too much incompatibilities.

[calestyo]

I'm not sure how much I can do this weekend... and I haven't had time yet to look at all your comments... (will do later).

Also as said before, I do have a pretty concrete "vision" of how it should look in the end, which depends however a bit on some general questions,... depending on that, some of the already existing commits might even change again.

[calestyo]

I've resolved a few obvious things above,... will answer on the remaining things later.

[digint]

Sorry for the delay, I was super busy lately.

I think we can now:

1. merge everything except the pathname normalization to master, which will go into the next bugfix release v0.32.6
2. create a separate merge request with the rest, which will break compatibility, along with my fix which removes // (142fa6c982c3996a46a24e8de8d5c530ef03022b). This will then go to v0.33.0.

If you don't yell "stop the presses", I will proceed with (1) and cherry-pick the commits, github should be smart enough to cope with the remaining commits on this MR.

[digint]

Merged to master (v0.32.6), except:

• pathname normalization
• exit code changes

The rest will get merged after more testing (did not get around to it yet...)