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

First KEP version for review #1

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

First KEP version for review #1

wants to merge 6 commits into from

Conversation

asierHuawei
Copy link
Owner

  • One-line PR description:
  • Issue link:
  • Other comments:

Copy link

@rata rata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asierHuawei Thanks! Left some comments, let me know what you think :)

To get started with this template:



Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spacing seems broken (compared to the kep template). It is not a problem now, but something to fix before opening the upstream PR




IMA namespaces allow to check the file integrity. This proposal adds file integrity inside containers deployed in kubernetes.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the file" --> which file? The rootfs?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is first of all about regular files I suppose @asierHuawei

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, regular files inside the container. I will change the wording to be more explicit.




* Allow IMA to work inside a container with remote attestation
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For an initial stage this can be fine. But more concrete goals derived by what allowing IMA namespace inside containers enables will probably be needed at some point

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say that IMA in container allows to control the POD integrity and using the mechanism of remote attestation one can definitely check the integrity at any given POD and any time.
@asierHuawei What you think?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DenisSemakin That's right, I will make it more explicit.




### Non-Goals
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-goals are usually used to put out of scope some things reviewers might wonder. It helps to clarify the scope. It is okay now to not have any, but I guess at some point we will add something here too (at least after conversations when people assume this will tackle X).

Maybe something to leave out of scope is IMA support for volumes and just the rootfs is used now?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will leave it empty for now. As I get more feedback from the community, I can start filling it in.


### Runtime specification

In order to run a container we need a bundle that contains a config.json file with all the configuration. There is a Linux specific configuration section where the namespaces are listed. According to the standard, the following namespace types SHOULD be supported.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't explain the runtime-spec change here. At most mention that a PR is merged/open for this, but not go into the details here in the KEP. Lot of people will comment things that will be lost with the upstream discussion for runtime-spec.

Let's keep the runtime-spec discussion in the runtime-spec PR and remove most of this from here.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I will link to a PR in runC that shows how it can be achieved.




This KEP is a policy KEP, not a feature KEP. It will start as GA.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this reasoning come from? I don't think this is ok

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a left over from the template. I removed it, thanks for noticing.

#### Story 3


As a cluster admin, I want to deny access to certain files inside the pod, so that a potential intruder can't access sensitive information.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be a way to deny access used by the IMA namespace? Why is this better than using the permission bits to deny access?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add more specific info. Thanks for the feedback.

#### Story 2


As a cluster admin, I want to deploy a only proven and certified pods, so that I can comply with internal policies as well as security regulations.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem the current proposal has any way to only allow certified pods. Can you comment on where this would be expanded to support this?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will change the wording.


#### Story 1

As a cluster admin, I want to detect undesired file changes, so that I can take out pods that have been compromised.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you comment at a high-level how the undesired file change can bubble-up so anyone knows and takes out pods? Or will this be automatic?

IMA and EVM can use a TPM chip as a hardware root of trust. Hence we can verify images against a set of golden hash values, as well as avoiding any further changes to the overlayfs to intercept calls and check the integrity of files.


### User Stories (Optional)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current user-story seem more aspirational for me (maybe I'm missing something), that is why I posted some questions to them (to see if they are really aspirational or do we have a high-level idea on how that can be accomplished). If we don't really know how that can be accomplished and they seem mostly aspirational, let's just remove them for now.

What actual things do you expect to be possible? I think this section should be that, not something aspirational. Something this will allow.

It is usually helpful to have this section completed early, but if you still don't know, leave it empty.

Maybe things that can be done is something like this? No idea, though:

 As a cluster admin, I want to guarantee the rootfs files are unchanged and verified when a container access it, so I can have a high guarantee that files were not tampered.

Maybe more can be created after the measure mode, some others after the appraisal mode too (if we plan to support it)?

But let's keep this section concrete and real, or not fill them yet if we need to play a little more with the feature to understand what will be possible.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip. I will leave it empty for now.


There is an ongoing discussion regarding the runtime changes.

https://github.com/opencontainers/runc/pull/3639
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is runc, an implementation of the specification. The spec PR seems to be this one: opencontainers/runtime-spec#1164

Comment on lines 98 to 99

Since IMA namespaces can be created when a container is launched, we can provide transparent integrity verification on any linux container.

