Multivalued claims value is not deserialized as expected

[shadow38]

Hello,

I'm using vouch-proxy with Keycloak.

Let's assume that userinfo endpoint returns this kind of value :

{
  "sub": "1a8278bd-b832-4274-88aa-12dc89e90637",
  "preferred_username": "g-ef71c9de-2ecd-44ef-a17f-694102108e4a",
  "email": "[email protected]",
  "grps": [
    "a",
    "b"
  ],
XXXX
}

In my HTTP header grps, I get the value "a","b" (with the quotes). It is not the behaviour I expect; in my case I would like to get a,b.

In Keycloak (v14), it is not possible to change the representation of a mulivalued claim. I tried with openresty to "clean" the header value with no success. (if it's possible, it seems too tricky for me!). Do you have any clue to get the header value without quotes ?

Thanks,

[bnfinet]

@shadow38 that is expected behavior

See the test handlers/validate_test.go: TestValidateRequestHandlerWithGroupClaims

https://github.com/vouch/vouch-proxy/blob/master/handlers/validate_test.go#L134-L149
https://github.com/vouch/vouch-proxy/blob/master/handlers/validate_test.go#L198-L203

As per discussion in #227 some folks have spaces in their group names.

[shadow38]

Maybe it is the expected behavior in the test but I'm not sure if this behaviour is correct in an HTTP context point of view.

According to https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.2, field value should be separated with the comma character and not with a space (as you pointed me issue #227)

But, I believe that adding "extra" double quotes when unnecessary seems odd; why a single string value did'nt do the same ? I mean that when I capture the HTTP header in the response, I get this :

grps = "a","b"

but the expected HTTP header content is :

grps = a,b

If case of a single valued field, I get the HTTP header as expected :

grps = a

Another way to achieve sending multivalued claim could be sending the same header several times : (it doesn't seem recommanded but by using this mechanism, you didn't have to escape any character, just send as many headers as claim values and use the single value serializer)

grps = a
grps = b

and then let the client concatenates field values according to the rfc.

IMO it is an issue related to an escaping character problematic which was solved partially. In my case the semantic of the HTTP header is broken (I'm using vouch-proxy with a legacy webapp which doesn't support double quoted value in header field) and I can't find a workaround.

In my usecase, I need to remove the double quotes in the nginx configuration. (in order not to patch the legacy system) It seems imposssible to achieve this with the map directive (http://nginx.org/en/docs/http/ngx_http_map_module.html). Maybe it is possible with openresty via a lua script but I did'nt find a correct way to do it....

[bnfinet]

Section 3.2.6 of rfc7230 which you linked to includes "quoted-string". It's clearly part of the spec. That term appears 12 times in the document.

sending the same header several times

That is clearly against the specification. See the 'Set-Cookie' chiding.

Quotes vs no quotes is somewhat unsettled in the cannon of data stores and line protocols but for this one it feels like we're within the bounds of accepted practice.

[bnfinet]

I'm curious about Nginx's embedded JavaScript. I'm not sure if it would be a better tool for you than Lua but it might offer a nice facility for manipulating the header and strings at the nginx level.

https://www.nginx.com/blog/harnessing-power-convenience-of-javascript-for-each-request-with-nginx-javascript-module

[shadow38]

Ok,

You are definitely right.
I finally found a way to get it working properly with openresty. It may be obvious for experienced user but it wasn't for me.

Here a part of my openresty config file; It may help other users who have similar needs as mine.

location /myapp {

    # send all requests to the `/vouch-validate` endpoint for authorization
    auth_request /vouch-validate;

    auth_request_set $auth_resp_x_vouch_user $upstream_http_x_vouch_user;
    proxy_set_header X-Vouch-User $auth_resp_x_vouch_user;

    auth_request_set $auth_resp_x_email           $upstream_http_x_vouch_idp_claims_email;
    auth_request_set $auth_resp_x_grps            $upstream_http_x_vouch_idp_claims_grps;

    proxy_set_header email        $auth_resp_x_email;

    access_by_lua_block {
        ngx.req.set_header("grps",  string.gsub(ngx.var.auth_resp_x_grps, "\"", ""))
    }

    proxy_set_header Host $http_host;
    proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
    proxy_set_header X-Forwarded-Proto $scheme;

    proxy_redirect off;
    proxy_buffering off;


    proxy_pass http://myserver/myapp;
}

Using the javascript facility should also do the trick. I didn't know that nginx embedded this kind of feature. Anyway, my server is running Debian 9 and I'm stuck with nginx 1.10 (this feature exists since nginx > 1.11.10).

[bnfinet]

@shadow38 thank you for sharing your solution. That's very much appreciated.

[ajknv]

I would like to re-raise this issue and argue against the principle rationale that has been cited ("some folks have spaces in their group names"). The problem with the current approach is that it violates the principle of "return it as you received it" -- if the claim value reported by my IdP is 'claim-value', then I should expect that my auth handler forwards it downstream as such, not as '"claim-value"'. The inconsistency between the handling of single-value and multi-value claims also violates the principle of least surprise.

IMHO, the proper and consistent way to address the prior cited problem is that if users want to put spaces in their group names, it should be incumbent upon them to accordingly also quote their group name when they set it up in their IdP. At the very least, addressing feature request #427 seems warranted so that users aren't put in the position of having to code Lua or Javascript to remediate unexpected changes to claim data.

P.S. It also seems like the current behavior could also be considered a defunct relic of issue #227 seeing as the ambiguous space-delimiter was changed anyway, rendering the quoting fix unnecessary.

[bnfinet]

@ajknv I agree that it may seem clunky to receive quoted values but it's been that way for several years. There's no security related reason to change it (you could almost argue quoted is more secure) and folks have designed their systems to accommodate the quotes. I don't want to break anyone's setup. That would violate the principle of least surprise :)

[ajknv]

Is there a reason to reject giving users control over this via simple config directives, as per #427? (The default can remain as-is.) It seems as if the person making the feature request even had working code for it that just needed to be accepted.

folks have designed their systems to accommodate the quotes

I have a downstream dependency that is not within my control that isn't designed to accommodate them. It seems as if I am not the first user to run into this, and being able to configure vouch-proxy to behave as needed is much preferable to having to change webservers and/or write other "man-in-the-middle" code.

[bnfinet]

@ajknv will the solution documented in #423 work for you?

I'd rather not add more knobs unless necessary.

[ajknv]

So the solution requires one of:

• Switch webserver to openresty (become decoupled from standard nginx maintenance and releases, pick up a bunch of features and attack surface I don't otherwise need)
• Switch webserver to nginx plus (paid product)
• Compile and load an additional module into nginx for javascript (complicate build and deployment workflows)

Then write code (which is not documented on the issue for the javascript solution) that has to run inside of the webserver engine on every request instead of just passing a header through?

I can't say that approach is suitable for my situation, no. It's a remarkable contrast to writing 2-3 additional declarative lines in a config file so that one of the handlers already in the request processing pipeline will simply produce the appropriate output for integration.

[bnfinet]

@ajknv I think of openresty as fairly safe

https://stack.watch/product/openresty/openresty/

There's a documented solution that works well. If you're administering VP you're already into advanced configuration of Nginx.

I'm sorry this one just isn't a priority for me right now. Maybe someday. I can see a point where it may make sense to rewrite the claims and headers logic in pursuit of something cleaner overall.

That said, I'm happy to continue to hear opinions from users like yourself.

@ajknv thank you for contributing to the discussion

[shadow38]

Actually openresty is not really needed. In my case we use debian and the packaged nginx by the distro (not sure but you may need to install nginx-extras) supports the configuration I posted few months ago.

[bnfinet]

see #533 for a solution using 'map' in nginx