support etcd credentials in etcd plugin (#2442)

* support etcd credentials in etcd plugin

fixes #2441

* try to fix cleanup of authentication

6 files changed, 156 insertions, 14 deletions

diff --git a/plugin/etcd/README.md b/plugin/etcd/README.md
index 516b6bb48..efa8b8b6b 100644
--- a/plugin/etcd/README.md
+++ b/plugin/etcd/README.md
@@ -31,7 +31,8 @@ etcd [ZONES...] {
     fallthrough [ZONES...]
     path PATH
     endpoint ENDPOINT...
-    upstream
+    credentials USERNAME PASSWORD
+    upstream [ADDRESS...]
     tls CERT KEY CACERT
 }
 ~~~
@@ -42,8 +43,12 @@ etcd [ZONES...] {
   queries for those zones will be subject to fallthrough.
 * **PATH** the path inside etcd. Defaults to "/skydns".
 * **ENDPOINT** the etcd endpoints. Defaults to "http://localhost:2379".
-* `upstream` resolve names found in etcd (think CNAMEs) If you want CoreDNS to act as a proxy for clients,
-  you'll need to add the forward plugin. CoreDNS will resolve CNAMEs against itself.
+* `credentials` is used to set the **USERNAME** and **PASSWORD** for accessing the etcd cluster.
+* `upstream` upstream resolvers to be used resolve external names found in etcd (think CNAMEs)
+  pointing to external names. If you want CoreDNS to act as a proxy for clients, you'll need to add
+  the proxy plugin. If no **ADDRESS** is given, CoreDNS will resolve CNAMEs against itself.
+  **ADDRESS** can be an IP address, and IP:port or a string pointing to a file that is structured
+  as /etc/resolv.conf.
 * `tls` followed by:
 
     * no arguments, if the server certificate is signed by a system-installed CA and no client cert is needed
@@ -205,4 +210,4 @@ If you query the zone name for `TXT` now, you will get the following response:
 ~~~ sh
 % dig +short skydns.local TXT @localhost
 "this is a random text message."
-~~~
+~~~
\ No newline at end of file
diff --git a/plugin/etcd/lookup_test.go b/plugin/etcd/lookup_test.go
index ba2bb04f1..e440baedb 100644
--- a/plugin/etcd/lookup_test.go
+++ b/plugin/etcd/lookup_test.go
@@ -289,7 +289,7 @@ func newEtcdPlugin() *Etcd {
 
 	endpoints := []string{"http://localhost:2379"}
 	tlsc, _ := tls.NewTLSConfigFromArgs()
-	client, _ := newEtcdClient(endpoints, tlsc)
+	client, _ := newEtcdClient(endpoints, tlsc, "", "")
 
 	return &Etcd{
 		Upstream:   upstream.New(),
diff --git a/plugin/etcd/setup.go b/plugin/etcd/setup.go
index 68d5f147d..d0cdbc705 100644
--- a/plugin/etcd/setup.go
+++ b/plugin/etcd/setup.go
@@ -48,6 +48,8 @@ func etcdParse(c *caddy.Controller) (*Etcd, error) {
 		tlsConfig *tls.Config
 		err       error
 		endpoints = []string{defaultEndpoint}
+		username  string
+		password  string
 	)
 	for c.Next() {
 		etc.Zones = c.RemainingArgs()
@@ -89,6 +91,15 @@ func etcdParse(c *caddy.Controller) (*Etcd, error) {
 					if err != nil {
 						return &Etcd{}, err
 					}
+				case "credentials":
+					args := c.RemainingArgs()
+					if len(args) == 0 {
+						return &Etcd{}, c.ArgErr()
+					}
+					if len(args) != 2 {
+						return &Etcd{}, c.Errf("credentials requires 2 arguments, username and password")
+					}
+					username, password = args[0], args[1]
 				default:
 					if c.Val() != "}" {
 						return &Etcd{}, c.Errf("unknown property '%s'", c.Val())
@@ -101,7 +112,7 @@ func etcdParse(c *caddy.Controller) (*Etcd, error) {
 			}
 
 		}
-		client, err := newEtcdClient(endpoints, tlsConfig)
+		client, err := newEtcdClient(endpoints, tlsConfig, username, password)
 		if err != nil {
 			return &Etcd{}, err
 		}
@@ -113,11 +124,15 @@ func etcdParse(c *caddy.Controller) (*Etcd, error) {
 	return &Etcd{}, nil
 }
 
-func newEtcdClient(endpoints []string, cc *tls.Config) (*etcdcv3.Client, error) {
+func newEtcdClient(endpoints []string, cc *tls.Config, username, password string) (*etcdcv3.Client, error) {
 	etcdCfg := etcdcv3.Config{
 		Endpoints: endpoints,
 		TLS:       cc,
 	}
+	if username != "" && password != "" {
+		etcdCfg.Username = username
+		etcdCfg.Password = password
+	}
 	cli, err := etcdcv3.New(etcdCfg)
 	if err != nil {
 		return nil, err
diff --git a/plugin/etcd/setup_test.go b/plugin/etcd/setup_test.go
index 0696dc3e3..29018d623 100644
--- a/plugin/etcd/setup_test.go
+++ b/plugin/etcd/setup_test.go
@@ -14,43 +14,69 @@ func TestSetupEtcd(t *testing.T) {
 		expectedPath       string
 		expectedEndpoint   []string
 		expectedErrContent string // substring from the expected error. Empty for positive cases.
+		username           string
+		password           string
 	}{
 		// positive
 		{
-			`etcd`, false, "skydns", []string{"http://localhost:2379"}, "",
+			`etcd`, false, "skydns", []string{"http://localhost:2379"}, "", "", "",
 		},
 		{
 			`etcd {
 	endpoint http://localhost:2379 http://localhost:3379 http://localhost:4379
 
-}`, false, "skydns", []string{"http://localhost:2379", "http://localhost:3379", "http://localhost:4379"}, "",
+}`, false, "skydns", []string{"http://localhost:2379", "http://localhost:3379", "http://localhost:4379"}, "", "", "",
 		},
 		{
 			`etcd skydns.local {
 	endpoint localhost:300
 }
-`, false, "skydns", []string{"localhost:300"}, "",
+`, false, "skydns", []string{"localhost:300"}, "", "", "",
 		},
 		//test for upstream
 		{
 			`etcd {
 	endpoint localhost:300
 	upstream 8.8.8.8:53 8.8.4.4:53
-}`, false, "skydns", []string{"localhost:300"}, "",
+}`, false, "skydns", []string{"localhost:300"}, "", "", "",
 		},
 		//test for optional upstream address
 		{
 			`etcd {
 	endpoint localhost:300
 	upstream
-}`, false, "skydns", []string{"localhost:300"}, "",
+}`, false, "skydns", []string{"localhost:300"}, "", "", "",
 		},
 		// negative
 		{
 			`etcd {
 	endpoints localhost:300
 }
-`, true, "", []string{""}, "unknown property 'endpoints'",
+`, true, "", []string{""}, "unknown property 'endpoints'", "", "",
+		},
+		// with valid credentials
+		{
+			`etcd {
+			endpoint http://localhost:2379
+			credentials username password
+		}
+			`, false, "skydns", []string{"http://localhost:2379"}, "", "username", "password",
+		},
+		// with credentials, missing password
+		{
+			`etcd {
+			endpoint http://localhost:2379
+			credentials username
+		}
+			`, true, "skydns", []string{"http://localhost:2379"}, "credentials requires 2 arguments", "username", "",
+		},
+		// with credentials, missing username and  password
+		{
+			`etcd {
+			endpoint http://localhost:2379
+			credentials
+		}
+			`, true, "skydns", []string{"http://localhost:2379"}, "Wrong argument count", "", "",
 		},
 	}
 
@@ -69,7 +95,7 @@ func TestSetupEtcd(t *testing.T) {
 			}
 
 			if !strings.Contains(err.Error(), test.expectedErrContent) {
-				t.Errorf("Test %d: Expected error to contain: %v, found error: %v, input: %s", i, test.expectedErrContent, err, test.input)
+				t.Errorf("Test %d: Expected error to contain: %v, found error: %v, input: %s", i, test.expectedErrContent, err.Error(), test.input)
 				continue
 			}
 		}
@@ -87,5 +113,19 @@ func TestSetupEtcd(t *testing.T) {
 				}
 			}
 		}
+
+		if !test.shouldErr {
+			if test.username != "" {
+				if etcd.Client.Username != test.username {
+					t.Errorf("Etcd username not correctly set for input %s. Excpeted: '%+v', actual: '%+v'", test.input, test.username, etcd.Client.Username)
+				}
+			}
+			if test.password != "" {
+				if etcd.Client.Password != test.password {
+					t.Errorf("Etcd password not correctly set for input %s. Excpeted: '%+v', actual: '%+v'", test.input, test.password, etcd.Client.Password)
+				}
+			}
+		}
+
 	}
 }
diff --git a/test/etcd_credentials_test.go b/test/etcd_credentials_test.go
new file mode 100644
index 000000000..70586e4d1
--- /dev/null
+++ b/test/etcd_credentials_test.go
@@ -0,0 +1,72 @@
+// +build etcd
+
+package test
+
+import (
+	"context"
+	"testing"
+)
+
+// uses some stuff from etcd_tests.go
+
+func TestEtcdCredentials(t *testing.T) {
+	corefile := `.:0 {
+    etcd skydns.test {
+        path /skydns
+    }
+}`
+
+	ex, _, _, err := CoreDNSServerAndPorts(corefile)
+	if err != nil {
+		t.Fatalf("Could not get CoreDNS serving instance: %s", err)
+	}
+	defer ex.Stop()
+
+	etc := etcdPlugin()
+	username := "root"
+	password := "password"
+	key := "foo"
+	value := "bar"
+
+	var ctx = context.TODO()
+
+	if _, err := etc.Client.Put(ctx, key, value); err != nil {
+		t.Errorf("Failed to put dummy value un etcd: %v", err)
+	}
+
+	if _, err := etc.Client.RoleAdd(ctx, "root"); err != nil {
+		t.Errorf("Failed to create root role: %s", err)
+	}
+	if _, err := etc.Client.UserAdd(ctx, username, password); err != nil {
+		t.Errorf("Failed to create user: %s", err)
+	}
+	if _, err := etc.Client.UserGrantRole(ctx, username, "root"); err != nil {
+		t.Errorf("Failed to assign role to root user: %v", err)
+	}
+	if _, err := etc.Client.AuthEnable(ctx); err != nil {
+		t.Errorf("Failed to enable authentication: %s", err)
+	}
+
+	etc2 := etcdPluginWithCredentials(username, password)
+
+	defer func() {
+		if _, err := etc2.Client.AuthDisable(ctx); err != nil {
+			t.Errorf("Fail to disable authentication: %v", err)
+		}
+	}()
+
+	resp, err := etc2.Client.Get(ctx, key)
+	if err != nil {
+		t.Errorf("Fail to retrieve value from etcd: %v", err)
+	}
+
+	if len(resp.Kvs) != 1 {
+		t.Errorf("Too many response found: %+v", resp)
+		return
+	}
+	actual := resp.Kvs[0].Value
+	expected := "bar"
+	if string(resp.Kvs[0].Value) != expected {
+		t.Errorf("Value doesn't match, expected:%s actual:%s", actual, expected)
+	}
+}
diff --git a/test/etcd_test.go b/test/etcd_test.go
index 2b90c3a6c..d03f7ce3d 100644
--- a/test/etcd_test.go
+++ b/test/etcd_test.go
@@ -23,6 +23,16 @@ func etcdPlugin() *etcd.Etcd {
 	return &etcd.Etcd{Client: cli, PathPrefix: "/skydns"}
 }
 
+func etcdPluginWithCredentials(username, password string) *etcd.Etcd {
+	etcdCfg := etcdcv3.Config{
+		Endpoints: []string{"http://localhost:2379"},
+		Username:  username,
+		Password:  password,
+	}
+	cli, _ := etcdcv3.New(etcdCfg)
+	return &etcd.Etcd{Client: cli, PathPrefix: "/skydns"}
+}
+
 // This test starts two coredns servers (and needs etcd). Configure a stubzones in both (that will loop) and
 // will then test if we detect this loop.
 func TestEtcdStubLoop(t *testing.T) {