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

Make the handledAccessFS set configurable. #12

Closed
gnoack opened this issue Aug 26, 2021 · 3 comments
Closed

Make the handledAccessFS set configurable. #12

gnoack opened this issue Aug 26, 2021 · 3 comments

Comments

@gnoack
Copy link
Collaborator

gnoack commented Aug 26, 2021

Compare opencontainers/runtime-spec#1111 (review)

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
Copy link
Collaborator Author

gnoack commented Aug 26, 2021

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
Copy link
Collaborator Author

gnoack commented Aug 28, 2021

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:

  • go-landlock is at a version that only supports Landlock ABI v1
  • It gets called with HandledAccessFS flags that belong to a future Landlock ABI version
  • The kernel it runs on also says that it supports a higher ABI version. (for example, 3)

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

  • Option A: The given additional HandledAccessFS flags are all supported on the kernel we run on.
  • Option B: The given additional HandledAccessFS flags are all not supported by the kernel we run on.
  • Option C: The given additional HandledAccessFS flags are partially supported by the kernel we run on -- but now we don't know which, and can't tell how to select the ones that would work for "best effort" mode.

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 added a commit that referenced this issue Aug 28, 2021
This checks that the provided set of handledAccessFS permissions is
within the set known to go-landlock. (see #12)
gnoack added a commit that referenced this issue Aug 28, 2021
@gnoack
Copy link
Collaborator Author

gnoack commented Aug 28, 2021

... 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.

@gnoack gnoack closed this as completed in 4e4877f Aug 28, 2021
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

1 participant