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

models, containerd, ecs-agent, host-ctr: support registry credentials #1955

Merged
merged 6 commits into from
Mar 7, 2022

Conversation

etungsten
Copy link
Contributor

@etungsten etungsten commented Feb 15, 2022

Issue number:
Resolves #1227

Description of changes:

Author: Erikson Tung <[email protected]>
Date:   Mon Feb 14 17:03:36 2022 -0800

    host-ctr: configure resolver's registry auth based on registry config
    
    host-ctr will set up the custom resolver's authorizer based on the
    registry credentials information stored in host-ctr.toml.
Author: Erikson Tung <[email protected]>
Date:   Mon Feb 14 16:20:38 2022 -0800

    host-ctr: move registryHosts to the registry pkg
Author: Erikson Tung <[email protected]>
Date:   Mon Feb 14 17:26:10 2022 -0800

    models, containerd, ecs-agent: add 'container-registry.credentials'
    
    This adds support for configuring registry auth credentials when pulling
    images. The setting is used to render containerd, ecs-agent
    configuration to enable authenticated pulls with the configured auth
    information.

Testing done:
I created a private docker image registry, "docker.io/etungsten/nginx", that's just a clone of the nginx image repository.

Registry credentials is provided via apiclient in the control container like so:

apiclient set --json '{"container-registry": {
      "credentials": [
        {
          "registry": "docker.io",
          "auth": "myauthstring"
        }
      ]
    }}'

For ECS variants:
The affected-services all restart as expected.

The rendered ECS configuration file:

bash-5.0# cat /etc/ecs/ecs.config
ECS_LOGFILE=/var/log/ecs/ecs-agent.log
ECS_LOGLEVEL="info"
ECS_ENGINE_AUTH_TYPE=dockercfg
ECS_ENGINE_AUTH_DATA='{"https://index.docker.io/v1/":{"email": ".","auth": "myauthstring"}}'

Ran an ECS tasks that runs the nginx image from my private registry. The task runs successfully.
Checking ECS agent logs. The image gets pulled successfully using the creds I provided

Feb 15 02:42:43 amazon-ecs-agent[3412]: level=info time=2022-02-15T02:42:43Z msg="Task engine [arn:aws:ecs:us-west-2:blep:task/566afd42-3dac-44d4-89cd-23ec62c07baa]: finished pulling image etungsten/nginx:latest for container test-ecs in 5.15496234s" module=docker_task_engine.go
Feb 15 02:42:43 amazon-ecs-agent[3412]: level=info time=2022-02-15T02:42:43Z msg="Handling container change event" taskARN="arn:aws:ecs:us-west-2:blep:task/566afd42-3dac-44d4-89cd-23ec62c07baa" container="test-ecs" runtimeID="" status="PULLED"
...
[ec2-user@ip-172-31-27-24 ~]$ curl localhost:80
<!DOCTYPE html>
<html>
<head>
<title>Welcome to nginx!</title>
...

For K8s variants:
The affected-services all restart as expected.

The rendered containerd configuration:

bash-5.0# cat /etc/containerd/config.toml 
...
[plugins."io.containerd.grpc.v1.cri".registry.configs."registry-1.docker.io".auth]
auth = "myauthstring"

I'm able to run a K8s pod that has a nginx container that pulls from my private registry, the manifest:

apiVersion: v1
kind: Pod
metadata:
  name: nginx
spec:
  containers:
  - name: nginx
    image: docker.io/etungsten/nginx:latest
    imagePullPolicy: Always

containerd logs shows it gets pulled successfully:

Feb 15 03:44:43 containerd[24471]: time="2022-02-15T03:44:43.020414307Z" level=info msg="PullImage \"docker.io/etungsten/nginx:latest\" returns image reference \"sha256:c316d5a335a5cf324b0dc83b3da82d7608724769f6454f6d9a621f3ec2534a5a\""
Feb 15 03:44:43 containerd[24471]: time="2022-02-15T03:44:43.022183856Z" level=info msg="CreateContainer within sandbox \"2f690146d2f24782bfcd86a18ccea5e385a103f922d6983a765bba2bbf72fcc4\" for container &ContainerMetadata{Name:nginx,Attempt:0,}"

