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

buildctl: Add configured TLS certificate to trust store when making calls to registry auth #4211

Merged
merged 1 commit into from
Sep 16, 2023

Conversation

njucjc
Copy link
Contributor

@njucjc njucjc commented Sep 7, 2023

Add configured TLS certificate to trust store when making calls to registry auth

@tonistiigi
Copy link
Member

#3689 (comment)

@njucjc
Copy link
Contributor Author

njucjc commented Sep 11, 2023

How about add an --auth-tls-config option to sovle this problem,as following:

[auth."registry.infra.svc.cluster.local:5000"]
  insecure = false
  ca: ["/path/to/my/ca.crt"]
  key:  ["/path/to/my/key.crt"]
  cert:  ["/path/to/my/cert.crt"]

@tonistiigi
Copy link
Member

tonistiigi commented Sep 11, 2023

The problem is that TOML config is only for buildkitd atm. while these parameters are mostly for buildctl. Eg. in remote cases these are completely different machines and reading buildkitd config from buildctl will become confusing.

Note that theoretically, the auth requests could happen either form client or the daemon side. By default it is client, but the client can refuse and instead send the registry credentials to the daemon where it would make the auth request. This is mostly for backward compatibility as the feature of avoiding sending credentials to daemon was added later.

I'd be ok with current flags but they need to take host=value as the parameter, not just the value(and multiple should be allowed).

@njucjc
Copy link
Contributor Author

njucjc commented Sep 11, 2023

The problem is that TOML config is only for buildkitd atm. while these parameters are mostly for buildctl. Eg. in remote cases these are completely different machines and reading buildkitd config from buildctl will become confusing.

Note that theoretically, the auth requests could happen either form client or the daemon side. By default it is client, but the client can refuse and instead send the registry credentials to the daemon where it would make the auth request. This is mostly for backward compatibility as the feature of avoiding sending credentials to daemon was added later.

I'd be ok with current flags but they need to take host=value as the parameter, not just the value(and multiple should be allowed).

how about this way

buildctl --registry-auth-tlscacert  registry.infra.svc.cluster.local=/path/to/my/ca.crt \
              --registry-auth-tlskeypair registry.infra.svc.cluster.local=/path/to/my/cert.crt:/path/to/my/key.crt

@tonistiigi
Copy link
Member

@njucjc SGTM. In tlskeypair this could be two flags, or separate with comma. I think if we start combining fields, then better to just do one csv flag, eg. smth like the docker context field https://docs.docker.com/engine/reference/commandline/context_update/#update-an-existing-context

FYI @AkihiroSuda

@njucjc
Copy link
Contributor Author

njucjc commented Sep 12, 2023

@njucjc SGTM. In tlskeypair this could be two flags, or separate with comma. I think if we start combining fields, then better to just do one csv flag, eg. smth like the docker context field https://docs.docker.com/engine/reference/commandline/context_update/#update-an-existing-context

FYI @AkihiroSuda

I think create an csv flag is better, because tlskeypair is related to each other, it is not suitable to separate when specifying the host. The new csv flag is as following:

buildctl --registry-auth-tlscontext host=https://myserver:2376,ca=/path/to/my/ca.crt,cert=/path/to/my/cert.crt,key=/path/to/my/key.crt

@njucjc njucjc force-pushed the support-auth-tls branch 3 times, most recently from 6a1b87e to d2b6df8 Compare September 12, 2023 04:23
@AkihiroSuda
Copy link
Member

@njucjc
Copy link
Contributor Author

njucjc commented Sep 12, 2023

This is mostly for backward compatibility as the feature of avoiding sending credentials to daemon was added later.

@AkihiroSuda TOML config is only for buildkitd atm, but by default, the auth request is happened from client (buildctl) side, I think use a csv flag is better and easy in this case. cc @tonistiigi

@tonistiigi
Copy link
Member

tonistiigi commented Sep 12, 2023

@AkihiroSuda Do you think we should support host dirs for buildkitd in general? I'm fine with it but, yes for this case /etc doesn't really work well as buildctl is not supposed to need root and access to /etc.

edit: sorry, I was actually referring to CRI pattern, but I don't think this is what you meant.

@AkihiroSuda
Copy link
Member

I meant that the dir structure should conform to /etc/containerd/certs.d, but its path can be like ~/.config/buildctl/certs.d or wherever else

@tonistiigi
Copy link
Member

Let's add the flags in buildctl for now. If we add ~/ config dir then we should probably use ~/.docker because this is where we load the credentials. But in that case, we should make sure that other tools also use the same config, so it requires a bigger coordination effort.

@njucjc
Copy link
Contributor Author

njucjc commented Sep 12, 2023

Let's add the flags in buildctl for now. If we add ~/ config dir then we should probably use ~/.docker because this is where we load the credentials. But in that case, we should make sure that other tools also use the same config, so it requires a bigger coordination effort.

+1

@njucjc
Copy link
Contributor Author

njucjc commented Sep 13, 2023

@AkihiroSuda @tonistiigi PTAL

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Design looks good, but ~ has to be expanded to the home dir

@AkihiroSuda
Copy link
Member

0.786 ERROR: Docs result differs. Please update with "make docs"
0.807  M docs/reference/buildctl.md

https://github.com/moby/buildkit/actions/runs/6168240536/job/16740610078?pr=4211

@njucjc
Copy link
Contributor Author

njucjc commented Sep 13, 2023

0.786 ERROR: Docs result differs. Please update with "make docs"
0.807  M docs/reference/buildctl.md

https://github.com/moby/buildkit/actions/runs/6168240536/job/16740610078?pr=4211

updated! @AkihiroSuda

@njucjc njucjc force-pushed the support-auth-tls branch 2 times, most recently from 906c113 to e4cfe34 Compare September 14, 2023 05:52
…alls to registry auth

Signed-off-by: njucjc <njucjc@gmail.com>
@njucjc
Copy link
Contributor Author

njucjc commented Sep 16, 2023

@tonistiigi @AkihiroSuda PTAL

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