kernel keyring control

[justincormack]

currently runc run has a command line option --no-new-keyring which disables the creation of an isolated kernel keyring for the process. This is kind of weird - I think this should be part of the OCI spec as it is just a specification of resource allocation, like having a new namespace. I can write up a proposal for this; obviously though this will be a breaking change so want to know what anyone else thinks.

[wking]

On Wed, Feb 14, 2018 at 01:45:24PM +0000, Justin Cormack wrote:

This is kind of weird - I think this should be part of the OCI spec as it is just a specification of resource allocation, like having a new namespace.

I agree. Tying this behavior to an OCI spec would allow managers like CRI-O to drive this behavior in a runtime-agnostic way. Currently CRI-O exposes it via the runc-specific flag (kubernetes-incubator/cri-o#1302), which makes swapping in other runtimes difficult (kubernetes-incubator/cri-o#1260).

… obviously though this will be a breaking change so want to know what anyone else thinks.

I don't think this is a breaking change. runtime-spec currently says nothing about keyrings, so their behavior is unspecified (in the C99 sense [1,2]). Runtime callers relying on runtime-spec should therefore be making no assumptions about container keyring handling. The new feature would need a minor bump, but not a major bump 3.

For existing runc users, runc can continue to treat 1.0.x configs as it already does, and error out if invoked with --no-new-keyring and a 1.1 or later configuration. That's a breaking change for runc callers, but the config versioning is for the configuration API 3, not the command line API. Dropping --no-new-keyring would be a breaking change to a command line API (like 4), but there is no runtime command line API spec that defines --no-new-keyring.

[justincormack]

I think the most general interface would be for creating a new session keyring to match the config for namespaces

"keyrings": [
{
  "type": "session"
}
]

joining an existing keyring/creating a new named on as session keyring:

"keyrings": [
{
  "type": "session",
  "name": "mykeyring",
}
]

Initially "session" is probably the only supported value, but it is extensible in future. In theory setting the process keyring might make sense too, but almost all use cases use session.

--no-new-keyring would correspond to an empty keyrings list; the first example above corresponds to the current default.

If this looks sane I will do a spec PR.

[wking]

Would you ever need multiple keyrings of the same type? I can't think of a reason, if it's possible at all. If it's not possible, I recommend using an object with ring-type keys:

"keyrings": {
  "session": {
    "name": "mykeyring"
  }
}

[justincormack]

I followed what we do for namespaces, where you also can't have more than one of a type. The problem is if name is empty, which is the normal null case for create a new keyring, then the session object is empty as far as Go is concerned. Its a bit of a Go-kernel mismatch where we are trying to distinguish between not set and null.

[wking]

On Thu, Feb 15, 2018 at 04:56:30PM +0000, Justin Cormack wrote:

I followed what we do for namespaces…

I think the namespace structure would have been easier to use if it had been an object (#598). I'm not clear on why the runtime-spec maintainers preferred to stick with an array there. If it was because of existing implementations for a namespaces array, that reasoning wouldn't apply to the new keyrings property. If that reasoning was because they really prefer arrays with an additional duplicate restriction (vs. an object with our generic key restriction 1), then yeah, they'll probably want an array here (and I'd be interested in hearing what that reason is).

The problem is if name is empty, which is the normal null case for create a new keyring, then the session object is empty as far as Go is concerned. Its a bit of a Go-kernel mismatch where we are trying to distinguish between not set and null.

I don't think there's a problem 2.

[justincormack]

Ah yes, you can distinguish between empty but nil map value and non existent one, if you try. So default as per current behaviour could be

"keyrings": {
  "session": {}
}