Headers for (namespaced) claims are truncated

[betagan]

TLDR
given a claim custom:roles with value administrator
expected result: a header X-Vouch-IdP-Claims-custom:roles with value administrator
actual result: a header X-Vouch-IdP-Claims-custom with value roles: administrator

Context
when we add custom claims in the vouch-proxy config.yml like

vouch:
  headers:
    claims:
      - given_name
      - custom:roles

vouch-proxy will pass these on to the application via headers. This works for claims like the given_name in the above example, but will produce unexpected results for claims containing a colon like in the example above. AWS Cognito uses this kind of namespace to identify attributes that are not part of their standard attribute set but rather added by the customer. Other IdP might have a similar namespace feature using colons.

In the above example, vouch-proxy will create a header named X-Vouch-IdP-Claims-custom and it will have the value roles: actual_content as the first colon is interpreted as the separator between header and value.

I realize the expected result is impossible as headers can't have colons, but I think vouch-proxy should perform some sort of substitution to achieve something similar. In Any case, rewriting part of the key into the value seems to be unintended.

I will open a PR with our current workaround, replacing colon with dash. It can certainly be reworked into a more complete solution though.

[bnfinet]

thanks @betagan that's an interesting corner case

Thanks much for the PR #184

Can I suggest expanding the replacement beyond colon to all non alphanum characters? And could you please update the documentation in the relevant portion of the configuration file to explain the transposition?
https://github.com/vouch/vouch-proxy/blob/master/config/config.yml_example#L95-L97

FYI I looked at the JWT RFC but it really doesn't have much guidance on naming conventions

   Claim Name
      The name portion of a claim representation.  A Claim Name is
      always a string.

Cheers!

[bnfinet]

actually, after re-reading your note above let me take that back...

If '_' works this is really specific to ':'

is that correct?

Did you test any other claim name aberrations by any chance?

[betagan]

I've given it more thought and we should replace all characters that are not allowed as part of a HTTP header field name. The RFC 822 states in 3.1.2.

The field-name must be composed of printable ASCII characters (i.e., characters that have values between 33. and 126., decimal, except colon).

I'll take the time to update the PR later.

[bnfinet]

@betagan friendly nudge, do you have any time in these busy days to improve this PR?

No real rush but I figured I'd ask since I was poking my nose at the Vouch issues and PRs.

[bnfinet]

@betagan any chance of moving this forward? Your help is very much appreciated.

If not, just let me know and I'll mark it as "help_wanted"

[redterror]

Piling onto this, Auth0 uses URLs for their claim namespace:

https://auth0.com/docs/tokens/guides/create-namespaced-custom-claims

So a custom claim would be of the form http://example.com/my-claim.

[bnfinet]

okie dokey

@betagan thanks for the contribution, I've refactored to something more robust in v0.10.0

These become dashes since they are not allowed as per the RFC: "(),/:;<=>?@[]{}"
https://greenbytes.de/tech/webdav/rfc7230.html#rfc.section.3.2.6

nginx doesn't like underscores so they become dashes too
http://nginx.org/en/docs/http/ngx_http_core_module.html#underscores_in_headers

And we apply golang's http.CanonicalHeaderKey()

And as per @redterror any prepended http:// or https:// is removed before transforming the claim into a header.

There is a log.Info which shows the configured header for the claim and a log.debug which shows the anticipated nginx variable.

I won't be surprised if a few folks out there will need to adjust their Nginx configs to accommodate these rules, however I expect a broader array of claims to be able to be accommodated such as #221 cognito:groups

[redterror]

@bnfinet any chance we could add . to the disallowed list? Nginx's docs say Valid names are composed of English letters, digits, hyphens, and possibly underscores (as controlled by the underscores_in_headers directive) per ignore_invalid_headers. I suspect other webservers strip periods as well.

In my example, my claim is being returned as Header X-Vouch-IdP-Claims-Example.com-Roles and the suggested nginx variable of $auth_resp_x_vouch_idp_claims_example.com_roles is invalid. The nginx error is:

nginx_1    | 2020/04/23 21:02:13 [emerg] 1#1: unknown "auth_resp_x_vouch_idp_claims_example" variable
nginx_1    | nginx: [emerg] unknown "auth_resp_x_vouch_idp_claims_example" variable

PS - The debug log lines are 💯.

[bnfinet]

oh that's a good catch. Just pushed v.0.10.2 which transforms . to - as well. Thanks for the suggestion.

wrt "Valid names are composed of English letters, digits, hyphens" makes me think this should be even more restrictive.