LDAP Auth: Fix WWW-Authenticate header with custom cred type #3656
Conversation
francois-maillard
changed the title
francois-maillard
changed the title
kong/plugins/ldap-auth/access.lua
Outdated
| @@ -165,7 +165,7 @@ local function do_authentication(conf) | |||
|
|
|||
| -- If both headers are missing, return 401 | |||
| if not (authorization_value or proxy_authorization_value) then | |||
| ngx.header["WWW-Authenticate"] = 'LDAP realm="kong"' | |||
| ngx.header["WWW-Authenticate"] = conf.header_type .. ' realm="kong"' | |||
There was a problem hiding this comment.
existing configurations will mostly have the old default set ldap (lowercase) so wouldn't this break an existing configuration after an upgrade?
Adding a migration that converts ldap to LDAP should fix the defaults. Not sure though about folks not using the default. But probably since no one complained about this issue before, it's safe to just convert ldap to LDAP?
What do you think @bungle ?
There was a problem hiding this comment.
I must say I had overlooked this issue. Yet, I'm not sure any client actually does anything with the LDAP type in WWW-Authenticate, since it's not listed in the list maintained by IANA
There was a problem hiding this comment.
What I wonder is why updating the default ldap to be capitalized? Especially if none are even registered as actual authentication schemes and this is a potential breaking change. Any reason?
There was a problem hiding this comment.
I capitalized the default value so as to avoid a breaking change from WWW-Authenticate: LDAP to WWW-Authenticate: ldap in the response (It doesn't impact the Authentication: ldap xxxxxxx header in a valid request since this check is case insensitive). That was not a good option since I overlooked the fact that the default value is stored in the DB and so will only be right in a fresh install.
Here are the options I see:
- Keep the original default value
ldap, in which case the defaultWWW-Authenticateheader will change to lowercase - Change the default value to
LDAP, and, as @Tieske proposed, add a migration step - Change my code to
ngx.header["WWW-Authenticate"] = upper(conf.header_type) .. ' realm="kong"'but in the case of basic auth, Kong would answerWWW-Authenticate: BASICwhile the official answer should beWWW-Authenticate: Basic
2 seems like the best option... If you agree with that, I can write the migration.
Regarding @Tieske remark:
Not sure though about folks not using the default.
If someone does not use the default, then the WWW-Authenticate: LDAP is not a valid response, so we'll fix a bug for them (actually, that's exactly the issue I have with basic auth).
There was a problem hiding this comment.
- is more work but sounds the sanest indeed. Although a migration means that this must target next (which it does) and will only make it into the next major release (far from now).
A possible solution could be to keep the updated default to LDAP:
local scheme = conf.header_type
if scheme == "ldap" then
-- ensure backwards compatibility (see GH PR #3656)
-- TODO: provide migration to capitalize older configurations
scheme = upper(scheme)
end
ngx.header["WWW-Authenticate"] = scheme .. ' realm="kong"'This way, this PR can go to master. A subsequent PR (that you can contribute later at your own pace) would introduce the migration (there are other similar migrations to copy-paste and tweak, it should be fairly simple), remove this piece code, and target the next branch.
francois-maillard
force-pushed
the
next
branch
2 times, most recently
from
3a7d905 to
7ce9e26
Compare
|
hmm... @thibaultcha , I followed your last proposal, and reworked my commits and targeted the master branch. |
| if scheme == "ldap" then | ||
| -- ensure backwards compatibility (see GH PR #3656) | ||
| -- TODO: provide migration to capitalize older configurations | ||
| scheme = upper(scheme) |
There was a problem hiding this comment.
upper isn't a valid Lua function, string.upper is. I was suggesting caching it at the top of this module, like string.lower
There was a problem hiding this comment.
yep... sorry, did it in a rush before leaving the office, it's fixed now, though the cassandra integration job failed with an error that does not seem related to my patch:
[ FAILED ] spec/02-integration/02-cmd/02-start_stop_spec.lua @ 114: kong start/stop /etc/hosts resolving in CLI resolves #cassandra hostname
There was a problem hiding this comment.
@francois-maillard Spurious failures can happen in the CI, in particular on PR builds. It is something we are working towards fixing, no worries for now, I'll restart the jobs if those failures are unrelated.
There was a problem hiding this comment.
All tests are green!
When using custom credential type for LDAP authentication, the WWW-Authenticate header that is sent back when auth failed should be accordingly set.
When using custom credential type for LDAP authentication via `conf.header_type`, the WWW-Authenticate header that is sent back when authentication failed should be accordingly set. From #3656
|
Manually merged, thank you! As discussed, will you now be able to provide a subsequent patch for the migration towards the capitalized default value @francois-maillard? That'd be greatly appreciated, thanks! |
|
@francois-maillard PS: cool picture :) |
|
Leaving in the south of france can be tough ;) |
1. Add postgres/cassandra migrations 2. Change default config.header_type value from ldap to LDAP 3. Remove temporary code in ldap-auth/access.lua
NOTE: Please read the CONTRIBUTING.md guidelines before submitting your patch,
and ensure you followed them all:
https://github.com/Kong/kong/blob/master/CONTRIBUTING.md#contributing
Summary
When using custom credential type for LDAP authentication (the
config.header_typesetting),the WWW-Authenticate header that is sent back when auth failed should be accordingly set.
Using
Basicas the header type should allow the use of any web browser as a client. When there's no (or an invalid) authentication data in the query, Kong replies with theWWW-Authenticateheader. Currently, this header doesn't useconfig.header_typeso it setsWWW-Authenticate: LDAP, which is not understood by the browser, hence the login popup won't show.Full changelog
kong/plugins/ldap-auth/access.luaspec/03-plugins/21-ldap-auth/01-access_spec.luaIssues resolved
I don't think a proper issue has been open, but it has been reported on discuss.konghq.com