[pluggable-validation / 3]: implement default validator. #171
Conversation
klihub
force-pushed
the
devel/pluggable-validation/add-default-validator
branch
4 times, most recently
from
e34091c to
c81c39d
Compare
klihub
force-pushed
the
devel/pluggable-validation/add-default-validator
branch
2 times, most recently
from
79d0572 to
8a54e4f
Compare
klihub
force-pushed
the
devel/pluggable-validation/add-default-validator
branch
3 times, most recently
from
df92e8b to
1e4902e
Compare
klihub
force-pushed
the
devel/pluggable-validation/add-default-validator
branch
2 times, most recently
from
01989d2 to
9a81688
Compare
|
When you have a moment can you please rebase on main? |
|
Can you rebase on |
Implement a new builtin plugin type. Builtin plugins can be registered from within the runtime during instantiation of the runtime adaptation. We'll use a builtin plugin for default container adjustment validation. Signed-off-by: Krisztian Litkey <[email protected]>
klihub
force-pushed
the
devel/pluggable-validation/add-default-validator
branch
2 times, most recently
from
cfe5b05 to
2a4fe5c
Compare
klihub
force-pushed
the
devel/pluggable-validation/add-default-validator
branch
2 times, most recently
from
9fd7543 to
e3f1bac
Compare
klihub
force-pushed
the
devel/pluggable-validation/add-default-validator
branch
5 times, most recently
from
bd7ec57 to
b3bec2c
Compare
Implement default (container creation/adjustment) validation as a builtin plugin. The default validator can be configured to reject OCI hook injection. Additionally, containers can be annotated with a set of required plugins. If annotated, these plugins must be present during container creation or else the creation of the container is rejected by the validator. Signed-off-by: Krisztian Litkey <[email protected]>
Split per container FieldOwners lookup for plugin field claiming and clearing from that of plugin field owner lookup so that the formers create missing owners but the latter does not. This makes field owner lookup a purely read-only operation, avoiding a data read/write race when we have both builtin and external validators present and we run validation in parallel on multiple goroutines. Also, do not export the new ownersFor and mustOwnersFor functions as they are never used externally. Signed-off-by: Krisztian Litkey <[email protected]>
klihub
force-pushed
the
devel/pluggable-validation/add-default-validator
branch
from
b3bec2c to
3c64287
Compare
Add default validator tests to verify - proper rejection of OCI Hooks - proper validation of required plugins - allowed toleration of missing required plugins - proper validation of annotated extra required plugins Signed-off-by: Krisztian Litkey <[email protected]>
klihub
force-pushed
the
devel/pluggable-validation/add-default-validator
branch
from
3c64287 to
c119aa7
Compare
README.md
Outdated
| #### Default Validation | ||
|
|
||
| A default validator plugin provides configurable minimal validation. It is | ||
| disabled by default. It can be enabled and selectively configured to |
There was a problem hiding this comment.
... Think we are missing the by default mode that stops plugins from mutating client (k8s,...) security profile content. Sure we don't have those PRs yet but should prep for them being merged. Chatted with Samuel on this .. conf could be either side here or by container runtime for default built-in right?
Sort of figured the default built-in validator plugin would be on if any other plugins were connected... and would have a config to allow/disallow all the various types of edits and the admin would have to force it off with a bold recommendation to not do that unless providing the same/similar functionality in an external mandatory validator.
There was a problem hiding this comment.
I think having the builtin default validator on by default is reasonable. What I mentioned to Mike is that the default-on can also be done in the container runtime through existing config-defaulting (e.g., containerd has a bunch of code for handing default configuration already) if that's easier code-wise. (It also might be easier for visibility, since it'd print in containerd config dump.)
Think we are missing the by default mode that stops plugins from mutating client (k8s,...) security profile content.
We have a couple things today that are somewhat dangerous that would be backwards-incompatible to restrict by default:
- OCI hook injection
- Mount injection
But I do think it's reasonable to say that the default validator should restrict new kinds of field adjustments if they can modify the container security boundary. We haven't merged #123, #124, #135, or #168 yet, which would all be candidates for this, but maybe we should mention in the readme that future kinds of adjustments may also be restricted by default?
There was a problem hiding this comment.
... Think we are missing the by default mode that stops plugins from mutating client (k8s,...) security profile content. Sure we don't have those PRs yet but should prep for them being merged. Chatted with Samuel on this .. conf could be either side here or by container runtime for default built-in right?
I think for restricting any new controls via validation, the best would be to add/enable those as part of the PR which introduces the new control itself. So basically once this one gets merged, rebase the above mentioned PRs and add a new commit to each which implements the corresponding restriction in the default validator.
There was a problem hiding this comment.
I think having the builtin default validator on by default is reasonable. What I mentioned to Mike is that the default-on can also be done in the container runtime through existing config-defaulting (e.g., containerd has a bunch of code for handing default configuration already) if that's easier code-wise. (It also might be easier for visibility, since it'd print in
containerd config dump.)
Yes. Since we don't have a 'top-level' configuration struct defined in NRI itself, but we do have it in the runtimes which we then convert into a set of NRI options, we tend to force defaults on the runtime side. I think we should do that also in this case.
Think we are missing the by default mode that stops plugins from mutating client (k8s,...) security profile content.
We have a couple things today that are somewhat dangerous that would be backwards-incompatible to restrict by default:
- OCI hook injection
- Mount injection
I was hoping to avoid backward-incompatible changes, which would be easy to do by not having the default validator on in the default configuration. But I think we don't need to make that decision for this PR. It's a more natural decision for the PRs which update the runtime with support for validation.
But I do think it's reasonable to say that the default validator should restrict new kinds of field adjustments if they can modify the container security boundary. We haven't merged #123, #124, #135, or #168 yet, which would all be candidates for this, but maybe we should mention in the readme that future kinds of adjustments may also be restricted by default?
Okay, so I added a 'Default Validation Scope' chapter at the end. It states that
- new configurable restriction control might be added to the default validator, and
- this typically will happen if we extend NRI with new controls which might have security implications
klihub
force-pushed
the
devel/pluggable-validation/add-default-validator
branch
3 times, most recently
from
ed5ad6e to
b136dfd
Compare
Add a section to the documentation briefly describing the core ideas and concepts of pluggable validation and the feature set provided by the default builtin validator. Co-authored-by: Chris Henzie <[email protected]> Signed-off-by: Krisztian Litkey <[email protected]>
klihub
force-pushed
the
devel/pluggable-validation/add-default-validator
branch
from
b136dfd to
00d85a9
Compare
github-project-automation
bot
moved this from Needs Reviewers
to Review In Progress
in Pull Request Review
github-project-automation
bot
moved this from Review In Progress
to Done
in Pull Request Review
4 participants
This PR is stacked on #169 and #170.
This PR is the third in a stack which splits up #163 into multiple PRs, which collectively implement configurable restrictions using the proposed pluggable validation API and a builtin default validator plugin.
This PR
/cc @chrishenzie