feat add cli option to add credentials files #5900

Shubhranshu153 · 2025-04-09T20:53:20Z

Description

Add a cli option to pass in credential config file path. This will override DOCKER_CONFIG path used to fetch credential. You can pass a file path or a file descriptor("fd://") or a pipe("pipe://")

Use case:

This will allow clients to configure the cred config location of the buildctl and pass in credentials via file descriptors and pipes as well if they dont want to write it to disk.

Related to issue: #5860

Shubhranshu153 · 2025-04-09T21:00:10Z

@tonistiigi @AkihiroSuda
When you get a chance take a look.
Thanks.

pendo324 · 2025-04-09T21:20:12Z

cmd/buildctl/build.go

+		if err != nil {
+			return nil, errors.Wrap(err, "parsing file descriptor")
+		}
+		f := os.NewFile(uintptr(fd), "")


Does go throw an error here if the file descriptor is above the OS limit (e.g. /proc/sys/fs/file-max or ulimit on Linux)?

I think the open call will fail if it is more than the limit. The newfile itself doesnt throw error.

There is no open if it is a fd. read(2) would fail.

fd is from a open file that is getting inherrited as a child process. if the fd is not inherited it would fail from what i tested which makes sense to me for the use case i am trying.
Do you suggest i check if the fd is valid and open already? @tonistiigi

pendo324 · 2025-04-09T22:10:33Z

cmd/buildctl/build.go

+		if err := configFile.LoadFromReader(f); err != nil {
+			return nil, errors.Wrap(err, fmt.Sprintf("loading config file %s", path))
+		}
+	case strings.HasPrefix(path, "pipe://"):


Should this work with Windows pipes? I think they start with \\.\pipe\ typically, but I'm not sure if they're opened like a regular file.

i am not very familiar with using windows pipe, if there is a specific requirement we can implement but at the moment i dont know if we do require it.

@profnandaa

cmd/buildctl/build.go

AkihiroSuda · 2025-04-10T00:27:33Z

cmd/buildctl/build.go

+		if err := configFile.LoadFromReader(f); err != nil {
+			return nil, errors.Wrapf(err, "loading config file %s", path)
+		}
+	case strings.HasPrefix(path, "pipe://"):


On Linux, pipe:// is probably unneeded and can be just treated in the same way as a regular file

it was more of a logical entry to differentiate, can remove it.

tonistiigi · 2025-04-10T00:31:46Z

cmd/buildctl/build.go

+	if path == "" {
+		return nil, fmt.Errorf("credential file path empty")
+	}
+	configFile := configfile.New("config-file")


This is a bit scary. There are functions like configFile.Save() that would now write out actual file to this path.

Maybe it would make sense for DockerAuthProviderConfig to take a limited interface that just implements GetAuthConfig(registry) instead? @AkihiroSuda

Does buildkit write back credentials to this file? or its more of a concern with it is possible to write back to the path and as such is a risk itself.

It's more of a concern that we pass full configfile to another component, so we can't really control what methods it is allowed to call. We are passing configfile, but it isn't actually a valid file.

in that case when the further component call it to write it would fail i suppose which is not ideal but is it a risk though as the config file if its a fd then it needs to be called as a child process or else it wont be valid. For pipes it is more tricky i suppose as they can be passed independently.
@tonistiigi @AkihiroSuda

tonistiigi · 2025-04-10T00:32:15Z

cmd/buildctl/build.go

+		if err != nil {
+			return nil, errors.Wrap(err, "parsing file descriptor")
+		}
+		f := os.NewFile(uintptr(fd), "")


There is no open if it is a fd. read(2) would fail.

tonistiigi · 2025-04-10T00:33:23Z

cmd/buildctl/build.go

+		if err := configFile.LoadFromReader(f); err != nil {
+			return nil, errors.Wrap(err, fmt.Sprintf("loading config file %s", path))
+		}
+	case strings.HasPrefix(path, "pipe://"):


@profnandaa

Shubhranshu153 · 2025-04-10T00:59:35Z

@tonistiigi
There is no open if it is a fd. read(2) would fail -> Open file descriptor needs to be passed along when a os.exec call is made

tonistiigi

Linter is failing, and I would still prefer to not pass the whole (fake) configfile.

Shubhranshu153 · 2025-04-24T22:00:56Z

@tonistiigi

 I would still prefer to not pass the whole (fake) configfile.

When we say pass the whole configfile are we stating the whole configfile struct into DockerAuthProvider and any component that uses DockerAuthProviderConfig has access to this configfile and we cannot control the use of it?

Shubhranshu153 · 2025-04-28T17:25:44Z

@tonistiigi @AkihiroSuda
Added interface implementing GetAuthConfig(registry) as per suggestion to circumnavigate configfile.Save(). Let me know if this solution works. To safegaurd behaviour may be i can add all the implementation and override Save() if its required.

tonistiigi · 2025-04-28T17:35:15Z

session/auth/authprovider/authprovider.go

@@ -32,6 +31,11 @@ import (
 	"google.golang.org/grpc/status"
 )

+// ConfigFile Interface exposes GetAuthConfig method.
+type ConfigFile interface {


AuthConfigProvider

tonistiigi · 2025-04-28T17:36:30Z

cmd/buildctl/build.go

+	return err
+}
+
+func processCredentialConfigFile(path string) (*configfile.ConfigFile, error) {


This should return the interface type

Thanks changed it.

Signed-off-by: Shubharanshu Mahapatra <shubhum@amazon.com>

github-actions bot added area/testing area/docs area/buildctl labels Apr 9, 2025

Shubhranshu153 force-pushed the feat-creds-by-fd branch from afdd91b to cae4b78 Compare April 9, 2025 20:56

pendo324 reviewed Apr 9, 2025

View reviewed changes

AkihiroSuda reviewed Apr 10, 2025

View reviewed changes

cmd/buildctl/build.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Apr 10, 2025

View reviewed changes

cmd/buildctl/build.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Apr 10, 2025

View reviewed changes

cmd/buildctl/build.go Outdated Show resolved Hide resolved

Shubhranshu153 force-pushed the feat-creds-by-fd branch 2 times, most recently from db09cc6 to 7626804 Compare April 10, 2025 00:18

AkihiroSuda approved these changes Apr 10, 2025

View reviewed changes

AkihiroSuda reviewed Apr 10, 2025

View reviewed changes

tonistiigi reviewed Apr 10, 2025

View reviewed changes

tonistiigi reviewed Apr 17, 2025

View reviewed changes

Shubhranshu153 force-pushed the feat-creds-by-fd branch from 7626804 to 955346e Compare April 24, 2025 22:04

Shubhranshu153 closed this Apr 25, 2025

Shubhranshu153 reopened this Apr 25, 2025

Shubhranshu153 force-pushed the feat-creds-by-fd branch from 955346e to 1227ab0 Compare April 28, 2025 17:22

github-actions bot added the area/session label Apr 28, 2025

tonistiigi reviewed Apr 28, 2025

View reviewed changes

Shubhranshu153 force-pushed the feat-creds-by-fd branch from 1227ab0 to af008c4 Compare April 28, 2025 20:37

feat add cli option to add credentials files

81c1d5b

Signed-off-by: Shubharanshu Mahapatra <shubhum@amazon.com>

Shubhranshu153 force-pushed the feat-creds-by-fd branch from af008c4 to 81c1d5b Compare April 28, 2025 20:50

feat add cli option to add credentials files #5900

Are you sure you want to change the base?

feat add cli option to add credentials files #5900

Conversation

Shubhranshu153 commented Apr 9, 2025

Description

Use case:

Uh oh!

Shubhranshu153 commented Apr 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shubhranshu153 commented Apr 10, 2025

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

Shubhranshu153 commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shubhranshu153 commented Apr 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Shubhranshu153 commented Apr 24, 2025 •

edited

Loading