Implement secrets pkg: backend and filedriver #375
Conversation
|
@vrothberg @umohnani8 @mheon PTAL |
|
Very nice work. Should the file driver have an encryption options, perhaps defaulted to None? Do you have a PR in Podman that uses this, to show how Secrets would work in the wild? |
There was a problem hiding this comment.
Nice work, @ashley-cui!
I am still torn regarding the driver but gravitate towards always specifying a driver in (SecretsManager).Store(name, data, driver). This way, it's easy to add a --driver flag to the CLI that allows to use custom secrets drivers.
I assume that docker has some pre-defined API/protocol to interact with those drivers. If that's the case, secrets drivers could be configured in the containers.conf as a map[string]string:
[secretdrivers]
"driver-a"="/usr/libexec/secretdriver-a`
"..."="..."
pkg/secrets/secrets.lock should be deleted.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashley-cui, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Updated. The bug that was haunting me ended up being a simple using the wrong variable bug 🤦 |
ashley-cui
force-pushed
the
secrets
branch
2 times, most recently
from
cd99dce to
a300b38
Compare
ashley-cui
force-pushed
the
secrets
branch
2 times, most recently
from
9cf854b to
b0297e8
Compare
There was a problem hiding this comment.
Final nits from me. Really nice work, @ashley-cui!
Let's get that in, and we can tackle Podman :) We're free to change things in the future, if we need to.
|
@vrothberg Thanks for all the thorough reviews :) |
This is the implementation of the backend of secrets. pkg/secrets takes a secret name and data and does these operations on that secret data: store, delete, lookup, and list, using a secretsmanager. The first driver implemented here is a filedriver - where the data is stored unencrypted on disk in a file. The secrets package can be easily expanded to use more drivers as the package implements an interface to accept different drivers Signed-off-by: Ashley Cui <[email protected]>
| const secretIDLength = 25 | ||
|
|
||
| // errInvalidPath indicates that the secrets path is invalid | ||
| var errInvalidPath = errors.New("invalid secrets path") |
There was a problem hiding this comment.
Can I ask why these aren't public? We usually do public errors in Podman-land so that errors.Cause() can be used to easily identify what went wrong.
There was a problem hiding this comment.
I can definitely make them public again if that's the right move
There was a problem hiding this comment.
Please don't. It would be very confusing to expose that to users as they cannot really treat the errors in a special way. If we need some in the future, we can selectively make them public.
ashley-cui
force-pushed
the
secrets
branch
3 times, most recently
from
13ac506 to
d7d9efa
Compare
|
@containers/podman-maintainers PTAL |
|
Nice job @ashley-cui |
aardvark,commit: acquire fs lock when performing commit to avoid `race` across parallel invocations.
This is the implementation of the backend of secrets. pkg/secrets takes a secret name and data and does operations on that secret data, most namely store, delete, lookup, and list, using a secretsmanager.
The first driver implemented here is a filedriver - where the data is stored unencrypted on disk in a file.
The secrets package can be easily expanded to use more drivers as the package implements an interface to accept different drivers
Related: containers/podman#3264, containers/buildah#1684
Signed-off-by: Ashley Cui [email protected]