landlock-lsm / go-landlock

A Go library for the Linux Landlock sandboxing feature
MIT License
105 stars 7 forks source link

Make the handledAccessFS set configurable. #12

Closed gnoack closed 2 years ago

gnoack commented 2 years ago

Compare https://github.com/opencontainers/runtime-spec/pull/1111#pullrequestreview-739022043

from @kailun-qin:

Removing the abi field from the config and will leverage the explicit access rights specified.

In this case, I think we need direct access to ruleset.handledAccessFS in go-landlock?

gnoack commented 2 years ago

Need to digest a bit how to do this best;

Obvious way is to just expose the field

err := landlock.Config{
  HandledAccessFS: ...,
}.RestrictPaths(...)

Alternative:

err := landlock.ConfigWithHandledAccessFS(...).RestrictPaths(...)

Or:

err := landlock.V1.WithHandledAccessFS(...).RestrictPaths(...)

The method-based API is clearly safer, but also a slightly more cumbersome to use, and it's probably more common in Go to just expose the struct fields directly.

Some considerations:

Fallback mechanism: In the future, it may be interesting to make the "minimal enforced set of operations" configurable. This could be made such an AccessFSSet field as well, with 0 being the same as "best effort" mode today, but that would then be "best effort" if it doesn't get set, and that is easy to happen accidentally. (The more secure mode should be the default, IMHO) Maybe such fine grained fallback mechanisms are best configured through special methods.

In other words, the configuration Config{HandledAccessFS: ...} should not be in "best effort" mode by default.

Possibility to configure more than the known Landlock versions: By exposing the field directly, it will be possible to use AccessFS flags that are not natively supported by the go-landlock library yet. Should this be permitted? (And can it trigger error cases that are currently considered bugs in go-landlock?)

Comments are welcome.

gnoack commented 2 years ago

Regarding the "possibility to configure more than the known Landlock versions": I believe that it's best to just forbid that.

Rationale:

The scenario is:

From the perspective of go-landlock, the following things are hard to distinguish:

I think it's best to make it a hard error when callers specify HandledAccessFS sets that go-landlock doesn't know about yet. This should not be a problem for users. When future Landlock ABI versions come around the corner, they'll just have to upgrade to a newer version of go-landlock that supports the flags that they want to use.

(If callers use a go-landlock provided lookup for their HandledAccessFS flags (compare issue #14), they would not end up passing in a too-broad set of flags anyway.)

gnoack commented 2 years ago

... re-thinking this right now ... I exposed the field just like that, but in the end, it's really difficult to verify correctness... suddenly, there are Config structures that can be in an invalid state (which was previously not possible). I attempted to fix that by making sure that AccessFSSet values are all valid, but all of this is really complicated to do in the end. I'm revising this, and will not expose the Config.HandledAccessFS, but instead provide a factory function to create a config with the desired value. The factory function can do the check, and users will see the error earlier (at config construction time) if they provide invalid AccessFSSet values.