Open bburky opened 3 years ago
Currently only HTTP headers request->attributes().request().http().headers()
may be matched. I think an enhancement could be to allow matching on request->attributes().destination().principal()
(or anything in the request attributes).
For the specific use case of matching the Host:
header, I just wanted to select workloads in the filter chain. If deploying Authservice at the sidecar (not the gateway), each workload will have a unique service account and this could allow easier matching. Also, this principal can't be influenced by an attacker and could be considered safer to match.
@incfly I think there are two problems here.
The expected behavior is like the following.
if (request) {
should_allow = false; // Resolve problem 2. Default behavior should be false/
for (chain <- chains) {
if (chain.Match(req) { // Resolve problem 1, header matching must be case-insensitive.
should_allow = chain.Process(req);
}
}
if (!should_allow) {
return DENY;
}
return ALLOW;
}
I think RFC 3982 and RFC7230 only say header "field names" are case-insensitive, not the field value. You have to read the spec to determine how to match each header.
https://httpwg.org/specs/rfc7230.html#rfc.section.2.7.3
The scheme and host are case-insensitive and normally provided in lowercase; all other components are compared in a case-sensitive manner.
I'm not sure how to best handle this in authservice. You could special case some headers and lowercase them before processing. Or provide an optional case insensitive matching mode. Or allow matching the request.host
attribute which I think may already be normalized by envoy.
I will +1 on the option to provide a option to match with case insenstive, similiar to Envoy's StringMatcher API
https://www.envoyproxy.io/docs/envoy/latest/api-v3/type/matcher/v3/string.proto#string-matcher
Where you can specify ignore_case
.
but back to this overall issue, it'll be solved if #140 is solved as well right? If we provide an option in the overall matches, that if no match found, return 403 instead of pass through at authservice level. EXAMPLE.COM
will returns 403 even earlier. Is that right?
Yes, addressing #140 will resolve the security impact of this issue.
There is already no security risk if an Istio AuthorizationPolicy is applied after authservice and requires a JWT for all requests (for example, requestPrincipals: ["*"]
). The docs don't discuss whether this is considered required, I recommend clarifying this.
I saw some issues that FilterChain matcher and TriggerRule might be merged? There is another issue in the filter chain matching that I wanted to point out while you are working on that area of the code. https://github.com/istio-ecosystem/authservice/issues/172 https://github.com/istio-ecosystem/authservice/issues/171
The
prefix
andequality
match operators match the header value case sensitively, and this is a problem for matchingHost:
.It is possible to bypass an authservice chain with an
equality
match onHost: example.com
by with a request likecurl http://EXAMPLE.COM
. If an Istio AuthorizationPolicy is used after Authservice, this isn't an auth bypass because the request would be rejected withRBAC: access denied
due to a missing JWT. I can't tell if using Istio AuthZ is considered optional or required though. Regardless this still a bug that I wanted your team to be aware of if you're fixing up that area of code.The related code is: https://github.com/istio-ecosystem/authservice/blob/e91ca6921c5a54ef21bb7ffb541227e656d1445e/src/filters/filter_chain.cc#L93-L113
I haven't tested this on a recent version of authservice, but I think the case sensitive behavior is still there.