fix(plugins/acme): username/password is a valid authentication method (…

…#13496)

* 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: fffonion/lua-resty-acme#121

Fix FTI-6143

.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: |

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

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

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,

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:

spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua

@@ -341,7 +341,121 @@ 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 = "[email protected]",
+              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",
+            enabled = true,
+            config = {
+              account_email = "[email protected]",
+              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 3.6.x standardized redis config to older (3.5.0) acme structure", function()
           -- [[ 3.6.x ]] --
           local acme = admin.plugins:insert {
             name = "acme",

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)

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,