The pod runs successfully:

$ kubectl get pods -A
NAMESPACE     NAME                       READY   STATUS    RESTARTS   AGE
default       nginx                      1/1     Running   0          3m40s
...

For host-containers:
The rendered host-ctr configuration:

bash-5.0# cat /etc/host-containers/host-ctr.toml
[creds."registry-1.docker.io"]
auth = "myauthstring"

I specified an nginx host-container via user-data:

[settings.host-containers.nginx]
source = "docker.io/etungsten/nginx:latest"
enabled = true

After configuring my registry credentials via apiclient, the host-container is able to pull the image and start:

bash-5.0# journalctl --no-hostname -u host-containers@nginx
Feb 15 02:58:34 systemd[1]: Started Host container: nginx.
Feb 15 02:58:36 host-ctr[3850]: time="2022-02-15T02:58:36Z" level=info msg="pulled image successfully" img="docker.io/etungsten/nginx:latest"
Feb 15 02:58:36 host-ctr[3850]: time="2022-02-15T02:58:36Z" level=info msg="unpacking image..." img="docker.io/etungsten/nginx:latest"
...

To test selinux changes, I tried accessing the container runtime configuration files from an unprivileged container and it fails as expected:

root@ubuntu:/test-rc# ls -lZ
total 68
....
drwxrwxrwt. 3 root root system_u:object_r:any_t:s0      60 Mar  1 01:15 cni
drwxrwxrwt. 2 root root system_u:object_r:secret_t:s0   60 Mar  1 01:15 containerd
...
drwxr-x---. 2 root root system_u:object_r:secret_t:s0  120 Mar  1 01:15 host-containers
....
drwxr-xr-x. 3 root root system_u:object_r:etc_t:s0     140 Mar  1 01:15 wicked
root@ubuntu:/test-rc# cd containerd/
bash: cd: containerd/: Permission denied
root@ubuntu:/test-rc# ls containerd/
ls: cannot open directory 'containerd/': Permission denied

Checked dmesg on the host and I see AVC denials as expected:

[  365.061339] audit: type=1400 audit(1646097682.561:5): avc:  denied  { search } for  pid=5436 comm="bash" name="/" dev="tmpfs" ino=1 scontext=system_u:system_r:container_t:s0:c327,c498 tcontext=system_u:object_r:secret_t:s0 tclass=dir permissive=0
[  365.078879] audit: type=1400 audit(1646097682.561:6): avc:  denied  { search } for  pid=5436 comm="bash" name="/" dev="tmpfs" ino=1 scontext=system_u:system_r:container_t:s0:c327,c498 tcontext=system_u:object_r:secret_t:s0 tclass=dir permissive=0
[  367.830809] audit: type=1400 audit(1646097685.329:7): avc:  denied  { read } for  pid=5573 comm="ls" name="/" dev="tmpfs" ino=1 scontext=system_u:system_r:container_t:s0:c327,c498 tcontext=system_u:object_r:secret_t:s0 tclass=dir permissive=0

Tested migration and they worked as expected. On downgrade the new credential settings gets deleted as expected and the OS boots up fine.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@etungsten etungsten changed the title Registry credentials models,containerd,ecs-agent,host-ctr: support registry credentials Feb 15, 2022
@etungsten etungsten force-pushed the registry-credentials branch 4 times, most recently from ec21e6e to 313109f Compare February 15, 2022 17:06
@etungsten etungsten changed the title models,containerd,ecs-agent,host-ctr: support registry credentials models, containerd, ecs-agent, host-ctr: support registry credentials Feb 15, 2022
@etungsten etungsten force-pushed the registry-credentials branch 3 times, most recently from fe8ed66 to b970624 Compare February 16, 2022 00:38
@stevehipwell
Copy link

@etungsten how much more work do you think is required to get this production ready? I'm just about to look at the steps to make Bottlerocket a production ready option for our clusters and #1227 is the final real hurdle.

