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

Passing auth result (client identity) to backend #794

Closed
myidpt opened this issue Apr 19, 2017 · 50 comments
Closed

Passing auth result (client identity) to backend #794

myidpt opened this issue Apr 19, 2017 · 50 comments
Labels
enhancement Feature requests. Not bugs or questions.
Milestone

Comments

@myidpt
Copy link
Contributor

myidpt commented Apr 19, 2017

When mTLS is enabled, Sever side Envoy needs to send the client identity to the backend through HTTP header, so that backend can do its own check/processing based on the client identity.
Questions:
What HTTP header do we use?
How do we support this for non-HTTP requests (This is a stretch goal)?

@mattklein123 mattklein123 added the question Questions that are neither investigations, bugs, nor enhancements label Apr 19, 2017
@mattklein123
Copy link
Member

@myidpt I'm not sure that this an Envoy specific question exactly. It seems like you need to write a filter to set some header based on some parameters of the client connection. Unless you are asking to specify some format that we would perform automatically for all requests? Can you expand please?

@myidpt
Copy link
Contributor Author

myidpt commented Apr 19, 2017

I think it would be good to automatically set the client identity for all requests that did mTLS. But it also works for us if we use a filter to achieve the goal.
I think this is a question for Envoy whether to automatically set the header for all mTLS requests.

@mattklein123
Copy link
Member

I'm open to setting a header by default, but:

  1. What would the header be.
  2. What would be in it?

Can you propose something?

@mattklein123 mattklein123 added the enhancement Feature requests. Not bugs or questions. label Apr 19, 2017
@myidpt
Copy link
Contributor Author

myidpt commented Apr 19, 2017

