From 97f8c5e1c6b0f6d0f0273c33a2af7e2873fc3394 Mon Sep 17 00:00:00 2001 From: Murillo <103451714+gruceo@users.noreply.github.com> Date: Tue, 13 Aug 2024 15:46:34 -0300 Subject: [PATCH 1/2] fix(plugins/acme): username/password is a valid authentication method Fixed an issue where username and password were not accepted as a valid authentication method. This is already accepted as valid authentication method in other plugins that use the shared Redis library such as the rate-limiting plugin. Depends on this PR of lua-resty-acme: https://github.com/fffonion/lua-resty-acme/pull/121 Fix FTI-6143 --- .github/workflows/build_and_test.yml | 13 ++ .../kong/fix-acme-username-password-auth.yml | 3 + kong/clustering/compat/checkers.lua | 15 ++ .../acme/storage/config_adapters/redis.lua | 2 + .../docker-compose-test-services.yml | 15 +- .../09-hybrid_mode/09-config-compat_spec.lua | 56 ++++++++ .../29-acme/05-redis_storage_spec.lua | 130 ++++++++++++++++++ spec/helpers.lua | 2 + 8 files changed, 235 insertions(+), 1 deletion(-) create mode 100644 changelog/unreleased/kong/fix-acme-username-password-auth.yml diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml index 9bcba8b5d1a1..6421ae583af7 100644 --- a/.github/workflows/build_and_test.yml +++ b/.github/workflows/build_and_test.yml @@ -198,6 +198,19 @@ jobs: ports: - 9411:9411 + redis-auth: + image: redis/redis-stack-server + # Set health checks to wait until redis has started + options: >- + --health-cmd "redis-cli ping" + --health-interval 10s + --health-timeout 5s + --health-retries 5 + ports: + - 6381:6379 + env: + REDIS_ARGS: "--requirepass passdefault" + steps: - name: Bump max open files run: | diff --git a/changelog/unreleased/kong/fix-acme-username-password-auth.yml b/changelog/unreleased/kong/fix-acme-username-password-auth.yml new file mode 100644 index 000000000000..63c7f620e529 --- /dev/null +++ b/changelog/unreleased/kong/fix-acme-username-password-auth.yml @@ -0,0 +1,3 @@ +message: "**ACME**: Fixed an issue where username and password were not accepted as valid authentication methods." +type: bugfix +scope: Plugin diff --git a/kong/clustering/compat/checkers.lua b/kong/clustering/compat/checkers.lua index 308c6ee35175..6504b18add73 100644 --- a/kong/clustering/compat/checkers.lua +++ b/kong/clustering/compat/checkers.lua @@ -43,6 +43,21 @@ local compatible_checkers = { function (config_table, dp_version, log_suffix) local has_update for _, plugin in ipairs(config_table.plugins or {}) do + if plugin.name == 'acme' then + local config = plugin.config + if config.storage_config.redis.username ~= nil then + log_warn_message('configures ' .. plugin.name .. ' plugin with redis username', + 'not work in this release', + dp_version, log_suffix) + end + + if config.storage_config.redis.password ~= nil then + log_warn_message('configures ' .. plugin.name .. ' plugin with redis password', + 'not work in this release. Please use redis.auth config instead', + dp_version, log_suffix) + end + end + if plugin.name == 'aws-lambda' then local config = plugin.config if config.aws_sts_endpoint_url ~= nil then diff --git a/kong/plugins/acme/storage/config_adapters/redis.lua b/kong/plugins/acme/storage/config_adapters/redis.lua index 48cb6362a761..0a41e06b8a30 100644 --- a/kong/plugins/acme/storage/config_adapters/redis.lua +++ b/kong/plugins/acme/storage/config_adapters/redis.lua @@ -7,6 +7,8 @@ local function redis_config_adapter(conf) ssl = conf.ssl, ssl_verify = conf.ssl_verify, ssl_server_name = conf.server_name, + username = conf.username, + password = conf.password, namespace = conf.extra_options.namespace, scan_count = conf.extra_options.scan_count, diff --git a/scripts/dependency_services/docker-compose-test-services.yml b/scripts/dependency_services/docker-compose-test-services.yml index 823b0c6e3f92..89fa458639fe 100644 --- a/scripts/dependency_services/docker-compose-test-services.yml +++ b/scripts/dependency_services/docker-compose-test-services.yml @@ -42,7 +42,20 @@ services: ports: - 127.0.0.1::9411 command: --logging.level.zipkin2=DEBUG - + redis-auth: + image: redis/redis-stack-server + ports: + - 127.0.0.1::6381 + environment: + - REDIS_ARGS=--requirepass passdefault + volumes: + - redis-auth-data:/data + healthcheck: + test: ["CMD", "redis-cli", "--pass", "passdefault", "ping"] + interval: 5s + timeout: 10s + retries: 10 volumes: postgres-data: redis-data: + redis-auth-data: diff --git a/spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua b/spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua index 3e3ca82af0a8..3ebdbac80290 100644 --- a/spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua +++ b/spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua @@ -341,6 +341,62 @@ describe("CP/DP config compat transformations #" .. strategy, function() describe("compatibility tests for redis standarization", function() describe("acme plugin", function() + it("translates standardized redis config to older acme structure", function() + -- [[ 3.8.x ]] -- + local acme = admin.plugins:insert { + name = "acme", + enabled = true, + config = { + account_email = "test@example.com", + storage = "redis", + storage_config = { + -- [[ new structure redis + redis = { + host = "localhost", + port = 57198, + username = "test", + password = "secret", + database = 2, + timeout = 1100, + ssl = true, + ssl_verify = true, + server_name = "example.test", + extra_options = { + namespace = "test_namespace", + scan_count = 13 + } + } + -- ]] + } + } + } + + local expected_acme_prior_38 = cycle_aware_deep_copy(acme) + expected_acme_prior_38.config.storage_config.redis = { + host = "localhost", + port = 57198, + username = "test", + auth = "secret", + password = "secret", + database = 2, + ssl = true, + ssl_verify = true, + ssl_server_name = "example.test", + namespace = "test_namespace", + scan_count = 13, + timeout = 1100, + server_name = "example.test", + extra_options = { + namespace = "test_namespace", + scan_count = 13 + } + } + do_assert(uuid(), "3.6.1", expected_acme_prior_38) + + -- cleanup + admin.plugins:remove({ id = acme.id }) + end) + it("translates standardized redis config to older acme structure", function() -- [[ 3.6.x ]] -- local acme = admin.plugins:insert { diff --git a/spec/03-plugins/29-acme/05-redis_storage_spec.lua b/spec/03-plugins/29-acme/05-redis_storage_spec.lua index d383c0c66c72..2c6f19bcdde4 100644 --- a/spec/03-plugins/29-acme/05-redis_storage_spec.lua +++ b/spec/03-plugins/29-acme/05-redis_storage_spec.lua @@ -555,4 +555,134 @@ describe("Plugin: acme (storage.redis)", function() end) end) + describe("redis authentication", function() + describe("happy path", function() + + it("should successfully connect to Redis with Auth using username/password", function() + local config = { + host = helpers.redis_host, + port = helpers.redis_auth_port, + database = 0, + username = "default", + password = "passdefault", + } + local storage, err = redis_storage.new(config) + assert.is_nil(err) + assert.not_nil(storage) + local err = storage:set("foo", "bar", 10) + assert.is_nil(err) + local value, err = storage:get("foo") + assert.is_nil(err) + assert.equal("bar", value) + end) + + it("should successfully connect to Redis with Auth using legacy auth", function() + local config = { + host = helpers.redis_host, + port = helpers.redis_auth_port, + database = 0, + auth = "passdefault", + } + local storage, err = redis_storage.new(config) + assert.is_nil(err) + assert.not_nil(storage) + local err = storage:set("foo", "bar", 10) + assert.is_nil(err) + local value, err = storage:get("foo") + assert.is_nil(err) + assert.equal("bar", value) + end) + + it("should successfully connect to Redis with Auth using just password", function() + local config = { + host = helpers.redis_host, + port = helpers.redis_auth_port, + database = 0, + password = "passdefault", + } + local storage, err = redis_storage.new(config) + assert.is_nil(err) + assert.not_nil(storage) + local err = storage:set("foo", "bar", 10) + assert.is_nil(err) + local value, err = storage:get("foo") + assert.is_nil(err) + assert.equal("bar", value) + end) + end) + + describe("unhappy path", function() + it("should not connect to Redis with Auth using just username", function() + local config = { + host = helpers.redis_host, + port = helpers.redis_auth_port, + database = 0, + username = "default", + } + local storage, err = redis_storage.new(config) + assert.is_nil(err) + assert.not_nil(storage) + local err = storage:set("foo", "bar", 10) + assert.equal("can't select database NOAUTH Authentication required.", err) + end) + + it("should not connect to Redis with Auth using wrong username", function() + local config = { + host = helpers.redis_host, + port = helpers.redis_auth_port, + database = 0, + username = "wrongusername", + } + local storage, err = redis_storage.new(config) + assert.is_nil(err) + assert.not_nil(storage) + local err = storage:set("foo", "bar", 10) + assert.equal("can't select database NOAUTH Authentication required.", err) + end) + + it("should not connect to Redis with Auth using wrong password and no username", function() + local config = { + host = helpers.redis_host, + port = helpers.redis_auth_port, + database = 0, + password = "wrongpassword" + } + local storage, err = redis_storage.new(config) + assert.is_nil(err) + assert.not_nil(storage) + local err = storage:set("foo", "bar", 10) + assert.equal("authentication failed WRONGPASS invalid username-password pair or user is disabled.", err) + end) + + it("should not connect to Redis with Auth using wrong password and correct username", function() + local config = { + host = helpers.redis_host, + port = helpers.redis_auth_port, + database = 0, + username = "default", + password = "wrongpassword" + } + local storage, err = redis_storage.new(config) + assert.is_nil(err) + assert.not_nil(storage) + local err = storage:set("foo", "bar", 10) + assert.equal("authentication failed WRONGPASS invalid username-password pair or user is disabled.", err) + end) + + it("should not connect to Redis with Auth using correct password and wrong username", function() + local config = { + host = helpers.redis_host, + port = helpers.redis_auth_port, + database = 0, + username = "kong", + password = "passdefault" + } + local storage, err = redis_storage.new(config) + assert.is_nil(err) + assert.not_nil(storage) + local err = storage:set("foo", "bar", 10) + assert.equal("authentication failed WRONGPASS invalid username-password pair or user is disabled.", err) + end) + end) + end) end) diff --git a/spec/helpers.lua b/spec/helpers.lua index 2f0e7a4ffb5a..67cc1daeb668 100644 --- a/spec/helpers.lua +++ b/spec/helpers.lua @@ -33,6 +33,7 @@ local OTELCOL_FILE_EXPORTER_PATH = os.getenv("KONG_SPEC_TEST_OTELCOL_FILE_EXPORT local REDIS_HOST = os.getenv("KONG_SPEC_TEST_REDIS_HOST") or "localhost" local REDIS_PORT = tonumber(os.getenv("KONG_SPEC_TEST_REDIS_PORT") or 6379) local REDIS_SSL_PORT = tonumber(os.getenv("KONG_SPEC_TEST_REDIS_SSL_PORT") or 6380) +local REDIS_AUTH_PORT = tonumber(os.getenv("KONG_SPEC_TEST_REDIS_AUTH_PORT") or 6381) local REDIS_SSL_SNI = os.getenv("KONG_SPEC_TEST_REDIS_SSL_SNI") or "test-redis.example.com" local TEST_COVERAGE_MODE = os.getenv("KONG_COVERAGE") local TEST_COVERAGE_TIMEOUT = 30 @@ -4340,6 +4341,7 @@ end redis_port = REDIS_PORT, redis_ssl_port = REDIS_SSL_PORT, redis_ssl_sni = REDIS_SSL_SNI, + redis_auth_port = REDIS_AUTH_PORT, blackhole_host = BLACKHOLE_HOST, From 8f1cb44cc71e57c47971f56196d8f869d72cc0c7 Mon Sep 17 00:00:00 2001 From: Murillo <103451714+gruceo@users.noreply.github.com> Date: Mon, 19 Aug 2024 07:45:01 -0300 Subject: [PATCH 2/2] translates 3.8.x standardized redis config to older (3.5.0) acme structure --- .../09-hybrid_mode/09-config-compat_spec.lua | 62 ++++++++++++++++++- 1 file changed, 60 insertions(+), 2 deletions(-) diff --git a/spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua b/spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua index 3ebdbac80290..fedf3ddf31eb 100644 --- a/spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua +++ b/spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua @@ -341,7 +341,65 @@ describe("CP/DP config compat transformations #" .. strategy, function() describe("compatibility tests for redis standarization", function() describe("acme plugin", function() - it("translates standardized redis config to older acme structure", function() + it("translates 3.8.x standardized redis config to older (3.5.0) acme structure", function() + -- [[ 3.8.x ]] -- + local acme = admin.plugins:insert { + name = "acme", + enabled = true, + config = { + account_email = "test@example.com", + storage = "redis", + storage_config = { + -- [[ new structure redis + redis = { + host = "localhost", + port = 57198, + username = "test", + password = "secret", + database = 2, + timeout = 1100, + ssl = true, + ssl_verify = true, + server_name = "example.test", + extra_options = { + namespace = "test_namespace", + scan_count = 13 + } + } + -- ]] + } + } + } + + local expected_acme_prior_38 = cycle_aware_deep_copy(acme) + expected_acme_prior_38.config.storage_config.redis = { + host = "localhost", + port = 57198, + -- username and password are not supported in 3.5.0 + --username = "test", + --password = "secret", + auth = "secret", + database = 2, + ssl = true, + ssl_verify = true, + ssl_server_name = "example.test", + namespace = "test_namespace", + scan_count = 13, + -- below fields are also not supported in 3.5.0 + --timeout = 1100, + --server_name = "example.test", + --extra_options = { + -- namespace = "test_namespace", + -- scan_count = 13 + --} + } + do_assert(uuid(), "3.5.0", expected_acme_prior_38) + + -- cleanup + admin.plugins:remove({ id = acme.id }) + end) + + it("translates 3.8.x standardized redis config to older (3.6.1) acme structure", function() -- [[ 3.8.x ]] -- local acme = admin.plugins:insert { name = "acme", @@ -397,7 +455,7 @@ describe("CP/DP config compat transformations #" .. strategy, function() admin.plugins:remove({ id = acme.id }) end) - it("translates standardized redis config to older acme structure", function() + it("translates 3.6.x standardized redis config to older (3.5.0) acme structure", function() -- [[ 3.6.x ]] -- local acme = admin.plugins:insert { name = "acme",