From 243070474f04b0e8e20561d95b6dfef97ae9401f Mon Sep 17 00:00:00 2001 From: Bogdan Malakhovich Date: Wed, 13 Sep 2023 21:45:15 +0200 Subject: [PATCH 1/5] fix: parse userinfo provided in nomad address - allow to have a scheme in the address with basic auth - fix nil point dereference error --- internal/cli/helpers.go | 45 +++++++++---- internal/cli/helpers_test.go | 123 +++++++++++++++++++++++++++++++++++ 2 files changed, 155 insertions(+), 13 deletions(-) create mode 100644 internal/cli/helpers_test.go diff --git a/internal/cli/helpers.go b/internal/cli/helpers.go index 72cc208c..534c2501 100644 --- a/internal/cli/helpers.go +++ b/internal/cli/helpers.go @@ -417,14 +417,29 @@ func clientOptsFromCLI(c *baseCommand) *api.Config { // format and if it is, it returns user, password and address. It returns "", "", // address otherwise. func handleBasicAuth(s string) (string, string, string) { - before, after, found := strings.Cut(s, "@") - if found { - user, pass, found := strings.Cut(before, ":") - if found { - return user, pass, after - } + beforeAt, afterAt, found := strings.Cut(s, "@") + if !found { + return "", "", s + } + + scheme, userpass, schemeFound := strings.Cut(beforeAt, "//") + if !schemeFound { + userpass = beforeAt + } + + user, pass, found := strings.Cut(userpass, ":") + if !found { + return "", "", s } - return "", "", before + + var addr string + if schemeFound { + addr = fmt.Sprintf("%s//%s", scheme, afterAt) + } else { + addr = afterAt + } + + return user, pass, addr } // clientOptsFromEnvironment populates api client conf with environment @@ -434,9 +449,11 @@ func clientOptsFromEnvironment(conf *api.Config) { // we support user:pass@addr here user, pass, addr := handleBasicAuth(v) conf.Address = addr - if user != "" && pass != "" { - conf.HttpAuth.Username = user - conf.HttpAuth.Password = pass + if user != "" || pass != "" { + conf.HttpAuth = &api.HttpBasicAuth{ + Username: user, + Password: pass, + } } } if v := os.Getenv("NOMAD_NAMESPACE"); v != "" { @@ -471,9 +488,11 @@ func clientOptsFromFlags(c *baseCommand, conf *api.Config) { // we support user:pass@addr here user, pass, addr := handleBasicAuth(cfg.address) conf.Address = addr - if user != "" && pass != "" { - conf.HttpAuth.Username = user - conf.HttpAuth.Password = pass + if user != "" || pass != "" { + conf.HttpAuth = &api.HttpBasicAuth{ + Username: user, + Password: pass, + } } } if cfg.namespace != "" { diff --git a/internal/cli/helpers_test.go b/internal/cli/helpers_test.go new file mode 100644 index 00000000..ee350553 --- /dev/null +++ b/internal/cli/helpers_test.go @@ -0,0 +1,123 @@ +package cli + +import ( + "testing" + + "github.com/hashicorp/nomad/api" + "github.com/shoenig/test/must" +) + +func TestHelpers_handleBasicAuth(t *testing.T) { + cases := []struct { + addr string + expectedUser string + expectedPass string + expectedAddr string + }{ + {"addr", "", "", "addr"}, + {"addr:port", "", "", "addr:port"}, + {"user:pass@addr", "user", "pass", "addr"}, + {"user:pass@addr:port", "user", "pass", "addr:port"}, + {"scheme://addr", "", "", "scheme://addr"}, + {"scheme://user:pass@addr", "user", "pass", "scheme://addr"}, + {"scheme://user:pass@addr:port", "user", "pass", "scheme://addr:port"}, + {"scheme://user:@addr:port", "user", "", "scheme://addr:port"}, + {"scheme://:pass@addr:port", "", "pass", "scheme://addr:port"}, + {"//user:pass@addr:port", "user", "pass", "//addr:port"}, + {"foo@bar", "", "", "foo@bar"}, + {"", "", "", ""}, + } + + for _, c := range cases { + t.Run(c.addr, func(t *testing.T) { + user, pass, addr := handleBasicAuth(c.addr) + + must.Eq(t, c.expectedUser, user) + must.Eq(t, c.expectedPass, pass) + must.Eq(t, c.expectedAddr, addr) + }) + } +} + +func TestHelpers_clientOptsFromEnvironment_Address(t *testing.T) { + cases := []struct { + name string + addr string + expectedAddress string + expectedHttpAuth *api.HttpBasicAuth + }{ + { + addr: "addr", + expectedAddress: "addr", + expectedHttpAuth: nil, + }, + { + addr: "scheme://user:pass@addr", + expectedAddress: "scheme://addr", + expectedHttpAuth: &api.HttpBasicAuth{Username: "user", Password: "pass"}, + }, + { + addr: "scheme://user:@addr", + expectedAddress: "scheme://addr", + expectedHttpAuth: &api.HttpBasicAuth{Username: "user", Password: ""}, + }, + { + addr: "scheme://:pass@addr", + expectedAddress: "scheme://addr", + expectedHttpAuth: &api.HttpBasicAuth{Username: "", Password: "pass"}, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + t.Setenv("NOMAD_ADDR", c.addr) + + conf := api.Config{HttpAuth: nil} + clientOptsFromEnvironment(&conf) + + must.Eq(t, c.expectedAddress, conf.Address) + must.Eq(t, c.expectedHttpAuth, conf.HttpAuth) + }) + } +} + +func TestHelpers_clientOptsFromFlags_Address(t *testing.T) { + cases := []struct { + addr string + expectedAddress string + expectedHttpAuth *api.HttpBasicAuth + }{ + { + addr: "addr", + expectedAddress: "addr", + expectedHttpAuth: nil, + }, + { + addr: "scheme://user:pass@addr", + expectedAddress: "scheme://addr", + expectedHttpAuth: &api.HttpBasicAuth{Username: "user", Password: "pass"}, + }, + { + addr: "scheme://user:@addr", + expectedAddress: "scheme://addr", + expectedHttpAuth: &api.HttpBasicAuth{Username: "user", Password: ""}, + }, + { + addr: "scheme://:pass@addr", + expectedAddress: "scheme://addr", + expectedHttpAuth: &api.HttpBasicAuth{Username: "", Password: "pass"}, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + cmd := baseCommand{nomadConfig: nomadConfig{address: c.addr}} + + conf := api.Config{HttpAuth: nil} + clientOptsFromFlags(&cmd, &conf) + + must.Eq(t, c.expectedAddress, conf.Address) + must.Eq(t, c.expectedHttpAuth, conf.HttpAuth) + }) + } +} From b14259315eb01804d37e03196aed2b2e996a6411 Mon Sep 17 00:00:00 2001 From: Bogdan Malakhovich Date: Thu, 14 Sep 2023 18:35:39 +0200 Subject: [PATCH 2/5] Use net/url to extract userinfo from a URL --- internal/cli/helpers.go | 40 ++++++++++++++---------------------- internal/cli/helpers_test.go | 18 ++++++++-------- 2 files changed, 24 insertions(+), 34 deletions(-) diff --git a/internal/cli/helpers.go b/internal/cli/helpers.go index 534c2501..80e40ac3 100644 --- a/internal/cli/helpers.go +++ b/internal/cli/helpers.go @@ -5,8 +5,8 @@ package cli import ( "fmt" + "net/url" "os" - "strings" "github.com/hashicorp/nomad/api" @@ -413,41 +413,32 @@ func clientOptsFromCLI(c *baseCommand) *api.Config { return conf } -// handlBasicAuth checks whether the NOMAD_ADDR string is in the user:pass@addr -// format and if it is, it returns user, password and address. It returns "", "", -// address otherwise. -func handleBasicAuth(s string) (string, string, string) { - beforeAt, afterAt, found := strings.Cut(s, "@") - if !found { +// extractBasicAuth removes the userinfo from the URL and returns the username, +// password, and the adjusted URL. If the URL does not contain userinfo, +// it returns an empty username and password, and the original URL. +func extractBasicAuth(s string) (string, string, string) { + u, err := url.Parse(s) + if err != nil { return "", "", s } - scheme, userpass, schemeFound := strings.Cut(beforeAt, "//") - if !schemeFound { - userpass = beforeAt - } - - user, pass, found := strings.Cut(userpass, ":") - if !found { + if u.User == nil { return "", "", s } - var addr string - if schemeFound { - addr = fmt.Sprintf("%s//%s", scheme, afterAt) - } else { - addr = afterAt - } + user := u.User.Username() + pass, _ := u.User.Password() + + u.User = nil - return user, pass, addr + return user, pass, u.String() } // clientOptsFromEnvironment populates api client conf with environment // variables present at the CLI's runtime. func clientOptsFromEnvironment(conf *api.Config) { if v := os.Getenv("NOMAD_ADDR"); v != "" { - // we support user:pass@addr here - user, pass, addr := handleBasicAuth(v) + user, pass, addr := extractBasicAuth(v) conf.Address = addr if user != "" || pass != "" { conf.HttpAuth = &api.HttpBasicAuth{ @@ -485,8 +476,7 @@ func clientOptsFromEnvironment(conf *api.Config) { func clientOptsFromFlags(c *baseCommand, conf *api.Config) { cfg := c.nomadConfig if cfg.address != "" { - // we support user:pass@addr here - user, pass, addr := handleBasicAuth(cfg.address) + user, pass, addr := extractBasicAuth(cfg.address) conf.Address = addr if user != "" || pass != "" { conf.HttpAuth = &api.HttpBasicAuth{ diff --git a/internal/cli/helpers_test.go b/internal/cli/helpers_test.go index ee350553..aaea6cf2 100644 --- a/internal/cli/helpers_test.go +++ b/internal/cli/helpers_test.go @@ -7,7 +7,7 @@ import ( "github.com/shoenig/test/must" ) -func TestHelpers_handleBasicAuth(t *testing.T) { +func TestHelpers_extractBasicAuth(t *testing.T) { cases := []struct { addr string expectedUser string @@ -15,22 +15,22 @@ func TestHelpers_handleBasicAuth(t *testing.T) { expectedAddr string }{ {"addr", "", "", "addr"}, - {"addr:port", "", "", "addr:port"}, - {"user:pass@addr", "user", "pass", "addr"}, - {"user:pass@addr:port", "user", "pass", "addr:port"}, + {"addr:1337", "", "", "addr:1337"}, + {"user:pass@addr", "", "", "user:pass@addr"}, + {"user:pass@addr:1337", "", "", "user:pass@addr:1337"}, {"scheme://addr", "", "", "scheme://addr"}, {"scheme://user:pass@addr", "user", "pass", "scheme://addr"}, - {"scheme://user:pass@addr:port", "user", "pass", "scheme://addr:port"}, - {"scheme://user:@addr:port", "user", "", "scheme://addr:port"}, - {"scheme://:pass@addr:port", "", "pass", "scheme://addr:port"}, - {"//user:pass@addr:port", "user", "pass", "//addr:port"}, + {"scheme://user:pass@addr:1337", "user", "pass", "scheme://addr:1337"}, + {"scheme://user:@addr:1337", "user", "", "scheme://addr:1337"}, + {"scheme://:pass@addr:1337", "", "pass", "scheme://addr:1337"}, + {"//user:pass@addr:1337", "user", "pass", "//addr:1337"}, {"foo@bar", "", "", "foo@bar"}, {"", "", "", ""}, } for _, c := range cases { t.Run(c.addr, func(t *testing.T) { - user, pass, addr := handleBasicAuth(c.addr) + user, pass, addr := extractBasicAuth(c.addr) must.Eq(t, c.expectedUser, user) must.Eq(t, c.expectedPass, pass) From ec3c0c4f867ed14a4f0d7ef9d57d0baa3e5c9a23 Mon Sep 17 00:00:00 2001 From: Bogdan Malakhovich Date: Thu, 14 Sep 2023 18:36:05 +0200 Subject: [PATCH 3/5] Remove unused name attribute from the test cases --- internal/cli/helpers_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/cli/helpers_test.go b/internal/cli/helpers_test.go index aaea6cf2..53b02e64 100644 --- a/internal/cli/helpers_test.go +++ b/internal/cli/helpers_test.go @@ -41,7 +41,6 @@ func TestHelpers_extractBasicAuth(t *testing.T) { func TestHelpers_clientOptsFromEnvironment_Address(t *testing.T) { cases := []struct { - name string addr string expectedAddress string expectedHttpAuth *api.HttpBasicAuth @@ -69,7 +68,7 @@ func TestHelpers_clientOptsFromEnvironment_Address(t *testing.T) { } for _, c := range cases { - t.Run(c.name, func(t *testing.T) { + t.Run(c.addr, func(t *testing.T) { t.Setenv("NOMAD_ADDR", c.addr) conf := api.Config{HttpAuth: nil} @@ -110,7 +109,7 @@ func TestHelpers_clientOptsFromFlags_Address(t *testing.T) { } for _, c := range cases { - t.Run(c.name, func(t *testing.T) { + t.Run(c.addr, func(t *testing.T) { cmd := baseCommand{nomadConfig: nomadConfig{address: c.addr}} conf := api.Config{HttpAuth: nil} From 7138b6351139c31ee8ad7b43a39e619d1176a6d2 Mon Sep 17 00:00:00 2001 From: Bogdan Malakhovich Date: Thu, 14 Sep 2023 18:45:42 +0200 Subject: [PATCH 4/5] Rename extractBasicAuth to removeBasicAuth to make it more intuitive that the resulting address doesn't include userinfo --- internal/cli/helpers.go | 8 ++++---- internal/cli/helpers_test.go | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/cli/helpers.go b/internal/cli/helpers.go index 80e40ac3..c18d6260 100644 --- a/internal/cli/helpers.go +++ b/internal/cli/helpers.go @@ -413,10 +413,10 @@ func clientOptsFromCLI(c *baseCommand) *api.Config { return conf } -// extractBasicAuth removes the userinfo from the URL and returns the username, +// removeBasicAuth removes userinfo from the URL and returns the username, // password, and the adjusted URL. If the URL does not contain userinfo, // it returns an empty username and password, and the original URL. -func extractBasicAuth(s string) (string, string, string) { +func removeBasicAuth(s string) (string, string, string) { u, err := url.Parse(s) if err != nil { return "", "", s @@ -438,7 +438,7 @@ func extractBasicAuth(s string) (string, string, string) { // variables present at the CLI's runtime. func clientOptsFromEnvironment(conf *api.Config) { if v := os.Getenv("NOMAD_ADDR"); v != "" { - user, pass, addr := extractBasicAuth(v) + user, pass, addr := removeBasicAuth(v) conf.Address = addr if user != "" || pass != "" { conf.HttpAuth = &api.HttpBasicAuth{ @@ -476,7 +476,7 @@ func clientOptsFromEnvironment(conf *api.Config) { func clientOptsFromFlags(c *baseCommand, conf *api.Config) { cfg := c.nomadConfig if cfg.address != "" { - user, pass, addr := extractBasicAuth(cfg.address) + user, pass, addr := removeBasicAuth(cfg.address) conf.Address = addr if user != "" || pass != "" { conf.HttpAuth = &api.HttpBasicAuth{ diff --git a/internal/cli/helpers_test.go b/internal/cli/helpers_test.go index 53b02e64..a7ef2038 100644 --- a/internal/cli/helpers_test.go +++ b/internal/cli/helpers_test.go @@ -7,7 +7,7 @@ import ( "github.com/shoenig/test/must" ) -func TestHelpers_extractBasicAuth(t *testing.T) { +func TestHelpers_removeBasicAuth(t *testing.T) { cases := []struct { addr string expectedUser string @@ -30,7 +30,7 @@ func TestHelpers_extractBasicAuth(t *testing.T) { for _, c := range cases { t.Run(c.addr, func(t *testing.T) { - user, pass, addr := extractBasicAuth(c.addr) + user, pass, addr := removeBasicAuth(c.addr) must.Eq(t, c.expectedUser, user) must.Eq(t, c.expectedPass, pass) From 6eedc8907796e3c36db41b96cff0bb8d2a6e36df Mon Sep 17 00:00:00 2001 From: Bogdan Malakhovich Date: Thu, 14 Sep 2023 19:31:47 +0200 Subject: [PATCH 5/5] Improve the signature of removeBasicAuth function --- internal/cli/helpers.go | 36 +++++++++++++++--------------------- internal/cli/helpers_test.go | 33 ++++++++++++++++----------------- 2 files changed, 31 insertions(+), 38 deletions(-) diff --git a/internal/cli/helpers.go b/internal/cli/helpers.go index c18d6260..97e1c9b9 100644 --- a/internal/cli/helpers.go +++ b/internal/cli/helpers.go @@ -413,36 +413,29 @@ func clientOptsFromCLI(c *baseCommand) *api.Config { return conf } -// removeBasicAuth removes userinfo from the URL and returns the username, -// password, and the adjusted URL. If the URL does not contain userinfo, -// it returns an empty username and password, and the original URL. -func removeBasicAuth(s string) (string, string, string) { - u, err := url.Parse(s) +// removeBasicAuth removes userinfo from a URL and returns the adjusted URL. +// The second return value is the userinfo that was removed. +func removeBasicAuth(addr string) (string, *url.Userinfo) { + u, err := url.Parse(addr) if err != nil { - return "", "", s + return addr, nil } - if u.User == nil { - return "", "", s - } - - user := u.User.Username() - pass, _ := u.User.Password() - + userinfo := u.User u.User = nil - - return user, pass, u.String() + return u.String(), userinfo } // clientOptsFromEnvironment populates api client conf with environment // variables present at the CLI's runtime. func clientOptsFromEnvironment(conf *api.Config) { if v := os.Getenv("NOMAD_ADDR"); v != "" { - user, pass, addr := removeBasicAuth(v) + addr, userinfo := removeBasicAuth(v) conf.Address = addr - if user != "" || pass != "" { + if userinfo != nil { + pass, _ := userinfo.Password() conf.HttpAuth = &api.HttpBasicAuth{ - Username: user, + Username: userinfo.Username(), Password: pass, } } @@ -476,11 +469,12 @@ func clientOptsFromEnvironment(conf *api.Config) { func clientOptsFromFlags(c *baseCommand, conf *api.Config) { cfg := c.nomadConfig if cfg.address != "" { - user, pass, addr := removeBasicAuth(cfg.address) + addr, userinfo := removeBasicAuth(cfg.address) conf.Address = addr - if user != "" || pass != "" { + if userinfo != nil { + pass, _ := userinfo.Password() conf.HttpAuth = &api.HttpBasicAuth{ - Username: user, + Username: userinfo.Username(), Password: pass, } } diff --git a/internal/cli/helpers_test.go b/internal/cli/helpers_test.go index a7ef2038..2467e89e 100644 --- a/internal/cli/helpers_test.go +++ b/internal/cli/helpers_test.go @@ -1,6 +1,7 @@ package cli import ( + "net/url" "testing" "github.com/hashicorp/nomad/api" @@ -10,31 +11,29 @@ import ( func TestHelpers_removeBasicAuth(t *testing.T) { cases := []struct { addr string - expectedUser string - expectedPass string expectedAddr string + expectedUser *url.Userinfo }{ - {"addr", "", "", "addr"}, - {"addr:1337", "", "", "addr:1337"}, - {"user:pass@addr", "", "", "user:pass@addr"}, - {"user:pass@addr:1337", "", "", "user:pass@addr:1337"}, - {"scheme://addr", "", "", "scheme://addr"}, - {"scheme://user:pass@addr", "user", "pass", "scheme://addr"}, - {"scheme://user:pass@addr:1337", "user", "pass", "scheme://addr:1337"}, - {"scheme://user:@addr:1337", "user", "", "scheme://addr:1337"}, - {"scheme://:pass@addr:1337", "", "pass", "scheme://addr:1337"}, - {"//user:pass@addr:1337", "user", "pass", "//addr:1337"}, - {"foo@bar", "", "", "foo@bar"}, - {"", "", "", ""}, + {"addr", "addr", nil}, + {"addr:1337", "addr:1337", nil}, + {"user:pass@addr", "user:pass@addr", nil}, + {"user:pass@addr:1337", "user:pass@addr:1337", nil}, + {"scheme://addr", "scheme://addr", nil}, + {"foo@bar", "foo@bar", nil}, + {"", "", nil}, + {"scheme://user:pass@addr", "scheme://addr", url.UserPassword("user", "pass")}, + {"scheme://user:pass@addr:1337", "scheme://addr:1337", url.UserPassword("user", "pass")}, + {"scheme://user:@addr:1337", "scheme://addr:1337", url.UserPassword("user", "")}, + {"scheme://:pass@addr:1337", "scheme://addr:1337", url.UserPassword("", "pass")}, + {"//user:pass@addr:1337", "//addr:1337", url.UserPassword("user", "pass")}, } for _, c := range cases { t.Run(c.addr, func(t *testing.T) { - user, pass, addr := removeBasicAuth(c.addr) + addr, userinfo := removeBasicAuth(c.addr) - must.Eq(t, c.expectedUser, user) - must.Eq(t, c.expectedPass, pass) must.Eq(t, c.expectedAddr, addr) + must.Eq(t, c.expectedUser, userinfo) }) } }