@lookuptable @lizan @wlu2016
I think we need more discussion on that.
Some options:
"from" header (https://en.wikipedia.org/wiki/List_of_HTTP_header_fields)
"x-ssl-" headers that some people used, probably not standardized.
Currently, we should have the client identity in it, resolved from client-cert SAN.

@PiotrSikora
Copy link
Contributor

From is supposed to contain reachable email address, so we probably shouldn't use it, see RFC7231, sec. 5.5.1.

@lookuptable
Copy link
Contributor

lookuptable commented Apr 19, 2017

Thinking more about this, does it make more sense for us to have our own filter to do header injection? This allows us to do Istio specific sanitization (e.g. ignore non-URI identities in the SAN list) and use headers like x-istio-service-account.

@mattklein123
Copy link
Member

I am skeptical that we are going to find a common header to fit this use case, and will probably need to make one up like the other x-envoy headers. However, since you already have your mixer filter, you could probably just do it there if you want specific functionality.

@wenchenglu
Copy link

we should give this a more generic name, x-service-account will be better than x-istio-service-account. We should also consider a consistent story passing other auth info to the backend like email address, group, issuer, etc. We have been using jwt token or json object to convey these info in the past. Since this is not an urgent feature, lets prepare a design to cover all these scenarios first.

@louiscryan
Copy link

I think there is utility in making this generic to Envoy. In advanced deployments having an edge proxy forward details about the TLS negotiation to backends can be useful x-forwarded-proto is an example of doing this but it doesn't help with mutual auth.

Theres some doc with HAProxy which is still pretty generic but it doesn't cover SANs

https://raymii.org/s/tutorials/haproxy_client_side_ssl_certificates.html#Sending_certificate_details_to_backend_application

and I wouldn't suggest we adopt their nomenclature. I think

x-forwarded-client-cert-****
e.g.
x-forwarded-client-cert-name -> common name
x-forwarded-client-cert-san -> SAN
etc..

fits better with whats already present. If Envoy was going to pick up generic header rewriting capabilities then I think we could use that.

Don't think we need to use x-envoy given that there's already a rough convention for x-forwarded-*.

@htuch - thoughts?

@htuch
Copy link
Member

htuch commented Apr 20, 2017

One issue to think about if we add fixed support into Envoy is how we handle multi-level proxying. XFF gets appended as you go, so you would have to be able to do something similar with the client cert info and make sure you keep all headers in sync (e.g. every proxy would have to insert all headers).

By generic header rewriting capabilities are you talking about something like https://support.citrix.com/article/CTX114461 (but obviously in the Envoy config and some way of getting at Envoy internal fields like the SSL cert name by label)?

BTW, looks like XFF and friends are now in the forwarded header covered by RFC https://tools.ietf.org/html/rfc7239.

@louiscryan
Copy link

I think side-car proxies might be the exception to XFF appending rules, there's not much use to the extra appending if the proxy is logically part of the receiving application.

Beyond that I think appending is the desired behavior though it clearly becomes tricky when were talking about appending details about things which are auth related. There is potential for the ultimate receiver to have confusion unless we're careful about the details. Maybe instead of separate headers for parts of the cert we have just one header and a format inside it? E.g.

X-Forwarded-Client-Cert: name=X; SAN=A; SAN=B; hash=1fd445e2

and then folks get to configure which of the fields in that format are filled out. This would allow normal header concatenation rules to apply while keeping the correlation with hops clear.

@mattklein123 mattklein123 removed the question Questions that are neither investigations, bugs, nor enhancements label Apr 23, 2017
@htuch
Copy link
Member

htuch commented Apr 24, 2017

Envoy isn't just used as a side-car though, so we need to support a number of different topologies, including classical multi-level proxies.

I like the idea of combining into a single XFCC. If I understand your proposal correctly, there is an extensible list of key/values for the client cert which are semi-colon separated, and then each level of proxy is able to either append a header or use a comma separator (per RFC 7230) to add additional cert info.

It will also be necessary for the "frontline" Envoy instance to be configured to scrub, rather than append to, the XFCC header.

@myidpt
Copy link
Contributor Author

myidpt commented Apr 24, 2017

Thanks @louiscryan and @htuch.

I think according to https://tools.ietf.org/html/rfc7230#section-3.2.2, we should always concatenate the info of additional certs to the end of the XFCC field, separated by comma.

When conveying the authentication results such as cert info, we need to think about the security risks. What if a non-frontline Envoy instance receives traffic from a malicious user, and that user injects fake auth results into XFCC? Maybe to prevent this, we should only trust the XFCC from clients that is authenticated through mTLS. Otherwise, the client XFCC header should be scrubbed.

@mattklein123
Copy link
Member

We should apply the same scrubbing that we use for other "trusted" headers already. If we do that, we won't have any issues.

@PiotrSikora
Copy link
Contributor

What would be the practical use for multiple XFCC values? Even with tiered proxies and service mesh, backend is only interested in the client's SSL certificate and not that of middle proxies that handled the traffic.

IMHO, we should create XF* headers only at the "frontline".

@htuch
Copy link
Member

htuch commented Apr 24, 2017

This is mostly just future proofing. It's probably fine to just do XFCC for frontline in a typical service mesh or Envoy deployment today.

Maybe one day some backend wants to verify the certs used by intermediary proxies are all of a certain vintage or provenance in some very large deployment.

I think it makes sense though to think about how this could be extended if we're defining an implicit standard here and not box ourselves in if an Envoy configuration wants this one day, all things being equal. Earlier design iterations would imply only frontline XF* ever. We can just implement it for frontline today.

@myidpt
Copy link
Contributor Author

myidpt commented Apr 30, 2017

I agree with htuch@. Maybe micro-services need to control quota, do logging or even authz based on the intermediary services? For example:
End user -> ... -> Service A -> ... -> Service C
End user -> ... -> Service B -> ... -> Service C
If Service C provisions high QPS for Service A, or only allows Service A to access some core data, XFCC chaining can be helpful.
Considering all use cases, IMHO, for now, we can implement a flag "forwarding Client Cert" in each upstream cluster, with four possible values:

  1. "forward only" - Only forward all XF* headers in the request to the next hop.
  2. "append and forward" - Append current hop results to the request's XF* headers and forward it to the next hop.
  3. "sanitize and set" - Sanitize all XF* headers in the request and set the current hop result in.
  4. "sanitize" - Do not send any XF* headers to the next hop.

@PiotrSikora
Copy link
Contributor

Please keep in mind that the biggest issue with XF* headers (and the reason behind Forwarded header) is that there is no way to match XF* headers together, i.e. in both "forward only" and "append and forward" options, there is no guarantee that all hops will set all XF* headers, and receiving end might see different number of various XF* headers with no ability to infer which proxy sent which certificate (or even if a client sent one!).

It's a security nightmare, unless you control the whole perimeter.

IMHO, we should consider using Forwarded header if we want to forward multiple XFCC headers, even though there is virtually no support for it in other proxies and runtimes.

@mattklein123
Copy link
Member

I don't have a strong preference on whether to make up a new header called XFCC or to use Forwarded, but I agree we should optionally support append like we currently do for XFF.

For reference, the current trusting/scrubbing behavior in the HTTP connection manager is controlled by the "use_remote_address" option and is used here:
https://github.com/lyft/envoy/blob/master/source/common/http/conn_manager_utility.cc#L46

I would suggest that we expand this into an enum of various scrub/append/set settings as per @myidpt depending on what we need.

Once the remote address is determined, Envoy then decides whether a request is internal or external, and then does additional scrubbing if external. Note that the internal vs. external logic is currently fairly simplistic (what was needed for Lyft) and may need to be expanded a bit with options depending on other use cases.

@myidpt
Copy link
Contributor Author

myidpt commented May 8, 2017

@PiotrSikora (Please correct me if I'm wrong)

So as I understand, the advantage of using Forwarded is to be able to mark "by whom" for each forwarded header, using by. Other than that, forwarded header should have same security concerns as XFCC. However, I think the security concerns can be alleviated with mTLS chain between proxies, through which the header's authenticity can be guaranteed through the proxy trust chain.

So we can add by to the XFCC, like:
X-Forwarded-Client-Cert: name=X; SAN=A; SAN=B; hash=1fd445e2; by=<SAN of the proxy's cert>, ...
(certificate SAN should always contain the identity of the proxy, when there are multiple, there should be a prioritization method to pick one). In this way, it is clear who has set each of the header. When mutual trust is established between proxies, the headers should be trusted.

To me, XFCC makes sense only when it is conveyed through mTLS. I'm not sure if it makes sense for non-mTLS connections because if the receiver never authenticates the sender, how can it trust the auth result header? So I suggest that the receiver only trusts the XFCC header sent through mTLS connections. (PS: just to think aloud, if we want to trust each chunk of XFCC header (each part separated by comma) through non-mTLS connections, maybe each XFCC chunk can contain a special value digest, that is the hash value of the XFCC chunk using the writer proxy's private key, so that the receiver is able to verify the authenticity of the header using the proxy's certificate, but this seems to much for now).

And I think the proxy should be able to be configured with the 4 options I mentioned. It's orthogonal to the security concern, right?

@timperrett
Copy link
Contributor

Folks, just chiming in here with a use case as this sounds like a feature we also need for a couple of systems. Specifically what we would like is, when receiving a mTLS request, have the ability to extract the CN, the DN and, less importantly, the SANs on the inbound client certificate, and then remap them onto a header that is propagated to the downstream cluster.

Personally I don't much care if its Forwarded or a custom header, but right now we are constructing our own custom header, because we had a similar discussion to the one that has already taken place here and decided that we'd make our own thing to be clear on the semantics. With that experience having been around this little track before, I'd vote for the XFCC. I like @myidpt's suggestion; parsing the details out would be pretty trivial.

It might also be worth considering what nginx does here [1], as its pretty extensive how they break apart the myriad of things available in the client certificate. Does anyone need all that? It's not clear - perhaps a subset would be enough.

[1] http://nginx.org/en/docs/http/ngx_http_ssl_module.html#var_ssl_client_verify

@mattklein123
Copy link
Member

mattklein123 commented May 17, 2017

@myidpt @PiotrSikora how should we move the ball forward on this one? The actual implementation here is not difficult. We just need to decide on which header to use and the various semantics. Can the two of you do a final proposal and we can get some +1s on it and then implement?

@myidpt
Copy link
Contributor Author

myidpt commented May 17, 2017

@PiotrSikora as I haven't heard from you, please comment on the following proposal:

For the security nature of the client cert info, I think it is better to choose the dedicated header (XFCC) for forwarding auth result, so that it is decoupled from other forwarded info, thus can be controlled more strictly.

XFCC format
The format of x-forwarded-client-cert (XFCC) is similar to the "Forwarded" header. The XFCC header value is a comma (",") separated string. Each substring an XFCC element, which holds information added by a single proxy. Each subsequent proxy may choose to append to the end of the request's XFCC header after a comma.
Each XFCC element is a semicolon ";" separated string. Each substring is a key-value pair, grouped together by an equals ("=") sign. The keys are case-insensitive, the values are case-sensitive.
In the implementation, Envoy will support the following keys:
BY - The SAN (CN if SAN is not available) of the proxy that writes the element.
Subject - Client cert's subject field. Value is quoted.
SAN - Client cert's subject alt name field. Can be multiple.
Hash - The hash value of the client's cert.
(Please comment if more fields are needed)
Example:
Suppose a request is sent through services using certs with the following identities:
foo.org -> proxy.org -> backend-proxy.org -> server
The server may receive XFCC header as:
X-Forwarded-Client-Cert: Subject="C=US, ST=California, L=San Francisco, O=Company A, CN=foo.org"; SAN=foo.org; by=proxy.org, Subject="C=US, ST=Florida, L=Miami, O=Company B, CN=proxy.org"; SAN=proxy.org; by=backend-proxy.org

Envoy implementation
Envoy config will have a "forward_client_cert" field in HTTP connection manager.
This field has four possible values:

  1. "forward_only" - Only forward the XFCC header in the request to the next hop.
  2. "append_forward" - Append current hop results to the request's XFCC header and forward it to the next hop.
  3. "sanitize_set" - Sanitize the XFCC header in the request and set the current hop result in.
  4. "sanitize" - Do not send the XFCC header to the next hop.

The default value is "sanitize".
Before modifying the XFCC header, Envoy will look at the mTLS result to check if the client is authenticated. If the authentication was not successful, Envoy will fall back to the default "sanitize" option.

Added according to feedback
The "forward_client_cert" will have one more field, to support forwarding XFCC header in a trusted, but non-mTLS env such as a private cluster:
5. "always_forward_only" - Only forward the XFCC header in the request to the next hop. Regardless of whether the downstream connection is mTLS.

Envoy config will also have a "set_current_client_cert_details" field in HTTP connection manager. Now it supports values: Subject, SAN (BY and Hash should be mandatory). Default value is SAN. Envoy will only set the specified fields from the current client cert into the header. This field works with append_forward, always_append_forward and sanitize_set modes.

TODO: Add a key in the XFCC element and configuration to enforce security based on the identity of the CA (client cert issuer).

@mattklein123
Copy link
Member

@myidpt +1 from me. @timperrett can you look at ^. @louiscryan @htuch @PiotrSikora ?

@louiscryan
Copy link

louiscryan commented May 17, 2017

+1 from me with a few suggestions

  • Can we give folks control over which fields are forwarded. I suspect Envoy will be asked to support others over time and the header could get quite unwieldly without some control, particularly when appending is enabled. A list of the cert field names to include should suffice. We should also decide what the default set is. Suggested name "forward_client_cert_details"

  • Should be able to forward a hash of the client cert

w.r.t to the modes I wonder if we need all this control. forward_only seems odd as it would omit a locally used credential while forwarding a remote certs details.

I wonder if it's useful for Envoy instead to always append a marker value if no cert was present on the local hop even if a cert was used on a downstream hop. E.g

X-Forwarded-Client-Cert: SAN=....; none; none

would imply that the two most immediate proxy hops did not use a client cert

@PiotrSikora
Copy link
Contributor

  1. I don't buy the argument that decoupling this from theForwarded header provides any security advantage. On the contrary, proxies and their operators need to sanitize multiple headers and keep the whole set in consistent state.

  2. Right now, there is no concept of trusted downstreams, and forward_client_cert: forward_only and forward_client_cert: append_forward cannot work without adding that first.

  3. In terms of forwarded values, do we really care about the whole Subject? Neither that or SAN are globally unique without corresponding CA, unless we want to restrict this feature to a single CA. I think something like SHA256 of the certificate or SPKI might be more useful.

@mattklein123
Copy link
Member

Right now, there is no concept of trusted downstreams

@PiotrSikora what do you mean exactly? We do implicitly have this support today via the poorly named "use_remote_address" field, and in fact we implement trusted downstreams with Envoy in Lyft today via mTLS for our POP edge proxies. In this case, we trust the downtream proxy to have correctly set XFF, etc.

@PiotrSikora
Copy link
Contributor

@mattklein123 ah, sorry, I've missed that... but unless I'm misreading it, listener either trusts all accepted connections or none of them, correct?

@myidpt
Copy link
Contributor Author

myidpt commented May 17, 2017

@louiscryan
Thanks for the suggestions. Based on that, a few improvements proposal and questions:

  1. We can add a field set_client_cert_details, and currently it supports values: Subject, SAN (BY and Hash should be mandatory). Default value is SAN. Envoy will only set the specified fields from the current client cert into the header. This field works with append_forward and sanitize_set modes.
  2. What is the purpose of the hash of client cert? Is it generated using the private key of the proxy, or it's the hash value in the cert?
  3. I guess forward_only can keep the XFCC header cleaner, by omitting less-important mTLS conections (such as between cluster-ingress proxy and server's side-car proxy)?
  4. You are right that XFCC may still be trusted even without TLS/mTLS (e.g., within a private cluster). But I don't know if the "none" place holder is meaningful vs. just do a forwarding. Shall we add options always_forward and always_append_forward to trust the headers even when the connection is not mTLS? This can be useful for downstream traffic without mTLS, but from a trusted private network.

@myidpt
Copy link
Contributor Author

myidpt commented May 17, 2017

@PiotrSikora

  1. Envoy supports per-downstream ssl_context (TLS/mTLS) config. So I think envoy should trust a downstream connection iff. it is a mTLS connection.
  2. Your point that "the identity in cert depends on the CA" is great, but it doesn't seem to be a big problem. The BY value scopes the cert information on the specific connection. Also, normally, I suppose the trusted root CAs configured on each proxy should be well controlled, so that conflicts in Subject or SAN should not happen. "Using SHA256 of the certificate or SPKI" may not work because certs keeps being rotated, and it is very hard to keep everything in sync.

@PiotrSikora
Copy link
Contributor

@myidpt SPKI value doesn't change when certs are rotated (at least, it shouldn't).

@myidpt
Copy link
Contributor Author

myidpt commented May 17, 2017

@PiotrSikora
I see. But in our case, the cert content may change, for example, a SAN may be added/removed in a cert. Does the SPKI change in this case?

@PiotrSikora
Copy link
Contributor

@myidpt that, I don't know... It might not... But SAN will change, so SPKI isn't worse in this case.

@mattklein123
Copy link
Member

ah, sorry, I've missed that... but unless I'm misreading it, listener either trusts all accepted connections or none of them, correct?

Yes, basically. How else could it work though? As @myidpt points out it really needs mTLS to be trusted. The current implementation doesn't enforce that, and IMO I don't think we should make that required since people have different types of deployments. I would rather get rid of "use_remote_address" and make it much more explicit what is done with XFF, etc. like we are discussing here for XFCC. I just put a note on the v2 API about this cc @htuch

@myidpt
Copy link
Contributor Author

myidpt commented May 17, 2017

@PiotrSikora
Sorry I'm not very familiar with SPKI. What does SPKI actually look like? I can only find this but it's not related to X.509 ...

@myidpt
Copy link
Contributor Author

myidpt commented May 18, 2017

@PiotrSikora
Thanks for the pointers. The idea seems to be putting the CA's public key info in the XFCC elements so that the receiver is able to verify if the CA in the certification chain is really allowed to issue the client's identity.

I think this can be useful for enforcing strong security, and we can consider it as a TODO. I think the implementation may not be very trivial here, such as how to configure passing the public key of which CA (there can be multiple CAs in the certificate chain), and how to get the public key of the CA and pass it to the HTTP connection manager.

@mattklein123
Copy link
Member

@myidpt @louiscryan @PiotrSikora I think we are pretty much converged here in terms of control options. As long as we have the ability to specify what fields get populated in XFCC we can always add more fields later. So I the only remaining question is whether to use XFCC or Forwarded as the header name? Can we resolve that today? I don't personally care that much, though I think I would vote for XFCC just to be very explicit about what we are doing.

@myidpt
Copy link
Contributor Author

myidpt commented May 18, 2017

I vote for XFCC.

The basic idea is to decouple the security-related headers from the non-security headers, so that special rules can be applied to them. Because we introduce BY into XFCC, XFCC can address the writer of each element, just like the Forwarded header.

@myidpt
Copy link
Contributor Author

myidpt commented May 18, 2017

@louiscryan @PiotrSikora @mattklein123 @htuch
I have also updated the proposal according to our feedback, by appending to the original one. Please review the appended part by searching for "Added according to feedback". Thanks!

@timperrett
Copy link
Contributor

@myidpt this looks good for the most part. One question: what's the rational for choosing the CN or the SAN(s)? I'm guessing the working assumption is that the latter is a superset of the former? Is that part of the spec? Whilst I don't think it would break our usage, just want to make sure we're making a valid assumption, as im not sure there is any requirement from a cert perspective for SANs to be a super set including the CN.

@louiscryan
Copy link

I am fine with proceeding with XFCC, we can always support config to use Forwarded later if/when it gets some traction.

@myidpt
Copy link
Contributor Author

myidpt commented May 18, 2017

@timperrett
SubjectAltName (SAN) is not a superset of CN. We use SAN because CN is deprecated in favor of SAN for identity checks: https://tools.ietf.org/html/rfc6125#section-2.3

@mattklein123
Copy link
Member

@myidpt OK. I think we are closed on the design here. Feel free to implement. Looking forward to landing this! :)

@myidpt
Copy link
Contributor Author

myidpt commented May 18, 2017

Cool, thanks everyone! I will start implementing it :)

@mattklein123 mattklein123 modified the milestone: 1.4.0 May 19, 2017
@timperrett
Copy link
Contributor

@myidpt just so i'm not polling and annoying you, what's a rough ETA for this feature? Many thanks in advance!

@myidpt
Copy link
Contributor Author

myidpt commented May 25, 2017

@timperrett
Yeah sorry I was distracted by the Istio launch and other stuff this week. Hopefully within next week.

@timperrett
Copy link
Contributor

@myidpt any news on this good sir?

@myidpt
Copy link
Contributor Author

myidpt commented Jun 5, 2017

Working on implementing integration tests. Will send the PR out today or tomorrow.

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.
Projects
None yet
Development

No branches or pull requests

8 participants