feat: allow usage of a environment variable or secret for sensitive params #47
Conversation
|
Hi, @thpiron Thanks for your PR. I think this feature must support almost every available parameter. Unfortunately, for this, a lot of work is needed. I started a draft of what I think this feature should be like in this branch: feat-support-multiple-conf-sources. Feel free to propose any changes you think are pertinent. |
|
Hello, I don't really think that putting everything in secrets is a good idea. It's only useful for sensitive data (ie passwords, key etc). That why I added the possibility to change the environment variable / secret names for the bindPassword. |
At some contexts the
But I agree that we must preserve the multiple usage of Despite of this, create a custom parameter to each of these values doesn’t make sense. @fcinqmars what do you think about this? |
|
I agree with @thpiron that normally secrets should only be used for sensitive values like credentials though I have seen them used more broadly. I don't think that server, base and bindDN necessarily qualify for secrets. From what I have seen, most LDAP servers are configured to allow any LDAP users to read the directory anyway. It usually is enabled by default. If the security controls are proper, knowing that information shouldn't matter in my opinion. Personally, I would only put what needs to be secret in the secrets. |
So are we talking about bindPassword and cacheKey?
Should this possibility exist for both these parameters or only for |
|
Yes my bad, it should be possible for both secrets, I'll update the branch to add this. |
thpiron
force-pushed
the
feat/move_credentials_to_env_or_secrets
branch
2 times, most recently
from
0b8847a to
f885af3
Compare
|
Added possibility to change label of cache key secret. |
| return bindPassword | ||
| } | ||
|
|
||
| b, err := os.ReadFile(fmt.Sprintf("/run/secrets/%s", strings.ToLower(label))) |
There was a problem hiding this comment.
I am wondering if maybe we should allow a file path there, just because that would not be usable on Windows. Furthermore, Docker Swarm and Docker Compose allow mounting secrets into other locations than /run/secrets
There was a problem hiding this comment.
Support for multiple paths could be a problem. We could detect the OS system and define the full path based on default values or direct read the file if the label starts with / and exists. But I prefer to maintain the plugin as simple as possible.
thpiron
force-pushed
the
feat/move_credentials_to_env_or_secrets
branch
from
888e7fc to
a28a99d
Compare
|
Thank you both for the review, it should be way better now. |
|
are there plans to implement this PR in the future? |
Hello,
Created this PR to solve issue #13.
Added possibility to set bindPassword from environment variable or docker secret (filesystem
/run/secrets/my_secret).No breaking change.