Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Treat key-auth's config.anonymous "" as null #5653

Closed
mtmail opened this issue Mar 9, 2020 · 1 comment · Fixed by #5906
Closed

Treat key-auth's config.anonymous "" as null #5653

mtmail opened this issue Mar 9, 2020 · 1 comment · Fixed by #5906
Labels

Comments

@mtmail
Copy link

mtmail commented Mar 9, 2020

Summary

In the past key-auth's config.anonymous was set to "" (empty string) in the database. Starting Kong 2.0 that causes failed to load anonymous consumer error.

Steps To Reproduce

We upgraded from Kong 1.4 to 2.0.2. (And previously 0.9 to 1.1, then 1.1 to 1.4). Very early we added key-auth plugin like this

curl -s -X POST \
  --url http://localhost:8001/services/ourservice/plugins/ \
  --data 'name=key-auth'

without any other config values set.

In Kong 1.4 and 2.0 in the database that get stored as

select * from plugins where name='key-auth' and consumer_id is null;

id          | edca177b-77b6-457c-9019-9105badbeea1
name        | key-auth
api_id      |
consumer_id |
config      | {"anonymous": "", "key_names": ["apikey"],
"hide_credentials": false, "run_on_preflight": true}

Based on the documentation https://docs.konghq.com/hub/kong-inc/key-auth/ I wouldn've expected the database to show "anonymous: null" or the key not existent. 91a5738dcf5 looks like the change from boolean to string type.

No problem in Kong 1.4. A URL request without any authentication key (key name apikey by default) set, e.g. curl http://localhost:8000/ourservice/ returns HTTP/1.1 401 Unauthorized and body {"message":"No API key found in request"}.

Starting Kong 2.0 the same request returns HTTP/1.1 500 Internal Server Error with body {"message":"An unexpected error occurred"}. The kong logfile contains *2400 [kong] handler.lua:193 [key-auth] failed to load anonymous consumer:failed to get from node cache: [postgres] length must be at least 1, client: 127.0.0.1, server: kong, request: "GET /ourservice HTTP/1.1", host: "localhost:8000"

It is no longer clear what set anonymous": "" in the database in the past, but it seems it was the default setting. Upgrading to Kong 2.0 broke the default behaviour.

Comment #3222 (comment) from Kong 0.12.1 shows an example how the value was set to empty string in the past.

#2539 shows how Kong 0.10. set the default to empty string and how previous logic looked at both empty string and null/nil.

.//kong/plugins/key-auth/handler.lua:    if conf.anonymous ~= "" and conf.anonymous ~= nil then

Expected behaviour

Either

I'd prefer the latter, but the Kong maintaners are more expert at backward compatibility, the DAO (which keeps changing) and side-effects with other auth plugins. I argue a Kong administrator setting the value to empty string made clear it's neither meant to be a UUID or username.

Workaround

