Configurable Restrictions of Selected NRI Features

[klihub]

Background

An increasing number of PRs or feature requests from the community propose adding new container mutation capabilities with direct security implications to NRI. See #123, #124, and #135 for some examples.

Based on the review discussions of these PRs it looks like

1. there is a desire to extend NRI with such capabilities
2. the ability to administratively lock down such capabilities would help accepting them
3. some existing capabilities in NRI could be put under such control (OCI hook injection)

Proposed Enhancement

Extend the current implementation of NRI with the notion of restrictions.

Restrictions allow one to disable a subset of the container mutation capabilities present in NRI. Restrictions are defined in the configuration. Initially a single global set of restrictions are defined and they apply to all plugins. This can later be updated to a more fine-grained model with further improvements, if desired.

Restrictions are communicated to an NRI plugin during registration. The plugin can then report and choose not to start up if the configured restrictions prevented it from functioning properly. Initially OCI hook injection would be subject to restrictions. For backward compatibility with the current implementation, OCI hooks would default to an unrestricted configuration.

NRI request/response processing is updated to check a plugin's response for potential restriction violations before applying the requested mutations to containers. If restrictions are violated, the plugin is forcibly disconnected with an error message.

Proposed Implementation

Protocol Additions

Define the set of possible restrictions. Initially this only consist of OCI hook injection.

message Restrictions {
    // If set to true, adjustment of OCI hooks is disabled.
    bool oci_hooks = 1;
}

Include configured restrictions also in the plugin configuration request.

message ConfigureRequest {
  // Any plugin-specific data, if present among the NRI configuration.
  string config = 1;
  // Name of the runtime NRI is running in.
  string runtime_name = 2;
  // Version of the runtime NRI is running in.
  string runtime_version = 3;
  // Configured registration timeout in milliseconds.
  int64 registration_timeout = 4;
  // Configured request processing timeout in milliseconds.
  int64 request_timeout = 5;
  // Extra restrictions imposed by the runtime.
  Restrictions restrictions = 6;
}

Add convenience functions for runtime restriction checking.

func (r *Restrictions) CheckAdjustment(a *ContainerAdjustment) error {
	if a == nil || r == nil {
		return nil
	}
	if err := r.checkOciHookAdjustment(a); err != nil {
		return err
	}
	return nil
}

func (r *Restrictions) checkOciHookAdjustment(a *ContainerAdjustment) error {
	if !r.OciHooks || a.Hooks == nil {
		return nil
	}

	switch {
	case a.Hooks.Prestart != nil:
		return OciHooksRestricted
	case a.Hooks.CreateRuntime != nil:
		return OciHooksRestricted
	case a.Hooks.CreateContainer != nil:
		return OciHooksRestricted
	case a.Hooks.StartContainer != nil:
		return OciHooksRestricted
	case a.Hooks.Poststart != nil:
		return OciHooksRestricted
	case a.Hooks.Poststop != nil:
		return OciHooksRestricted
	}

	return nil
}

Add convenience functions for plugin side permission checks.

func (r *Restrictions) AllowOciHooks() bool {
	return r == nil || !r.OciHooks
}

Runtime Adaptation Changes

Add a new programmatic option for the NRI runtime adaptation interface to set the configured restrictions. Update the plugin response processing logic to first check a response for any potential violation of configured restrictions. If a violation is found, log an error and disconnect the plugin.

Plugin Stub Changes

Extract restrictions from the configuration message and make it available to the plugin.

Sample Plugin Changes

In the OCI hook injector plugin, check restrictions during plugin configuration logging an error and exiting if OCI hook injection is disallowed.

Changes for Pending PRs

• For PR api: add seccomp adjustment #123

Add restrictions for seccomp policy adjustment.

message Restrictions {
    // If set to true, adjustment of OCI hooks is disabled.
    bool oci_hooks = 1;
    // If set to true, adjustment of seccpomp policy is disabled.
    bool seccomp_policy = 2;
}

Implement/update restriction checking functions for seccomp policy.

