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

gruceo · 2024-08-13T18:49:47Z

Fixed an issue where username and password were not accepted as valid authentication methods.

Depends on this PR of lua-resty-acme: fffonion/lua-resty-acme#121

Fix FTI-6143

Summary

Checklist

The Pull Request has tests
A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix FTI-6143

.github/workflows/build_and_test.yml

kong/plugins/acme/storage/config_adapters/redis.lua

spec/helpers.lua

nowNick

Looks good to me! 👍

kong/plugins/acme/storage/config_adapters/redis.lua

gruceo · 2024-08-15T11:27:50Z

@fffonion can you take another look at the PR, please?

fffonion

one comment about changelog, otherwise lgtm

fffonion · 2024-08-15T19:00:16Z

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


we probably need an addtional changelog for the bumped dependency, as I
heard from @AndyZhang0707

Done in 004627a

fffonion · 2024-08-15T19:02:07Z

also note the tests failed in spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua:340

gruceo · 2024-08-15T22:05:24Z

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


Thanks @fffonion. The test failure in spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua:340 was actually eye-opening. It revealed that removing the new username/password fields will cause issues to older DPs. Therefore I'm going with the safest option which is to not do anything (just log the warning instead). I'd like some input from @nowNick here as well.

This is the updated log warning in the CP side when we create acme plugin and have DP 3.6.1:

kong-cp | 2024/08/15 22:21:38 [warn] 1811#0: *1294 [lua] checkers.lua:20: log_warn_message(): [clustering] Kong Gateway v3.8.0 configures acme plugin with redis username which is incompatible with dataplane version 3.6.1 and will not work in this release.. [id: e79130ba-32cb-456d-a541-9a159d072142, host: 0c0d4012bade, ip: 172.18.0.8, version: 3.6.1], client: 172.18.0.8, server: kong_cluster_listener, request: "GET /v1/outlet?node_id=e79130ba-32cb-456d-a541-9a159d072142&node_hostname=0c0d4012bade&node_version=3.6.1 HTTP/1.1", host: "kong-cp:8005" kong-cp | 2024/08/15 22:21:38 [warn] 1811#0: *1294 [lua] checkers.lua:20: log_warn_message(): [clustering] Kong Gateway v3.8.0 configures acme plugin with redis password which is incompatible with dataplane version 3.6.1 and will not work in this release. Please use redis.auth config instead.. [id: e79130ba-32cb-456d-a541-9a159d072142, host: 0c0d4012bade, ip: 172.18.0.8, version: 3.6.1], client: 172.18.0.8, server: kong_cluster_listener, request: "GET /v1/outlet?node_id=e79130ba-32cb-456d-a541-9a159d072142&node_hostname=0c0d4012bade&node_version=3.6.1 HTTP/1.1", host: "kong-cp:8005"

If we do not remove it via

config.storage_config.redis.username = nil config.storage_config.redis.password = nil

The config will be refused by DP. A log warning may be not enough.

It doesn't get refused. Added a test to prove it: 3ff5eb8

This is interesting. How can the older schema validate the storage_config if it has unknown fields in it? This might be a sign of something else being wrong. E.g. sub-record validation does not work or something?

I think the test here is probably wrong that send @gruceo to wrong direction.

@bungle I think it's because some of the storage_config fields like username and password are actually supported in some older versions, it's just that the acme plugin didn't use it. I added this extra test: 8f1cb44

As you can see, username and password are supported until 3.6.1 but not in 3.5.0. In a 3.8.x CP pushing unkwnon fields to a 3.5.0 DP, the config reverts to older schema here: https://github.com/Kong/kong/blob/master/kong/clustering/compat/checkers.lua#L141-L162

kong-cp | 2024/08/19 10:57:34 [warn] 536#0: *1296 [lua] checkers.lua:20: log_warn_message(): [clustering] Kong Gateway v3.8.0 configures acme plugin with redis username which is incompatible with dataplane version 3.5.0 and will not work in this release. [id: df7ec872-23ac-4180-8f5b-1b810efadfcb, host: 0bfb8a368e8d, ip: 172.18.0.7, version: 3.5.0], client: 172.18.0.7, server: kong_cluster_listener, request: "GET /v1/outlet?node_id=df7ec872-23ac-4180-8f5b-1b810efadfcb&node_hostname=0bfb8a368e8d&node_version=3.5.0 HTTP/1.1", host: "kong-cp:8005" kong-cp | 2024/08/19 10:57:34 [warn] 536#0: *1296 [lua] checkers.lua:20: log_warn_message(): [clustering] Kong Gateway v3.8.0 configures acme plugin with redis password which is incompatible with dataplane version 3.5.0 and will not work in this release. Please use redis.auth config instead. [id: df7ec872-23ac-4180-8f5b-1b810efadfcb, host: 0bfb8a368e8d, ip: 172.18.0.7, version: 3.5.0], client: 172.18.0.7, server: kong_cluster_listener, request: "GET /v1/outlet?node_id=df7ec872-23ac-4180-8f5b-1b810efadfcb&node_hostname=0bfb8a368e8d&node_version=3.5.0 HTTP/1.1", host: "kong-cp:8005" kong-cp | 2024/08/19 10:57:34 [warn] 536#0: *1296 [lua] checkers.lua:20: log_warn_message(): [clustering] Kong Gateway v3.8.0 adapts acme plugin redis configuration to older version which is incompatible with dataplane version 3.5.0 and will revert to older schema. [id: df7ec872-23ac-4180-8f5b-1b810efadfcb, host: 0bfb8a368e8d, ip: 172.18.0.7, version: 3.5.0], client: 172.18.0.7, server: kong_cluster_listener, request: "GET /v1/outlet?node_id=df7ec872-23ac-4180-8f5b-1b810efadfcb&node_hostname=0bfb8a368e8d&node_version=3.5.0 HTTP/1.1", host: "kong-cp:8005"

