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

Add configurable verification of HttpOnly cookies in JWTAuthentication filter #7025

Open
andrewtikhonov opened this issue May 21, 2019 · 25 comments
Labels
enhancement Feature requests. Not bugs or questions. help wanted Needs help!

Comments

@andrewtikhonov
Copy link

andrewtikhonov commented May 21, 2019

Title: Add configurable verification of HttpOnly cookies in JWTAuthentication filter

Problem:

One of the methods to protect against XSS attacks and token theft in web apps is the HttpOnly cookie that is set by the server, and that is not accessible from Javascript. A digest of the cookie value is recorded in the token and is verified by the server when the browser sends a request.

Proposal:

It would be very useful for JWT Authentication filter to extract the value from the cookie, take a sha256 digest and verify that a claim with the corresponding value is in the token.

Headers:

Cookie: secure_key=secure_value_1234567890

Envoy config

verify_secure_cookie:
key: "secure_key"
transform: [sha256, plaintext]
claim_name: "cookie_key"

@qiwzhang
Copy link
Contributor

I need some clarifications: In the JWT token, what claim name should be checked? Maybe also from Envoy config?

For example: a request has following headers:

Authorization: Bearer TOKEN
Cooke: secure_key=secure_value_1234567890

After Envoy Jwt_Authn filter verified the TOKEN, it also needs to verify the cookie. If it has following config:

verify_secure_cookie:
cookie_key: "secure_key"
jwt_claim_key: "jwt_secure_key"
transform: sha256

The filter will look for "jwt_claim_key" field in the TOKEN claim, and verify its value is sha256 of cookie value from "secure_key".

Is this correct?

I feel it is better to have a separate filter for this function. The verified payload is written to dynamicMetadata in the stream_info. Other filter can access it to perform such verification.

@andrewtikhonov
Copy link
Author

andrewtikhonov commented May 22, 2019

Absolutely correct. Sorry, forgot to add the claim name.

It can be a separate filter, but no one will need to do such a verification somewhere else. It will always be in connection with the token verification, which makes me think it has to be the auth filter.

@mattklein123 mattklein123 added the enhancement Feature requests. Not bugs or questions. label May 23, 2019
@andrewtikhonov
Copy link
Author

Any thoughts ?

@yangminzhu
Copy link
Contributor

I think these custom operations (e.g. sha256, etc.) seems too specific to be added to jwt filter unless there is a more generic way to specify and support these custom operations and allowing it to be reused in other filters.

