add api for conditional append forward in xfcc header #41683
Conversation
Signed-off-by: Rama Chavali <[email protected]>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
@wbpcode Can you PTAL if the API makes sense? |
There was a problem hiding this comment.
Thanks for the contribution. One of my questions is that could we reuse the filter chain matcher to use different HCM for this requirements? Then it needn't to change any core code and API and the requirements could be addressed.
I will prefer the filter chain matcher if possible.
/wait-any
| // This field is valid only when :ref:`forward_client_cert_details | ||
| // <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.forward_client_cert_details>` | ||
| // is SANITIZE_SET_OR_APPEND_FORWARD and the client connection is mTLS. If the client certificate matches the matcher, | ||
| // the client certificate information will be appended to the request’s XFCC header. Otherwise, the XFCC header will be sanitized. | ||
| SubjectAltNameMatcher append_forward_matcher = 60; | ||
|
|
There was a problem hiding this comment.
The requirement is reasonable, But I think this API is too specific for the feature requirement. I think maybe we can add a generic matcher tree here. And make the following result as the match result. Then, we can get a very flexible and powerful way to customize the client cert forward behavior?
message ForwardClientCertConfig {
// How to handle the :ref:`config_http_conn_man_headers_x-forwarded-client-cert` (XFCC) HTTP
// header.
ForwardClientCertDetails forward_client_cert_details = 1
[(validate.rules).enum = {defined_only: true}];
// This field is valid only when :ref:`forward_client_cert_details
// <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.forward_client_cert_details>`
// is APPEND_FORWARD or SANITIZE_SET and the client connection is mTLS. It specifies the fields in
// the client certificate to be forwarded. Note that in the
// :ref:`config_http_conn_man_headers_x-forwarded-client-cert` header, ``Hash`` is always set, and
// ``By`` is always set when the client certificate presents the URI type Subject Alternative Name
// value.
SetCurrentClientCertDetails set_current_client_cert_details = 2;
}
There was a problem hiding this comment.
What other generic matching use cases do you anticipate? BTW, filter chain match does not work for us. The same api call can come from both types of clients
There was a problem hiding this comment.
What other generic matching use cases do you anticipate?
With the generic matching, 1. we can use more flexible matching conditions, 2. we can select any behavior based on the matching results and no only the SANITIZE_SET_OR_APPEND_FORWARD, 3. we can also customize the SetCurrentClientCertDetails based on the matching results.
But anyway, I still prefer to use the filter chain matching. We can enhance the filter chain matching to support san matcher. (To enhance envoy.config.listener.v3.FilterChainMatch or filter chain matcher tree envoy.config.listener.v3.filter_chain_matcher.
There was a problem hiding this comment.
I see what you are saying about filter chain matcher. Let me think about how it works with Istio Ingress Gateway
repokitteh-read-only
bot
added
waiting:any
waiting
and removed
waiting:any
waiting
labels
3 participants
API PR for #41658. Opening this for feedback. Will fill rest of the details soon
-->
Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]