Our current pattern for AL2 is to persist a read only set of Docker credentials to /var/lib/kubelet/config.json as part of the bootstrap, these credentials are rotated in the LT which will be applied to new instances. Our nodes are usually replaced within 2 weeks and always updated within 4 weeks. We'd like to follow the same pattern with Bottlerocket including the full node replacement (we're not going to use the built in OS update).

@etungsten
Copy link
Contributor Author

Hi @stevehipwell , thanks for reaching out. Unfortunately I can't give a firm date on when this feature will be in a formal release. There are still discussions to be had on these changes. Feel free to subscribe to notifications as I update this PR. I'll also try to be as detailed as I can in the PR description to give a sense of what the progress is. In general, once this PR gets merged, it would most likely be in the next minor release after that.

@etungsten
Copy link
Contributor Author

Push above sets up mount points for directories for where we write out configuration files that might contain secrets so we can set up selinux labels for them

@etungsten etungsten force-pushed the registry-credentials branch 5 times, most recently from 0fa9f03 to bd66dc7 Compare February 22, 2022 20:32
Copy link
Contributor

@webern webern left a comment

Choose a reason for hiding this comment

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

Is there a migration needed?

sources/host-ctr/cmd/host-ctr/registry.go Outdated Show resolved Hide resolved
@etungsten
Copy link
Contributor Author

etungsten commented Mar 1, 2022

Push above fixes the mount unit so that the SElinux labels actually apply to the mount directory.

Tested things and they all still work as expected.

Also tried accessing the container runtime configuration files from an unprivileged container and it fails as expected:

root@ubuntu:/test-rc# ls -lZ
total 68
....
drwxrwxrwt. 3 root root system_u:object_r:any_t:s0      60 Mar  1 01:15 cni
drwxrwxrwt. 2 root root system_u:object_r:secret_t:s0   60 Mar  1 01:15 containerd
...
drwxr-x---. 2 root root system_u:object_r:secret_t:s0  120 Mar  1 01:15 host-containers
....
drwxr-xr-x. 3 root root system_u:object_r:etc_t:s0     140 Mar  1 01:15 wicked
root@ubuntu:/test-rc# cd containerd/
bash: cd: containerd/: Permission denied
root@ubuntu:/test-rc# ls containerd/
ls: cannot open directory 'containerd/': Permission denied

Checked dmesg on the host and I see AVC denials as expected:

[  365.061339] audit: type=1400 audit(1646097682.561:5): avc:  denied  { search } for  pid=5436 comm="bash" name="/" dev="tmpfs" ino=1 scontext=system_u:system_r:container_t:s0:c327,c498 tcontext=system_u:object_r:secret_t:s0 tclass=dir permissive=0
[  365.078879] audit: type=1400 audit(1646097682.561:6): avc:  denied  { search } for  pid=5436 comm="bash" name="/" dev="tmpfs" ino=1 scontext=system_u:system_r:container_t:s0:c327,c498 tcontext=system_u:object_r:secret_t:s0 tclass=dir permissive=0
[  367.830809] audit: type=1400 audit(1646097685.329:7): avc:  denied  { read } for  pid=5573 comm="ls" name="/" dev="tmpfs" ino=1 scontext=system_u:system_r:container_t:s0:c327,c498 tcontext=system_u:object_r:secret_t:s0 tclass=dir permissive=0

@etungsten
Copy link
Contributor Author

Push above adds settings descriptions to the README and the migrations for the new setting.

@etungsten etungsten marked this pull request as ready for review March 1, 2022 23:37
@etungsten
Copy link
Contributor Author

Push above fixes a typo caught by @webern

@etungsten
Copy link
Contributor Author

Push above fixes the key name for identitytoken in hostr-ctr.toml config file.

Release.toml Outdated Show resolved Hide resolved
Comment on lines +43 to +47
{{#if (eq registry "docker.io" )}}
[plugins."io.containerd.grpc.v1.cri".registry.configs."registry-1.docker.io".auth]
{{else}}
[plugins."io.containerd.grpc.v1.cri".registry.configs."{{registry}}".auth]
{{/if}}
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a potential edge case if someone specifies docker.io and registry-1.docker.io, where we'd end up with two entries. Not sure if that's worth handling though, especially if it doesn't cause an error in the client. I'd be OK with undefined or non-deterministic behavior.

Copy link
Contributor Author

@etungsten etungsten Mar 4, 2022

Choose a reason for hiding this comment

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

Good call out. I tested this and containerd does complain about duplicate entries and then refuse to start:

Mar 04 00:27:32 containerd[124031]: containerd: failed to load TOML: /etc/containerd/config.toml: (37, 2): duplicated tables
Mar 04 00:27:32 systemd[1]: containerd.service: Main process exited, code=exited, status=1/FAILURE
Mar 04 00:27:32 systemd[1]: containerd.service: Failed with result 'exit-code'.
Mar 04 00:27:32 systemd[1]: Failed to start containerd container runtime.

But I kinda see setting both docker.io and registry-1.docker.io akin to setting the same registry twice which would also make containerd fail.

We do not enforce uniqueness of the registries in the container-registry.credentials settings model. We would essentially need to wrap Vec<RegistryCredential> into its own settings model type that would special case docker.io and enforce uniqueness of the registry field. Lemme know what you think!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, I can remove this conditional and we can just make users specify registry-1.docker.io when they want to set up credentials for dockerhub. And for ECS variants, https://index.docker.io/v1/.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm good with your current approach, after further thought and based on our discussion out of band.

Without the conditional, on aws-ecs-1 to override registry credentials for Docker Hub, you'd need to specify both so that host-ctr and ecs-agent both get the form they expect (and ignore the other form). That's awkward and confusing enough that it's worth the extra bit implementation effort to paper over it.

packages/host-ctr/etc-host\x2dcontainers.mount Outdated Show resolved Hide resolved
packages/host-ctr/host-ctr.spec Outdated Show resolved Hide resolved
sources/models/src/aws-ecs-1/defaults.d/51-aws-ecs-1.toml Outdated Show resolved Hide resolved
packages/ecs-agent/ecs.config Outdated Show resolved Hide resolved
sources/host-ctr/cmd/host-ctr/registry.go Outdated Show resolved Hide resolved
@etungsten
Copy link
Contributor Author

Push above rebases onto latest develop.

@etungsten etungsten force-pushed the registry-credentials branch 2 times, most recently from b3659de to eaf229b Compare March 4, 2022 19:12
@etungsten
Copy link
Contributor Author

etungsten commented Mar 4, 2022

This push addresses majority of @bcressey 's comments.

The push above fixes formatting.

Tested things and they still work as described in the PR.

@etungsten
Copy link
Contributor Author

etungsten commented Mar 4, 2022

Push above moves the migrations to "(1.6.1, 1.6.2)" now that we're planning this for v1.6.2

Copy link
Contributor

@arnaldo2792 arnaldo2792 left a comment

Choose a reason for hiding this comment

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

The commit message that starts with this:

ecs-agent, host-ctr, containerd: add mount units for configuration di

Seems truncated

README.md Outdated Show resolved Hide resolved
@etungsten
Copy link
Contributor Author

The commit message that starts with this:

ecs-agent, host-ctr, containerd: add mount units for configuration di

Seems truncated

The title is truncated because it was too long, but the message is still there.

@etungsten
Copy link
Contributor Author

Push above fixes typo found by @arnaldo2792

@etungsten
Copy link
Contributor Author

Tested migration again and it still works as expected

This adds support for configuring registry auth credentials when pulling
images. The setting is used to render containerd, ecs-agent
configuration to enable authenticated pulls with the configured auth
information.
host-ctr will set up the custom resolver's authorizer based on the
registry credentials information stored in host-ctr.toml.
…rectories

containerd,ecs-agent,host-ctr: add context to configuration directory mounts

We set selinux labels to the mount options so that under-privileged
processes won't be able to read registry credential secrets from the
container runtime configuration files.
@etungsten
Copy link
Contributor Author

Push above rebases to fix conflicts in Release.toml and in sources/Cargo.toml

@etungsten etungsten merged commit 7282bf5 into bottlerocket-os:develop Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Configure image registry credentials in containerd
5 participants