From 2d6791d0f463b3cec5f169595b31e34d7f27933c Mon Sep 17 00:00:00 2001 From: jannfis Date: Sat, 28 Mar 2020 20:10:16 +0100 Subject: [PATCH 1/6] Fix possible panic when generating Dex config from malformed YAML --- util/dex/config.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/util/dex/config.go b/util/dex/config.go index 4f6c5b4204279..fcadf1732ca75 100644 --- a/util/dex/config.go +++ b/util/dex/config.go @@ -58,14 +58,23 @@ func GenerateDexConfigYAML(settings *settings.ArgoCDSettings) ([]byte, error) { if err != nil { return nil, err } - connectors := dexCfg["connectors"].([]interface{}) + connectors, ok := dexCfg["connectors"].([]interface{}) + if !ok { + return nil, fmt.Errorf("malformed Dex configuration found") + } for i, connectorIf := range connectors { - connector := connectorIf.(map[string]interface{}) + connector, ok := connectorIf.(map[string]interface{}) + if !ok { + return nil, fmt.Errorf("malformed Dex configuration found") + } connectorType := connector["type"].(string) if !needsRedirectURI(connectorType) { continue } - connectorCfg := connector["config"].(map[string]interface{}) + connectorCfg, ok := connector["config"].(map[string]interface{}) + if !ok { + return nil, fmt.Errorf("malformed Dex configuration found") + } connectorCfg["redirectURI"] = dexRedirectURL connector["config"] = connectorCfg connectors[i] = connector From ea5279f899cae7479909ac095e6e77d394bb129b Mon Sep 17 00:00:00 2001 From: jannfis Date: Sun, 29 Mar 2020 10:32:23 +0200 Subject: [PATCH 2/6] Add first batch of tests for util/dex --- util/dex/dex_test.go | 108 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) create mode 100644 util/dex/dex_test.go diff --git a/util/dex/dex_test.go b/util/dex/dex_test.go new file mode 100644 index 0000000000000..29fd078939ed9 --- /dev/null +++ b/util/dex/dex_test.go @@ -0,0 +1,108 @@ +package dex + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + // "github.com/argoproj/argo-cd/common" + "github.com/argoproj/argo-cd/util/settings" +) + +var malformedDexConfig = ` +valid: + yaml: valid +yaml: + valid +` + +var goodDexConfig = ` +connectors: +# GitHub example +- type: github + id: github + name: GitHub + config: + clientID: aabbccddeeff00112233 + clientSecret: $dex.github.clientSecret + orgs: + - name: your-github-org + +# GitHub enterprise example +- type: github + id: acme-github + name: Acme GitHub + config: + hostName: github.acme.com + clientID: abcdefghijklmnopqrst + clientSecret: $dex.acme.clientSecret + orgs: + - name: your-github-org +` + +var goodSecrets = map[string]string{ + "dex.github.clientSecret": "foobar", + "dex.acme.clientSecret": "barfoo", +} + +func Test_GenerateDexConfig(t *testing.T) { + + t.Run("Empty settings", func(t *testing.T) { + s := settings.ArgoCDSettings{} + config, err := GenerateDexConfigYAML(&s) + assert.NoError(t, err) + assert.Nil(t, config) + }) + + t.Run("Invalid URL", func(t *testing.T) { + s := settings.ArgoCDSettings{ + URL: ":://localhost/foo/bar", + DexConfig: goodDexConfig, + } + config, err := GenerateDexConfigYAML(&s) + assert.Error(t, err) + assert.Nil(t, config) + }) + + t.Run("No URL set", func(t *testing.T) { + s := settings.ArgoCDSettings{ + URL: "", + DexConfig: "invalidyaml", + } + config, err := GenerateDexConfigYAML(&s) + assert.NoError(t, err) + assert.Nil(t, config) + }) + + t.Run("Invalid YAML", func(t *testing.T) { + s := settings.ArgoCDSettings{ + URL: "http://localhost", + DexConfig: "invalidyaml", + } + config, err := GenerateDexConfigYAML(&s) + assert.NoError(t, err) + assert.Nil(t, config) + }) + + t.Run("Valid YAML but incorrect Dex config", func(t *testing.T) { + s := settings.ArgoCDSettings{ + URL: "http://localhost", + DexConfig: malformedDexConfig, + } + config, err := GenerateDexConfigYAML(&s) + assert.Error(t, err) + assert.Nil(t, config) + }) + + t.Run("Valid YAML and correct Dex config", func(t *testing.T) { + s := settings.ArgoCDSettings{ + URL: "http://localhost", + DexConfig: goodDexConfig, + Secrets: goodSecrets, + } + config, err := GenerateDexConfigYAML(&s) + assert.NoError(t, err) + assert.NotNil(t, config) + }) + +} From 5529ca7c3aca1c5758074579fcc88957ee4b33b8 Mon Sep 17 00:00:00 2001 From: jannfis Date: Sun, 29 Mar 2020 12:38:37 +0200 Subject: [PATCH 3/6] More tests --- util/dex/dex_test.go | 54 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/util/dex/dex_test.go b/util/dex/dex_test.go index 29fd078939ed9..c35c37f8c1118 100644 --- a/util/dex/dex_test.go +++ b/util/dex/dex_test.go @@ -3,6 +3,7 @@ package dex import ( "testing" + "github.com/ghodss/yaml" "github.com/stretchr/testify/assert" // "github.com/argoproj/argo-cd/common" @@ -39,7 +40,25 @@ connectors: orgs: - name: your-github-org ` +var badDexConfig = ` +connectors: +# GitHub example +- type: github + id: github + name: GitHub + config: foo +# GitHub enterprise example +- type: github + id: acme-github + name: Acme GitHub + config: + hostName: github.acme.com + clientID: abcdefghijklmnopqrst + clientSecret: $dex.acme.clientSecret + orgs: + - name: your-github-org +` var goodSecrets = map[string]string{ "dex.github.clientSecret": "foobar", "dex.acme.clientSecret": "barfoo", @@ -94,7 +113,27 @@ func Test_GenerateDexConfig(t *testing.T) { assert.Nil(t, config) }) + t.Run("Valid YAML but incorrect Dex config", func(t *testing.T) { + s := settings.ArgoCDSettings{ + URL: "http://localhost", + DexConfig: badDexConfig, + } + config, err := GenerateDexConfigYAML(&s) + assert.Error(t, err) + assert.Nil(t, config) + }) + t.Run("Valid YAML and correct Dex config", func(t *testing.T) { + s := settings.ArgoCDSettings{ + URL: "http://localhost", + DexConfig: goodDexConfig, + } + config, err := GenerateDexConfigYAML(&s) + assert.NoError(t, err) + assert.NotNil(t, config) + }) + + t.Run("Secret dereference", func(t *testing.T) { s := settings.ArgoCDSettings{ URL: "http://localhost", DexConfig: goodDexConfig, @@ -103,6 +142,21 @@ func Test_GenerateDexConfig(t *testing.T) { config, err := GenerateDexConfigYAML(&s) assert.NoError(t, err) assert.NotNil(t, config) + var dexCfg map[string]interface{} + err = yaml.Unmarshal(config, &dexCfg) + if err != nil { + panic(err.Error()) + } + connectors, ok := dexCfg["connectors"].([]interface{}) + assert.True(t, ok) + for i, connectorsIf := range connectors { + config := connectorsIf.(map[string]interface{})["config"].(map[string]interface{}) + if i == 0 { + assert.Equal(t, "foobar", config["clientSecret"]) + } else if i == 1 { + assert.Equal(t, "barfoo", config["clientSecret"]) + } + } }) } From a9216ddfa80110566332c6eb6f685ab09d25abee Mon Sep 17 00:00:00 2001 From: jannfis Date: Sun, 29 Mar 2020 14:15:52 +0200 Subject: [PATCH 4/6] More tests --- util/dex/dex_test.go | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/util/dex/dex_test.go b/util/dex/dex_test.go index c35c37f8c1118..cf5dcbc8741c7 100644 --- a/util/dex/dex_test.go +++ b/util/dex/dex_test.go @@ -1,6 +1,9 @@ package dex import ( + "bytes" + "net/http" + "net/http/httptest" "testing" "github.com/ghodss/yaml" @@ -10,6 +13,8 @@ import ( "github.com/argoproj/argo-cd/util/settings" ) +const invalidURL = ":://localhost/foo/bar" + var malformedDexConfig = ` valid: yaml: valid @@ -75,7 +80,7 @@ func Test_GenerateDexConfig(t *testing.T) { t.Run("Invalid URL", func(t *testing.T) { s := settings.ArgoCDSettings{ - URL: ":://localhost/foo/bar", + URL: invalidURL, DexConfig: goodDexConfig, } config, err := GenerateDexConfigYAML(&s) @@ -159,4 +164,37 @@ func Test_GenerateDexConfig(t *testing.T) { } }) + t.Run("Redirect config", func(t *testing.T) { + types := []string{"oidc", "saml", "microsoft", "linkedin", "gitlab", "github", "bitbucket-cloud"} + for _, c := range types { + assert.True(t, needsRedirectURI(c)) + } + assert.False(t, needsRedirectURI("invalid")) + }) +} + +func Test_DexReverseProxy(t *testing.T) { + t.Run("Good case", func(t *testing.T) { + f := NewDexHTTPReverseProxy("127.0.0.1", "/") + assert.NotNil(t, f) + }) + + t.Run("Invalid URL for Dex reverse proxy", func(t *testing.T) { + // Can't test for now, since it would call exit + t.Skip() + f := NewDexHTTPReverseProxy(invalidURL, "/") + assert.Nil(t, f) + }) + + t.Run("Round Tripper", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + assert.Equal(t, "/", req.URL.String()) + })) + defer server.Close() + rt := NewDexRewriteURLRoundTripper(server.URL, http.DefaultTransport) + assert.NotNil(t, rt) + req, err := http.NewRequest("GET", "/", bytes.NewBuffer([]byte(""))) + assert.NoError(t, err) + rt.RoundTrip(req) + }) } From 1e8e77ee1ed8771667237b751828f4946d2fa4d3 Mon Sep 17 00:00:00 2001 From: jannfis Date: Sun, 29 Mar 2020 15:25:02 +0200 Subject: [PATCH 5/6] More tests --- util/dex/dex_test.go | 38 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/util/dex/dex_test.go b/util/dex/dex_test.go index cf5dcbc8741c7..750533a188d64 100644 --- a/util/dex/dex_test.go +++ b/util/dex/dex_test.go @@ -2,8 +2,10 @@ package dex import ( "bytes" + "fmt" "net/http" "net/http/httptest" + "strings" "testing" "github.com/ghodss/yaml" @@ -175,8 +177,40 @@ func Test_GenerateDexConfig(t *testing.T) { func Test_DexReverseProxy(t *testing.T) { t.Run("Good case", func(t *testing.T) { - f := NewDexHTTPReverseProxy("127.0.0.1", "/") - assert.NotNil(t, f) + fakeDex := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + rw.WriteHeader(http.StatusOK) + })) + defer fakeDex.Close() + fmt.Printf("Fake Dex listening on %s\n", fakeDex.URL) + server := httptest.NewServer(http.HandlerFunc(NewDexHTTPReverseProxy(fakeDex.URL, "/"))) + fmt.Printf("Fake API Server listening on %s\n", server.URL) + defer server.Close() + resp, err := http.Get(server.URL) + assert.NotNil(t, resp) + assert.NoError(t, err) + fmt.Printf("%s\n", resp.Status) + }) + + t.Run("Bad case", func(t *testing.T) { + fakeDex := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + rw.WriteHeader(http.StatusInternalServerError) + })) + defer fakeDex.Close() + fmt.Printf("Fake Dex listening on %s\n", fakeDex.URL) + server := httptest.NewServer(http.HandlerFunc(NewDexHTTPReverseProxy(fakeDex.URL, "/"))) + fmt.Printf("Fake API Server listening on %s\n", server.URL) + defer server.Close() + client := &http.Client{ + CheckRedirect: func(req *http.Request, via []*http.Request) error { + return http.ErrUseLastResponse + }} + resp, err := client.Get(server.URL) + assert.NotNil(t, resp) + assert.NoError(t, err) + assert.Equal(t, 303, resp.StatusCode) + location, _ := resp.Location() + fmt.Printf("%s %s\n", resp.Status, location.RequestURI()) + assert.True(t, strings.HasPrefix(location.RequestURI(), "/login?sso_error")) }) t.Run("Invalid URL for Dex reverse proxy", func(t *testing.T) { From e64cec3dae5eb9b24369af6ca113d11bf5f125ea Mon Sep 17 00:00:00 2001 From: jannfis Date: Sun, 29 Mar 2020 15:26:07 +0200 Subject: [PATCH 6/6] Use constants --- util/dex/dex_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/util/dex/dex_test.go b/util/dex/dex_test.go index 750533a188d64..57870a55039b7 100644 --- a/util/dex/dex_test.go +++ b/util/dex/dex_test.go @@ -188,6 +188,7 @@ func Test_DexReverseProxy(t *testing.T) { resp, err := http.Get(server.URL) assert.NotNil(t, resp) assert.NoError(t, err) + assert.Equal(t, http.StatusOK, resp.StatusCode) fmt.Printf("%s\n", resp.Status) }) @@ -207,7 +208,7 @@ func Test_DexReverseProxy(t *testing.T) { resp, err := client.Get(server.URL) assert.NotNil(t, resp) assert.NoError(t, err) - assert.Equal(t, 303, resp.StatusCode) + assert.Equal(t, http.StatusSeeOther, resp.StatusCode) location, _ := resp.Location() fmt.Printf("%s %s\n", resp.Status, location.RequestURI()) assert.True(t, strings.HasPrefix(location.RequestURI(), "/login?sso_error"))