egress policy enhacements for GA - #1751
Lior Lieberman (LiorLieberman) wants to merge 1 commit into
Conversation
|
|
||
| // Matches every destination. |
There was a problem hiding this comment.
What is the reason to remove all?
all can be useful if users want to use their own ext_proc on top of Substrate's ext_proc. They could set the EgressPolicy for every actor to all and then rely on their own ext_proc to do authz.
There was a problem hiding this comment.
I think it was used only in cidr rules. But we should add something like that back.
first open question in the description is how do we enable all SNIs -- e.g MITM all SNIs or inject header to all hosts?
There was a problem hiding this comment.
I think it was used only in cidr rules.
all was not under the cidr rules before. It was directly under EgressRule.
But I agree that an all rule at the top does not make sense anymore since only HTTP(S) traffic is supported for GA.
There was a problem hiding this comment.
But we should add something like that back.
first open question in the description is
how do we enable all SNIs -- e.g MITM all SNIs or inject header to all hosts?
Can we add all under HTTPRule, HTTPSRule, and TLSPassthroughRule?
Do we need to support this for GA? |
| // | ||
| // +k8s:optional | ||
| // +k8s:unionMember | ||
| google.protobuf.Empty all = 3; | ||
| TLSPassthroughRule tls_passthrough = 3; |
There was a problem hiding this comment.
Should TLSPassthroughRule be evaluated before HTTPRule and HTTPSRule?
If a HTTPSRule for a given hostname appears before a TLSPassthroughRule for the same hostname (i.e., SNI == hostname), what would happen? Should MITM happen or not?
There was a problem hiding this comment.
I wonder if it is better to move TLSPassthrough out of EgressRule, and make it directly under EgressPolicy. Here are two reasons:
-
TLSPassthroughRuleandHTTP(S)Ruledo not share an evaluation point. AHTTP(S)Rulerule is defined to be evaluated per HTTP request, at the request checkpoint, against the request hostname (Host/:authority).TLSPassthroughRuleis decided once per connection, from the ClientHello, against the SNI — before any request exists. -
It makes it easier to get all the TLSPassthrough SNI patterns. Keeping
TLSPassthroughRuleunderEgressRulemeans we need to scan through all the EgressRules in an EgressPolicy and merge all theTLSPassthroughRule.
| } | ||
|
|
||
| // EgressRuleEffects contains effects applied by a matching hostname rule. | ||
| // EgressRuleEffects contains effects applied by a matching HTTP rule. |
There was a problem hiding this comment.
a matching HTTP rule -> a matching HTTP or HTTPS rule
#1538 fwiw |
|
I thought explicitly talked on this header modifier (on Monday and yesterday) among other things and decided we are going to go with the most simple thing for now (that other projects are also doing). #1538 approach was an alternative in the doc as well that we ruled out for GA fore cred injection |
BREAKING API changes prior in preparation for GA.
Follow up 2 Egress dedicated meetings from the week, and the enhancements doc that came out of it.
This PR has proto-only change to the EgressPolicy API. No implementation; the gateway,
store contract, and e2e helpers stop compiling until the follow-up lands.
EgressRuleis now a union of protocol handlers:http,https,tls_passthrough. One ordered list, first match decides, deny by default.The
hostnames,cidrs, andallrule kinds are removed. No field numbersor names are reserved: we are pre-GA and existing policies must be recreated.
http: cleartext HTTP, matched per request on the authority and port. Carrieseffects.https: MITMed HTTPS that the gateway intercepts . [[Matched on SNI and port at theClientHello, then per request on the authority;]]. Carries
effects. MITM is off unless a name islisted here.
tls_passthrough: TLS forwarded without decryption, matched once perconnection on SNI and port. No effects.
Name fields are
host_patternsonhttpandhttps,sni_patternsontls_passthrough. Same wildcard grammar as before, documented once onHTTPRule.host_patterns.portsadded to all three handlers, 1 to 65535, empty means any portCredential injection is now conditional: a header is replaced only when the
Actor's request already carries it, with any placeholder value. Requests
without the header pass unchanged. Comments only, no new fields.
Unsupported in v1 rules for CIDRRules, arbitrary TCP, non-TCP protocols.
google/protobuf/empty.protoimport dropped; nothing uses it now.Questions
how do we enable all SNIs -- e.g MITM all SNIs or inject header to all hosts?
The handler is called
https, notmitmHttps. Everything in anhttpsblock is intercepted; the block name alone has to carry that. Is that clear
enough, or do we want "intercepted" in the field name after all?
CIDR rules are removed and not part of v1. We revisit them with the
tcphandler. (TODO: lior to include meeting notes:).
inject_static_headersandCredentialHeaderInjectionstill say "inject"while the behavior is replacement. Should we rename to
replace_headersnow, while weare already breaking the API?
follow-ups