Repository: moby/buildkit

Issue:
#5886 broke tlsdir option
### Contributing guidelines and issue reporting guide

- [x] I've read the [contributing guidelines](https://github.com/moby/buildkit/blob/master/.github/CONTRIBUTING.md) and wholeheartedly agree. I've also read the [issue reporting guide](https://github.com/moby/buildkit/blob/master/.github/issue_reporting_guide.md).

### Well-formed report checklist

- [x] I have found a bug that the documentation does not mention anything about my problem
- [x] I have found a bug that there are no open or closed issues that are related to my problem
- [x] I have provided version/information about my environment and done my best to provide a reproducer

### Description of bug

## Bug description

Title says it all...   The `--tlsdir` option no longer works.

With `.pem` files, `buildctl` complains about not finding `ca.crt`.
With `cert-manager`-style naming, it's `ca.pem` that's missing.

~~The changes introduced in #5886 are so obviously wrong,
I'm surprised that the PR was ever merged.
Did anyone ever test the changes??~~

~~If _any_ of the 6 files fails to `stat`, an error is returned.
I suspect that's **not** the intended behavior.~~

**Constructive revision suggested by ChatGPT:**
> The current implementation requires that all six TLS-related files be present—otherwise, it fails early.
> This behavior appears to be unintended and can be confirmed by reviewing the relevant code paths.
>
> It seems the PR may not have been tested before merging, and this oversight was missed during review.

## Reproduction

```console
$ docker run --rm -it --entrypoint sh moby/buildkit:rootless -c 'for i in key cert ca; do touch /tmp/$i.pem; done; ls -l /tmp; buildctl --tlsdir /tmp build'
total 0
-rw-r--r--    1 user     user             0 Apr 18 11:41 ca.pem
-rw-r--r--    1 user     user             0 Apr 18 11:41 cert.pem
-rw-r--r--    1 user     user             0 Apr 18 11:41 key.pem
error: stat /tmp/ca.crt: no such file or directory
$ docker run --rm -it --entrypoint sh moby/buildkit:rootless -c 'for i in tls.key tls.crt ca.crt; do touch /tmp/$i; done; ls -l /tmp; buildctl --tlsdir /tmp build'
total 0
-rw-r--r--    1 user     user             0 Apr 18 11:42 ca.crt
-rw-r--r--    1 user     user             0 Apr 18 11:42 tls.crt
-rw-r--r--    1 user     user             0 Apr 18 11:42 tls.key
error: stat /tmp/ca.pem: no such file or directory
$
```


## Version information

```console
$ docker run --rm -it --entrypoint sh moby/buildkit:rootless -c 'buildctl --version'
buildctl github.com/moby/buildkit v0.21.0 52b004d2afe20c5c80967cc1784e718b52d69dae
$ 
```


Discussion:
@andraxin: @tonistiigi  @GoodOwl @jsternberg: @andraxin we'll take a look into this but please remember to be polite in accordance with the [Moby community guidelines](https://github.com/moby/moby/blob/master/CONTRIBUTING.md#moby-community-guidelines). It's understandable to have some frustration when a bug ends up in the codebase and breaks some existing piece of functionality, but it's inevitable that stuff like that will happen in a large open source project. Directing comments that can be interpreted as inflammatory or insulting is something that is not conducive to the community we want to build.

We're all trying our best and appreciate the work of both internal and external contributors in making a good piece of software.@andraxin: @jsternberg  
I'm somewhat challenged in the political correctness department.
So much so, that I had to consult an independent 3rd-party... 😏 

Here's what ChatGPT told me (that I wish you did):

> Key tone issues:
> 
>  -   Phrases like "so obviously wrong", "I'm surprised that the PR was ever merged", and "Did anyone ever test the changes??" come across as dismissive and inflammatory, rather than constructive.
> 
>  -   These statements may be interpreted as blaming or attacking contributors, which can be discouraging in a collaborative, volunteer-driven project.

No attack was intended, only pinpointing accountability.

I updated the issue description with a constructive version
suggested by ChatGPT.  I hope that helps.

BTW, on a closer look at the different guides, it would  seem
that @GoodOwl had violated the contributing guidelines by
submitting untested code and not including additional tests
for the proposed changes.

These appear to have _also_ been missed during review.

That said, I couldn't find any tests for the original behavior
of `--tlsdir` either; so, that might explain the oversight...@GoodOwl: Hey, sorry for the trouble I caused :( 
I really didn't try to test it with only only one set of files at a time.

I'll try to find a proper fix asap@thaJeztah: Looks like it created empty branches for the existing file names; unlike some other languages, golang switch statements don't have an implicit fall through; https://github.com/moby/buildkit/blob/e143ae102ad327cac07c9d2358b01b0e0b2c3c35/cmd/buildctl/common/common.go#L46-L55


It probably was meant to be;

```go
switch v {
case "ca.pem", "ca.crt":
	caCert = file
case "cert.pem", "tls.crt":
	cert = file
case "key.pem", "tls.key":
	key = file
}
```
@andraxin: > Looks like it created empty branches for the existing file names; unlike some other languages, golang switch statements don't have an implicit fall through;
> 
> [buildkit/cmd/buildctl/common/common.go](https://github.com/moby/buildkit/blob/e143ae102ad327cac07c9d2358b01b0e0b2c3c35/cmd/buildctl/common/common.go#L46-L55)
> 
> Lines 46 to 55 in [e143ae1](/moby/buildkit/commit/e143ae102ad327cac07c9d2358b01b0e0b2c3c35)
>  switch v { 
>  case "ca.pem": 
>  case "ca.crt": 
>  	caCert = file 
>  case "cert.pem": 
>  case "tls.crt": 
>  	cert = file 
>  case "key.pem": 
>  case "tls.key": 
>  	key = file 
> 
> It probably was meant to be;
> 
> switch v {
> case "ca.pem", "ca.crt":
> 	caCert = file
> case "cert.pem", "tls.crt":
> 	cert = file
> case "key.pem", "tls.key":
> 	key = file
> }

The no-op `case` statements could certainly be problematic in their own right.
Note, however, that the issue reported here would persist even with those fixed.
The errors reported come from `os.Stat` **before** the `switch` statement is reached
(or becomes effective, depending on which files are actually present).

The simplest fix is likely to omit the `else` clause altogether and fail later "as normal"
should any of the 3 required variables remain unset / empty (essentially, falling back
to handling any of the _other_ `--tls*` options being called with an empty string).

Also note that the current logic (which would remain with the suggested `else`-less fix)
suffers from "aliasing".  In other words, if **both** set of files are present, the ones listed
later (in this case, the `cert-manager` variants) will be used, which _may_ introduce new
failure modes, should the folder happen to contain independent sets of credentials from
different modalities, and users expected the `*.pem` versions getting picked up as before.@GoodOwl: I moved the tlsDir logic to a function and added tests to it to make sure both styles will work (pem or cert-manager).
I removed the `else` clause, as suggested.

Regarding the possibility of multiple credential files lying in the same folder, I think that the user should be responsible and explicitly use the "tlscacert/tlscert/tlskey" trio instead of relying on `--tlsdir` 

 I created an MR here https://github.com/moby/buildkit/pull/5935, please let me know if you think something can be improved@andraxin: @GoodOwl   
Thanks for the swift action!  I agree that the "aliasing" issue is an edge case
that can be resolved as you suggested.  I brought that up just for clarity.

Personally, I'd very much appreciate better documentation for this option.
The current one-liners leave a lot to the imagination (or trial and error): 
- https://github.com/moby/buildkit/blob/master/cmd/buildctl/main.go#L90
- https://github.com/moby/buildkit/blob/master/docs/reference/buildctl.md?plain=1#L32

Now, that 2 _different_ styles are supported, _maybe_ (just maybe) listing
both sets of names _explicitly_ would be helpful.  A word about "aliasing"
may or may not add much value, though.

Solution patch:
diff --git a/cmd/buildctl/common/common.go b/cmd/buildctl/common/common.go
index 93202c60b..05a77782e 100644
--- a/cmd/buildctl/common/common.go
+++ b/cmd/buildctl/common/common.go
@@ -30,38 +30,25 @@ func ResolveClient(c *cli.Context) (*client.Client, error) {
 		serverName = uri.Hostname()
 	}
 
-	var caCert string
-	var cert string
-	var key string
+	var (
+		caCert string
+		cert   string
+		key    string
+		err    error
+	)
 
 	tlsDir := c.GlobalString("tlsdir")
 
 	if tlsDir != "" {
-		// Look for ca.pem or ca.crt and, if it exists, set caCert to that
-		// Look for cert.pem or tls.crt and, if it exists, set cert to that
-		// Look for key.pem or tls.key and, if it exists, set key to that
-		for _, v := range [6]string{"ca.pem", "cert.pem", "key.pem", "ca.crt", "tls.crt", "tls.key"} {
-			file := filepath.Join(tlsDir, v)
-			if _, err := os.Stat(file); err == nil {
-				switch v {
-				case "ca.pem":
-				case "ca.crt":
-					caCert = file
-				case "cert.pem":
-				case "tls.crt":
-					cert = file
-				case "key.pem":
-				case "tls.key":
-					key = file
-				}
-			} else {
-				return nil, err
-			}
-		}
-
+		// Fail straight away if TLS was specified both ways
 		if c.GlobalString("tlscacert") != "" || c.GlobalString("tlscert") != "" || c.GlobalString("tlskey") != "" {
 			return nil, errors.New("cannot specify tlsdir and tlscacert/tlscert/tlskey at the same time")
 		}
+
+		caCert, cert, key, err = resolveTLSFilesFromDir(tlsDir)
+		if err != nil {
+			return nil, err
+		}
 	} else {
 		caCert = c.GlobalString("tlscacert")
 		cert = c.GlobalString("tlscert")
@@ -128,3 +115,29 @@ func ParseTemplate(format string) (*template.Template, error) {
 	}
 	return template.New("").Funcs(funcs).Parse(format)
 }
+
+// resolveTLSFilesFromDir scans a TLS directory for known cert/key filenames.
+func resolveTLSFilesFromDir(tlsDir string) (caCert, cert, key string, err error) {
+	oneOf := func(either, or string) (string, error) {
+		for _, name := range []string{either, or} {
+			fpath := filepath.Join(tlsDir, name)
+			if _, err := os.Stat(fpath); err == nil {
+				return fpath, nil
+			} else if !os.IsNotExist(err) {
+				return "", err
+			}
+		}
+		return "", errors.Errorf("directory did not contain one of the needed files: %s or %s", either, or)
+	}
+
+	if caCert, err = oneOf("ca.pem", "ca.crt"); err != nil {
+		return "", "", "", err
+	}
+	if cert, err = oneOf("cert.pem", "tls.crt"); err != nil {
+		return "", "", "", err
+	}
+	if key, err = oneOf("key.pem", "tls.key"); err != nil {
+		return "", "", "", err
+	}
+	return caCert, cert, key, nil
+}
diff --git a/cmd/buildctl/main.go b/cmd/buildctl/main.go
index 59f010448..76a251cd6 100644
--- a/cmd/buildctl/main.go
+++ b/cmd/buildctl/main.go
@@ -87,7 +87,7 @@ func main() {
 		},
 		cli.StringFlag{
 			Name:  "tlsdir",
-			Usage: "directory containing CA certificate, client certificate, and client key",
+			Usage: "directory containing CA certificate, client certificate, and client key. Supported file names are (ca.pem, cert.pem, key.pem) or (ca.crt, tls.crt, tls.key)",
 			Value: "",
 		},
 		cli.IntFlag{
diff --git a/docs/reference/buildctl.md b/docs/reference/buildctl.md
index 96e7e3539..2d97d0e54 100644
--- a/docs/reference/buildctl.md
+++ b/docs/reference/buildctl.md
@@ -29,7 +29,7 @@ GLOBAL OPTIONS:
    --tlscacert value      CA certificate for validation
    --tlscert value        client certificate
    --tlskey value         client key
-   --tlsdir value         directory containing CA certificate, client certificate, and client key
+   --tlsdir value         directory containing CA certificate, client certificate, and client key. Supported file names are (ca.pem, cert.pem, key.pem) or (ca.crt, tls.crt, tls.key)
    --timeout value        timeout backend connection after value seconds (default: 5)
    --wait                 block RPCs until the connection becomes available
    --help, -h             show help
diff --git a/frontend/dockerui/build.go b/frontend/dockerui/build.go
index fe2dbd05d..6ced71d67 100644
--- a/frontend/dockerui/build.go
+++ b/frontend/dockerui/build.go
@@ -61,16 +61,12 @@ func (bc *Client) Build(ctx context.Context, fn BuildFunc) (*ResultBuilder, erro
 			} else {
 				p = platforms.DefaultSpec()
 			}
-
-			k := platforms.FormatAll(p)
-			p = extendWindowsPlatform(p, img.Platform)
-			p = platforms.Normalize(p)
-
+			expPlat := makeExportPlatform(p, img.Platform)
 			if bc.MultiPlatformRequested {
-				res.AddRef(k, ref)
-				res.AddMeta(fmt.Sprintf("%s/%s", exptypes.ExporterImageConfigKey, k), config)
+				res.AddRef(expPlat.ID, ref)
+				res.AddMeta(fmt.Sprintf("%s/%s", exptypes.ExporterImageConfigKey, expPlat.ID), config)
 				if len(baseConfig) > 0 {
-					res.AddMeta(fmt.Sprintf("%s/%s", exptypes.ExporterImageBaseConfigKey, k), baseConfig)
+					res.AddMeta(fmt.Sprintf("%s/%s", exptypes.ExporterImageBaseConfigKey, expPlat.ID), baseConfig)
 				}
 			} else {
 				res.SetRef(ref)
@@ -79,10 +75,7 @@ func (bc *Client) Build(ctx context.Context, fn BuildFunc) (*ResultBuilder, erro
 					res.AddMeta(exptypes.ExporterImageBaseConfigKey, baseConfig)
 				}
 			}
-			expPlatforms.Platforms[i] = exptypes.Platform{
-				ID:       k,
-				Platform: p,
-			}
+			expPlatforms.Platforms[i] = expPlat
 			return nil
 		})
 	}
@@ -133,3 +126,16 @@ func extendWindowsPlatform(p, imgP ocispecs.Platform) ocispecs.Platform {
 	}
 	return p
 }
+
+func makeExportPlatform(p, imgP ocispecs.Platform) exptypes.Platform {
+	p = platforms.Normalize(p)
+	exp := exptypes.Platform{
+		ID: platforms.FormatAll(p),
+	}
+	if p.OS == "windows" {
+		p = extendWindowsPlatform(p, imgP)
+		p = platforms.Normalize(p)
+	}
+	exp.Platform = p
+	return exp
+}



Test patch:
diff --git a/cmd/buildctl/common/common_test.go b/cmd/buildctl/common/common_test.go
new file mode 100644
index 000000000..4469a4715
--- /dev/null
+++ b/cmd/buildctl/common/common_test.go
@@ -0,0 +1,74 @@
+package common
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func writeTempFile(t *testing.T, dir, name, content string) string {
+	t.Helper()
+	path := filepath.Join(dir, name)
+	require.NoError(t, os.WriteFile(path, []byte(content), 0644))
+	return path
+}
+
+func TestResolveTLSFilesFromDir(t *testing.T) {
+	t.Run("all files present for cert-manager style", func(t *testing.T) {
+		dir := t.TempDir()
+		ca := writeTempFile(t, dir, "ca.crt", "ca")
+		cert := writeTempFile(t, dir, "tls.crt", "cert")
+		key := writeTempFile(t, dir, "tls.key", "key")
+
+		caOut, certOut, keyOut, err := resolveTLSFilesFromDir(dir)
+		require.Equal(t, ca, caOut)
+		require.Equal(t, cert, certOut)
+		require.Equal(t, key, keyOut)
+		require.NoError(t, err)
+	})
+
+	t.Run("all files present for pem style", func(t *testing.T) {
+		dir := t.TempDir()
+		ca := writeTempFile(t, dir, "ca.pem", "ca")
+		cert := writeTempFile(t, dir, "cert.pem", "cert")
+		key := writeTempFile(t, dir, "key.pem", "key")
+
+		caOut, certOut, keyOut, err := resolveTLSFilesFromDir(dir)
+		require.Equal(t, ca, caOut)
+		require.Equal(t, cert, certOut)
+		require.Equal(t, key, keyOut)
+		require.NoError(t, err)
+	})
+
+	t.Run("mixed set is present", func(t *testing.T) {
+		dir := t.TempDir()
+		ca := writeTempFile(t, dir, "ca.crt", "ca-cert-manager")
+		cert := writeTempFile(t, dir, "cert.pem", "cert-pem")
+		key := writeTempFile(t, dir, "key.pem", "key-pem")
+		// ca for cert-manager, cert and key for pem
+
+		caOut, certOut, keyOut, err := resolveTLSFilesFromDir(dir)
+		require.Equal(t, ca, caOut)
+		require.Equal(t, cert, certOut)
+		require.Equal(t, key, keyOut)
+		require.NoError(t, err)
+	})
+
+	t.Run("all files present for cert-manager and pem styles and pem is chosen", func(t *testing.T) {
+		dir := t.TempDir()
+		writeTempFile(t, dir, "ca.crt", "ca-cert-manager")
+		writeTempFile(t, dir, "tls.crt", "cert-cert-manager")
+		writeTempFile(t, dir, "tls.key", "key-cert-manager")
+		ca := writeTempFile(t, dir, "ca.pem", "ca-pem")
+		cert := writeTempFile(t, dir, "cert.pem", "cert-pem")
+		key := writeTempFile(t, dir, "key.pem", "key-pem")
+
+		caOut, certOut, keyOut, err := resolveTLSFilesFromDir(dir)
+		require.Equal(t, ca, caOut)
+		require.Equal(t, cert, certOut)
+		require.Equal(t, key, keyOut)
+		require.NoError(t, err)
+	})
+}
diff --git a/frontend/dockerui/build_test.go b/frontend/dockerui/build_test.go
new file mode 100644
index 000000000..378f209b2
--- /dev/null
+++ b/frontend/dockerui/build_test.go
@@ -0,0 +1,100 @@
+package dockerui
+
+import (
+	"testing"
+
+	"github.com/containerd/platforms"
+	"github.com/moby/buildkit/exporter/containerimage/exptypes"
+	ocispecs "github.com/opencontainers/image-spec/specs-go/v1"
+	"github.com/stretchr/testify/require"
+)
+
+func TestNormalizePlatform(t *testing.T) {
+	testCases := []struct {
+		p, imgP  ocispecs.Platform
+		expected exptypes.Platform
+	}{
+		{
+			p: ocispecs.Platform{
+				Architecture: "arm64",
+				OS:           "linux",
+				Variant:      "v8",
+			},
+			imgP: ocispecs.Platform{
+				Architecture: "arm64",
+				OS:           "linux",
+			},
+			expected: exptypes.Platform{
+				ID: "linux/arm64", // Not "linux/arm64/v8" https://github.com/moby/buildkit/issues/5915
+				Platform: ocispecs.Platform{
+					Architecture: "arm64",
+					OS:           "linux",
+				},
+			},
+		},
+		{
+			p: ocispecs.Platform{
+				Architecture: "arm64",
+				OS:           "linux",
+				Variant:      "v8",
+			},
+			imgP: ocispecs.Platform{
+				Architecture: "arm64",
+				OS:           "linux",
+				Variant:      "v8",
+			},
+			expected: exptypes.Platform{
+				ID: "linux/arm64",
+				Platform: ocispecs.Platform{
+					Architecture: "arm64",
+					OS:           "linux",
+				},
+			},
+		},
+		{
+			p: ocispecs.Platform{
+				Architecture: "amd64",
+				OS:           "windows",
+			},
+			imgP: ocispecs.Platform{
+				Architecture: "amd64",
+				OS:           "windows",
+				OSVersion:    "10.0.19041.0",
+			},
+			expected: exptypes.Platform{
+				ID: "windows/amd64",
+				Platform: ocispecs.Platform{
+					Architecture: "amd64",
+					OS:           "windows",
+					OSVersion:    "10.0.19041.0",
+				},
+			},
+		},
+		{
+			p: ocispecs.Platform{
+				Architecture: "amd64",
+				OS:           "windows",
+				OSVersion:    "10.0.19041.0",
+			},
+			imgP: ocispecs.Platform{
+				Architecture: "amd64",
+				OS:           "windows",
+				OSVersion:    "11.0.22000.0",
+			},
+			expected: exptypes.Platform{
+				ID: "windows(10.0.19041.0)/amd64",
+				Platform: ocispecs.Platform{
+					Architecture: "amd64",
+					OS:           "windows",
+					OSVersion:    "10.0.19041.0",
+				},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		require.Equal(t, tc.expected, makeExportPlatform(tc.p, tc.imgP))
+		// the ID needs to always be formatall(normalize(p))
+		require.Equal(t, platforms.FormatAll(platforms.Normalize(tc.p)), tc.expected.ID)
+	}
+}