fix(key-auth): keep query params order if hide_credentials is true

[battlebyte]

When hide_credentials=true, query parameters should not be sorted alphabetically. This alters the original request and can have unintended consequences for upstream services. One example is authentication with the pseudo header (request-target) as described in https://datatracker.ietf.org/doc/html/draft-cavage-http-signatures#section-2.3. Since the order is not preserved, the signature does not match and the upstream rejects the request.

• query parameters order is not altered when hide_credentials=true
• add two new test cases in spec/03-plugins/09-key-auth

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

---

• To see the specific tasks where the Asana app for GitHub is being used, see below:
  • https://app.asana.com/0/0/1206863362995171

[CLAassistant]

All committers have signed the CLA.

[gszr]

@battlebyte A few thoughts...

The RFC2616 (HTTP 1.1) Section 4.2 says:

The order in which header fields with differing field names are
received is not significant. However, it is "good practice" to send
general-header fields first, followed by request-header or response-
header fields, and ending with the entity-header fields.

Thus, Kong's behavior is not deviating from the RFC.

The draft you linked also reinforces the fact that there may be alterations:

A signed HTTP message needs to be tolerant of some trivial
alterations during transmission as it goes through gateways, proxies,
and other entities

In the same section, the draft also says:

In order to generate the string that is signed with a key, the client
MUST use the values of each HTTP header field in the headers
Signature Parameter, in the order they appear in the headers
Signature Parameter.

The verification also MUST follow the same process to recreate the signature (https://datatracker.ietf.org/doc/html/draft-cavage-http-signatures#section-2.5).

Therefore, I see this as a non-conforming implementation on the client signing or the server verifying the signature.

[battlebyte]

I agree with your points @gszr , although those are in relation to the headers. The order of the headers is not important because, as you point out, the client MUST use the values of each HTTP header field in the headers Signature Parameter, in the order they appear in the headers Signature Parameter.

The point here is that one of those headers is the (request-target) pseudo-header. This header includes the path and the query string, that is the reason why the order of the query string arguments should not be altered. See an example in slide 6 here https://revenue-ie.github.io/paye-employers-documentation/PIT3/examples/REST_Request_Authentication_V1.pdf.

I hope this clarifies.

[battlebyte]

If you see the example in https://datatracker.ietf.org/doc/html/draft-cavage-http-signatures#section-3.1.3, the headers that will be part of the signature are:

headers="(request-target) (created) host digest content-length"

Those headers can be received in any order, that is not a problem because the upstream knows how they should be ordered to calculate the signature.

The key point, is that the (request-target) header contains the HTTP method and the request path, eventually including the query parameters. For example:

(request-target): get /this/is/a/sample/path?zparam=1&tparam=2&aparam=3

With the current implementation of key-auth plugin, if hide_credentials=true, the order of the query params are altered and placed in alphabetical order. So the upstream will receive the request as:

/this/is/a/sample/path?aparam=3&tparam=2&zparam=1

And the signature calculated by the upstream will not match the signature provided by the client.

[battlebyte]

See my comments in the main conversation.

[battlebyte]

@gruceo @gszr let me know if my previous explanation helps clarifies the purpose of this PR.

[gszr]

@battlebyte I have taken the liberty to rebase and make a couple of changes to the code:

• Use ngx.re.gsub with the oj flags (explanation here for anyone interested) as commonly done in our code base
• Remove some formatting issues

I will also request @bungle to take a look at this one.

Also, the oauth2 plugin removes the header in the same way; should this fix also be applied there?

[battlebyte]

Thanks for the review and changes @gszr .
Regarding oauth2, I think the same fix should be applied there to avoid altering the query params ordering.

[gszr]

Thanks for the review and changes @gszr . Regarding oauth2, I think the same fix should be applied there to avoid altering the query params ordering.

Agreed; once this one is merged we can submit a separate PR.

[bungle]

Such a small PR which on a surface looks good and perhaps better than what we have, just raises a lot of small questions, for example:

Can querystring param names be urlencoded, e.g. key= -> %6B%65%79= or k%65y=, this will complicate the processing. Or is this somehow handled already (let's figure this out).

discussion ongoing

[gszr]

If an error occurs in the regex gsub, remove_query_key will return nil, and this call to set_raw_query will now throw an error. Let's change remove_query_key to return nil, err and log the error from here instead.
Also, I wonder if now we should respond with a 500 in case the query key clearing cannot be done - it can also be seen as a security issue if the plugin config has hide_credentials and it has no effect.

[battlebyte]

Fixed in the latest commit. I think returning 500 makes sense but I'm waiting on the final decision you guys make.

[gszr]

This is a new error condition; IMO it makes sense to respond with a 500 in that case. Otherwise, the token will be leaked to the upstream service, even though the expectation is it was removed.

[battlebyte]

Thanks @bungle , that makes sense. Your proposal is a much better solution. Thanks for taking the time!

[battlebyte]

Hi @bungle , what are the next steps to close this topic?