DNS providers: Don't return half-populated configs from environment

[mholt]

I'm trying to make DNS providers with a custom config, but pre-populating default values from environment variables.

For example, take the Cloudflare provider: https://pkg.go.dev/github.com/go-acme/lego/v3@v3.3.0/providers/dns/cloudflare?tab=doc - the env variable logic is split into two places: NewDefaultConfig() and NewDNSProvider(). But these functions can't be used together!

Only NewDNSProvider() fills in default credentials from the environment, but it does not expose the config for further customization. So the config is only half-filled out, and we cannot use NewDefaultConfig() and NewDNSProvider() together.

If a default config was pre-populated from the environment and then returned from NewDefaultConfig(), this would make it possible for us to avoid putting DNS provider credentials in app config files. It'd be nice to call just one function and get a config that is filled out all the way from the environment. Otherwise, we'll have to copy+paste and repeat the env variable logic for every DNS provider that does this in our own applications. 😢

So, could we move all the environment variable logic into NewDefaultConfig() rather than having it split in two places like it is now? 😁

[ldez]

Hello,

The split is historical but also a design choice.

• if you want to use env vars, you just have to call NewDNSProvider()
• if you don't want to use env vars, you can use Config or NewDefaultConfig() and NewDNSProviderConfig(config *Config)

NewDefaultConfig() creates a kind provider "neutral" configuration, NewDefaultConfig() must stay "neutral".

NewDNSProvider() manage the cases related to env vars, so I don't see a use-case where you need to use NewDNSProviderConfig(config *Config) and env vars.

Could you explain your use-case exactly?

A solution can be to create another function called NewConfigFromEnvVars(), but some providers will not support that or partially like gcloud, acme-dns, azure, designate, lightsail, oraclecloud, route53.

[mholt]

Thanks for the reply on a weekend.

We'd like to use env vars as defaults, but we need to customize certain parameters beyond what the environment specifies (for example, an environment may only specify a credential, but we need to specify a custom timeout or TTL). We have to do this per-client, so we cannot use a global value like an environment variable.

The goal is to prevent putting secrets inside application config, which is not necessarily secret.

NewDefaultConfig() creates a kind provider "neutral" configuration, NewDefaultConfig() must stay "neutral".

What do you mean by "neutral"? It uses config from the environment just as much as NewDNSProvider() does. There's not really a difference in how opinionated/vanilla they are.

NewDNSProvider() manage the cases related to env vars, so I don't see a use-case where you need to use NewDNSProviderConfig(config *Config) and env vars.

For example, when we need to customize the TTL in the app but have credentials in environment variables. We can't do both (set a custom TTL AND get the credentials from the environment) unless we copy half the environment logic for all DNS providers and keep them synced over time...

[ldez]

TTL, PropagationTimeout, PollingInterval, etc are not related to a specific provider, so for me it's provider neutral.

The way to authenticate is specific to each provider, so not provider neutral.

I think that the ambiguity comes from the term Default in NewDefaultConfig(): it's a way to define the real default values (if no env vars like TTL, PropagationTimeout, PollingInterval, ...), it's the default values from lego not from env vars. For me, this reinforces the neutral aspect of this function.

[ldez]

In addition, I have already spent time finding a way to have several configurations for a single provider, but it is quite difficult because for certain providers (ex: gcloud, route53, ...) it is not possible because of the internal behavior of clients who do not really use env vars (and these clients are complex and cannot be replaced by simple HTTP calls)

[mholt]

TTL, PropagationTimeout, PollingInterval, etc are not related to a specific provider, so for me it's provider neutral.

The way to authenticate is specific to each provider, so not provider neutral.

Ah, I see what you mean. ("Generic" or "common" maybe.)

However, even though those properties aren't related to a specific provider, they are still hard-coded into each provider's Config structs like so:

lego/providers/dns/cloudflare/cloudflare.go

Lines 22 to 33 in 6375826

	type Config struct {
	AuthEmail string
	AuthKey string
	AuthToken string
	ZoneToken string
	TTL int
	PropagationTimeout time.Duration
	PollingInterval time.Duration
	HTTPClient *http.Client
	}

In other words, TTL, timeouts, base URLs, or credentials -- they're all the same; they're all specific to that one provider. In reality, providers don't share anything, or have anything in common with other providers.

I think there are two good ways to fix this, either:

1. NewDefaultConfig() should return the same thing every time (disregarding all env variables) and that a new function such as NewConfig() or NewConfigFromEnv() should handle all environment variable logic.

2. Or, NewDefaultConfig() should simply handle all env variable logic instead of just half of it. Since the returned configs can be modified, the env variables just act as defaults anyway, and are optional.

I think I prefer option 2. I could probably submit a PR for at least a few of the providers, but doing this for all 60 by myself would be a pain.

[ldez]

3. creates another function NewConfigFromEnv()

[mholt]

