Service mesh: add mTLS auth method #24263

meyskens · 2023-03-09T13:51:38Z

This adds an mTLS auth handler to the Serice Mesh auth package.
It will listen on a given port and does a mutual TLS handshake with
SPIFFE IDs it received. This will assure the both sides got the needed
certificates.

In order to integrate with the datapath tables it also improves the SPIFFE
interface to use the Cilium Numeric Identities. And convert them from and
to valid SNI fields. As well as implement code to validate the URI SANS
inside the certificates.

This can be enabled in the in the Helm chart.

Fixes: #23807

Add mtls-spiffe as auth mode in the CiliumNetworkPolicy

meyskens · 2023-03-09T14:03:50Z

How to test this (I know you cannot wait ;) ):

Enable it via Helm:

auth:
  mTLS:
    enabled: true

Install SPIRE: https://github.com/meyskens/cilium-spiffe-poc/tree/meyskens/cilium-mtls (make install)

Deploy a policy to use auth, I used the connectivity test pods for this

apiVersion: "cilium.io/v2"
kind: CiliumNetworkPolicy
metadata:
  name: auth-egress
  namespace: cilium-test
spec:
  endpointSelector:
    matchLabels:
      kind: client
  egress:
    - toEndpoints:
        - matchLabels:
            kind: echo
      toPorts:
        - ports:
            - port: "8080"
              protocol: TCP
      auth:
        type: "mtls-spiffe"
    - toEndpoints:
      - matchLabels:
          "k8s:io.kubernetes.pod.namespace": "kube-system"
          "k8s:k8s-app": "kube-dns"
      toPorts:
        - ports:
            - port: "53"
              protocol: ANY
          rules:
            dns:
              - matchPattern: "*"

Then set up a connection between two pods and scan the logs of the Cilium agent for logs like

level=debug msg="auth: egress policy is requiring authentication type mtls-spiffe for identity 30086->41044, endpoint 10.244.1.148->10.244.0.130" subsys=auth

Create a SPIFFE ID for the identities:

kubectl exec -n spire spire-server-0 -- \
		/opt/spire/bin/spire-server entry create \
		-spiffeID spiffe://spiffe.cilium.io/cilium-id/ENTER-ID-HERE \
		-parentID spiffe://spiffe.cilium.io/dclient \
		-selector cilium:mtls

