review: simplify

cmd/zz_gen_cmd_dnshelp.go

@@ -2101,6 +2101,8 @@ func displayDNSHelp(w io.Writer, name string) error {
 		ew.writeln(`Credentials:`)
 		ew.writeln(`	- "OVH_APPLICATION_KEY":	Application key`)
 		ew.writeln(`	- "OVH_APPLICATION_SECRET":	Application secret`)
+		ew.writeln(`	- "OVH_CLIENT_ID":	Client ID (OAuth2)`)
+		ew.writeln(`	- "OVH_CLIENT_SECRET":	Client secret (OAuth2)`)
 		ew.writeln(`	- "OVH_CONSUMER_KEY":	Consumer key`)
 		ew.writeln(`	- "OVH_ENDPOINT":	Endpoint URL (ovh-eu or ovh-ca)`)
 		ew.writeln()

docs/content/dns/zz_gen_ovh.md

@@ -42,6 +42,8 @@ lego --email you@example.com --dns ovh --domains my.example.org run
 |-----------------------|-------------|
 | `OVH_APPLICATION_KEY` | Application key |
 | `OVH_APPLICATION_SECRET` | Application secret |
+| `OVH_CLIENT_ID` | Client ID (OAuth2) |
+| `OVH_CLIENT_SECRET` | Client secret (OAuth2) |
 | `OVH_CONSUMER_KEY` | Consumer key |
 | `OVH_ENDPOINT` | Endpoint URL (ovh-eu or ovh-ca) |
 
@@ -82,6 +84,22 @@ When requesting the consumer key, the following configuration can be used to def
 }
 ```
 
+## OAuth2 Client Credentials
+
+Another method for authentication is by using OAuth2 client credentials.
+
+An IAM policy and service account can be created by following the [OVH guide](https://help.ovhcloud.com/csm/en-manage-service-account?id=kb_article_view&sysparm_article=KB0059343).
+
+Following IAM policies need to be authorized for the affected domain :
+
+* dnsZone:apiovh:record/create
+* dnsZone:apiovh:record/delete
+* dnsZone:apiovh:refresh
+
+## Important note
+
+Both authentication methods cannot be used at the same time.
+
 
 
 ## More information

providers/dns/ovh/ovh.go

@@ -15,7 +15,7 @@ import (
 
 // OVH API reference:       https://eu.api.ovh.com/
 // Create a Token:          https://eu.api.ovh.com/createToken/
-// Create a OAuth client:   https://eu.api.ovh.com/console-preview/?section=%2Fme&branch=v1#post-/me/api/oauth2/client
+// Create a OAuth2 client:   https://eu.api.ovh.com/console-preview/?section=%2Fme&branch=v1#post-/me/api/oauth2/client
 
 // Environment variables names.
 const (
@@ -23,21 +23,25 @@ const (
 
 	EnvEndpoint = envNamespace + "ENDPOINT"
 
-	// Authenticate using application key
-	EnvApplicationKey    = envNamespace + "APPLICATION_KEY"
-	EnvApplicationSecret = envNamespace + "APPLICATION_SECRET"
-	EnvConsumerKey       = envNamespace + "CONSUMER_KEY"
-
-	// Authenticate using OAuth2 client
-	EnvClientId     = envNamespace + "CLIENT_ID"
-	EnvClientSecret = envNamespace + "CLIENT_SECRET"
-
 	EnvTTL                = envNamespace + "TTL"
 	EnvPropagationTimeout = envNamespace + "PROPAGATION_TIMEOUT"
 	EnvPollingInterval    = envNamespace + "POLLING_INTERVAL"
 	EnvHTTPTimeout        = envNamespace + "HTTP_TIMEOUT"
 )
 
+// Authenticate using application key.
+const (
+	EnvApplicationKey    = envNamespace + "APPLICATION_KEY"
+	EnvApplicationSecret = envNamespace + "APPLICATION_SECRET"
+	EnvConsumerKey       = envNamespace + "CONSUMER_KEY"
+)
+
+// Authenticate using OAuth2 client.
+const (
+	EnvClientID     = envNamespace + "CLIENT_ID"
+	EnvClientSecret = envNamespace + "CLIENT_SECRET"
+)
+
 // Record a DNS record.
 type Record struct {
 	ID        int64  `json:"id,omitempty"`
@@ -48,22 +52,22 @@ type Record struct {
 	Zone      string `json:"zone,omitempty"`
 }
 
-type ApplicationConfig struct {
-	ApplicationKey    string
-	ApplicationSecret string
-	ConsumerKey       string
-}
-
+// OAuth2Config the OAuth2 specific configuration.
 type OAuth2Config struct {
-	ClientId     string
+	ClientID     string
 	ClientSecret string
 }
 
 // Config is used to configure the creation of the DNSProvider.
 type Config struct {
-	APIEndpoint        string
-	ApplicationConfig  *ApplicationConfig
-	OAuth2Config       *OAuth2Config
+	APIEndpoint string
+
+	ApplicationKey    string
+	ApplicationSecret string
+	ConsumerKey       string
+
+	OAuth2Config *OAuth2Config
+
 	PropagationTimeout time.Duration
 	PollingInterval    time.Duration
 	TTL                int
@@ -94,43 +98,11 @@ type DNSProvider struct {
 // Credentials must be passed in the environment variables:
 // OVH_ENDPOINT (must be either "ovh-eu" or "ovh-ca"), OVH_APPLICATION_KEY, OVH_APPLICATION_SECRET, OVH_CONSUMER_KEY.
 func NewDNSProvider() (*DNSProvider, error) {
-	// If OVH_CLIENT_ID is set create an OAuth2Config variant of config
-	if _, err := env.Get(EnvClientId); err == nil {
-		// Both authentication are mutually exclusive
-		if _, err := env.Get(EnvApplicationKey); err == nil {
-			return nil, fmt.Errorf("ovh: set %v or %v but not both", EnvApplicationKey, EnvClientId)
-		}
-
-		values, err := env.Get(EnvEndpoint, EnvClientId, EnvClientSecret)
-		if err != nil {
-			return nil, fmt.Errorf("ovh: %w", err)
-		}
-
-		config := NewDefaultConfig()
-		config.APIEndpoint = values[EnvEndpoint]
-		config.OAuth2Config = &OAuth2Config{
-			ClientId:     values[EnvClientId],
-			ClientSecret: values[EnvClientSecret],
-		}
-
-		return NewDNSProviderOAuth2Config(config)
-	}
-
-	// Else create an ApplicationConfig variant of config
-	values, err := env.Get(EnvEndpoint, EnvApplicationKey, EnvApplicationSecret, EnvConsumerKey)
+	config, err := createConfigFromEnvVars()
 	if err != nil {
 		return nil, fmt.Errorf("ovh: %w", err)
 	}
 
-	config := NewDefaultConfig()
-	config.APIEndpoint = values[EnvEndpoint]
-
-	config.ApplicationConfig = &ApplicationConfig{
-		ApplicationKey:    values[EnvApplicationKey],
-		ApplicationSecret: values[EnvApplicationSecret],
-		ConsumerKey:       values[EnvConsumerKey],
-	}
-
 	return NewDNSProviderConfig(config)
 }
 
@@ -140,55 +112,7 @@ func NewDNSProviderConfig(config *Config) (*DNSProvider, error) {
 		return nil, errors.New("ovh: the configuration of the DNS provider is nil")
 	}
 
-	apiConfig := config.ApplicationConfig
-	if apiConfig == nil {
-		return nil, errors.New("ovh: the configuration of ApplicationConfig is nil")
-	}
-
-	if config.APIEndpoint == "" || apiConfig.ApplicationKey == "" || apiConfig.ApplicationSecret == "" || apiConfig.ConsumerKey == "" {
-		return nil, errors.New("ovh: credentials missing")
-	}
-
-	client, err := ovh.NewClient(
-		config.APIEndpoint,
-		apiConfig.ApplicationKey,
-		apiConfig.ApplicationSecret,
-		apiConfig.ConsumerKey,
-	)
-	if err != nil {
-		return nil, fmt.Errorf("ovh: %w", err)
-	}
-
-	client.Client = config.HTTPClient
-
-	return &DNSProvider{
-		config:    config,
-		client:    client,
-		recordIDs: make(map[string]int64),
-	}, nil
-}
-
-// NewDNSProviderConfig return a DNSProvider instance configured for OVH.
-func NewDNSProviderOAuth2Config(config *Config) (*DNSProvider, error) {
-	if config == nil {
-		return nil, errors.New("ovh: the configuration of the DNS provider is nil")
-	}
-
-	oauth2Config := config.OAuth2Config
-	if oauth2Config == nil {
-		return nil, errors.New("ovh: the configuration of OAuth2Config is nil")
-	}
-
-	if config.APIEndpoint == "" || oauth2Config.ClientId == "" || oauth2Config.ClientSecret == "" {
-		return nil, errors.New("ovh: credentials missing")
-	}
-
-	client, err := ovh.NewOAuth2Client(
-		config.APIEndpoint,
-		oauth2Config.ClientId,
-		oauth2Config.ClientSecret,
-	)
-
+	client, err := newClient(config)
 	if err != nil {
 		return nil, fmt.Errorf("ovh: %w", err)
 	}
@@ -289,3 +213,95 @@ func (d *DNSProvider) CleanUp(domain, token, keyAuth string) error {
 func (d *DNSProvider) Timeout() (timeout, interval time.Duration) {
 	return d.config.PropagationTimeout, d.config.PollingInterval
 }
+
+func createConfigFromEnvVars() (*Config, error) {
+	firstAppKeyEnvVar := findFirstValuedEnvVar(EnvApplicationKey, EnvApplicationSecret, EnvConsumerKey)
+	firstOAuth2EnvVar := findFirstValuedEnvVar(EnvClientID, EnvClientSecret)
+
+	if firstAppKeyEnvVar != "" && firstOAuth2EnvVar != "" {
+		return nil, fmt.Errorf("can't use both %s and %s at the same time", firstAppKeyEnvVar, firstOAuth2EnvVar)
+	}
+
+	config := NewDefaultConfig()
+
+	if firstOAuth2EnvVar != "" {
+		values, err := env.Get(EnvEndpoint, EnvClientID, EnvClientSecret)
+		if err != nil {
+			return nil, err
+		}
+
+		config.APIEndpoint = values[EnvEndpoint]
+		config.OAuth2Config = &OAuth2Config{
+			ClientID:     values[EnvClientID],
+			ClientSecret: values[EnvClientSecret],
+		}
+
+		return config, nil
+	}
+
+	values, err := env.Get(EnvEndpoint, EnvApplicationKey, EnvApplicationSecret, EnvConsumerKey)
+	if err != nil {
+		return nil, err
+	}
+
+	config.APIEndpoint = values[EnvEndpoint]
+
+	config.ApplicationKey = values[EnvApplicationKey]
+	config.ApplicationSecret = values[EnvApplicationSecret]
+	config.ConsumerKey = values[EnvConsumerKey]
+
+	return config, nil
+}
+
+func findFirstValuedEnvVar(envVars ...string) string {
+	for _, envVar := range envVars {
+		if env.GetOrFile(envVar) != "" {
+			return envVar
+		}
+	}
+
+	return ""
+}
+
+func newClient(config *Config) (*ovh.Client, error) {
+	if config.OAuth2Config == nil {
+		return newClientApplicationKey(config)
+	}
+
+	return newClientOAuth2(config)
+}
+
+func newClientApplicationKey(config *Config) (*ovh.Client, error) {
+	if config.APIEndpoint == "" || config.ApplicationKey == "" || config.ApplicationSecret == "" || config.ConsumerKey == "" {
+		return nil, errors.New("credentials are missing")
+	}
+
+	client, err := ovh.NewClient(
+		config.APIEndpoint,
+		config.ApplicationKey,
+		config.ApplicationSecret,
+		config.ConsumerKey,
+	)
+	if err != nil {
+		return nil, fmt.Errorf("new client: %w", err)
+	}
+
+	return client, nil
+}
+
+func newClientOAuth2(config *Config) (*ovh.Client, error) {
+	if config.APIEndpoint == "" || config.OAuth2Config.ClientID == "" || config.OAuth2Config.ClientSecret == "" {
+		return nil, errors.New("credentials are missing")
+	}
+
+	client, err := ovh.NewOAuth2Client(
+		config.APIEndpoint,
+		config.OAuth2Config.ClientID,
+		config.OAuth2Config.ClientSecret,
+	)
+	if err != nil {
+		return nil, fmt.Errorf("new OAuth2 client: %w", err)
+	}
+
+	return client, nil
+}

providers/dns/ovh/ovh.toml

@@ -34,7 +34,7 @@ When requesting the consumer key, the following configuration can be used to def
 }
 ```
 