cc @nowNick

.github/workflows/build_and_test.yml

outsinre · 2024-08-16T06:49:38Z

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


If we do not remove it via

config.storage_config.redis.username = nil config.storage_config.redis.password = nil

The config will be refused by DP. A log warning may be not enough.

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

…cture

gruceo requested review from fffonion and nowNick August 13, 2024 18:49

pull-request-size bot added the size/L label Aug 13, 2024

github-actions bot assigned gruceo Aug 13, 2024

github-actions bot added plugins/acme cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Aug 13, 2024

gruceo force-pushed the acme-username branch from e0fb855 to 6eaf7b9 Compare August 13, 2024 21:27

github-actions bot added the chore Not part of the core functionality of kong, but still needed label Aug 13, 2024

gruceo force-pushed the acme-username branch 3 times, most recently from 5ae44a9 to 2501475 Compare August 13, 2024 22:10

gruceo marked this pull request as ready for review August 14, 2024 00:04

fffonion reviewed Aug 14, 2024

View reviewed changes

.github/workflows/build_and_test.yml Show resolved Hide resolved

kong/plugins/acme/storage/config_adapters/redis.lua Show resolved Hide resolved

spec/helpers.lua Show resolved Hide resolved

nowNick approved these changes Aug 14, 2024

View reviewed changes

kong/plugins/acme/storage/config_adapters/redis.lua Show resolved Hide resolved

gruceo force-pushed the acme-username branch from 2501475 to 500ca0d Compare August 14, 2024 13:01

gruceo added this to the 3.8.0 milestone Aug 14, 2024

gruceo requested a review from fffonion August 14, 2024 13:41

gruceo changed the title ~~fix(plugins): username and password are accepted authentication methods~~ fix(plugins/acme): username/password is a valid authentication method Aug 14, 2024

gruceo force-pushed the acme-username branch from 500ca0d to c234ce9 Compare August 15, 2024 17:07

github-actions bot added the core/clustering label Aug 15, 2024

fffonion approved these changes Aug 15, 2024

View reviewed changes

gruceo force-pushed the acme-username branch from 61a36dc to 004627a Compare August 15, 2024 22:01

gruceo commented Aug 15, 2024

View reviewed changes

outsinre reviewed Aug 16, 2024

View reviewed changes

gruceo force-pushed the acme-username branch from 7e724dd to 3ff5eb8 Compare August 16, 2024 11:44

brentos self-requested a review August 16, 2024 16:40

bungle force-pushed the acme-username branch from 3ff5eb8 to 97f8c5e Compare August 19, 2024 09:11

translates 3.8.x standardized redis config to older (3.5.0) acme stru…

8f1cb44

…cture

bungle approved these changes Aug 19, 2024

View reviewed changes

gruceo removed the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Aug 19, 2024

gruceo merged commit 28c5f6a into master Aug 19, 2024
29 checks passed

gruceo deleted the acme-username branch August 19, 2024 12:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gruceo commented Aug 13, 2024

nowNick left a comment

gruceo commented Aug 15, 2024

fffonion left a comment

fffonion Aug 15, 2024

gruceo Aug 15, 2024 •

edited

Loading

fffonion commented Aug 15, 2024

gruceo Aug 15, 2024 •

edited

Loading

outsinre Aug 16, 2024

gruceo Aug 16, 2024

bungle Aug 19, 2024

bungle Aug 19, 2024

gruceo Aug 19, 2024

outsinre Aug 16, 2024

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

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

Conversation

gruceo commented Aug 13, 2024

Summary

Checklist

Issue reference

nowNick left a comment

Choose a reason for hiding this comment

gruceo commented Aug 15, 2024

fffonion left a comment

Choose a reason for hiding this comment

fffonion Aug 15, 2024

Choose a reason for hiding this comment

gruceo Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

fffonion commented Aug 15, 2024

gruceo Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

outsinre Aug 16, 2024

Choose a reason for hiding this comment

gruceo Aug 16, 2024

Choose a reason for hiding this comment

bungle Aug 19, 2024

Choose a reason for hiding this comment

bungle Aug 19, 2024

Choose a reason for hiding this comment

gruceo Aug 19, 2024

Choose a reason for hiding this comment

outsinre Aug 16, 2024

Choose a reason for hiding this comment

gruceo Aug 15, 2024 •

edited

Loading

gruceo Aug 15, 2024 •

edited

Loading