(this will not be needed once #23802 is built)

Try the connection again and enjoy a brand new mTLS experience <3

christarazi

Nice work!

One major concern I have is that I don't see any tests. I realize from reviewing that this seems to be more of integration code so unit testing probably wasn't obvious. However, I can imagine a test that can spin off two TLS clients / servers and validate the code paths without too much complication.

Otherwise, I only have code nits.

pkg/auth/mtls_authhandler.go

pkg/auth/spire/certificate_provider.go

pkg/auth/spire/delegate.go

pkg/auth/mtls_authhandler.go

meyskens · 2023-03-09T21:36:23Z

@christarazi i agree on the testing side, I have been thinking about this for a bit, it would be good to have it covered with testing on some kind of integration test in the package between the mTLS and SPIFFE package. Need to discuss however if we make it part of this PR or a separate work item.

youngnick

Amazing work @meyskens! I'm really impressed with how quickly you've pulled functional code together.

However, I'm in a similar boat to @christarazi here, the changes look great, but I'd like to see unit tests, particularly for mtls_authhandler.go and certificate_provider.go. I think just tests that exercise the code paths inside the if statements are a great place to start, even for the internal functions. Let's lock in tests while this is a small change, and then we can have greater confidence as the changesets and assoicated testing get bigger.

kaworu

Thanks for the update @meyskens!

Couple of comments about context handling and locking, but overall LGTM.

pkg/auth/mtls_authhandler.go

pkg/auth/spire/delegate.go

mhofstetter

looks great 🎉 just some small suggestions

pkg/auth/certs/provider.go

pkg/auth/mtls_authhandler.go

jrajahalme

Looks great to me!

nathanjsweet

LGTM for API changes

pkg/auth/mtls_authhandler.go

kaworu

Thanks @meyskens! Couple of nitpicking comments but the patch LGTM 🎉

pkg/auth/mtls_authhandler.go

kaworu · 2023-03-15T13:20:08Z

@nathanjsweet question: will the socket mount shared between the spire agent and cilium agent be a problem in the context of OpenShift?

meyskens · 2023-03-15T14:53:32Z

The SPIRE installation finalization will be part of #23806
For now we have this socket setup but for some clusters it might be that we need to change this with a more complex setup. It isn't yet final

christarazi · 2023-03-15T19:43:14Z

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.19' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: migrate-svc restart count values do not match

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

This adds an mTLS auth handler to the Serice Mesh auth package. It will listen on a given port and does a mutual TLS handshake with SPIFFE IDs it received. This will assure the both sides got the needed certificates. In order to integrate with the datapath tables it also improves the SPIFFE interface to use the Cilium Numeric Identities. And convert them from and to valid SNI fields. As well as implement code to validate the URI SANS inside the certificates. Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>

Removing the debuig line that tells which SVID it received. The delegate API will send the state of the world on every sync. In very large deployments this will make a lot of debug logs. Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>

This adds the ability to enable mtls-spiffe support in the Helm chart, It will set the required config flags to the defaults as wel as mount the spire socket to the agent pods. Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>

meyskens · 2023-03-16T09:37:51Z

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.19' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: migrate-svc restart count values do not match

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

youngnick

LGTM, nice work @meyskens !

meyskens · 2023-03-17T07:22:48Z

CI fails seem to be a flake or not related to this PR

sayboras · 2023-03-20T07:08:25Z

test-1.16-4.19 failure should be fixed as part of #24336

test-1.24-5.4 failure is related to new test, which could be a flake #22950.

sayboras · 2023-03-20T07:09:03Z

/mlh new-flake Cilium-PR-K8s-1.24-kernel-5.4

👍 created #24453

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 9, 2023

meyskens force-pushed the mtls-spiffe branch 2 times, most recently from aecb3fb to bde87f1 Compare March 9, 2023 13:58

meyskens added kind/feature This introduces new functionality. release-note/major This PR introduces major new functionality to Cilium. area/servicemesh GH issues or PRs regarding servicemesh labels Mar 9, 2023

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 9, 2023

meyskens marked this pull request as ready for review March 9, 2023 14:05

meyskens requested review from a team as code owners March 9, 2023 14:05

meyskens requested review from nathanjsweet, christarazi, youngnick and kaworu March 9, 2023 14:05

meyskens force-pushed the mtls-spiffe branch 5 times, most recently from bd51500 to 57d3468 Compare March 9, 2023 15:35

meyskens requested a review from a team as a code owner March 9, 2023 15:35

meyskens requested a review from qmonnet March 9, 2023 15:35

christarazi reviewed Mar 9, 2023

View reviewed changes

meyskens force-pushed the mtls-spiffe branch from 57d3468 to 8b483f7 Compare March 9, 2023 21:49

youngnick reviewed Mar 10, 2023

View reviewed changes

meyskens force-pushed the mtls-spiffe branch from 8b483f7 to c51497a Compare March 10, 2023 13:23

meyskens force-pushed the mtls-spiffe branch from 9446995 to 92e473e Compare March 13, 2023 14:15

meyskens requested a review from kaworu March 13, 2023 14:18

kaworu requested changes Mar 13, 2023

View reviewed changes

pkg/auth/mtls_authhandler.go Outdated Show resolved Hide resolved

pkg/auth/mtls_authhandler.go Outdated Show resolved Hide resolved

pkg/auth/spire/delegate.go Show resolved Hide resolved

meyskens force-pushed the mtls-spiffe branch from b97edff to 704af36 Compare March 14, 2023 09:20

meyskens requested review from kaworu and mhofstetter March 14, 2023 13:20

mhofstetter reviewed Mar 14, 2023

View reviewed changes

pkg/auth/certs/provider.go Outdated Show resolved Hide resolved

pkg/auth/mtls_authhandler.go Outdated Show resolved Hide resolved

jrajahalme approved these changes Mar 14, 2023

View reviewed changes

meyskens force-pushed the mtls-spiffe branch from 704af36 to 3d7a4d0 Compare March 14, 2023 15:42

mhofstetter approved these changes Mar 14, 2023

View reviewed changes

nathanjsweet approved these changes Mar 14, 2023

View reviewed changes

christarazi reviewed Mar 14, 2023

View reviewed changes

pkg/auth/mtls_authhandler.go Show resolved Hide resolved

kaworu approved these changes Mar 15, 2023

View reviewed changes

pkg/auth/mtls_authhandler.go Outdated Show resolved Hide resolved

pkg/auth/mtls_authhandler.go Show resolved Hide resolved

christarazi approved these changes Mar 15, 2023

View reviewed changes

meyskens added 3 commits March 16, 2023 10:29

Add mTLS auth to the Helm chart

d699086

This adds the ability to enable mtls-spiffe support in the Helm chart, It will set the required config flags to the defaults as wel as mount the spire socket to the agent pods. Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>

meyskens force-pushed the mtls-spiffe branch from 3d7a4d0 to d699086 Compare March 16, 2023 09:29

youngnick approved these changes Mar 17, 2023

View reviewed changes

sayboras added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 20, 2023

aditighag merged commit 971ec32 into cilium:master Mar 20, 2023
41 checks passed

meyskens deleted the mtls-spiffe branch March 21, 2023 07:48

lizrice mentioned this pull request Apr 13, 2023

Confusing name for CNP authentication type #24867

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Service mesh: add mTLS auth method #24263

Service mesh: add mTLS auth method #24263

meyskens commented Mar 9, 2023

meyskens commented Mar 9, 2023

christarazi left a comment

meyskens commented Mar 9, 2023

youngnick left a comment

kaworu left a comment

mhofstetter left a comment

jrajahalme left a comment

nathanjsweet left a comment

kaworu left a comment

kaworu commented Mar 15, 2023

meyskens commented Mar 15, 2023

christarazi commented Mar 15, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

meyskens commented Mar 16, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

youngnick left a comment

meyskens commented Mar 17, 2023

sayboras commented Mar 20, 2023

sayboras commented Mar 20, 2023 •

edited by maintainer-s-little-helper bot

Service mesh: add mTLS auth method #24263

Service mesh: add mTLS auth method #24263

Conversation

meyskens commented Mar 9, 2023

meyskens commented Mar 9, 2023

christarazi left a comment

Choose a reason for hiding this comment

meyskens commented Mar 9, 2023

youngnick left a comment

Choose a reason for hiding this comment

kaworu left a comment

Choose a reason for hiding this comment

mhofstetter left a comment

Choose a reason for hiding this comment

jrajahalme left a comment

Choose a reason for hiding this comment

nathanjsweet left a comment

Choose a reason for hiding this comment

kaworu left a comment

Choose a reason for hiding this comment

kaworu commented Mar 15, 2023

meyskens commented Mar 15, 2023

christarazi commented Mar 15, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

meyskens commented Mar 16, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

youngnick left a comment

Choose a reason for hiding this comment

meyskens commented Mar 17, 2023

sayboras commented Mar 20, 2023

sayboras commented Mar 20, 2023 • edited by maintainer-s-little-helper bot

christarazi commented Mar 15, 2023 •

edited by maintainer-s-little-helper bot

meyskens commented Mar 16, 2023 •

edited by maintainer-s-little-helper bot

sayboras commented Mar 20, 2023 •

edited by maintainer-s-little-helper bot