We changed the value in the database with a SQL statement. It was unclear how to unset a value. Setting it to null via the Admin API returned {"message":"invalid primary key: '{id=\"expected a valid UUID\"}'","name":"invalid primary key","fields":{"id":"expected a valid UUID"},"code":1

Additional Details & Logs

  • Kong version (2.0.2)
  • Ubuntu 16
@dsteinkopf
Copy link

I am having the same problem with key-auth after upgrading from 1.4 to 2.0 And it seems we have a very similar problem with the plugin basic-auth: Here the old value of config.anonymous isn't "" (empty string) but it is missing instead. But the resulting error is the same. May this problem could be fixed at the same time. Thank you!

In the meantime as a workaround setting config.anonymous to null works for me:

curl -i -X PATCH --url http://mykong/plugins/<plugin_is> --data "config.anonymous="

bungle added a commit that referenced this issue May 14, 2020
### Summary

On PR #5879 @murillopaula proposed a fix to a common but arbitrary plugin
field called `config.anonymous`. The PR tried to tackle two things at the
same time. It tried to infer `JSON` input (which we don't do), and it tried
to fix issue where upgraded databases still contained empty strings `""`
stored to `config.anonymous` (instead of `null`). This only fixes the issue
for this field. It also added `legacy` flag to that field, which in my
opinion the field is not, or that it is not any special to any other field
that could contain empty string `""`. So I didn't think it was a good idea
to start flagging fields to `legacy` one by one.

Thus I discussed this with @murillopaula and out of those discussions came
one winner that we both agreed upon. The solution is to run automatic
transformation on `select` to database fields that are not legacy, and
convert the value to `nil` (remove field) or `null` (when nulls were
requested). This is implemented directly in `process_auto_fields` which
we call automatically on daos. The solution looks like this:

```lua
if context == "select"
   and not field.legacy
   and field.type == "string"
   and (field.len_min or 1) > 0
   and data[key] == ""
then
  data[key] = nulls and null or nil
end
```

This fixes the issue for all the fields that are updated to new schema which
if not `len_min=0` will not accept `""` anymore as input, and thus are auto-
translated to `nil`/`null`.

### Issues resolved

Fix #5653 (the database `""`)
Close #5879 (previous attempt)
bungle added a commit that referenced this issue May 15, 2020
### Summary

On PR #5879 @murillopaula proposed a fix to a common but arbitrary plugin
field called `config.anonymous`. The PR tried to tackle two things at the
same time. It tried to infer `JSON` input (which we don't do), and it tried
to fix issue where upgraded databases still contained empty strings `""`
stored to `config.anonymous` (instead of `null`). This only fixes the issue
for this field. It also added `legacy` flag to that field, which in my
opinion the field is not, or that it is not any special to any other field
that could contain empty string `""`. So I didn't think it was a good idea
to start flagging fields to `legacy` one by one.

Thus I discussed this with @murillopaula and out of those discussions came
one winner that we both agreed upon. The solution is to run automatic
transformation on `select` to database fields that are not legacy, and
convert the value to `nil` (remove field) or `null` (when nulls were
requested). This is implemented directly in `process_auto_fields` which
we call automatically on daos. The solution looks like this:

```lua
if context == "select"
   and not field.legacy
   and field.type == "string"
   and (field.len_min or 1) > 0
   and data[key] == ""
then
  data[key] = nulls and null or nil
end
```

This fixes the issue for all the fields that are updated to new schema which
if not `len_min=0` will not accept `""` anymore as input, and thus are auto-
translated to `nil`/`null`.

### Issues resolved

Fix #5653 (the database `""`)
Close #5879 (previous attempt)
bungle added a commit that referenced this issue May 19, 2020
### Summary

On PR #5879 @murillopaula proposed a fix to a common but arbitrary plugin
field called `config.anonymous`. The PR tried to tackle two things at the
same time. It tried to infer `JSON` input (which we don't do), and it tried
to fix issue where upgraded databases still contained empty strings `""`
stored to `config.anonymous` (instead of `null`). This only fixes the issue
for this field. It also added `legacy` flag to that field, which in my
opinion the field is not, or that it is not any special to any other field
that could contain empty string `""`. So I didn't think it was a good idea
to start flagging fields to `legacy` one by one.

Thus I discussed this with @murillopaula and out of those discussions came
one winner that we both agreed upon. The solution is to run automatic
transformation on `select` to database fields that are not legacy, and
convert the value to `nil` (remove field) or `null` (when nulls were
requested). This is implemented directly in `process_auto_fields` which
we call automatically on daos. The solution looks like this:

```lua
if context == "select"
   and not field.legacy
   and field.type == "string"
   and (field.len_min or 1) > 0
   and data[key] == ""
then
  data[key] = nulls and null or nil
end
```

This fixes the issue for all the fields that are updated to new schema which
if not `len_min=0` will not accept `""` anymore as input, and thus are auto-
translated to `nil`/`null`.

### Issues resolved

Fix #5653 (the database `""`)
Close #5879 (previous attempt)
bungle added a commit that referenced this issue May 19, 2020
#5906)

### Summary

On PR #5879 @murillopaula proposed a fix to a common but arbitrary plugin
field called `config.anonymous`. The PR tried to tackle two things at the
same time. It tried to infer `JSON` input (which we don't do), and it tried
to fix issue where upgraded databases still contained empty strings `""`
stored to `config.anonymous` (instead of `null`). This only fixes the issue
for this field. It also added `legacy` flag to that field, which in my
opinion the field is not, or that it is not any special to any other field
that could contain empty string `""`. So I didn't think it was a good idea
to start flagging fields to `legacy` one by one.

Thus I discussed this with @murillopaula and out of those discussions came
one winner that we both agreed upon. The solution is to run automatic
transformation on `select` to database fields that are not legacy, and
convert the value to `nil` (remove field) or `null` (when nulls were
requested). This is implemented directly in `process_auto_fields` which
we call automatically on daos. The solution looks like this:

```lua
if context == "select"
   and not field.legacy
   and field.type == "string"
   and (field.len_min or 1) > 0
   and data[key] == ""
then
  data[key] = nulls and null or nil
end
```

This fixes the issue for all the fields that are updated to new schema which
if not `len_min=0` will not accept `""` anymore as input, and thus are auto-
translated to `nil`/`null`.

### Issues resolved

Fix #5653 (the database `""`)
Close #5879 (previous attempt)
snt8amr pushed a commit to snt8amr/kong that referenced this issue May 30, 2020
Fix Kong#5653

We could not configure an auth plugin with config.anonymous="" in CE 2.0
or EE 1.5.

It also broke users that had config.anonymous="" configured. To
reproduce, create an auth plugin with config.anonymous="" (empty string)
in 0.13, then upgrade all the way to 2.0.

This is the breaking change: Kong#5552

---

We have three options to fix it:

(a) a migration;
(b) start special-casing "" to mean null for this field in the code of
these plugins;
(c) add legacy back to those anonymous fields of string type fields,
meaning that in legacy fields "" auto-translates to null

This is the execution of (c).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants