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.

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

Add SubscribeToRotatedIdentities interface #24300

Merged
merged 1 commit into from Apr 25, 2023

Conversation

meyskens
Copy link
Member

@meyskens meyskens commented Mar 10, 2023

This change adds a SubscribeToRotatedIdentities function.
This gives a channel which is used to pass identity updates
back from the certificate proider to the auth manager.
In the auth manager there can better be acted upon to
receive the IDs and re-trigger a mTLS handshake if needed.

Add a mechanism for the SPIRE server to signal rotated certificates for re-authenticating connections

@maintainer-s-little-helper 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 10, 2023
@meyskens meyskens force-pushed the mtls-signal-rotation branch 2 times, most recently from 287cf26 to b828145 Compare March 27, 2023 09:15
@meyskens meyskens added kind/feature This introduces new functionality. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/servicemesh GH issues or PRs regarding servicemesh labels Mar 27, 2023
@maintainer-s-little-helper 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 27, 2023
@meyskens meyskens force-pushed the mtls-signal-rotation branch 2 times, most recently from 1342cf0 to 41eb691 Compare March 27, 2023 09:50
@meyskens meyskens marked this pull request as ready for review March 27, 2023 11:15
@meyskens meyskens requested a review from a team as a code owner March 27, 2023 11:15
Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

LGTM with one non-blocking comment about cilium-id.

pkg/auth/spire/certificate_provider.go Outdated Show resolved Hide resolved
Copy link
Member

@mhofstetter mhofstetter left a comment

Choose a reason for hiding this comment

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

looks good, some minor suggestions

Documentation/cmdref/cilium-agent.md Outdated Show resolved Hide resolved
pkg/auth/manager.go Show resolved Hide resolved
pkg/auth/spire/delegate.go Outdated Show resolved Hide resolved
@meyskens
Copy link
Member Author

meyskens commented Mar 29, 2023

/test

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

Click to show.

Test Name

K8sDatapathServicesTest Checks E/W loadbalancing (ClusterIP, NodePort from inside cluster, etc) Tests NodePort inside cluster (kube-proxy) with IPSec and externalTrafficPolicy=Local

Failure Output

FAIL: Request from k8s1 to service http://[fd04::11]:30152 failed

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

@meyskens meyskens added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 29, 2023
@julianwiedmann
Copy link
Member

Let's wait for tests to complete ⏳. mlh will set the label automatically if all is green.

@julianwiedmann julianwiedmann removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 29, 2023
@sayboras sayboras added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Apr 1, 2023
@youngnick
Copy link
Contributor

Seems like this is currently blocked by #24471.

@meyskens
Copy link
Member Author

Updated to latest changes on master

@meyskens
Copy link
Member Author

/test

Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

LGTM ✔️

@sayboras
Copy link
Member

integration test failures seem related to this change

--- FAIL: TestSpireDelegateClient_GetCertificateForIdentity (0.01s)
                                                                   
    --- FAIL: TestSpireDelegateClient_GetCertificateForIdentity/get_certificate_for_numeric_identity (0.00s)
                                                                                                            
    certificate_provider_test.go:391: SpireDelegateClient.GetCertificateForIdentity() error = no SPIFFE ID for spiffe://test.cilium.io/identity/1234, wantErr false
────────────────────────────────────────────────────────────────────────────────────────────────
                                                                                                
--- FAIL: TestSpireDelegateClient_SubscribeToRotatedIdentities (0.00s)
                                                                      
    --- FAIL: TestSpireDelegateClient_SubscribeToRotatedIdentities/receive_1_updated_event (0.00s)
                                                                                                  
level=error msg="failed to convert spiffe ID spiffe://test.cilium.io/cilium-id/1234 to numeric identity" error="SPIFFE ID spiffe://test.cilium.io/cilium-id/1234 does not belong to our trust domain or is not in the valid format" subsys=spire
    certificate_provider_test.go:486: SpireDelegateClient.SubscribeToRotatedIdentities() = [], want [{1234}]
────────────────────────────────────────────────────────────────────────────────────────────────
                                                                                                
--- FAIL: TestSpireDelegateClient_ValidateIdentity (0.00s)
                                                          
    --- FAIL: TestSpireDelegateClient_ValidateIdentity/validate_with_correct_SPIFFE_ID (0.00s)
                                                                                              
    certificate_provider_test.go:219: SpireDelegateClient.ValidateIdentity() = false, want true
────────────────────────────────────────────────────────────────────────────────────────────────
                                                                                                
--- FAIL: TestSpireDelegateClient_sniToSPIFFEID (0.00s)
                                                       
    --- FAIL: TestSpireDelegateClient_sniToSPIFFEID/convert_valid_numeric_identity (0.00s)

@meyskens
Copy link
Member Author

Rebased with fixes to test

@meyskens
Copy link
Member Author

/test

This change adds a SubscribeToRotatedIdentities function.
This gives a channel which is used to pass identity updates
back from the certificate proider to the auth manager.
In the auth manager there can better be acted upon to
receive the IDs and re-trigger a mTLS handshake if needed.

Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>
@meyskens
Copy link
Member Author

/test

@meyskens
Copy link
Member Author

meyskens commented Apr 25, 2023

rest-runtime fails due to network issue

docker: Error response from daemon: Get "[https://registry-1.docker.io/v2/":](https://registry-1.docker.io/v2/) read tcp 10.0.2.15:33340->52.1.184.176:443: read: connection reset by peer.

@meyskens
Copy link
Member Author

/test-runtime

@maintainer-s-little-helper 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 Apr 25, 2023
@ti-mo ti-mo merged commit d42f4af into cilium:main Apr 25, 2023
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/servicemesh GH issues or PRs regarding servicemesh dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. 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/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants