From 8b4abc6ef6dacf79d31bc7a1c2a7dec77ece0676 Mon Sep 17 00:00:00 2001 From: Andrei Aaron Date: Thu, 21 Mar 2024 19:23:37 +0200 Subject: [PATCH] Add a job to check zot config examples (and fix existing examples) (#2322) * fix: Add credentials config verification (cherry picked from commit e7fdfa0bcc8d5bf80163cae53a821682cf5322fe) Signed-off-by: Andrei Aaron * fix: Update golang version to 1.21.x Signed-off-by: onidoru <25552941+onidoru@users.noreply.github.com> Signed-off-by: Nikita Kotikov <25552941+onidoru@users.noreply.github.com> (cherry picked from commit cbc0f89dfb4067a9deafe6b602a7c1a6d61b3413) Signed-off-by: Andrei Aaron * fix: LDAP credentials files are now required, add more tests Signed-off-by: onidoru <25552941+onidoru@users.noreply.github.com> Signed-off-by: Nikita Kotikov <25552941+onidoru@users.noreply.github.com> (cherry picked from commit b74366d50b126581d086b47dc620589375874805) Signed-off-by: Andrei Aaron * fix: Update error handling, add more tests Signed-off-by: onidoru <25552941+onidoru@users.noreply.github.com> Signed-off-by: Nikita Kotikov <25552941+onidoru@users.noreply.github.com> (cherry picked from commit 8a61bbc2d482e1bd5c0c6240dde59c9850ac417b) Signed-off-by: Andrei Aaron * fix: Add coverage Signed-off-by: Andrei Aaron --------- Signed-off-by: onidoru <25552941+onidoru@users.noreply.github.com> Signed-off-by: Nikita Kotikov <25552941+onidoru@users.noreply.github.com> Signed-off-by: Andrei Aaron Co-authored-by: onidoru Co-authored-by: Nikita Kotikov <25552941+onidoru@users.noreply.github.com> --- .github/workflows/verify-config.yaml | 45 +++++++ Makefile | 2 +- examples/README.md | 4 +- examples/config-example.json | 3 +- examples/config-example.yaml | 3 +- pkg/cli/server/root.go | 31 ++++- pkg/cli/server/root_test.go | 195 ++++++++++++++++++++++++++- 7 files changed, 270 insertions(+), 13 deletions(-) create mode 100644 .github/workflows/verify-config.yaml diff --git a/.github/workflows/verify-config.yaml b/.github/workflows/verify-config.yaml new file mode 100644 index 00000000..5486f952 --- /dev/null +++ b/.github/workflows/verify-config.yaml @@ -0,0 +1,45 @@ +name: "Verify Example Config Files" + +# Validate all example config files are relevant and valid. + +on: + push: + branches: + - main + pull_request: + branches: [main] + release: + types: + - published + +permissions: read-all + +jobs: + verify-config: + name: Verify Config Files + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Install go + uses: actions/setup-go@v5 + with: + cache: false + go-version: 1.21.x + - name: Cache go dependencies + id: cache-go-dependencies + uses: actions/cache@v4 + with: + path: | + ~/go/pkg/mod + key: ${{ runner.os }}-go-mod-${{ hashFiles('**/go.sum') }} + restore-keys: | + ${{ runner.os }}-go-mod- + - name: Install go dependencies + if: steps.cache-go-dependencies.outputs.cache-hit != 'true' + run: | + cd $GITHUB_WORKSPACE + go mod download + - name: run verify-config + run: | + cd $GITHUB_WORKSPACE + make verify-config diff --git a/Makefile b/Makefile index 13e89e6e..f4ad0dcd 100644 --- a/Makefile +++ b/Makefile @@ -391,7 +391,7 @@ verify-config: _verify-config verify-config-warnings verify-config-commited .PHONY: _verify-config _verify-config: binary rm -f output.txt - $(foreach file, $(wildcard examples/config-*), ./bin/zot-$(OS)-$(ARCH) verify $(file) 2>&1 | tee -a output.txt || exit 1;) + $(foreach file, $(filter-out examples/config-ldap-credentials.json, $(wildcard examples/config-*)), ./bin/zot-$(OS)-$(ARCH) verify $(file) 2>&1 | tee -a output.txt || exit 1;) .PHONY: verify-config-warnings verify-config-warnings: _verify-config diff --git a/examples/README.md b/examples/README.md index 16efa6ce..03db18f1 100644 --- a/examples/README.md +++ b/examples/README.md @@ -225,14 +225,14 @@ authentication: "startTLS":false, "baseDN":"ou=Users,dc=example,dc=org", "userAttribute":"uid", - "bindDN":"cn=ldap-searcher,ou=Users,dc=example,dc=org", - "bindPassword":"ldap-searcher-password", + "credentialsFile": "config-ldap-credentials.json", "skipVerify":false, "subtreeSearch":true }, ``` NOTE: When both htpasswd and LDAP configuration are specified, LDAP authentication is given preference. +NOTE: The separate file for storing DN and password credentials must be created. You can see example in `examples/config-ldap-credentials.json` file. **OAuth2 authentication** (client credentials grant type) support via _Bearer Token_ configured with: diff --git a/examples/config-example.json b/examples/config-example.json index e61b97d4..729c8a38 100644 --- a/examples/config-example.json +++ b/examples/config-example.json @@ -18,8 +18,7 @@ "startTLS": false, "baseDN": "ou=Users,dc=example,dc=org", "userAttribute": "uid", - "bindDN": "cn=ldap-searcher,ou=Users,dc=example,dc=org", - "bindPassword": "ldap-searcher-password", + "credentialsFile": "examples/config-ldap-credentials.json", "skipVerify": false, "subtreeSearch": true }, diff --git a/examples/config-example.yaml b/examples/config-example.yaml index cdbe537d..be0180f1 100644 --- a/examples/config-example.yaml +++ b/examples/config-example.yaml @@ -8,8 +8,7 @@ http: ldap: address: ldap.example.org basedn: ou=Users,dc=example,dc=org - binddn: cn=ldap-searcher,ou=Users,dc=example,dc=org - bindpassword: ldap-searcher-password + credentialsFile: examples/config-ldap-credentials.json port: 389 skipverify: false starttls: false diff --git a/pkg/cli/server/root.go b/pkg/cli/server/root.go index 71fe9f77..2ce6fa69 100644 --- a/pkg/cli/server/root.go +++ b/pkg/cli/server/root.go @@ -2,6 +2,7 @@ package server import ( "context" + "errors" "fmt" "net" "net/http" @@ -856,15 +857,37 @@ func readLDAPCredentials(ldapConfigPath string) (config.LDAPCredentials, error) if err := viperInstance.ReadInConfig(); err != nil { log.Error().Err(err).Msg("failed to read configuration") - return config.LDAPCredentials{}, err + return config.LDAPCredentials{}, errors.Join(zerr.ErrBadConfig, err) } var ldapCredentials config.LDAPCredentials - if err := viperInstance.Unmarshal(&ldapCredentials); err != nil { - log.Error().Err(err).Msg("failed to unmarshal new config") + metaData := &mapstructure.Metadata{} + if err := viperInstance.Unmarshal(&ldapCredentials, metadataConfig(metaData)); err != nil { + log.Error().Err(err).Msg("failed to unmarshal ldap credentials config") - return config.LDAPCredentials{}, err + return config.LDAPCredentials{}, errors.Join(zerr.ErrBadConfig, err) + } + + if len(metaData.Keys) == 0 { + log.Error().Err(zerr.ErrBadConfig). + Msg("failed to load ldap credentials config due to the absence of any key:value pair") + + return config.LDAPCredentials{}, zerr.ErrBadConfig + } + + if len(metaData.Unused) > 0 { + log.Error().Err(zerr.ErrBadConfig).Strs("keys", metaData.Unused). + Msg("failed to load ldap credentials config due to unknown keys") + + return config.LDAPCredentials{}, zerr.ErrBadConfig + } + + if len(metaData.Unset) > 0 { + log.Error().Err(zerr.ErrBadConfig).Strs("keys", metaData.Unset). + Msg("failed to load ldap credentials config due to unset keys") + + return config.LDAPCredentials{}, zerr.ErrBadConfig } return ldapCredentials, nil diff --git a/pkg/cli/server/root_test.go b/pkg/cli/server/root_test.go index ad7e6230..38c08c3c 100644 --- a/pkg/cli/server/root_test.go +++ b/pkg/cli/server/root_test.go @@ -1159,7 +1159,7 @@ storage: content := []byte(`{"distSpecVersion":"1.1.0","storage":{"rootDirectory":"/tmp/zot"}, "http":{"address":"127.0.0.1","port":"8080","realm":"zot", "auth":{"openid":{"providers":{"oidc":{"issuer":"http://127.0.0.1:5556/dex", - "clientid":"client_id","scopes":["openid"]}}}}}, + "clientid":"client_id","scopes":["openid"]}}}}}, "log":{"level":"debug"}}`) _, err = tmpfile.Write(content) So(err, ShouldBeNil) @@ -1236,6 +1236,197 @@ storage: err = cli.NewServerRootCmd().Execute() So(err, ShouldBeNil) }) + + Convey("Test verify good ldap config", t, func(c C) { + tmpFile, err := os.CreateTemp("", "zot-test*.json") + So(err, ShouldBeNil) + defer os.Remove(tmpFile.Name()) + + tmpCredsFile, err := os.CreateTemp("", "zot-cred*.json") + So(err, ShouldBeNil) + defer os.Remove(tmpCredsFile.Name()) + + content := []byte(`{ + "bindDN":"cn=ldap-searcher,ou=Users,dc=example,dc=org", + "bindPassword":"ldap-searcher-password" + }`) + + _, err = tmpCredsFile.Write(content) + So(err, ShouldBeNil) + err = tmpCredsFile.Close() + So(err, ShouldBeNil) + + content = []byte(fmt.Sprintf(`{ "distSpecVersion": "1.1.0-dev", + "storage": { "rootDirectory": "/tmp/zot" }, "http": { "address": "127.0.0.1", "port": "8080", + "auth": { "ldap": { "credentialsFile": "%v", "address": "ldap.example.org", "port": 389, + "startTLS": false, "baseDN": "ou=Users,dc=example,dc=org", + "userAttribute": "uid", "userGroupAttribute": "memberOf", "skipVerify": true, "subtreeSearch": true }, + "failDelay": 5 } }, "log": { "level": "debug" } }`, + tmpCredsFile.Name()), + ) + + _, err = tmpFile.Write(content) + So(err, ShouldBeNil) + err = tmpFile.Close() + So(err, ShouldBeNil) + + os.Args = []string{"cli_test", "verify", tmpFile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldBeNil) + }) + + Convey("Test verify bad ldap config: key is missing", t, func(c C) { + tmpFile, err := os.CreateTemp("", "zot-test*.json") + So(err, ShouldBeNil) + defer os.Remove(tmpFile.Name()) + + tmpCredsFile, err := os.CreateTemp("", "zot-cred*.json") + So(err, ShouldBeNil) + defer os.Remove(tmpCredsFile.Name()) + + // `bindDN` key is missing + content := []byte(`{ + "bindPassword":"ldap-searcher-password" + }`) + + _, err = tmpCredsFile.Write(content) + So(err, ShouldBeNil) + err = tmpCredsFile.Close() + So(err, ShouldBeNil) + + content = []byte(fmt.Sprintf(`{ "distSpecVersion": "1.1.0-dev", + "storage": { "rootDirectory": "/tmp/zot" }, "http": { "address": "127.0.0.1", "port": "8080", + "auth": { "ldap": { "credentialsFile": "%v", "address": "ldap.example.org", "port": 389, + "startTLS": false, "baseDN": "ou=Users,dc=example,dc=org", + "userAttribute": "uid", "userGroupAttribute": "memberOf", "skipVerify": true, "subtreeSearch": true }, + "failDelay": 5 } }, "log": { "level": "debug" } }`, + tmpCredsFile.Name()), + ) + + _, err = tmpFile.Write(content) + So(err, ShouldBeNil) + err = tmpFile.Close() + So(err, ShouldBeNil) + + os.Args = []string{"cli_test", "verify", tmpFile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) + So(err.Error(), ShouldContainSubstring, "invalid server config") + }) + + Convey("Test verify bad ldap config: unused key", t, func(c C) { + tmpFile, err := os.CreateTemp("", "zot-test*.json") + So(err, ShouldBeNil) + defer os.Remove(tmpFile.Name()) + + tmpCredsFile, err := os.CreateTemp("", "zot-cred*.json") + So(err, ShouldBeNil) + defer os.Remove(tmpCredsFile.Name()) + + content := []byte(`{ + "bindDN":"cn=ldap-searcher,ou=Users,dc=example,dc=org", + "bindPassword":"ldap-searcher-password", + "extraKey": "extraValue" + }`) + + _, err = tmpCredsFile.Write(content) + So(err, ShouldBeNil) + err = tmpCredsFile.Close() + So(err, ShouldBeNil) + + content = []byte(fmt.Sprintf(`{ "distSpecVersion": "1.1.0-dev", + "storage": { "rootDirectory": "/tmp/zot" }, "http": { "address": "127.0.0.1", "port": "8080", + "auth": { "ldap": { "credentialsFile": "%v", "address": "ldap.example.org", "port": 389, + "startTLS": false, "baseDN": "ou=Users,dc=example,dc=org", + "userAttribute": "uid", "userGroupAttribute": "memberOf", "skipVerify": true, "subtreeSearch": true }, + "failDelay": 5 } }, "log": { "level": "debug" } }`, + tmpCredsFile.Name()), + ) + + _, err = tmpFile.Write(content) + So(err, ShouldBeNil) + err = tmpFile.Close() + So(err, ShouldBeNil) + + os.Args = []string{"cli_test", "verify", tmpFile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) + So(err.Error(), ShouldContainSubstring, "invalid server config") + }) + + Convey("Test verify bad ldap config: empty credentials file", t, func(c C) { + tmpFile, err := os.CreateTemp("", "zot-test*.json") + So(err, ShouldBeNil) + defer os.Remove(tmpFile.Name()) + + tmpCredsFile, err := os.CreateTemp("", "zot-cred*.json") + So(err, ShouldBeNil) + defer os.Remove(tmpCredsFile.Name()) + + // `bindDN` key is missing + content := []byte(``) + + _, err = tmpCredsFile.Write(content) + So(err, ShouldBeNil) + err = tmpCredsFile.Close() + So(err, ShouldBeNil) + + content = []byte(fmt.Sprintf(`{ "distSpecVersion": "1.1.0-dev", + "storage": { "rootDirectory": "/tmp/zot" }, "http": { "address": "127.0.0.1", "port": "8080", + "auth": { "ldap": { "credentialsFile": "%v", "address": "ldap.example.org", "port": 389, + "startTLS": false, "baseDN": "ou=Users,dc=example,dc=org", + "userAttribute": "uid", "userGroupAttribute": "memberOf", "skipVerify": true, "subtreeSearch": true }, + "failDelay": 5 } }, "log": { "level": "debug" } }`, + tmpCredsFile.Name()), + ) + + _, err = tmpFile.Write(content) + So(err, ShouldBeNil) + err = tmpFile.Close() + So(err, ShouldBeNil) + + os.Args = []string{"cli_test", "verify", tmpFile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) + So(err.Error(), ShouldContainSubstring, "invalid server config") + }) + + Convey("Test verify bad ldap config: no keys set in credentials file", t, func(c C) { + tmpFile, err := os.CreateTemp("", "zot-test*.json") + So(err, ShouldBeNil) + defer os.Remove(tmpFile.Name()) + + tmpCredsFile, err := os.CreateTemp("", "zot-cred*.json") + So(err, ShouldBeNil) + defer os.Remove(tmpCredsFile.Name()) + + // empty json + content := []byte(`{}`) + + _, err = tmpCredsFile.Write(content) + So(err, ShouldBeNil) + err = tmpCredsFile.Close() + So(err, ShouldBeNil) + + content = []byte(fmt.Sprintf(`{ "distSpecVersion": "1.1.0-dev", + "storage": { "rootDirectory": "/tmp/zot" }, "http": { "address": "127.0.0.1", "port": "8080", + "auth": { "ldap": { "credentialsFile": "%v", "address": "ldap.example.org", "port": 389, + "startTLS": false, "baseDN": "ou=Users,dc=example,dc=org", + "userAttribute": "uid", "userGroupAttribute": "memberOf", "skipVerify": true, "subtreeSearch": true }, + "failDelay": 5 } }, "log": { "level": "debug" } }`, + tmpCredsFile.Name()), + ) + + _, err = tmpFile.Write(content) + So(err, ShouldBeNil) + err = tmpFile.Close() + So(err, ShouldBeNil) + + os.Args = []string{"cli_test", "verify", tmpFile.Name()} + err = cli.NewServerRootCmd().Execute() + So(err, ShouldNotBeNil) + So(err.Error(), ShouldContainSubstring, "invalid server config") + }) } func TestApiKeyConfig(t *testing.T) { @@ -1248,7 +1439,7 @@ func TestApiKeyConfig(t *testing.T) { content := []byte(`{"distSpecVersion":"1.1.0","storage":{"rootDirectory":"/tmp/zot"}, "http":{"address":"127.0.0.1","port":"8080","realm":"zot", "auth":{"openid":{"providers":{"oidc":{"issuer":"http://127.0.0.1:5556/dex", - "clientid":"client_id","scopes":["openid"]}}}}}, + "clientid":"client_id","scopes":["openid"]}}}}}, "log":{"level":"debug"}}`) err = os.WriteFile(tmpfile.Name(), content, 0o0600)