-## OAuth2 client credentials
+## OAuth2 Client Credentials
 
 Another method for authentication is by using OAuth2 client credentials.
 
@@ -48,7 +48,7 @@ Following IAM policies need to be authorized for the affected domain :
 
 ## Important note
 
-Either authentication methods can be used **but** not both at the same time and doing so will result in an error.
+Both authentication methods cannot be used at the same time.
 '''
 
 [Configuration]
@@ -57,10 +57,8 @@ Either authentication methods can be used **but** not both at the same time and
     OVH_APPLICATION_KEY = "Application key"
     OVH_APPLICATION_SECRET = "Application secret"
     OVH_CONSUMER_KEY = "Consumer key"
-  [Configuration.OAuth2Client]
-    OVH_ENDPOINT = "Endpoint URL (ovh-eu or ovh-ca)"
-    OVH_CLIENT_ID = "Client ID"
-    OVH_CLIENT_SECRET = "Client secret"
+    OVH_CLIENT_ID = "Client ID (OAuth2)"
+    OVH_CLIENT_SECRET = "Client secret (OAuth2)"
   [Configuration.Additional]
     OVH_POLLING_INTERVAL = "Time between DNS propagation check"
     OVH_PROPAGATION_TIMEOUT = "Maximum waiting time for DNS propagation"

providers/dns/ovh/ovh_test.go

@@ -15,7 +15,7 @@ var envTest = tester.NewEnvTest(
 	EnvApplicationKey,
 	EnvApplicationSecret,
 	EnvConsumerKey,
-	EnvClientId,
+	EnvClientID,
 	EnvClientSecret).
 	WithDomain(envDomain)
 
@@ -38,7 +38,7 @@ func TestNewDNSProvider(t *testing.T) {
 			desc: "success client id",
 			envVars: map[string]string{
 				EnvEndpoint:     "ovh-eu",
-				EnvClientId:     "E",
+				EnvClientID:     "E",
 				EnvClientSecret: "F",
 			},
 		},
@@ -70,7 +70,7 @@ func TestNewDNSProvider(t *testing.T) {
 				EnvApplicationSecret: "C",
 				EnvConsumerKey:       "D",
 			},
-			expected: "ovh: unknown endpoint 'foobar', consider checking 'Endpoints' list or using an URL",
+			expected: "ovh: new client: unknown endpoint 'foobar', consider checking 'Endpoints' list or using an URL",
 		},
 		{
 			desc: "missing application key",
@@ -106,7 +106,7 @@ func TestNewDNSProvider(t *testing.T) {
 			desc: "missing client secret",
 			envVars: map[string]string{
 				EnvEndpoint:     "ovh-eu",
-				EnvClientId:     "A",
+				EnvClientID:     "A",
 				EnvClientSecret: "",
 			},
 			expected: "ovh: some credentials information are missing: OVH_CLIENT_SECRET",
@@ -118,10 +118,10 @@ func TestNewDNSProvider(t *testing.T) {
 				EnvApplicationKey:    "B",
 				EnvApplicationSecret: "C",
 				EnvConsumerKey:       "D",
-				EnvClientId:          "E",
+				EnvClientID:          "E",
 				EnvClientSecret:      "F",
 			},
-			expected: "ovh: set OVH_APPLICATION_KEY or OVH_CLIENT_ID but not both",
+			expected: "ovh: can't use both OVH_APPLICATION_KEY and OVH_CLIENT_ID at the same time",
 		},
 	}
 
@@ -154,6 +154,8 @@ func TestNewDNSProviderConfig(t *testing.T) {
 		applicationKey    string
 		applicationSecret string
 		consumerKey       string
+		clientID          string
+		clientSecret      string
 		expected          string
 	}{
 		{
@@ -165,61 +167,101 @@ func TestNewDNSProviderConfig(t *testing.T) {
 		},
 		{
 			desc:     "missing credentials",
-			expected: "ovh: credentials missing",
+			expected: "ovh: credentials are missing",
 		},
 		{
-			desc:              "missing api endpoint",
+			desc:              "application key: missing api endpoint",
 			apiEndpoint:       "",
 			applicationKey:    "B",
 			applicationSecret: "C",
 			consumerKey:       "D",
-			expected:          "ovh: credentials missing",
+			expected:          "ovh: credentials are missing",
 		},
 		{
-			desc:              "invalid api endpoint",
+			desc:              "application key: invalid api endpoint",
 			apiEndpoint:       "foobar",
 			applicationKey:    "B",
 			applicationSecret: "C",
 			consumerKey:       "D",
-			expected:          "ovh: unknown endpoint 'foobar', consider checking 'Endpoints' list or using an URL",
+			expected:          "ovh: new client: unknown endpoint 'foobar', consider checking 'Endpoints' list or using an URL",
 		},
 		{
-			desc:              "missing application key",
+			desc:              "application key: missing application key",
 			apiEndpoint:       "ovh-eu",
 			applicationKey:    "",
 			applicationSecret: "C",
 			consumerKey:       "D",
-			expected:          "ovh: credentials missing",
+			expected:          "ovh: credentials are missing",
 		},
 		{
-			desc:              "missing application secret",
+			desc:              "application key: missing application secret",
 			apiEndpoint:       "ovh-eu",
 			applicationKey:    "B",
 			applicationSecret: "",
 			consumerKey:       "D",
-			expected:          "ovh: credentials missing",
+			expected:          "ovh: credentials are missing",
 		},
 		{
-			desc:              "missing consumer key",
+			desc:              "application key: missing consumer key",
 			apiEndpoint:       "ovh-eu",
 			applicationKey:    "B",
 			applicationSecret: "C",
 			consumerKey:       "",
-			expected:          "ovh: credentials missing",
+			expected:          "ovh: credentials are missing",
+		},
+		{
+			desc:         "oauth2: success",
+			apiEndpoint:  "ovh-eu",
+			clientID:     "B",
+			clientSecret: "C",
+		},
+		{
+			desc:         "oauth2: missing api endpoint",
+			apiEndpoint:  "",
+			clientID:     "B",
+			clientSecret: "C",
+			expected:     "ovh: credentials are missing",
+		},
+		{
+			desc:         "oauth2: invalid api endpoint",
+			apiEndpoint:  "foobar",
+			clientID:     "B",
+			clientSecret: "C",
+			expected:     "ovh: new OAuth2 client: unknown endpoint 'foobar', consider checking 'Endpoints' list or using an URL",
+		},
+		{
+			desc:         "oauth2: missing client id",
+			apiEndpoint:  "ovh-eu",
+			clientID:     "",
+			clientSecret: "C",
+			expected:     "ovh: credentials are missing",
+		},
+		{
+			desc:         "oauth2: missing client secret",
+			apiEndpoint:  "ovh-eu",
+			clientID:     "B",
+			clientSecret: "",
+			expected:     "ovh: credentials are missing",
 		},
 	}
 
+	// The OVH client use the same env vars than lego, so it requires to clean them.
+	defer envTest.RestoreEnv()
+	envTest.ClearEnv()
+
 	for _, test := range testCases {
 		t.Run(test.desc, func(t *testing.T) {
-			defer envTest.RestoreEnv()
-			envTest.ClearEnv()
-
 			config := NewDefaultConfig()
 			config.APIEndpoint = test.apiEndpoint
-			config.ApplicationConfig = &ApplicationConfig{
-				ApplicationKey:    test.applicationKey,
-				ApplicationSecret: test.applicationSecret,
-				ConsumerKey:       test.consumerKey,
+			config.ApplicationKey = test.applicationKey
+			config.ApplicationSecret = test.applicationSecret
+			config.ConsumerKey = test.consumerKey
+
+			if test.clientID != "" || test.clientSecret != "" {
+				config.OAuth2Config = &OAuth2Config{
+					ClientID:     test.clientID,
+					ClientSecret: test.clientSecret,
+				}
 			}
 
 			p, err := NewDNSProviderConfig(config)
@@ -237,81 +279,6 @@ func TestNewDNSProviderConfig(t *testing.T) {
 	}
 }
 
-func TestNewDNSProviderOAuth2Config(t *testing.T) {
-	testCases := []struct {
-		desc         string
-		apiEndpoint  string
-		clientId     string
-		clientSecret string
-		expected     string
-	}{
-		{
-			desc:         "success",
-			apiEndpoint:  "ovh-eu",
-			clientId:     "B",
-			clientSecret: "C",
-		},
-		{
-			desc:     "missing credentials",
-			expected: "ovh: credentials missing",
-		},
-		{
-			desc:         "missing api endpoint",
-			apiEndpoint:  "",
-			clientId:     "B",
-			clientSecret: "C",
-			expected:     "ovh: credentials missing",
-		},
-		{
-			desc:         "invalid api endpoint",
-			apiEndpoint:  "foobar",
-			clientId:     "B",
-			clientSecret: "C",
-			expected:     "ovh: unknown endpoint 'foobar', consider checking 'Endpoints' list or using an URL",
-		},
-		{
-			desc:         "missing client id",
-			apiEndpoint:  "ovh-eu",
-			clientId:     "",
-			clientSecret: "C",
-			expected:     "ovh: credentials missing",
-		},
-		{
-			desc:         "missing client secret",
-			apiEndpoint:  "ovh-eu",
-			clientId:     "B",
-			clientSecret: "",
-			expected:     "ovh: credentials missing",
-		},
-	}
-
-	for _, test := range testCases {
-		t.Run(test.desc, func(t *testing.T) {
-			defer envTest.RestoreEnv()
-			envTest.ClearEnv()
-
-			config := NewDefaultConfig()
-			config.APIEndpoint = test.apiEndpoint
-			config.OAuth2Config = &OAuth2Config{
-				ClientId:     test.clientId,
-				ClientSecret: test.clientSecret,
-			}
-
-			p, err := NewDNSProviderOAuth2Config(config)
-
-			if test.expected == "" {
-				require.NoError(t, err)
-				require.NotNil(t, p)
-				require.NotNil(t, p.config)
-				require.NotNil(t, p.client)
-				require.NotNil(t, p.recordIDs)
-			} else {
-				require.EqualError(t, err, test.expected)
-			}
-		})
-	}
-}
-
 func TestLivePresent(t *testing.T) {
 	if !envTest.IsLiveTest() {
 		t.Skip("skipping live test")