landlock-lsm / go-landlock

A Go library for the Linux Landlock sandboxing feature
MIT License
115 stars 8 forks source link

Make Restrict() future-proof #4

Closed gnoack closed 3 years ago

gnoack commented 3 years ago

This package does not provide stability guarantees yet, and we need to fix the API so that we can provide these guarantees.

Things I'd like to fix:

Prior art for backwards compatibility: @l0kod's rust-landlock supports multiple backwards compatibility strategies. (See ErrorThreshold enum)

gnoack commented 3 years ago

Future Landlock Compatibility

Main risk: Migrating from Landlock ABI N to ABI N+1 could lock down the calling process more than originally intended.

Migration between Landlock ABIs should therefore be explicit, as it can be a potentially breaking change.

I assume that for most users it should not break in practice though... the use cases I've come across often only rely on basic file operations, which are already supported in Landlock V1.

Option A: Use Go module versioning to track Landlock ABI versions

We could make the Go module version be the same as the used Landlock ABI version.

Go packages are supposed to give compatibility guarantees within major versions (doc), but an upgrade between major versions is a step where library users are naturally supposed to double check that it keeps working.

Pros:

Cons:

Option B: Ask users to pass their desired ABI version

Ask users to pass "V1/V2" into the Restrict() function. That would make it very explicit what's happening.

Pros:

Cons:

gnoack commented 3 years ago

Old kernel compatibility

The scenario is that Restrict() is called on a kernel that has an older version of Landlock, or where Landlock is not supported.

The use cases I know of are:

Option A: Make the mode (graceful downgrade / strict) a parameter?

Option B: Always gracefully downgrade, but return an indicator whether it was downgraded

That would make it possible for users to tell whether a downgrade happened and react accordingly:

downgraded, err := golandlock.Restrict(...)
if err != nil {
  // ...
}
if downgraded {
  panic("Could not enforce Landlock rules")
}

Assumption: When users use the strict mode, they do it because they want to panic or log.Fatal otherwise. Maybe what is needed is not to always run in strict mode, but to just make it detectable what level of enforcement Landlock managed to enforce.

gnoack commented 3 years ago

I consider the above change to be OK for fixing the "arguments" and "flexibility" concerns. Feedback is welcome.

gnoack commented 3 years ago

I consider backwards and forwards-compatibility to be appropriately addressed with these changes.

l0kod commented 3 years ago

Main risk: Migrating from Landlock ABI N to ABI N+1 could lock down the calling process more than originally intended.

Migration between Landlock ABIs should therefore be explicit, as it can be a potentially breaking change.

Just to be clear, future (kernel) Landlock versions will always be compatible with older versions (e.g. the default behavior or implicit options will never change). This is required for (simple) backward compatibility (i.e. by avoiding user space to give an ABI version to the kernel) and to get a deterministic access-control.

l0kod commented 3 years ago

Comment about AccessFSRoughlyRead: https://github.com/gnoack/golandlock/commit/f6bba1285ffd2c64b2b8774876c29c62213d1e5a#r53543969

l0kod commented 3 years ago

Comment about VMax: https://github.com/gnoack/golandlock/commit/da2b49f57ce238379d93496f1c3ed843125e7536#r53544451