var (
	RestrictionError        = fmt.Errorf("restriction violation")
	OciHooksRestricted      = fmt.Errorf("%w: OCI hook adjustment disabled", RestrictionError)
        SeccompPolicyRestricted = fmt.Errorf("%w: seccomp policy adjustment disabled, RestrictionError)
)

func (r *Restrictions) CheckAdjustment(a *ContainerAdjustment) error {
	if a == nil || r == nil {
		return nil
	}
	if err := r.checkOciHookAdjustment(a); err != nil {
		return err
	}
	if err := r.checkSeccompPolicyAdjustment(a); err != nil {
		return err
	}
	return nil
}

func (r *Restrictions) checkSeccompPolicyAdjustment(a *ContainerAdjustment) error {
	if !r.SeccompPolicy || a.Linux == nil || a.Linux.SeccompPolicy == nil {
		return nil
	}

        return SeccompPolicyRestricted
}

Add convenience functions for plugin side permissions checks.

func (r *Restrictions) AllowSeccompPolicy() bool {
	return r == nil || !r.SeccompPolicy
}

• For PR api: add namespace adjustment #124 (or Allow to adjust LinuxNamespaces #135)

Add restrictions for linux namespace adjustment.

message Restrictions {
    // If set to true, adjustment of OCI hooks is disabled.
    bool oci_hooks = 1;
    // If set to true, adjustment of seccpomp policy is disabled.
    bool seccomp_policy = 2;
    // If set to true, adjustment of linux namespaces is disabled.
    bool namespaces = 3;
}

Implement/update violation checking for namespaces.

var (
	RestrictionError        = fmt.Errorf("restriction violation")
	OciHooksRestricted      = fmt.Errorf("%w: OCI hook adjustment disabled", RestrictionError)
        SeccompPolicyRestricted = fmt.Errorf("%w: seccomp policy adjustment disabled, RestrictionError)
        NamespacesRestricted    = fmt.Errorf("%w: namespace adjustment disabled, RestrictionError)
)

func (r *Restrictions) CheckAdjustment(a *ContainerAdjustment) error {
	if a == nil || r == nil {
		return nil
	}
	if err := r.checkOciHookAdjustment(a); err != nil {
		return err
	}
	if err := r.checkSeccompPolicyAdjustment(a); err != nil {
		return err
	}
	if err := r.checkNamespaceAdjustment(a); err != nil {
		return err
	}
	return nil
}

func (r *Restrictions) checkNamespaceAdjustment(a *ContainerAdjustment) error {
	if !r.Namespaces || a.Linux == nil || len(a.Linux.Namespaces) == 0 {
		return nil
	}

        return NamespacesRestricted
}

Add convenience functions for plugins to check permissions.

func (r *Restrictions) AllowNamespaces() bool {
	return r == nil || !r.Namespaces
}

Potential Future Improvements

If we implement a mechanism to securely authenticate and identify plugins, this scheme can be updated to allow additional, more fine-grained per plugin (or per authorization level) restrictions.

TODO: Provide a reasonably detailed description of how that would work.

[klihub]

/cc @samuelkarp @kad

[klihub]

/cc @haircommander

[haircommander]

I almost am thinking of restrictions as the opposite: I think for any hook an admin installs, they'd want a subset of containers to be modified but some of them not to be, instead of having a policy on which aspects of hooks are allowed to run. as in: an admin probably expects every hook installed works as expected, but may want only some containers to be affected. I'm not sure of a platform agnostic way to do this. In cri-o, we have the "allowed_annotations" notion that is enabled on runtime class level, but i'm not sure that's the right level here

[samuelkarp]

@haircommander I think we probably want both: allowlist/denylist for modifiable attributes (so an admin can categorically say "NRI plugins cannot inject hooks" for example) and allowlist/denylist for pods that can be modified.

[champtar]

Right now with mount or devices adjustments you can escape to the host,
I understand people wanting something more fine grained than NRI on/off,
but right now adding seccomp or namespace adjustment doesn't change how unsecure NRI is,
so do we really need to block all PRs until we figure out fine grained permissions ?

[samuelkarp]

Right now with mount or devices adjustments you can escape to the host

Yes, also with hook adjustments.

[klihub]

Right now with mount or devices adjustments you can escape to the host, I understand people wanting something more fine grained than NRI on/off, but right now adding seccomp or namespace adjustment doesn't change how unsecure NRI is, so do we really need to block all PRs until we figure out fine grained permissions ?

@champtar Shouldn't we anyway go for the eventual granular controls which everybody considers fine-grained enough in multiple smaller steps ? We could first add the basic capability to lock down selected features globally, starting with OCI hooks. Then we could rebase and update #123 and #124/#135 to also add lockdown for the new control they introduce. This shouldn't be too difficult, and it could make it easier for some to accept the seccomp- and namespace-mutating changes.

[klihub]

as in: an admin probably expects every hook installed works as expected, but may want only some containers to be affected. I'm not sure of a platform agnostic way to do this. In cri-o, we have the "allowed_annotations" notion that is enabled on runtime class level, but i'm not sure that's the right level here

@haircommander One way could be something similar to what is suggested above for implementing the feature-based restrictions: implement the (container NRI processing exclusion) logic in NRI itself, add an option for configuring it, and take the necessary configuration data for it from the runtime configuration.

One thing I find difficult to tell here is how flexible/versatile we'd need to be in recognizing when a container should be excluded from NRI processing. For instance, would it be enough to allow exclusion based on namespace and/or the absence/presence of some annotation ?

Also, it's worth noting that for some classes of plugins it might be problematic if they are completely unaware of the existence of some containers... for instance, if a plugin needs to know about all resources assigned to containers.

[haircommander]

for CRI-O's purposes, kubernetes namespaces would be sufficient. runtime class could also work. we just need some way it's exposed in the kubernetes API so we can connect policy decisions.

[klihub]

for CRI-O's purposes, kubernetes namespaces would be sufficient. runtime class could also work. we just need some way it's exposed in the kubernetes API so we can connect policy decisions.

@haircommander Any comments/thoughts regarding the potentially problematic aspects of hiding some pods/containers completely from plugins ?

A few possibilities come to mind, but none stands out as an obvious winner to me. Ones that crossed my mind are

• don't solve it, just document it
• don't hide containers completely, just hide the (create, update) events where a plugin could alter the container
• don't hide events, but pass additional info to indicate that container is hands-off for plugin alterations

These all come with their own set of problems.

[champtar]

Right now with mount or devices adjustments you can escape to the host, I understand people wanting something more fine grained than NRI on/off, but right now adding seccomp or namespace adjustment doesn't change how unsecure NRI is, so do we really need to block all PRs until we figure out fine grained permissions ?

@champtar Shouldn't we anyway go for the eventual granular controls which everybody considers fine-grained enough in multiple smaller steps ? We could first add the basic capability to lock down selected features globally, starting with OCI hooks. Then we could rebase and update #123 and #124/#135 to also add lockdown for the new control they introduce. This shouldn't be too difficult, and it could make it easier for some to accept the seccomp- and namespace-mutating changes.

@klihub I fail to understand why some people push back on namespace / seccomp when everything is already fully open.
Securing NRI can be done in // instead of delaying everything by multiple months, but I don't think I'm going to convince you :)

[klihub]

@klihub I fail to understand why some people push back on namespace / seccomp when everything is already fully open. Securing NRI can be done in // instead of delaying everything by multiple months, but I don't think I'm going to convince you :)

@champtar I feel your frustration and I am sorry about it. But I think it's not me you'd need to convince. I'd be ready to go forward both with the pending PRs and this. I just don't know what I should do next to help things progress. I promised to write a design proposal (which this issue tries to be) instead of filing a PR, so that we can can discuss it, suggest better alternatives, additions or other improvements, and/or give an A-OK for me to progress with this.

Some additions have been suggested by @haircommander, and I know @kad has one cooking for per-plugin restrictions, but what comes to the acceptance of this as a whole, there's no verdict one way or the other whether this is good to go forward with. I would have a first implementation and additional commits for controlling seccomp and namespace adjustment (which ideally would come in each as part of the corresponding pending PR). I could file a PR from for review, but I would feel much more content to do it if I knew that folks are fine with this as a starting point.

[klihub]

@champtar And sorry for my misleading previous 'in multiple steps' comment. What I was trying to say, is that if this is OK as a starting point and it is OK to go for more granular controls in multiple steps, then I think it should be possible to progress with this and the pending PRs pretty fast. But then I failed to say it.