@ldez

3. creates another function NewConfigFromEnvVars()

Isn't that what option 1 is?

[ldez]

nope because in my solution NewDefaultConfig() will keep this current behavior.

[mholt]

nope because in my solution NewDefaultConfig() will keep this current behavior.

I see... I guess that'd be fine for my requirements, but I think it's a bit inconsistent just so you know: it might be surprising if only some environment variables get treated as defaults. IMO it should return the same value every time...

But as long as there's a way to invoke all the env variable logic in one function call that returns the Config, that's all I need at least.

[ldez]

so I propose to:

• create a function NewCommonConfig()
• deprecate NewDefaultConfig() and call NewCommonConfig() inside this function
• create a function NewConfigFromEnv() who call NewCommonConfig()

[mholt]

Cool, that'd work for me. I don't even think NewDefaultConfig() needs to be renamed, as long as it's clear what "default" means in the docs.

[ldez]

ok so just create a function NewConfigFromEnv() who call NewDefaultConfig()

[mholt]

Okay! I'll try to get around to a PR soon.

[mholt]

Continuing the discussion:

There are several thing that can produce those errors (not only missing env vars) and message details (in the missing env var case) are important.

Why does it matter where the config comes from? i.e. why is it an error if the config does not come from env variables but instead comes from setting the field manually like config.APIToken = "foo"?

Sure, NewConfigFromEnv() could return an error, but what good does the error do, since the config isn't actually used yet? I think validating the config in that function is incorrect and can lead to bugs. The config should be validated when it is used, in NewDNSProviderConfig() (which does return an error).

Also NewDefaultConfig() (which does the same thing: reads env variables) does not return an error.

I'm OK with NewConfigFromEnv() returning an error, but only for the right reasons... it kind of defeats the purpose if it requires that env variables are set.

[ldez]

Currently I follow another way that NewConfigFromEnv(), I need a bit of time to have a concrete answer to your initial need.

[ldez]

The current state of my thoughts:
create a function NewConfigFromEnv() (no error returned) but this function will be never called in lego.

---

impossible:

• lightsail
• route53

limited:

• oraclecloud
• googlecloud

special cases:

• cloudflare
• checkdomain
• easydns
• httpreq
• joker
• pdns
• zoneee

[ldez]

I am in doubt because the real problem that you are trying to solve is the fact of being able to have several configurations for the same DNS.

I have already spent time finding a way to have several configurations for a single provider, but it is quite difficult because of non homogeneous providers behavior.

The solution that you propose can be interesting but it is a very specific approach for caddy and which is not really global.

I would therefore be tempted to minimize the impact of the change in order to keep the space clean for other solutions.

So my best proposal is still to create a function NewConfigFromEnv() (no error returned) but this function will be never called in lego.

example:

func NewConfigFromEnv() *Config {
	config := NewDefaultConfig()
	config.AuthEmail, _ = env.GetOneWithFallback(EnvEmail, EnvAltEmail)
	config.AuthKey, _ = env.GetOneWithFallback(EnvAPIKey, EnvAltAPIKey)
	config.AuthToken, _ = env.GetOneWithFallback(EnvDNSAPIToken, EnvAltDNSAPIToken)
	config.ZoneToken, _ = env.GetOneWithFallback(EnvZoneAPIToken, EnvAltZoneAPIToken, EnvDNSAPIToken, EnvAltDNSAPIToken)

	return config
}

Are you agree with my proposal?

[mholt]

the real problem that you are trying to solve is the fact of being able to have several configurations for the same DNS.

Yep, that is part of it (or one of the problems), anyway. We also want for some settings to be able to be configured without being required to use env vars for all settings.

I have already spent time finding a way to have several configurations for a single provider, but it is quite difficult because of non homogeneous providers behavior.

Indeed, I understand this difficulty. I think the current implementation of the config parameters is mostly fine, but it just needs a little reworking to be more consistent and organized, which will make it useful in more situations -- especially enterprise environments.

The solution that you propose can be interesting but it is a very specific approach for caddy and which is not really global.

Well, I disagree, in that this can benefit any application that uses lego. Yes, Caddy will use this feature, but so can any other Go program that doesn't want to add global state.

I would therefore be tempted to minimize the impact of the change in order to keep the space clean for other solutions.

I definitely understand and appreciate this.

Are you agree with my proposal?

I will think on it... it looks like more work because the env variables have to be brought out into constants, and it still duplicates the logic (you would be loading credentials both in NewConfigFromEnv() and NewDNSProvider() if you won't reuse NewConfigFromEnv()). So adding a new env variable / config parameter, for instance, would require updating it in two places, which is more error-prone.

So, I mean, I guess I'm not opposed to your proposal -- it's just more work than I personally want to invest, especially for all the DNS providers. I won't expect you to do it, either, don't worry. :) I will think on it and see how it sits over time.

Thank you for the attention!