Various issues with shell driver of `podman secret`

[coolwanglu]

Issue Description

I'm been experimenting the shell driver of podman secrets. It's not really working well.

Steps to reproduce the issue

Setup

I created a simple shell script test.sh

podman secret create \
  --driver=shell \
  --driver-opts=list=/bin/true \
  --driver-opts=lookup="cat $2" \
  --driver-opts=store=/bin/true \
  --driver-opts=delete=/bin/true \
  --replace=true \
  "$1" - <<<SECRET

The idea is, test.sh name filename will allow me to register the content of filename as secret name. This is similar to LoadCredential= in systemd.

Test Cases

Test Case 1

1. Make sure podman secret ls is empty, make sure /var/lib/containers/storage/secrets is empty (or does not exist).
2. Run as root (same below) echo SECRET | podman secret create test1 -
3. podman secret ls should show test1
4. test.sh test2 /test2
5. podman secret ls shows only test2. test1 disappeared
6. /var/lib/containers/storage/secrets/filedriver/secretsdata.json still contains test1.

Test Case 2

1. Make sure podman secret ls is empty, make sure /var/lib/containers/storage/secrets is empty (or does not exist).
2. Run as root (same below) echo SECRET | podman secret create test1 -
3. echo SECRET | podman secret create test2 -
4. podman secret ls should show test1 and test2
5. test.sh test3 /test3 got error Error: deleting secret : more than one result secret with prefix : secret is ambiguous

Issue 1: Bug with --replace for new secret

When --replace is specified, podman tries to delete the old secret, however, the ID field is not set for new secrets.

This means we are deleting a secret with an empty string. The file driver will throw an "no such secret" error, which is muted, that's why the file driver works.

However, if the driver does not throw an error, podman will go ahead and try to delete the metadata. Because an empty ID does not exist, it is now interpreted as a prefix (which is even worse in my opinion).

This explains both test cases: if there is only one entry in the store, it is matched with the empty prefix and deleted. If there are multiple entries, podman will complain the prefix is ambigiuous.

Proposal

A workaround is to check SECRET_ID is not empty in the shell script. But I think this is hacky, because the shell driver treats any error as "no such secret" error.

The correct solution should be avoid deleting anything, when adding a new secret.

Issue 2: Incorrect logic with multiple drivers

Let's say I run the following:

• podman secret create --driver=DRIVER1 --replace secret ...
• podman secret create --driver=DRIVER2 --replace secret ...

Currently podman asks DRIVER2 to delete the secret, which doesn't make sense to me. We should use DRIVER1.

Issue 3: Quoting/Escaping issues with shell scripts

It seems that if a script (e.g. lookup/delete/store/list) ends with ", it will be removed. Examples:

• --driver-opts=list="echo \"SECRET\"" will produce "list": "echo \"SECRET" in podman secret inspect
• --driver-opts=list="echo \"SECRET\" " works as expected, note the trialing whitespace.

Issue 4: list command for shell driver

Throughout the codebase I cannot find where it is used, nor can I trigger it.
Is it expected?

Describe the results you received

See above

Describe the results you expected

See above

podman info output

podman version 5.4.2 on Debian.

Podman in a container

No

Privileged Or Rootless

Privileged

Upstream Latest Release

No

Additional environment details

No response

Additional information

No response

[Honny1]

Thank you, @coolwanglu. It looks like you did a nice investigation into where the problem is. Feel free to create a PR to fix this.

WDYT? @mheon, @Luap99

[github-actions]

A friendly reminder that this issue had no activity for 30 days.

[Samathingamajig]

I am interested in trying to create a PR for these issues. I have a few questions before I can proceed.

Issue 1:

If --replace is called when the secret does not already exist, should this behave as if --replace wasn't added, or should it error?

I believe behaving like --replace wasn't added and just make the secret like normal is the most intuitive.

Issue 2:

I just wanted to clarify that secrets in podman are globally unique, even if created via different drivers?

If so, then I think it makes the most sense to delete the secret using the OLD driver rather than the NEW driver. Effectively, if DRIVER1 currently holds secret, then if we attempt to create secret secret on DRIVER2 with --replace, it will behave like DRIVER1.delete(secret) then DRIVER2.create(secret, value).

It could also make sense to display an error or warning that the secret is moving across a different driver.

[Samathingamajig]

/assign

[mheon]

For 1, it should add without error; that matches the behavior of our other commands with --replace.
For 2, I believe secrets should be considered globally unique, but @ashley-cui can confirm that.

[Samathingamajig]

I created a PR to address issues 1 and 2 containers/container-libs#548

[Samathingamajig]

Issue 3 is due to an upstream issue in the github.com/spf13/pflag module.

In particular, when there is only 1 key/value pair, quotation marks at the end are trimmed (genuinely no idea why this is the case), and if there is more than 1 key/value pair, the line is treated as a CSV text, which requires any "column" with double quotes in it to start and end with " and then every time you want a single " you must type two like "".

https://github.com/spf13/pflag/blob/196624cc28da007ab68e13213b7fb6447211ad31/string_to_string.go#L25-L39

var ss []string
n := strings.Count(val, "=")
switch n {
case 0:
	return fmt.Errorf("%s must be formatted as key=value", val)
case 1:
	ss = append(ss, strings.Trim(val, `"`))
default:
	r := csv.NewReader(strings.NewReader(val))
	var err error
	ss, err = r.Read()
	if err != nil {
		return err
	}
}

Very similar to this existing issue #20064

If we want to "fix" this, we either need

1. A patch in the upstream module
2. The upstream module to expose a new parsing rule that has intuitive quotation rules
3. A custom parser written in this repository

None of these are great options, my preference is the 2nd. The 1st could break someone else's backwards compatibility, and the 3rd is a weird thing for Podman to maintain.

Here's a Gist to reproduce this bug minimally: https://gist.github.com/Samathingamajig/df4f13939d61eb0ba7f0b96ec8e395e2

[Samathingamajig]

Regarding issue 4, the SecretsDriver interface owns the method List(), and I only see usage within the *_test.go files. It's never called within the podman or container-libs repositories.