Maybe you could try to use a Lua filter (https://github.com/envoyproxy/envoy/tree/v1.10.0/examples/lua) to do this?

@andrewtikhonov
Copy link
Author

andrewtikhonov commented Jul 1, 2019

Sha256 is not specific. It's a means to transform the data so that it cannot be restored. I don't mind having a general config saying "use sha256 for for hash operations" and "use hash transform" in the JWT filter. But that's premature optimisation or premature generalisation to be precise.

Sha256 is already implemented in envoy/common/crypto/utility.cc and is used in /filters/http/common/aws it's just a matter of writing code.

@yangminzhu
Copy link
Contributor

Sorry I did't mean sha256 is specific or there is any problem of supporting sha256 in the code, the problem is how to support this properly in the filter API so that it's not a solution that only works for some specific use cases.
For example, what if the other user want to use a hash algorithm other than sha256 or the other user want to do some preprocessing and only use part of the the cookie header to calculate the hash?

@andrewtikhonov
Copy link
Author

andrewtikhonov commented Jul 3, 2019

Yangmin, what am I doing wrong ? I'm doing JWT auth and want payload to be in the dynamicMetadata() (see payload_in_metadata: "my_payload") so that the lua filter after jwt_auth can check the token payload. Before JWT_auth filter I capture some headers using another lua filter so that the arguments can be reconstructed.

Problem:
I'm getting "nil", dynamicMetadata() contains only my map with recorded header values and not a trace of token payload.

Logs:

[2019-07-03 17:08:52.833][12][info][lua] [source/extensions/filters/http/lua/lua_filter.cc:535] script log: ------------- Request dynamic_key: envoy.lua
[2019-07-03 17:08:52.833][12][error][lua] [source/extensions/filters/http/lua/lua_filter.cc:541] script log: [string "function envoy_on_request(request_handle)..."]:9: bad argument #1 to 'pairs' (table expected, got nil)
[

Envoy Config:

          - name: envoy.lua
            typed_config:
              "@type": type.googleapis.com/envoy.config.filter.http.lua.v2.Lua
              inline_code: |
                function envoy_on_request(request_handle)
                  local headers = request_handle:headers()
                  local dm = request_handle:streamInfo():dynamicMetadata()

                  dm:set("envoy.lua", ":path", headers:get(":path"))
                  dm:set("envoy.lua", ":scheme", headers:get(":scheme"))
                  dm:set("envoy.lua", ":authority", headers:get(":authority"))
                end
          - name: envoy.filters.http.jwt_authn
            config:
              providers:
                provider1:
                  issuer: "Authentication Service"
                  forward: true
                  remote_jwks:
                    http_uri:
                      uri: http://localhost:8080/.public-jwks
                      cluster: keyserver
                  from_headers:
                  - name: token
                  payload_in_metadata: "my_payload"
              rules:
                 - match:
                     prefix: /s/service1
                   requires:
                     provider_name: provider1
                 - match:
                     prefix: /
          - name: envoy.lua
            typed_config:
              "@type": type.googleapis.com/envoy.config.filter.http.lua.v2.Lua
              inline_code: |
                function envoy_on_request(request_handle)
                  local dynamic = request_handle:streamInfo():dynamicMetadata()

                  for dynamic_key, dynamic_value in pairs(dynamic) do
                    request_handle:logInfo("------------- Request dynamic_key: " .. dynamic_key)
                  end

                  local dm = request_handle:streamInfo():dynamicMetadata():get("envoy.filters.http.jwt_authn")
                  for dm_key, dm_value in pairs(dm) do
                    request_handle:logInfo("------------- Request dm " .. dm_key .. ":" .. dm_value)
                  end
                end

@andrewtikhonov
Copy link
Author

It's only there when JWT auth is triggered and successfully completes.

@andrewtikhonov
Copy link
Author

How do i implement such changes in c++ ?

@qiwzhang
Copy link
Contributor

@andrewtikhonov I am OK to implement such verification feature in jwt_authn filter.

@andrewtikhonov
Copy link
Author

Am I the only one who requested this ?

@stale
Copy link

stale bot commented Aug 31, 2019

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 31, 2019
@andrewtikhonov
Copy link
Author

Activity

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Sep 2, 2019
@andrewtikhonov
Copy link
Author

Activity

2 similar comments
@andrewtikhonov
Copy link
Author

Activity

@andrewtikhonov
Copy link
Author

Activity

@stale
Copy link

stale bot commented Nov 14, 2019

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 14, 2019
@dio dio added the help wanted Needs help! label Nov 14, 2019
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Nov 14, 2019
@vemod
Copy link

vemod commented Oct 15, 2020

Actually this feature is a must when working with JWT and JS frontends. We have our fronend apps (vuejs, reactjs etc) which communicate with backend via gateway. and its common practice to use httponly cookies with jwt so js apps can't steal them. So without this feature the usage of a gateway for jwt validation before passing valid requests to backend services is obsolete.

I tried the lua approach but its also not good/working because of POOR cookie support. there is no way to access cookies in lua except for fetching cookie header and parsing it manually by splitting etc..

I tried using header_to_metadata but this doesn't work as well because header_to_metadata puts the value not into headers but into dynamicMetadata (I suppose). So jwt_authn is not able to read jwt token from dynamicMetadata. am I missing something here?

so i would vote +5 for this feature. please :)

@vemod
Copy link

vemod commented Oct 16, 2020

so i fount a workaround for this problem. its not that nice but I think it cleaner then processing cookies in lua. So the solution is to use hybrid approach. this is how it looks like in my solution.

http_filters:
          - name: jwtcookie.filter
            typed_config:
              "@type": type.googleapis.com/envoy.extensions.filters.http.header_to_metadata.v3.Config
              request_rules:
                - cookie: "MPJWT"
                  on_header_present:
                    key: "x-jwt-token"
                    type: STRING
                  on_header_missing:
                    key: "x-jwt-token"
                    type: STRING
                    value: ' '
          - name: jwtcookie.lua
            typed_config:
              "@type": type.googleapis.com/envoy.extensions.filters.http.lua.v3.Lua
              inline_code: |
                function envoy_on_request(request_handle)
                  local jwt = request_handle:streamInfo():dynamicMetadata():get("envoy.filters.http.header_to_metadata")["x-jwt-token"]
                  if jwt == nil or jwt == '' or jwt == ' ' then
                    return
                  end
                  request_handle:logTrace("Passing jwt value from cookie into authorization header: " .. jwt)
                  request_handle:headers():add("Authorization", "Bearer " .. jwt)
                end
          - name: envoy.filters.http.jwt_authn
            typed_config:
              "@type": type.googleapis.com/envoy.extensions.filters.http.jwt_authn.v3.JwtAuthentication
              providers:
                myProvider1:
                  issuer: "https://myhost.com"
                  audiences: [ "https://myhost.com" ]
                  local_jwks:
                    filename: /etc/envoy/jwks/jwks.json
                  from_headers:
                  - name: Authorization
                    value_prefix: "Bearer "
                  forward: true
                  forward_payload_header: x-jwt-payload
              rules:
                - match:
                    prefix: /
                  requires:
                    provider_name: myProvider1
          - name: envoy.filters.http.router

The order is important

  1. header_to_metadata can read a specific cookie value and store it into metadata (dynamicMetadata).
  2. lua filter puts the value from dynamicMetadata into header
  3. generic jwt validation

I would love to skip step 2. not sure if there is a namespace or a way to make header_to_metadata write the value back into headers. or may be we need a new extensions like header_to_header or header_transformer.

well at least with this solution no need to mess around cookie parsing from a sting (in lua its quite dirty).

a note on on_header_missing:
empty values are not allowed for headers in header_to_metadata. thus on_header_missing: " ". or alternatively use this lua code. its matter of taste/style:

function envoy_on_request(request_handle)
   local jwt = request_handle:streamInfo():dynamicMetadata():get("envoy.filters.http.header_to_metadata")
   if jwt["x-jwt-token"] == nil or jwt["x-jwt-token"] == '' then
      return
   end

   request_handle:logTrace("Passing jwt value from cookie into authorization header: " .. jwt["x-jwt-token"])
   request_handle:headers():add("Authorization", "Bearer " .. jwt["x-jwt-token"])
end

@vemod
Copy link

vemod commented Oct 16, 2020

btw: cookie/header name is case sensitive in header_to_metadata :(

@hellraisercenobit
Copy link

@vemod thx a lot for your "patch"
how did you deal with grpc-web withCredentials call?
with envoy i got this error:

as been blocked by CORS policy: Response to preflight request doesn't pass access control check: The value of the 'Access-Control-Allow-Origin' header in the response must not be the wildcard '*' when the request's credentials mode is 'include'. The credentials mode of requests initiated by the XMLHttpRequest is controlled by the withCredentials attribute.

In the response envoy always set the response with Access-Control-Allow-Origin: '*'..
But in the config i have this:
allow_origin_string_match:
- exact: "http://localhost:4200"

Any way to force the response header with the origin instead of *?

@vemod
Copy link

vemod commented Nov 11, 2020

Any way to force the response header with the origin instead of *?

you can force response headers by setting them in lua like in my example with auth request headers. to do that you need to implement
function envoy_on_response(response_handle)

@vemod
Copy link

vemod commented Nov 11, 2020

The value of the 'Access-Control-Allow-Origin' header in the response must not be the wildcard '*' when the request's credentials mode is 'include'.

have you tried to set allow_credentials? you might need to set that in order correct headers are included or set them in lua

@hellraisercenobit
Copy link

hellraisercenobit commented Nov 12, 2020

@vemod allow_credentials: true into cors config not working at all with something like this on client side using grpc-web:

this.greeterClient = new GreeterServicePromiseClient('http://localhost:8080', null, <any>{
     'withCredentials': true
}

Can i declare multiple function with inline_code?
No luck with this (Access-Control-Allow-Origin is always set to *) maybe something is wrong i'm not familiar with this syntax.

              '@type': type.googleapis.com/envoy.extensions.filters.http.lua.v3.Lua
              inline_code: |
                function envoy_on_request(request_handle)
                  local jwt = request_handle:streamInfo():dynamicMetadata():get("envoy.filters.http.header_to_metadata")["x-jwt-token"]
                  if jwt == nil or jwt == '' or jwt == ' ' then
                    return
                  end
                  request_handle:logTrace("Passing jwt value from cookie into authorization header: " .. jwt)
                  request_handle:headers():add("Authorization", "Bearer " .. jwt)
                end
                function envoy_on_response(response_handle)
                  response_handle:logTrace("Try to add override response headers")
                  response_handle:headers():add("Access-Control-Allow-Origin", "http://localhost:8080")
                end

@hellraisercenobit
Copy link

@vemod my bad the problem was coming from an old chrome cors extension.
forgot to uninstall ^^
Your solution is ok ;) thx a lot!
Will be better to have a native support 👍
Anyway JWT filter is awesome 😄.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests. Not bugs or questions. help wanted Needs help!
Projects
None yet
Development

No branches or pull requests

7 participants