Adds a test case for L7 policy with TLS #1406

meyskens · 2023-02-21T13:02:10Z

This adds a test to check if HTTPS traffic can get inspected in a CNP. To enable this it adds new helper functions to provision secrets, certificates and a public CA bundle into the test setup.

It implements part of the work described in cilium/cilium#23784 (but not all, see comment in issue).

A few parts I am not sure of are in here: like the location of the cabundle in the project so feedback is welcome

meyskens · 2023-02-21T13:04:43Z

cc @jrajahalme (i have no permissions to add you as a reviewer here)

ldelossa · 2023-02-21T15:37:13Z

@meyskens looks like you have some linter issues to fix up before tests will pass.

meyskens · 2023-02-21T16:00:32Z

Noticed, cleaned up the small mistake and added the proper gate to the 2nd TLS test :)

meyskens · 2023-02-22T08:54:53Z

Multicluster test failed (don't think this is releated to this PR):

job.batch/cilium-cli condition met
Error from server: Get "https://10.101.248.4:10250/containerLogs/kube-system/cilium-cli-zfvq9/test?timestamps=true": No agent available
Error: Process completed with exit code 1.

tklauser

A few parts I am not sure of are in here: like the location of the cabundle in the project so feedback is welcome

Storing it in connectivity/check/assets LGTM.

Some spelling nits inline, but feel free to ignore in case there are no other change requests.

Out of curiosity: how would a successful test run for these tests look? It seems like they are currently skipped on all CI workflows. So I guess we'd need to extend the cilium ClusterRole in order to be able to test these?

connectivity/check/features.go

connectivity/check/secrets.go

connectivity/suite.go

meyskens · 2023-02-22T12:38:20Z

@tklauser yes indeed, the helmchart has an option to set the secrets in the RBAC with a flag which we can set to run these in the CI. I discussed with @aanm yesterday to add this install option in this repo also to run them, but it seemed L7 wasn't even enabled in some test scenarios so we were unsure. When this is merged a follow up should be made in cilium/cilium to enable the tests there in a CI run.

meyskens · 2023-02-22T12:41:35Z

install command needed to make them run ./cilium install --version v1.13.0 --kube-proxy-replacement=partial --helm-set ingressController.enabled=true --helm-set tls.secretsBackend=k8s (technically l7 only + secretsBackend is enough but i added ingress as that is in my usual setup)

meyskens · 2023-02-24T10:26:22Z

EKS test seems to fail due to too many clusters 2023-02-24 10:23:39 [✖] AWS::EC2::VPC/VPC: CREATE_FAILED – "Resource handler returned message: \"The maximum number of VPCs has been reached. (Service: Ec2, Status Code: 400, Request ID: b74adfb6-004c-4ec4-b826-b265c0d26d62)\" (RequestToken: 8fe7466e-4158-d711-cf22-6860437920c6, HandlerErrorCode: GeneralServiceException)"

jrajahalme

Really nice work! I'd like to see the curl client verify the certificate to guard against possible bugs. We currently do not support proxy choosing a certificate based on the SNI, but we could in future. Or even simpler, Cilium agent could have a bug and pass a wrong cert to the proxy.

.github/in-cluster-test-scripts/aks-azure-ipam-install.sh

connectivity/check/test.go

jrajahalme · 2023-02-24T12:43:43Z

connectivity/tests/world.go

+// PodToWorldWithTLSIntercept sends an HTTPS request to one.one.one.one (default value of ExternalTarget) from from random client
+func PodToWorldWithTLSIntercept(curlOpts ...string) check.Scenario {
+	s := &podToWorldWithTLSIntercept{
+		curlOpts: []string{"-k"}, // skip TLS verification as it will be our internal cert


Skipping the certificate verification could hide bugs in our secret/policy handling, maybe? It is a bit more work to pass the cacert to curl, but I think it would be worth it.

I added the ability to via a command inject the ca cert into the pod. It is not the best way IMO however we cannot restart the pod or change it's spec to load a secret as the design assumes it to be there "forever".
Any suggestions to improve the way it does this very welcome!

connectivity/tests/world.go

jrajahalme · 2023-02-24T12:53:32Z

connectivity/suite.go

+	ct.NewTest("client-egress-l7-tls-deny-without-headers").
+		WithFeatureRequirements(check.RequireFeatureEnabled(check.FeatureL7Proxy)).
+		WithFeatureRequirements(check.RequireFeatureEnabled(check.FeatureSecretBackendK8s)).
+		WithCABundleSecret().


Could the CA bundle Secret be created automatically when needed by WithCertificate() or WithSecret()? As such it does not take any input and feels like unnecessary boilerplate.

The CA Bundle here refer to the public CA store and isn't really related to a certificate being created or not.
That is why i made the initial decision not to do that. Happy to change that if you want.

jrajahalme · 2023-02-24T13:11:31Z

I tested this locally, and the first invocation failed:

  ℹ️  📜 Applying secret 'cabundle' to namespace 'cilium-test'..
  ℹ️  📜 Applying secret 'externaltarget-tls' to namespace 'cilium-test'..
  ℹ️  📜 Applying CiliumNetworkPolicy 'l7-policy-tls' to namespace 'cilium-test'..
  [-] Scenario [client-egress-l7-tls-deny-without-headers/pod-to-world-with-tls-intercept]
  [.] Action [client-egress-l7-tls-deny-without-headers/pod-to-world-with-tls-intercept/https-to-one.one.one.one-0: cilium-test/client-784c67ffc4-l2zfs (10.244.0.79) -> one.one.one.one-https (one.one.one.one:443)]
  [.] Action [client-egress-l7-tls-deny-without-headers/pod-to-world-with-tls-intercept/https-to-one.one.one.one-1: cilium-test/client2-67754cb6fb-zrzk8 (10.244.0.37) -> one.one.one.one-https (one.one.one.one:443)]
  ❌ command "curl -w %{local_ip}:%{local_port} -> %{remote_ip}:%{remote_port} = %{response_code} --silent --fail --show-error --output /dev/null --connect-timeout 2 --max-time 10 -k https://one.one.one.one:443" failed with unexpected exit code: command terminated with exit code 35 (expected 22, found 35)
  📄 No flows recorded during action https-to-one.one.one.one-1
  📄 No flows recorded during action https-to-one.one.one.one-1
  ℹ️  📜 Deleting secret 'cabundle' from namespace 'cilium-test'..
  ℹ️  📜 Deleting secret 'externaltarget-tls' from namespace 'cilium-test'..
  ℹ️  📜 Deleting CiliumNetworkPolicy 'l7-policy-tls' from namespace 'cilium-test'..
[=] Test [client-egress-l7-tls-headers]
..

[=] Skipping Test [dns-only]

[=] Skipping Test [to-fqdns]

📋 Test Report
❌ 1/2 tests failed (1/4 actions), 32 tests skipped, 0 scenarios skipped:
Test [client-egress-l7-tls-deny-without-headers]:
  ❌ client-egress-l7-tls-deny-without-headers/pod-to-world-with-tls-intercept/https-to-one.one.one.one-1: cilium-test/client2-67754cb6fb-zrzk8 (10.244.0.37) -> one.one.one.one-https (one.one.one.one:443)
connectivity test failed: 1 tests failed

Unfortunatly I did not have hubble enabled, so did not see the flows. I was not able to reproduce this, after enabling hubble it worked as expected, also after uninstall/install. I tried it like this:

$ cilium install --helm-set tls.secretsBackend=k8s
$ cilium hubble enable
$ cilium hubble port-forward&
$ cilium connectivity test --test tls --debug
...
  🐛 Executing command [curl -w %{local_ip}:%{local_port} -> %{remote_ip}:%{remote_port} = %{response_code} --silent --fail --show-error --output /dev/null --connect-timeout 2 --max-time 10 -k https://one.one.one.one:443]
  🐛 command "curl -w %{local_ip}:%{local_port} -> %{remote_ip}:%{remote_port} = %{response_code} --silent --fail --show-error --output /dev/null --connect-timeout 2 --max-time 10 -k https://one.one.one.one:443" failed as expected: command terminated with exit code 22
  📄 Validating flows for peer cilium-test/client-784c67ffc4-564qg
  🐛 Validating 38 flows against 3 requirements
  ✅ DNS request found at 0
  ✅ DNS response found at 4
  ✅ SYN found at 28
  ✅ L3/L4 Drop not found
  ✅ SYN found at 28
  ✅ L7 Drop found at 34
  ✅ L3/L4 Drop not found
  ✅ Flow validation successful for peer cilium-test/client-784c67ffc4-564qg (first: 0, last: 34, matched: 4)
...

maintainer-s-little-helper · 2023-02-28T13:07:40Z

Commit 583f9c6 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

meyskens · 2023-03-03T08:36:42Z

https://github.com/cilium/cilium-cli/actions/runs/4293305139/jobs/7480818048#step:11:4833

This is quite interesting the test seems to flake... not entirely sure why, do no merge until this is fixed

meyskens · 2023-03-03T08:38:56Z

Not sure if this was caused by a flake in the test or an issue in the flow matching? Aborting flow matching: context deadline exceeded. Any suggestions?

This adds a test to check if HTTPS traffic can get inspected in a CNP. To enable this it adds new helper functions to provision secrets, certificates and a public CA bundle into the test setup. It also adds the functionality to insert a file into a pod, this is done for having the certificate chain checked in curl. Signed-off-by: Maartje Eyskens <[email protected]>

This enables the Envoy l7 proxy backend in the test setups as well as the tls secrets backend to be k8s. This allows the L7 ans TLS connectivity tests to run. Signed-off-by: Maartje Eyskens <[email protected]>

meyskens requested review from a team as code owners February 21, 2023 13:02

meyskens requested review from tommyp1ckles and ldelossa February 21, 2023 13:02

meyskens temporarily deployed to ci February 21, 2023 13:02 — with GitHub Actions Inactive

meyskens mentioned this pull request Feb 21, 2023

Migrate TLS policy tests from Ginkgo to cilium-cli connectivity test cilium/cilium#23784

Closed

9 tasks

meyskens force-pushed the meyskens/egress-tls branch from fa26abf to 12024c1 Compare February 21, 2023 15:57

meyskens temporarily deployed to ci February 21, 2023 15:57 — with GitHub Actions Inactive

ldelossa approved these changes Feb 22, 2023

View reviewed changes

tklauser requested a review from jrajahalme February 22, 2023 12:00

tklauser approved these changes Feb 22, 2023

View reviewed changes

connectivity/check/features.go Outdated Show resolved Hide resolved

connectivity/check/secrets.go Outdated Show resolved Hide resolved

connectivity/suite.go Outdated Show resolved Hide resolved

connectivity/suite.go Outdated Show resolved Hide resolved

meyskens had a problem deploying to ci February 22, 2023 12:39 — with GitHub Actions Error

meyskens force-pushed the meyskens/egress-tls branch from 22f282f to 0a173b3 Compare February 22, 2023 12:39

meyskens temporarily deployed to ci February 22, 2023 12:40 — with GitHub Actions Inactive

meyskens requested review from a team as code owners February 24, 2023 10:13

meyskens requested a review from nbusseneau February 24, 2023 10:13

meyskens temporarily deployed to ci February 24, 2023 10:13 — with GitHub Actions Inactive

meyskens force-pushed the meyskens/egress-tls branch from 1ffcbd6 to 71ec07a Compare February 24, 2023 10:22

meyskens temporarily deployed to ci February 24, 2023 10:22 — with GitHub Actions Inactive

meyskens force-pushed the meyskens/egress-tls branch from 71ec07a to ac5a4b6 Compare February 24, 2023 10:29

meyskens temporarily deployed to ci February 24, 2023 10:29 — with GitHub Actions Inactive

jrajahalme reviewed Feb 24, 2023

View reviewed changes

tommyp1ckles approved these changes Feb 25, 2023

View reviewed changes

meyskens force-pushed the meyskens/egress-tls branch from ac5a4b6 to 12a57b9 Compare February 28, 2023 13:02

meyskens temporarily deployed to ci February 28, 2023 13:02 — with GitHub Actions Inactive

maintainer-s-little-helper bot added the dont-merge/needs-sign-off label Feb 28, 2023

meyskens had a problem deploying to ci February 28, 2023 13:07 — with GitHub Actions Failure

meyskens force-pushed the meyskens/egress-tls branch from 583f9c6 to 76feee7 Compare February 28, 2023 13:10

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off label Feb 28, 2023

meyskens temporarily deployed to ci February 28, 2023 13:10 — with GitHub Actions Inactive

meyskens requested review from jrajahalme and removed request for nbusseneau February 28, 2023 13:16

tklauser requested a review from nbusseneau March 1, 2023 16:44

nbusseneau approved these changes Mar 2, 2023

View reviewed changes

meyskens force-pushed the meyskens/egress-tls branch from 76feee7 to 8a5366d Compare March 3, 2023 14:03

meyskens temporarily deployed to ci March 3, 2023 14:03 — with GitHub Actions Inactive

meyskens added 2 commits March 6, 2023 10:06

Enable L7 and k8s secrets in tests

a92cf38

This enables the Envoy l7 proxy backend in the test setups as well as the tls secrets backend to be k8s. This allows the L7 ans TLS connectivity tests to run. Signed-off-by: Maartje Eyskens <[email protected]>

meyskens force-pushed the meyskens/egress-tls branch from 8a5366d to a92cf38 Compare March 6, 2023 09:06

meyskens temporarily deployed to ci March 6, 2023 09:06 — with GitHub Actions Inactive

jrajahalme approved these changes Mar 7, 2023

View reviewed changes

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 7, 2023

tklauser merged commit eecce64 into cilium:master Mar 7, 2023

meyskens deleted the meyskens/egress-tls branch March 7, 2023 11:30

meyskens mentioned this pull request Mar 16, 2023

Run Cilium CLI L7 tests cilium/cilium#24414

Merged

chenrui333 mentioned this pull request Mar 21, 2023

cilium-cli 0.13.2 Homebrew/homebrew-core#126245

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds a test case for L7 policy with TLS #1406

Adds a test case for L7 policy with TLS #1406

meyskens commented Feb 21, 2023

meyskens commented Feb 21, 2023

ldelossa commented Feb 21, 2023

meyskens commented Feb 21, 2023

meyskens commented Feb 22, 2023 •

edited

Loading

tklauser left a comment

meyskens commented Feb 22, 2023

meyskens commented Feb 22, 2023

meyskens commented Feb 24, 2023

jrajahalme left a comment

jrajahalme Feb 24, 2023

meyskens Feb 28, 2023

jrajahalme Feb 24, 2023

meyskens Feb 28, 2023

jrajahalme commented Feb 24, 2023

maintainer-s-little-helper bot commented Feb 28, 2023

meyskens commented Mar 3, 2023 •

edited

Loading

meyskens commented Mar 3, 2023

Adds a test case for L7 policy with TLS #1406

Adds a test case for L7 policy with TLS #1406

Conversation

meyskens commented Feb 21, 2023

meyskens commented Feb 21, 2023

ldelossa commented Feb 21, 2023

meyskens commented Feb 21, 2023

meyskens commented Feb 22, 2023 • edited Loading

tklauser left a comment

Choose a reason for hiding this comment

meyskens commented Feb 22, 2023

meyskens commented Feb 22, 2023

meyskens commented Feb 24, 2023

jrajahalme left a comment

Choose a reason for hiding this comment

jrajahalme Feb 24, 2023

Choose a reason for hiding this comment

meyskens Feb 28, 2023

Choose a reason for hiding this comment

jrajahalme Feb 24, 2023

Choose a reason for hiding this comment

meyskens Feb 28, 2023

Choose a reason for hiding this comment

jrajahalme commented Feb 24, 2023

maintainer-s-little-helper bot commented Feb 28, 2023

meyskens commented Mar 3, 2023 • edited Loading

meyskens commented Mar 3, 2023

meyskens commented Feb 22, 2023 •

edited

Loading

meyskens commented Mar 3, 2023 •

edited

Loading