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.

Sign up for GitHub

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

Jump to bottom

Policy: Add TLS SNI support #22398

Merged
merged 2 commits into from
Dec 2, 2022
Merged

Policy: Add TLS SNI support #22398

merged 2 commits into from
Dec 2, 2022

Conversation

jrajahalme
Copy link
Member

@jrajahalme jrajahalme commented Nov 28, 2022 •
edited

Add new serverNames field to Cilium Network Policy to restrict the allowed TLS SNIs. This is implemented as an Envoy redirect, but does not need TLS termination, as TLS SNI extension is in clear in the Client Hello message.

As a follow-up, a test case will be added to https://github.com/cilium/cilium-cli

CiliumNetworkPolicy now supports enforcement of SNI in TLS connections.

@jrajahalme jrajahalme added kind/feature This introduces new functionality. release-note/major This PR introduces major new functionality to Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. labels Nov 28, 2022
@jrajahalme jrajahalme requested review from a team as code owners November 28, 2022 16:56
@jrajahalme jrajahalme mentioned this pull request Nov 28, 2022
Merged
@jrajahalme
Copy link
Member Author

/test

nebril
nebril requested changes Nov 29, 2022
View reviewed changes
Copy link
Member

@nebril nebril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool stuff!

I would really like to see new rule validation logic unit tested, other comments are nitpicks.

pkg/envoy/server.go Show resolved Hide resolved
pkg/envoy/server.go Outdated Show resolved Hide resolved
pkg/policy/api/rule_validation.go Outdated Show resolved Hide resolved
@jrajahalme
Copy link
Member Author

@nebril Added the unit tests :-)

@jrajahalme
Copy link
Member Author

jrajahalme commented Nov 30, 2022 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.24-kernel-5.4' failed:

Click to show.

Test Name

K8sDatapathConfig Iptables Skip conntrack for pod traffic

Failure Output

FAIL: Found 1 k8s-app=cilium logs matching list of errors that must be investigated:

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.24-kernel-5.4 so I can create one.

nathanjsweet
nathanjsweet requested changes Nov 30, 2022
View reviewed changes
Copy link
Member

@nathanjsweet nathanjsweet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nits.

pkg/policy/api/rule_validation.go Outdated Show resolved Hide resolved
pkg/policy/api/rule_validation.go Outdated Show resolved Hide resolved
@jrajahalme
Copy link
Member Author

Runtime test hit an unrelated flake: #22470

@jrajahalme
Copy link
Member Author

test-1.24-5.4 failed due to an error in Cilium agent logs. Issued PR #22474 to fix that.

@jrajahalme
Copy link
Member Author

/test

@jrajahalme
Copy link
Member Author

/test-runtime

@jrajahalme
Copy link
Member Author

/ci-external-workloads

@jrajahalme
docker: Update Envoy image
81b1c13 
Use Envoy image with Cilium TLS wrapper fallback to non-TLS socket when
policy has no TLS context.

This allows SNI policies to work without TLS termination and/or origination.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
policy: Add serverNames to L4 policy
564825e 
Add a list of allowed server names (TLS SNI) to L4 policy.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
Copy link
Member Author

Rebased due to CRD schema version bump on master.

@jrajahalme
Copy link
Member Author

jrajahalme commented Dec 1, 2022 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.25-kernel-4.19' hit: #22019 (94.82% similarity)

@maintainer-s-little-helper maintainer-s-little-helper bot mentioned this pull request Dec 1, 2022
Closed
@jrajahalme
Copy link
Member Author

  • Travis CI fails with a build error for amd64 and with an apt-get error for arm64
  • ci-external-workloads is broken due to cilium-cli version check hassle (unrelated).
  • test-1.16-4.9 failed provisioning a VM (is this related of ending support for 4.9?)
  • test-1.25-4.19 hit known flake CI: K8sDatapathConfig Iptables Skip conntrack for pod traffic #22019

@joestringer
Copy link
Member

test-1.16-4.9 failed provisioning a VM (is this related of ending support for 4.9?)

If you click on Pipeline Steps from that job, then scroll down until you see the red icon with an exclamation mark, click the console icon next to it, then check the provisioning failure, there is some apt failures there too. I think that Ubuntu had an outage around the time you kicked off CI for this PR.

@jrajahalme
Copy link
Member Author

/test-1.16-4.9

@jrajahalme jrajahalme added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 2, 2022
@jrajahalme
Copy link
Member Author

Both fails on required checks are know flakes:

@aanm aanm merged commit 79c6f57 into master Dec 2, 2022
@aanm aanm deleted the policy-add-sni-support branch December 2, 2022 12:48
jrajahalme added a commit to jrajahalme/cilium that referenced this pull request Dec 12, 2022
@jrajahalme
policy: Fix PerSelectorPolicy.IsEmpty
f024516 
Empty PerSelectorPolicy can not have any SNIs.

Modify existing unit test to have a case with SNI without any L7 rules
that would have catched this error.

Fixes: cilium#22398
Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme jrajahalme mentioned this pull request Dec 12, 2022
Closed
@joestringer joestringer mentioned this pull request Dec 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@nebril nebril nebril approved these changes

@aanm aanm aanm approved these changes

Assignees
No one assigned
Labels
kind/feature This introduces new functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/major This PR introduces major new functionality to Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jrajahalme @joestringer @nathanjsweet @nebril @aanm