Closed Shikugawa closed 2 years ago
This adds JWKS verification and removes support for using an ID Token? That seems fine.
I see a RejectOnlyIDToken test was added. Did you check what happens if an invalid ID token and a valid session cookie is provided? You want to make sure double Authorization
headers aren't sent upstream. It looks like SetIdTokenHeader()
calls SetHeader()
which just does an Add()
on the new header, I don't think an existing one will be removed.
I think you may want to delete any existing ID Token header in SetIdTokenHeader()
. I'm not sure how envoy normalizes headers, you may be guaranteed that incoming headers are lowercase? (You want to be sure that you remove any uppercase incoming AuThOrIzAtIoN
headers too.)
Actually, this is going into the headers
field of an ext authz OkHttpResponse, which I think does the right thing, you may still want to specifically test this though.
By leaving append as false, the filter will either add a new header, or override an existing one if there is a match
sounds good. I think adding test is reasonable. Merging for now.
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: incfly, Shikugawa
The full list of commands accepted by this bot can be found here.
The pull request process is described here
This PR changes only to allow requests which have only session ID. After this PR merged to upstream, authservice will reject requests which have only ID token on the request header.
close #189