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

[thpiron]

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]

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]

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]

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]

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]

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]

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

[thpiron]

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

[fcinqmars]

Hi @thpiron,

Thank you for you PR. I completed the review, let me know if something does not feel right or is unclear.

@wiltonsr Feel free to chime in

[fcinqmars]

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

[wiltonsr]

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]

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

[bclingan]

are there plans to implement this PR in the future?

[hineios]

@wiltonsr what do you feel that still needs to be done in order to merge this PR?

[wiltonsr]

Hello, @hineios

This branch has conflicts with main, and we did not reach a consensus on allowing a file path.

[thpiron]

Hello,
Sorry I'm not using ldapAuth anymore but I'm ready to work on this PR this week if we can find a suitable solution.

if the path is configurable by an environment variable would this be OK for you @wiltonsr ?
Looking for an env variable like for example LDAP_AUTH_CACHE_KEY_FILE and look for the secret in this file? (this is used by a lot of containers that need secrets, like postgres, mariadb, wordpress and a lot more)

It would be pretty easy to implement and should solve the path issues on windows.

[hineios]

Thank you all, for your prompt replies.

Although I'm not overly familiar with Go and Traefik's plugin development, I'd be able to contribute with some development and testing.

Nevertheless, and not wanting to throw too much sand into the gearbox, I'd like to make a suggestion.

Nexus' Configuration as Code Plugin does something quite interesting with its YAML configuration files: it allows variable interpolation on values, allowing usage of environment variables and even files as values.

This would allow us to keep backwards compatible behaviour on all configuration fields, without needing to add Docker Secret variations for all. All these could be supported concurrently:

        ldapAuth:
          BindPassword: "very-secret-password"
          BindPassword: "${VAR_WITH_VERY_SECRET_PASSWORD}"
          BindPassword: "${file:/run/secrets/very_secret_secret}"

What do you think?

[wiltonsr]

if the path is configurable by an environment variable would this be OK for you @wiltonsr ?

This will fit well for our use case, too.

The @hineios's suggestion it's also very reasonable, but will demand more development effort.

[thpiron]

@hineios you can already do something like that for variable by defining your middleware in a traefik dynamic configuration file.

You can use go templating and sprig function: see https://doc.traefik.io/traefik/providers/file/#go-templating and https://masterminds.github.io/sprig/

I updated my PR, and added some very simple test cases to check if my changes are OK.

[wiltonsr]

ASAP I will review and test this PR.

[wiltonsr]

@thpiron, thanks for your contribution.

The code has only one duplicate logConfigParams(config). Please fix that, and I will merge this PR.

[thpiron]

The code has only one duplicate logConfigParams(config). Please fix that, and I will merge this PR.

It"s fixed

[wiltonsr]

Available on v0.1.11.