IMA and EVM can use a TPM chip as a hardware root of trust. Hence we can verify images against a set of golden hash values, as well as avoiding any further changes to the overlayfs to intercept calls and check the integrity of files. -->
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was moved inside a comment, so not visible

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following your suggestion, I decided to remove it. Maybe I should remove it completely instead of commenting it out?

Should we enable IMA namespaces by default when enabling user namespaces?


There will be a CRI API change which will allow the pod to use IMA namespaces and specify the namespace policy.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This text doesn't seem consistent with the CRI change proposed (no way to select a policy). Is this something you plan to add later?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make the change to move IMA to Pod.SecurityContext. One thing that I am concerned about is a mixed cloud scenario, with some hosts or guests running Windows or machines without hardware TPM. In that case, we want to make sure that the pods are deployed to the machines that have proper HW support. That's why I mentioned pinning the pods or using node affinity. These are mechanisms available in K8S. Should I expand this reasoning?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant the policy. Here it says we can specify the namespace policy in the CRI, but the change seems to be a bool, so... how can the policy be specified?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, my bad. That was a left over. I will remove any reference about policies.




IMA is only available in Linux hosts and Linux containers. Unfortunately, IMA is not a separate namespace, which is needed in order to isolate it and be used inside containers. Upcoming kernel patches should add support for IMA namespaces.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems confusing, you say it is not a separate namespace however the KEP talks all the time about the IMA namespace.

I guess you refer to the fact that won't be unshared? Not sure that is relevant here, but if you need to go into those details, please don't say it is not a namespace here and that it is a namespace in the rest of the KEP

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am referring to the fact that currently the Linux kernel doesn't support IMA namespaces. I guess the patches will be merged soon.

For now, IMA only works at host level, but newer kernels will have support for IMA namespaces. Regarding shared or unshared, I guess that IMA namespaces will be shared among the containers in the same pod, as it happens now with user namespaces.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently IMA namespace cannot "live" without any user namespace. IOW one user_ns -- one IMA namespace. Is there any initial user namespace in Kuber by defaut? @asierHuawei @rata

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, kubernetes 1.25 supports userns for stateless pods (this is alpha). I'll rework this to depend on idmap mounts for stateless pods soon (will need Linux >=6.3) and then add support for stateful pods

Copy link

@DenisSemakin DenisSemakin Jan 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rata Hm... sounds good, good news. From our side we'll try to implement and prepare all necessary low-level environment (e.g. patches for kernel) until Linux 6.3 is not released

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DenisSemakin cool. Do you need more patches to the kernel besides the ima ns patchset?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rata If we're talking about IMA namespaces as a standalone feature then the answer is... I guess it's "No". All patches provided by Stefan cover the basic functionality. But there's still opened question I see here: "will it be enough those patches for our purpose?"...
In case of remote attestation and pod (container) verification the answer is definitely "Yes". Here is a link to which patches we would like to add for this: https://lore.kernel.org/linux-integrity/[email protected]/


```
### Kubernetes pod resource
We propose the following change in the podTemplate to enable IMA namespaces.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pod.spec.

I think this and some others I've already mentioned in my previous review.

- containerPort: 80
```

The item pod.spec.ima.policy will automatically enable IMA for all the container in the pod with a given policy. Since all containers in a pod share the same namespaces, we need to have this policy in advance when creating the pod and its infrastructure container.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please specify (not just an example pod.spec) which fields are added and which type they will be

out Outdated
@@ -0,0 +1 @@
ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDSghO9XxHwLdURMMMGFJC/Ew24rbqHyejKOuTDXI4knRZwLY6ckl01Ixp39/3m4LEeuh7Qpl/HjvkwETyrUUNoEGKP8+I9xapOzRyCJaG2SpU1KMsb6FhdTOCCZeSfoA2fofxArhabrn4IXfKo8rS356DgZnDZjo46+cV5fbkZtzgAa4wiEOcCyxDMuEo7wkP6BITTgOuQgqitSNZRckRoUonxV9rVpQ+PdIyW3QRw+WLyYqahoNwIEJWQnhT/DyhzKjyxG7fVWJWiaQN4j42Ly9CdKhzJ8k68UZUVjm06kPNoGB1M1KsltVOF4C+o1JCXQKKYVWeEvcQafvocijgx root@mscphis01197
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems commited by mistake

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

Successfully merging this pull request may close these issues.

3 participants