wiltonsr / ldapAuth

An open source Traefik Middleware that enables authentication via LDAP in a similar way to Traefik Enterprise
https://plugins.traefik.io/plugins/628c9eb7ffc0cd18356a979c/ldap-auth
Apache License 2.0
111 stars 10 forks source link

feat: allow usage of a environment variable or secret for sensitive params #47

Open thpiron opened 1 year ago

thpiron commented 1 year ago

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.

wiltonsr commented 1 year ago

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.

thpiron commented 1 year ago

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). And this would break the possibility to have multiple ldapAuth middlewares with different configuration as the secret must be on the traefik container, so can't be duplicated.

That why I added the possibility to change the environment variable / secret names for the bindPassword.

wiltonsr commented 1 year ago

It's only useful for sensitive data (ie passwords, key etc).

At some contexts the server, base and bindDN can also be sensitive data.

And this would break the possibility to have multiple ldapAuth middlewares with different configuration as the secret must be on the traefik container, so can't be duplicated.

But I agree that we must preserve the multiple usage of ldapAuth in different services.

Despite of this, create a custom parameter to each of these values doesn’t make sense.

@fcinqmars what do you think about this?

fcinqmars commented 1 year ago

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.

wiltonsr commented 1 year ago

Personally, I would only put what needs to be secret in the secrets.

So are we talking about bindPassword and cacheKey?

That why I added the possibility to change the environment variable / secret names for the bindPassword.

Should this possibility exist for both these parameters or only for bindPassword?

thpiron commented 1 year ago

Yes my bad, it should be possible for both secrets, I'll update the branch to add this.

thpiron commented 1 year ago

Added possibility to change label of cache key secret. Rebased on main and fixed some conflicts, ready to be reviewed for merging.

thpiron commented 1 year ago

Thank you both for the review, it should be way better now.

bclingan commented 5 months ago

are there plans to implement this PR in the future?