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

fix(key-auth): add missing www-authenticate headers #11794

Merged

Conversation

nowNick
Copy link
Contributor

@nowNick nowNick commented Oct 19, 2023

Summary

When kong returns 401 Unauthorized response it should return WWW-Authenticate header with proper challenge. Key auth was missing this header on some responses. This PR also adds a possibility to configure an optional parameter - realm (defaults to null).

Related PRs:

RFCs & Materials

Checklist

Full changelog

  • add WWW-Authenticate header to all key-auth 401 response

Issue reference

@bungle
Copy link
Member

bungle commented Oct 19, 2023

@nowNick,

How does this behave in AND/OR scenario? E.g. basic-auth AND/OR key-auth (or do we just skip that complexity for now)?

@nowNick
Copy link
Contributor Author

nowNick commented Oct 19, 2023

Hey @bungle !

When I was thinking about it I came to the conclusion that it's not necessary to handle those scenarios or it's impossible to do them, but my understanding could be wrong.

Basically when it comes to OR I think we do not need to handle the WWW-Authenticate since the response should be 200 unless there's request termination plugin turned on that forbids anonymous access.... well... now that I think of it maybe we should add proper WWW-Authenticate headers to that plugin as well. Will it be possible to look up what auth plugins the request has fallen through so that we could create a proper WWW-Authenticate header at the request termination plugin turn?
I'm basing my understanding on this: https://docs.konghq.com/gateway/latest/kong-plugins/authentication/reference/#multiple-authentication

Now when it comes to AND scenario I wonder whether it's possible to return all of the challenges. Correct me if I'm wrong but if there's the so called AND condition - when a single auth plugin fails and it returns 401 will another auth plugin code execute or will the request be terminated since the AND condition already failed (basically an early exit)?
If so I concluded that it's impossible to incorporate other challenges to certain plugin 401 response if there's AND condition between them. Is my understanding correct?

@nowNick nowNick force-pushed the feat/implement-missing-www-authenticate-headers-key-auth branch from cb5d571 to 0f90ce0 Compare November 8, 2023 16:55
@pull-request-size pull-request-size bot added size/L and removed size/M labels Nov 8, 2023
@nowNick nowNick force-pushed the feat/implement-missing-www-authenticate-headers-key-auth branch 2 times, most recently from b767d7a to 30c3868 Compare November 8, 2023 17:29
@nowNick nowNick marked this pull request as ready for review November 8, 2023 17:29
@nowNick nowNick requested a review from kikito November 9, 2023 11:29
@nowNick nowNick force-pushed the feat/implement-missing-www-authenticate-headers-key-auth branch from 30c3868 to 1cb2851 Compare November 13, 2023 14:38
Copy link
Member

@kikito kikito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with a couple minor comments

kong/plugins/key-auth/handler.lua Outdated Show resolved Hide resolved
@nowNick nowNick force-pushed the feat/implement-missing-www-authenticate-headers-key-auth branch from 1cb2851 to e9b9882 Compare December 1, 2023 09:05
@nowNick nowNick requested a review from bungle December 1, 2023 09:06
@nowNick nowNick force-pushed the feat/implement-missing-www-authenticate-headers-key-auth branch from e9b9882 to 296a965 Compare December 5, 2023 08:00
bungle
bungle previously requested changes Dec 5, 2023
Copy link
Member

@bungle bungle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small change req.

kong/plugins/key-auth/handler.lua Outdated Show resolved Hide resolved
@locao
Copy link
Contributor

locao commented Dec 19, 2023

@nowNick there conflicts preventing us from merging this PR, can you check, please?

@nowNick nowNick force-pushed the feat/implement-missing-www-authenticate-headers-key-auth branch from 296a965 to 5f84f96 Compare January 2, 2024 14:28
@pull-request-size pull-request-size bot added size/M and removed size/L labels Jan 2, 2024
@nowNick nowNick requested a review from bungle January 2, 2024 14:31
@kikito kikito self-requested a review January 22, 2024 12:23
@nowNick nowNick force-pushed the feat/implement-missing-www-authenticate-headers-key-auth branch from 6240940 to 42c4b99 Compare February 2, 2024 15:53
@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 2, 2024
@nowNick nowNick force-pushed the feat/implement-missing-www-authenticate-headers-key-auth branch 2 times, most recently from dea7748 to 8e95a55 Compare February 8, 2024 14:37
@nowNick nowNick force-pushed the feat/implement-missing-www-authenticate-headers-key-auth branch from 8e95a55 to f771a18 Compare February 9, 2024 08:47
@nowNick nowNick marked this pull request as ready for review February 9, 2024 09:40
@nowNick nowNick force-pushed the feat/implement-missing-www-authenticate-headers-key-auth branch 2 times, most recently from 35f2cbf to 690c8ae Compare February 15, 2024 11:40
end

