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

agent: add container launch control parameters from kernel commandline #6707

Merged
merged 1 commit into from
Jun 21, 2023

Conversation

Xynnn007
Copy link
Contributor

@Xynnn007 Xynnn007 commented Apr 24, 2023

Hi, I'm not sure whether it is still ok to make PR to CCv0 branch. If not, please tell me the correct thing to do.

About this PR

Add three commdline parameters to control the container launch process

image_policy_file is the URI of the image security policy file which is used to control the allowlist and denylist of the containers to be run inside the guest. If not set, use kbs:///default/security-policy/test by default.

image_registry_auth_file is the URI of the auth file which is needed to access an private registry. If not set, use kbs:///default/credential/test by default.

simple_signing_sigstore_config is a configuration file that shows where the signature files are when simple signing is enabled. If not set, use kbs:///default/sigstore-config/test by default.

All the config items support both kbs://.. scheme, file://.. scheme, or a direct absolute path /..., which means either the file is to be fetched from a remote KBS that aa_kbc_params points to, or from the local filesystem of the guest.

Fixes: #6640

`image_policy_file` is the URI of the image security policy file which
is used to control the allowlist and denylist of the containers to be
run inside the guest.

`image_registry_auth_file` is the URI of the auth file which is needed
to access an private registry.

`simple_signing_sigstore_config` is a configuration file that shows
where the signature files are when simple signing is enabled.

All the config items support both `kbs://..` scheme, `file://..` scheme,
or a direct absolute path `/...`, which means either the file is to be
fetched from a remote KBS that `aa_kbc_params` points to, or from the
local filesystem of the guest.

Fixes: kata-containers#6640

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>
@katacontainersbot katacontainersbot added the size/large Task of significant size label Apr 24, 2023
@katacontainersbot
Copy link
Contributor

Can one of the admins verify this patch?

@Tim-Zhang
Copy link
Member

/test

@fitzthum
Copy link
Contributor

fitzthum commented May 9, 2023

I like this idea. In the last release we added resource URIs that allow the KBS to serve multiple versions of the signature policy and other resources, but currently the URI of the signature policy is hard-coded so we don't really use that feature.

One suggestion would be to simplify the interface by just supplying repository/id from the command line. These could be used to generate the three URIs. Do you think that would be simpler? It is slightly less flexible.

@Xynnn007
Copy link
Contributor Author

Xynnn007 commented May 10, 2023

Thanks, Tobin!

One suggestion would be to simplify the interface by just supplying repository/id from the command line. These could be used to generate the three URIs. Do you think that would be simpler? It is slightly less flexible.

In my opinion the kernel parameter here to specify these configurations are underlying things and they'd better be flexible enough. For the convenience of users, we can consider a CLI tool or something that receives a simple request from user and translate that request into a complex one.

User can maintain an default "secure" config on the test tag in KBS. That means an auth file without any credential, an empty sigstore config and a "deny all" policy.

My core point of view is that the underlying things should be as extensible as possible, so that only encapsulation is required in the subsequent development process, so as to avoid repeated development at the bottom layer in order to expand a certain feature.

@fitzthum
Copy link
Contributor

fitzthum commented May 10, 2023

In my opinion the kernel parameter here to specify these configurations are underlying things and they'd better be flexible enough. For the convenience of users, we can consider a CLI tool or something that receives a simple request from user and translate that request into a complex one.

Maybe we could implement this in the shim? Rather than setting the kernel params directly the user could set an annotation that specifies some kind of pod identity. The shim would use this to formulate the path for the configs and add them to the kernel params.

@Xynnn007
Copy link
Contributor Author

Xynnn007 commented May 11, 2023

Maybe we could implement this in the shim? Rather than setting the kernel params directly the user could set an annotation that specifies some kind of pod identity. The shim would use this to formulate the path for the configs and add them to the kernel params.

I like this idea. Maybe the process can be described as the following:

  1. shim parses the identity of the pod and the address of KBS from annotation. Let's assume the identity is "A" and the address "addr"
  2. shim derives the three parameters: kbs://add/<repo>/security-policy/A, kbs://add/<repo>/credential/A and kbs://add/<repo>/sigstore-config/A. These three parameters can then be injected by kernel commandline.

Up to know, AS can already know the requested policy and the identity of the pod. We can think whether we also need to embed the identity into CONFIGID-like fields before launch the VM.

@stevenhorsman
Copy link
Member

Thanks Ding, this PR looks good and would help a use-case that we've heard in the peer pods community where people want to effectively use the offline_fs_kbc for image pull credentials, but cc_kbc for pulling runtime secrets/decryption keys from remote attestation.

From what I understand this would allow us to just embed the cc_kbc module built attestation-agent into the peer pod VM image and then rather than use the offline_fs_kbc writing our credentials to /etc/aa-offline_fs_kbc-resources.json, we can update our dynamic image pull secret injector to write to somewhere like /etc/containers/auth.json and then set image_registry_auth_file=file:///etc/containers/auth.json in /etc/agent-config.toml in the peer pod VM image.

I guess with the remote-hypervisor we don't use annotations, or the shim to set the kernel_params, so Tobin's question isn't so important. I do prefer the idea of the annotation over overloading the kernel_params even more than we already do though!

@Xynnn007
Copy link
Contributor Author

@stevenhorsman Sounds great to me Steve!

@fitzthum Hey Tobin, can we get this PR approved/merged now? It can be done in future about the derivation of the kernel commandline.

Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

LGTM - thanks @Xynnn007.
Just one question - will we need to bump image-rs in order to test this, or is the 0.6 release version compatible with this?

@Xynnn007
Copy link
Contributor Author

LGTM - thanks @Xynnn007.
Just one question - will we need to bump image-rs in order to test this, or is the 0.6 release version compatible with this?

v0.6.0 is compatible with this.

@stevenhorsman
Copy link
Member

LGTM - thanks @Xynnn007.
Just one question - will we need to bump image-rs in order to test this, or is the 0.6 release version compatible with this?

v0.6.0 is compatible with this.

Thanks - Do you think it is worth adding some CI tests for this behaviour and then we can repurpose them to check the annotation when that gets added?

@Xynnn007
Copy link
Contributor Author

LGTM - thanks @Xynnn007.
Just one question - will we need to bump image-rs in order to test this, or is the 0.6 release version compatible with this?

v0.6.0 is compatible with this.

Thanks - Do you think it is worth adding some CI tests for this behaviour and then we can repurpose them to check the annotation when that gets added?

Ci test sounds good to me. With the base CI we can extend it into different aims, including the stated annotation test. Furthermore, any potential test in future that would have a specific parameter in kernel parameter would benefit from this, like the dm-verity for encrypted rootfs.

@stevenhorsman
Copy link
Member

Ci test sounds good to me. With the base CI we can extend it into different aims, including the stated annotation test. Furthermore, any potential test in future that would have a specific parameter in kernel parameter would benefit from this, like the dm-verity for encrypted rootfs.

Would you be able to raise an issue on the tests repo for this? I won't have capacity to look at it myself in the short-term, but might be able to find someone else to help and otherwise it would be good to track so I don't forget about it.

Copy link
Contributor

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

LGTM. This is against CCv0 so we may need to introduce again in main before too long.

@fitzthum
Copy link
Contributor

One thing to mention is that we may need to think about the best way to handle some of these features from the KBS side. For instance, we are introducing the ability for different pods to request different resources, but we don't really have any logic to group resources together in the KBS.

@stevenhorsman
Copy link
Member

I flagged this up on last weeks CoCo call as available to review and there have been no takers, so I think we can merge it in now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/large Task of significant size
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants