OpenSSL cannot read SSH keys mounted as secrets

[sagikazarmark]

What is the issue?

While working on #7202 I noticed that mounting an SSH key as a secret in a container always leads to Load key "/ssh-key": error in libcrypto errors.

Dagger version

dagger v0.11.2 (registry.dagger.io/engine) darwin/arm64

Steps to reproduce

The code reproducing the issue can be found in my daggerverse:

git clone https://github.com/sagikazarmark/daggerverse.git
cd daggerverse
git checkout ssh-key-repro
cd ssh-key-repro
chmod 600 id_ed25519
 
# Test that mounts the ssh key as a file
dagger call test-ok stdout
 
# Test that reads the ssh key as a file and creates a secret
dagger call test-ok-too stdout
 
# Test that reads the ssh key as a file and creates a secret
dagger call test-fail --ssh-key file:id_ed25519 stdout

Log output

❯ dagger -- call test-ok stdout
ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIF0PEuo6tWBJ3kuKjBAx/MxxKnzO6AwL/rRo2YjxqA7i [email protected]
 
❯ dagger -- call test-ok-too stdout
ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIF0PEuo6tWBJ3kuKjBAx/MxxKnzO6AwL/rRo2YjxqA7i [email protected]
 
❯ dagger -- call test-fail --ssh-key file:id_ed25519 stdout
✔ SshKeyRepro.testFail(
    sshKey: ✔ setSecret(name: "60a21fa96beb12b2d78c933778c32887cf5ddfbffc6d2755e4ad8d889c693c5f"): Secret! 0.0s
  ): Container! 1.4s
  ✔ Wolfi.container(packages: ["git", "openssh"]): Container! 1.2s
    ✔ Apko.wolfi(packages: ["git", "openssh"]): Container! 1.0s
      ✔ Container.from(address: "cgr.dev/chainguard/apko"): Container! 0.7s
        ✔ HTTP GET 0.4s
        ✔ remotes.docker.resolver.HTTPRequest 0.2s
          ✔ HTTP HEAD 0.2s
✘ Container.stdout: String! 0.1s
! process "ssh-keygen -y -f /ssh-key" did not complete successfully: exit code: 255
  ✘ exec ssh-keygen -y -f /ssh-key 0.1s
  ! process "ssh-keygen -y -f /ssh-key" did not complete successfully: exit code: 255
  ┃ Load key "/ssh-key": error in libcrypto
 
Error: response from query: input: container.import.withMountedSecret.withExec.stdout resolve: process "ssh-keygen -y -f /ssh-key" did not complete successfully: exit code: 255
 
Stderr:
Load key "/ssh-key": error in libcrypto

[aweris]

When passing a *Secret type file like --ssh-key file:id_ed25519, Dagger trims whitespace from the file content, which corrupts the sshkey file format.

To reproduce compare mounted file contents from @sagikazarmark's reproduce module.

[jedevc]

When passing a *Secret type file like --ssh-key file:id_ed25519, Dagger trims whitespace from the file content, which corrupts the sshkey file format.

dagger/cmd/dagger/flags.go

Lines 362 to 367 in 030ffc1

	case fileSecretSource:
	filePlaintext, err := os.ReadFile(v.sourceVal)
	if err != nil {
	return nil, fmt.Errorf("failed to read secret file %q: %w", v.sourceVal, err)
	}
	plaintext = strings.TrimSpace(string(filePlaintext))

This looks like a failure introduced by #6845. cc @kpenfound @sipsma @vito

[aweris]

This looks like a failure introduced by #6845.

The change introduced by #6845 also valid concerns, I wonder, is it meaningful to add TrimSpace option to ContainerWithMountedSecretOpts?

[marcosnils]

👋 from discord

yes, that make sense. Maybe this might be changing the current implementation to https://go.dev/play/p/h5UXovuGHsl could be more accurate

here's the Z unicode category for the reference:

thoughts? @kpenfound @jedevc @aweris ?

[sagikazarmark]

Here is an ugly workaround then until it's fixed:

	return dag.
		Wolfi().
		Container(WolfiContainerOpts{
			Packages: []string{"git", "openssh"},
		}).
		WithMountedSecret("/ssh-key", sshKey).
		WithExec([]string{"cp", "/ssh-key", "/ssh-key2"}).
		WithExec([]string{"sh", "-c", "echo '' >> /ssh-key2"}).
		WithExec([]string{"ssh-keygen", "-y", "-f", "/ssh-key2"})

sagikazarmark/daggerverse@72f87df

[sagikazarmark]

Actually, this is a terrible idea: this would result in the ssh key being cached.

[sagikazarmark]

This looks like a failure introduced by #6845.

The change introduced by #6845 also valid concerns, I wonder, is it meaningful to add TrimSpace option to ContainerWithMountedSecretOpts?

The current implementation trims spaces upon reading the file (ie. dag.SetSecret is unaffected) which kinda makes sense, but at the same time makes it impossible to add this as an option to ContainerWithMountedSecretOpts.

[marcosnils]

Actually, this is a terrible idea: this would result in the ssh key being cached.

You could use WithMountedTemp and copy the "workaround" key there. That will not store it in the cache.

[sagikazarmark]

That's a good idea, thank you @marcosnils

[aweris]

The current implementation trims spaces upon reading the file (ie. dag.SetSecret is unaffected) which kinda makes sense, but at the same time makes it impossible to add this as an option to ContainerWithMountedSecretOpts.

not affecting secret mounting to dag kinda makes sense but not sure about modifying file content event it's a whitespace.

I think, this issue would only apply to files like SSH keys or TLS certificates. There are two options available:

1. not touch files even white spaces to keep contents the same as the source
2. apply @marcosnils suggestion and keep that specific whitespace in the files

option 2 is kinda ugly but will make both sides happy.

[jedevc]

I don't think we should do this with options at the point of use. Secrets should be marked to do this at point of load.

Something like @sipsma's suggestion in #6845 (review) seems reasonable:

--token=file-raw:/some/token/file/where/trailing/newlines/matter

That said, I'm almost tempted to suggest reverting #6845 since this is a pretty big edge case, but I have no suggestions to how to otherwise solve the original problem 🤔 So probably not an option.

[aweris]

That said, I'm almost tempted to suggest reverting #6845

after reading this I reconsidered this option and it made sense.

The example from #6845, uses echo $TOKEN command which adds a new line to the file. if this is unwanted behavior I would prefer changing this to echo -n $TOKEN or something else rather than expecting dagger cli would solve it for me.

[marcosnils]

That said, I'm almost tempted to suggest reverting #6845 since this is a pretty big edge case, but I have no suggestions to how to otherwise solve the original problem 🤔 So probably not an option.

SGTM @jedevc

[sipsma]

I'm good with reverting the change to strip newlines. I think we could consider the inverse of my suggestion on that PR and instead support a CLI syntax that does strip newlines for use cases that call for that. But I think this situation proves it's probably better to default to the exact contents of the file/command/env var.