set_consumer(consumer, credential)

return true
end

local function set_anonymous_consumer(anonymous)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some small refactoring - I've extracted from KeyAuthHandler:access function following funcs:

  • set_anonymous_consumer
  • logical_OR_authentication
  • logical_AND_authentication

@nowNick nowNick force-pushed the feat/implement-missing-www-authenticate-headers-key-auth branch 2 times, most recently from 836ac9e to adc0ede Compare February 15, 2024 12:21
@nowNick nowNick linked an issue Feb 19, 2024 that may be closed by this pull request
1 task
@nowNick nowNick force-pushed the feat/implement-missing-www-authenticate-headers-key-auth branch from adc0ede to e2f8178 Compare February 27, 2024 08:41
@kikito kikito requested a review from flrgh March 12, 2024 18:30
@flrgh flrgh dismissed bungle’s stale review March 12, 2024 21:52

changes were made

@bungle
Copy link
Member

bungle commented Mar 15, 2024

Hey @bungle !

When I was thinking about it I came to the conclusion that it's not necessary to handle those scenarios or it's impossible to do them, but my understanding could be wrong.

Basically when it comes to OR I think we do not need to handle the WWW-Authenticate since the response should be 200 unless there's request termination plugin turned on that forbids anonymous access.... well... now that I think of it maybe we should add proper WWW-Authenticate headers to that plugin as well. Will it be possible to look up what auth plugins the request has fallen through so that we could create a proper WWW-Authenticate header at the request termination plugin turn? I'm basing my understanding on this: https://docs.konghq.com/gateway/latest/kong-plugins/authentication/reference/#multiple-authentication

It is possible at least in theory, but I agree it feels like a stretch. Not part of this PR. We would need to modify all auth plugins to add/append something in context. So lets skip it for now.

Now when it comes to AND scenario I wonder whether it's possible to return all of the challenges. Correct me if I'm wrong but if there's the so called AND condition - when a single auth plugin fails and it returns 401 will another auth plugin code execute or will the request be terminated since the AND condition already failed (basically an early exit)?

The other plugins (that come after) access phase will not be executed if one auth plugin terminates. But they are still collected, so their header_filter will still be called. And in header filter it is possible to do something like this. But again, complicates the thing and all our auth plugins need to implement header_filter phase too. Which is probably not a good idea.

Comment on lines +101 to +105
return { status = 500, message = message }
end

local function unauthorized(message, www_auth_content)
return { status = 401, message = message, headers = { ["WWW-Authenticate"] = www_auth_content } }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original one had tables preallocated, this will allocate some memory on each failure. I am not sure was the original intent to avoid creating tables. But this is so small thing that I am fine with this.

Copy link
Member

@bungle bungle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with a comment.

kong/plugins/key-auth/schema.lua Outdated Show resolved Hide resolved
When serve returns 401 Unauthorized response it should
return WWW-Authenticate header as well with proper challenge.
Not all key-auth 401 responses had this header.
This commit also adds an option to configure the realm
for protected resource. By default it is empty therefore it
is not displayed but it can be configured to
be present in www_authenticate header.

Fix: #7772
KAG-321
@nowNick nowNick force-pushed the feat/implement-missing-www-authenticate-headers-key-auth branch from e2f8178 to 8c5d73d Compare March 19, 2024 09:31
@github-actions github-actions bot added the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Mar 19, 2024
@bungle bungle merged commit 2772b0e into master Mar 19, 2024
26 checks passed
@bungle bungle deleted the feat/implement-missing-www-authenticate-headers-key-auth branch March 19, 2024 15:39
@team-gateway-bot
Copy link
Collaborator

Successfully created cherry-pick PR for master:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants