Envoy TLS support with header imposition #9486

jrajahalme · 2019-10-23T15:21:53Z

Add API support for TLS proxying for HTTP, with support for header modifications and (k8s) secrets.

This change is

houndci-bot · 2019-10-23T15:22:00Z

pkg/envoy/server.go

+	}
+}
+
+func GetEnvoyHTTPRules(certManager policy.CertificateManager, l7Rules *api.L7Rules) *cilium.PortNetworkPolicyRule_HttpRules {


exported function GetEnvoyHTTPRules should have comment or be unexported

houndci-bot · 2019-10-23T15:22:01Z

pkg/envoy/server.go

+	egressTLSClusterName  = "egress-cluster-tls"
+	ingressClusterName    = "ingress-cluster"
+	ingressTLSClusterName = "ingress-cluster-tls"
+	EnvoyTimeout          = 300 * time.Second // must be smaller than endpoint.EndpointGenerationTimeout


exported const EnvoyTimeout should have comment (or a comment on this block) or be unexported

houndci-bot · 2019-10-23T15:22:05Z

pkg/policy/repository.go

+	return p.getEnvoyHTTPRules(p.certManager, l7Rules)
+}
+
+func (p *Repository) SetEnvoyRulesFunc(f func(CertificateManager, *api.L7Rules) *cilium.PortNetworkPolicyRule_HttpRules) {


exported method Repository.SetEnvoyRulesFunc should have comment or be unexported

houndci-bot · 2019-10-23T15:22:05Z

pkg/policy/repository.go

+	return p.certManager.GetTLSContext(context.TODO(), tls)
+}
+
+func (p *Repository) GetEnvoyHTTPRules(l7Rules *api.L7Rules) *cilium.PortNetworkPolicyRule_HttpRules {


exported method Repository.GetEnvoyHTTPRules should have comment or be unexported

houndci-bot · 2019-10-23T15:22:06Z

pkg/policy/repository.go

 }

 // GetSelectorCache() returns the selector cache used by the Repository
 func (p *Repository) GetSelectorCache() *SelectorCache {
 	return p.selectorCache
 }

+// GetSelectorCache() returns the selector cache used by the Repository


comment on exported method Repository.GetTLSContext should be of the form "GetTLSContext ..."

houndci-bot · 2019-10-23T15:22:06Z

pkg/policy/repository.go

+
+// PolicyContext is an interface policy resolution functions use to access the Repository.
+// This way testing code can run without mocking a full Repository.
+type PolicyContext interface {


type name will be used as policy.PolicyContext by other packages, and that stutters; consider calling this Context

houndci-bot · 2019-10-23T15:22:06Z

pkg/policy/repository.go

 )

+type CertificateManager interface {


exported type CertificateManager should have comment or be unexported

houndci-bot · 2019-10-23T15:22:07Z

pkg/policy/l4.go

+	return json.Marshal(&redacted)
+}
+
+type PerEpData struct {


exported type PerEpData should have comment or be unexported

houndci-bot · 2019-10-23T15:22:07Z

pkg/policy/l4.go

+// MarshalJSON marsahls a redacted version of the TLSContext. We want
+// to see which fields are present, but not reveal their values in any
+// logs, etc.
+func (t *TLSContext) MarshalJSON() ([]byte, error) {


receiver name t should be consistent with previous receiver name a for TLSContext

houndci-bot · 2019-10-23T15:22:08Z

pkg/policy/l4.go


 	"github.com/sirupsen/logrus"
 )

+// TLS context holds the secret values resolved from an 'api.TLSContext'


comment on exported type TLSContext should be of the form "TLSContext ..." (with optional leading article)

jrajahalme · 2019-10-23T15:22:46Z

test-me-please

houndci-bot · 2019-10-26T00:26:56Z

pkg/envoy/server.go

+
+	headerMatches := make([]*cilium.HeaderMatch, 0, len(h.HeaderMatches))
+	for _, hdr := range h.HeaderMatches {
+		var mismatch_action cilium.HeaderMatch_MismatchAction


don't use underscores in Go names; var mismatch_action should be mismatchAction

houndci-bot · 2019-10-26T00:26:56Z

pkg/envoy/server.go

+
+	headerMatches := make([]*cilium.HeaderMatch, 0, len(h.HeaderMatches))
+	for _, hdr := range h.HeaderMatches {
+		var mismatch_action cilium.HeaderMatch_MismatchAction


don't use underscores in Go names; var mismatch_action should be mismatchAction

houndci-bot · 2019-10-26T00:26:56Z

pkg/envoy/server.go

+
+	headerMatches := make([]*cilium.HeaderMatch, 0, len(h.HeaderMatches))
+	for _, hdr := range h.HeaderMatches {
+		var mismatch_action cilium.HeaderMatch_MismatchAction


don't use underscores in Go names; var mismatch_action should be mismatchAction

houndci-bot · 2019-10-26T00:26:58Z

pkg/policy/api/http.go

+type MismatchAction string
+
+const (
+	MismatchActionLog     MismatchAction = "LOG"     // Keep checking other matches


exported const MismatchActionLog should have comment (or a comment on this block) or be unexported

houndci-bot · 2019-10-26T00:26:58Z

pkg/policy/api/http.go

+type MismatchAction string
+
+const (
+	MismatchActionLog     MismatchAction = "LOG"     // Keep checking other matches


exported const MismatchActionLog should have comment (or a comment on this block) or be unexported

jrajahalme · 2019-10-29T23:41:18Z

test-me-please

jrajahalme · 2019-10-29T23:57:54Z

test-me-please

jrajahalme · 2019-10-30T00:58:09Z

test-me-please

coveralls · 2019-10-30T01:24:12Z

Coverage decreased (-0.09%) to 45.846% when pulling db040ef on pr/jrajahalme/envoy-tls-support-wip into c829701 on master.

jrajahalme · 2019-10-30T15:51:47Z

test-me-please

pkg/policy/api/l4.go

aanm · 2019-12-12T16:45:21Z

pkg/policy/api/l4.go

+}
+
+// Equal returns true if 'a' and 'b' have the same contents.
+func (a *TLSContext) Equal(b *TLSContext) bool {


To avoid copying the structures as would happen if they would be passed by value instead of by reference. I see that passing by value would prevent (accidental) mutations which would be nice.

Upon examining the call sites I recalled that the entities being compared are already pointers, so it is best to keep this Equal() function to avoid code repetition and keep the call sites more readable.

pkg/envoy/server.go

pkg/k8s/apis/cilium.io/v2/register.go

This option will set the root certificate used to search for TLS certificates defined in a CNP for L7 TLS policy enforcement. Signed-off-by: André Martins <andre@cilium.io>

Signed-off-by: André Martins <andre@cilium.io>

Add API definitions for k8s secrets and plumb them into Envoy policy updates. Add certificate manager to find certificates either locally or in k8s Map keys "tls.key" and "tls.crt" are used for the private key and the certificate, respectively, when creating a TLS secret like so: $ kubectl create secret tls test-tls --key="file1" --cert="file2" Support explicit overrides for the default item names. If given, these must be found. Signed-off-by: Jarno Rajahalme <jarno@covalent.io> Signed-off-by: André Martins <andre@cilium.io>

Add PolicyContext, a collection of references to resources needed during policy computation. Initially this holds just a SelectorCache, but later commits will add more items without then needing to update and bloat all the function prototypes. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Resolve TLS contexts earlier in the process. Get the default namespace from the security identity rather than the rule. This allows for a default namespace to be well defined even if none of the rules have a namespace label. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Supporting secrets is easier if we translate HTTP policies while in the policy computation context. Translating HTTP policies erlier also reduces unnecessary work of re-translating them when policy remains but IDs change. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Allow replacing logging, deleting, adding, and replacing mismatching headers. Now that we can have rules with side-effect, we must track if HTTP rules can be short circuited. If no rules have side-effects, then the policy evaluationn can be stopped as soon as a decision to pass traffic has been found. If rules include side-effects, we must evaluate all applicable rules. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

houndci-bot · 2019-12-12T21:41:35Z

pkg/envoy/server.go

+	}
+}
+
+func GetEnvoyHTTPRules(certManager policy.CertificateManager, l7Rules *api.L7Rules, ns string) (*cilium.HttpNetworkPolicyRules, bool) {


exported function GetEnvoyHTTPRules should have comment or be unexported

houndci-bot · 2019-12-12T21:41:36Z

pkg/envoy/server.go

+	}
+}
+
+func GetEnvoyHTTPRules(certManager policy.CertificateManager, l7Rules *api.L7Rules, ns string) (*cilium.HttpNetworkPolicyRules, bool) {


exported function GetEnvoyHTTPRules should have comment or be unexported

jrajahalme · 2019-12-12T21:41:39Z

test-me-please

aanm · 2019-12-16T10:03:11Z

@jrajahalme it's a real CI failure:

22:01:27  contrib/scripts/check-logging-subsys-field.sh
22:01:27  contrib/scripts/check-fmt.sh
22:01:28  Unformatted Go source code:
22:01:28  ./pkg/envoy/server.go
22:01:28  diff -u ./pkg/envoy/server.go.orig ./pkg/envoy/server.go
22:01:28  --- ./pkg/envoy/server.go.orig	2019-12-12 22:01:28.436460706 +0000
22:01:28  +++ ./pkg/envoy/server.go	2019-12-12 22:01:28.436460706 +0000
22:01:28  @@ -605,7 +605,7 @@
22:01:28   		// the order of the rules in the imported policies
22:01:28   		// changes, so there is minimal likelihood of
22:01:28   		// unnecessary policy updates.
22:01:28  -		
22:01:28  +
22:01:28   		// SortHeaderMatches(headerMatches)
22:01:28   	}
22:01:28   
22:01:28  make: *** [precheck] Error 1
22:01:28  Makefile:371: recipe for target 'precheck' failed
22:01:29  Makefile:87: recipe for target 'jenkins-precheck' failed
22:01:29  make: *** [jenkins-precheck] Error 1

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Add simple TLS test to access an external resource via the proxy. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2019-12-16T12:06:22Z

test-me-please

jrajahalme · 2019-12-17T11:37:16Z

CI errors seem unrelated, external connectivity issues (1.1.1.1), etc. Trying again.

jrajahalme · 2019-12-17T11:37:23Z

test-me-please

aanm · 2019-12-17T20:56:39Z

test-me-please

joestringer

LGTM!

I have a bunch of super trivial feedback below that could be addressed in a follow-up PR, the only one that would be meaningful for the next RC would be if we wish to change the CLI arg name.

pkg/crypto/certificatemanager/certificate_manager.go

joestringer · 2019-12-19T14:30:40Z

pkg/crypto/certificatemanager/certificate_manager.go

+			err = fmt.Errorf("certificates not found locally in %s nor in k8s secret %s/%s ", m.rootPath, ns, name)
+			return "", "", "", err
+		}
+		err = fmt.Errorf("certificates not found in k8s secret %s/%s", ns, name)


Is there any meaningful distinction between the error on line 162 and the error here, or could we unconditionally use the error on line 162 to replace lines 161-165?

It seems to me like if either local or k8s-based certificate detection was successful (and TLS is configured in the specified policy) then this function would have already returned by this point. It's the fact that neither provided certificates that leads us to this error condition.

Also the path should probably be certPath rather than m.rootPath, as that's the full path that was searched for the local certs.

Cleaned up in the follow-up PR #9807

joestringer · 2019-12-19T14:37:28Z

pkg/option/config.go

@@ -89,6 +89,10 @@ const (
 	// BPFRoot is the Path to BPF filesystem
 	BPFRoot = "bpf-root"

+	// CertsDirectory is the root directory used to find out certificates used
+	// in L7 HTTPs policy enforcement.
+	CertsDirectory = "certificates-directory"


nit: Should this CLI arg be more specific, like policy-certificates-dir, to ensure there's no mixups with the kvstore certs?

Maybe, will need to discuss this more. We could also generalizer this to all "secrets", not just certificates. In that case a name like "local-secrets-dir" would be a better match?

pkg/envoy/server.go

joestringer · 2019-12-19T14:58:16Z

pkg/policy/repository.go

+type PolicyContext interface {
+	GetSelectorCache() *SelectorCache
+	GetTLSContext(tls *api.TLSContext) (ca, public, private string, err error)
+	GetEnvoyHTTPRules(l7Rules *api.L7Rules) (*cilium.HttpNetworkPolicyRules, bool)


nit: I think it'd be useful to document particularly the canShortcut bool here in context of what users or implementations of this interface can and should do. The whole interface could do with a bit of documentation.

Added documentation to all functions in this interface.

joestringer · 2019-12-19T15:00:56Z

pkg/envoy/server.go

 				}

 				pnp.Rules = append(pnp.Rules, rule)
 			}
 		}

+		// Short-circuit rules if a rule allows all and all other rules can be short-circuited
+		if allowAll && canShortCircuit {
+			log.Info("Short circuiting HTTP rules due to rule allowing all and no other rules needing attention")


This looks like a debug-level log, I don't think it's meaningful to most users and it's prone to repeating itself in the logs when a policy is evaluated.

Right, changed to Debug

joestringer · 2019-12-19T15:05:59Z

pkg/k8s/apis/cilium.io/v2/register.go

+			Properties:  Secret,
+		},
+		"value": {
+			Description: "Value containst the header value that must be present in the request. If both Secret " +


Suggested change

Description: "Value containst the header value that must be present in the request. If both Secret " +

Description: "Value contains the header value that must be present in the request. If both Secret " +

joestringer · 2019-12-19T15:11:19Z

pkg/policy/api/utils.go

+		h.Value != o.Value ||
+		h.Secret != nil && o.Secret == nil ||
+		h.Secret == nil && o.Secret != nil ||
+		h.Secret != nil && o.Secret != nil && *h.Secret != *o.Secret {


Can this use !h.Secret.Equal(o.Secret)? (Ignore if this is autogenerated..)

Secret.Equal() did not exist, but I factored it out of this for clarity.

joestringer · 2019-12-19T15:13:36Z

pkg/policy/api/l4.go

+	// TerminatingTLS is the TLS context for the connection terminated by
+	// the L7 proxy.  For egress policy this specifies the server-side TLS
+	// parameters to be applied on the connections originated from the local
+	// POD and terminated by the L7 proxy. For ingress policy this specifies


nit: This API wording usually uses the Cilium endpoint terminology rather than POD. (Same below)

Changed, thanks!

joestringer · 2019-12-20T15:48:12Z

@jrajahalme merged 🎉

At this point it's probably more useful to merge this and get rc2 out and allow people to try it out and test it. It will be easier to iterate on in the master tree.

Please do follow up on the remaining feedback items above in a follow up PR.

jrajahalme · 2020-01-03T22:35:26Z

@joestringer Addressed your comments in #9807

jrajahalme added the wip label Oct 23, 2019

jrajahalme requested a review from a team October 23, 2019 15:21

jrajahalme requested review from a team as code owners October 23, 2019 15:21

jrajahalme requested review from a team October 23, 2019 15:21

jrajahalme requested review from a team as code owners October 23, 2019 15:21

houndci-bot reviewed Oct 23, 2019

View reviewed changes

jrajahalme force-pushed the pr/jrajahalme/envoy-tls-support-wip branch 2 times, most recently from 4c6a6c7 to 760dbd3 Compare October 26, 2019 00:26

houndci-bot reviewed Oct 26, 2019

View reviewed changes

jrajahalme force-pushed the pr/jrajahalme/envoy-tls-support-wip branch 2 times, most recently from 6f01efc to 85a6486 Compare October 29, 2019 23:39

jrajahalme force-pushed the pr/jrajahalme/envoy-tls-support-wip branch from 85a6486 to 0f2ed90 Compare October 29, 2019 23:57

jrajahalme force-pushed the pr/jrajahalme/envoy-tls-support-wip branch from 0f2ed90 to 38f22bd Compare October 30, 2019 00:57

jrajahalme mentioned this pull request Dec 12, 2019

Initial Envoy TLS support #9263

Closed

3 tasks

aanm requested changes Dec 12, 2019

View reviewed changes

jrajahalme and others added 7 commits December 12, 2019 18:21

option: add 'certificates-directory' option to look for TLS certificates

9816843

This option will set the root certificate used to search for TLS certificates defined in a CNP for L7 TLS policy enforcement. Signed-off-by: André Martins <andre@cilium.io>

pkg/k8s: add wrapper to get k8s secrets

78e8f57

Signed-off-by: André Martins <andre@cilium.io>

jrajahalme force-pushed the pr/jrajahalme/envoy-tls-support-wip branch from a158c2d to b65236a Compare December 12, 2019 21:41

houndci-bot reviewed Dec 12, 2019

View reviewed changes

jrajahalme added 2 commits December 16, 2019 14:03

test: Add runtime test for imposing an HTTP header

6d2c6cd

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

k8s: TLS testing

db040ef

Add simple TLS test to access an external resource via the proxy. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme force-pushed the pr/jrajahalme/envoy-tls-support-wip branch from b65236a to db040ef Compare December 16, 2019 12:05

joestringer approved these changes Dec 19, 2019

View reviewed changes

joestringer removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Dec 20, 2019

joestringer merged commit da34687 into master Dec 20, 2019

joestringer deleted the pr/jrajahalme/envoy-tls-support-wip branch December 20, 2019 15:47

jrajahalme mentioned this pull request Jan 3, 2020

TLS follow-up fixes #9807

Merged

jrajahalme mentioned this pull request Feb 24, 2023

Sync TLS interception secrets to Envoy via SDS #24020

Closed

jrajahalme mentioned this pull request Apr 7, 2023

policy: Do not share same policy for multiple cached selectors #24788

Merged

	Description: "Value containst the header value that must be present in the request. If both Secret " +
	Description: "Value contains the header value that must be present in the request. If both Secret " +

Envoy TLS support with header imposition #9486

Envoy TLS support with header imposition #9486

Uh oh!

Conversation

jrajahalme commented Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrajahalme commented Oct 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrajahalme commented Oct 29, 2019

Uh oh!

jrajahalme commented Oct 29, 2019

Uh oh!

jrajahalme commented Oct 30, 2019

Uh oh!

coveralls commented Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrajahalme commented Oct 30, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrajahalme Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrajahalme commented Dec 12, 2019

Uh oh!

aanm commented Dec 16, 2019

Uh oh!

jrajahalme commented Dec 16, 2019

Uh oh!

jrajahalme commented Dec 17, 2019

Uh oh!

jrajahalme commented Dec 17, 2019

Uh oh!

aanm commented Dec 17, 2019

Uh oh!

jrajahalme commented Oct 23, 2019 •

edited

Loading

coveralls commented Oct 30, 2019 •

edited

Loading

jrajahalme Dec 12, 2019 •

edited

Loading