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

Support multiple IDPs and identity transformations on Supervisor FederationDomains #1419

Merged
merged 81 commits into from
Sep 13, 2023

Conversation

cfryanr
Copy link
Member

@cfryanr cfryanr commented Feb 13, 2023

This PR implements the proposal to support multiple identity providers and identity transformations from #1407.

Notes

The changes look larger than they really are because I also moved some pre-existing interfaces to new packages to help avoid circular package dependencies, and improved the organization of some exiting packages.

The easiest way to play with this code is to checkout this branch and then:

# Compiles and deploys onto a local Kind cluster.
./hack/prepare-for-integration-tests.sh
# Configures the Supervisor to have a FederationDomain with two working identity providers.
# Follow the directions printed to the screen by this script to perform auth using both IDPs.
./hack/prepare-supervisor-on-kind.sh --ldap --oidc

Remaining work to get this branch ready

Stabilize pre-existing integration tests

  • Finish new FederationDomain conditions, and change those integration tests which make a FedertaionDomain before making an IDP to wait for the FD to become ready after the IDP is created

Remaining production code changes needed

  • Finish all the todos in the federationdomain watcher controller
  • Add the IDP Display Name to the sub claim of the Supervisor-issued ID tokens. The sub claim is supposed to be a unique identifier, but is not necessarily unique without this update.

Add new unit tests

  • Endpoints manager test should test having different multiple IDPs in each FederationDomain
  • federation_domain_issuer_test.go could test the new getters
  • Helpers in downstream_session.go should be unit tested indirectly via tests for the callers, but maybe the new ApplyIdentityTransformations method could have its own unit test
  • Auth handler
  • Callback handler
  • Post login handler
  • Token handler

Add new integration tests

  • New integration tests in supervisor_login_tests.go for multiple IDPs and transformations/policies, including testing refresh flows
  • New integration test in e2e_test.go to exercise the whole auth experience with multiple IDPs configured
  • New integration test that covers the new status.conditions and CRD validations of FederationDomains in detail

Add docs (see "docs story" sections in proposal doc from #1407 for more details). These were added in a separate PR so it can be merged later. See #1660.

  • New docs about multiple FederationDomains
  • New docs about multiple IDPs
  • New docs about identity transformations and policies, with lots of examples and pointers for how to use CEL

Release note:

TBD

@cfryanr cfryanr marked this pull request as draft February 13, 2023 23:01
@codecov
Copy link

codecov bot commented Feb 13, 2023

Codecov Report

Merging #1419 (5573c62) into main (fee737b) will increase coverage by 3.65%.
The diff coverage is 93.96%.

@@            Coverage Diff             @@
##             main    #1419      +/-   ##
==========================================
+ Coverage   75.55%   79.21%   +3.65%     
==========================================
  Files         166      163       -3     
  Lines       15159    15757     +598     
==========================================
+ Hits        11454    12482    +1028     
+ Misses       3403     2959     -444     
- Partials      302      316      +14     
Files Changed Coverage Δ
internal/clientcertissuer/issuer.go 100.00% <ø> (ø)
internal/concierge/server/server.go 17.80% <0.00%> (ø)
internal/controller/kubecertagent/kubecertagent.go 92.70% <0.00%> (ø)
internal/controller/utils.go 0.00% <0.00%> (ø)
.../federationdomain/clientregistry/clientregistry.go 97.69% <ø> (ø)
internal/federationdomain/csp/csp.go 100.00% <ø> (ø)
internal/federationdomain/csrftoken/csrftoken.go 100.00% <ø> (ø)
internal/federationdomain/dynamiccodec/codec.go 100.00% <ø> (ø)
...iondomain/endpoints/discovery/discovery_handler.go 79.48% <ø> (ø)
...tiondomain/endpoints/jwks/dynamic_jwks_provider.go 100.00% <ø> (ø)
... and 37 more

... and 9 files with indirect coverage changes

@cfryanr cfryanr force-pushed the multiple_idps_and_transformations branch from 4134cf3 to 786197e Compare May 8, 2023 21:07
@cfryanr cfryanr force-pushed the multiple_idps_and_transformations branch 5 times, most recently from 2777f8b to 497b86a Compare June 5, 2023 22:22
Forgot to remove this in the previous commit which removed writing that
condition from the controller code.
Also try to avoid flakes by using RetryOnConflict when calling Update
on the FederationDomain.
To make the subject of the downstream ID token more unique when
there are multiple IDPs. It is possible to define two IDPs in a
FederationDomain using the same identity provider CR, in which
case the only thing that would make the subject claim different
is adding the IDP display name into the values of the subject claim.
@cfryanr cfryanr force-pushed the multiple_idps_and_transformations branch 2 times, most recently from 908f2a0 to 29eee1f Compare September 11, 2023 19:52
@cfryanr cfryanr force-pushed the multiple_idps_and_transformations branch from 29eee1f to 8faf3b0 Compare September 11, 2023 20:08
@cfryanr cfryanr force-pushed the multiple_idps_and_transformations branch from 4c2e9b2 to 84498d5 Compare September 12, 2023 16:34
@cfryanr cfryanr force-pushed the multiple_idps_and_transformations branch from 7df9702 to 2cecc17 Compare September 13, 2023 19:31
These extra timeout contexts were only in the new multiple IDPs e2e
test. Remove this possible cause of test cleanup flakes where the test
runs slow enough in CI that this timeout context has already expired
and then the cleanup function fails with context deadline exceeded
errors.
@cfryanr cfryanr merged commit 06d456f into main Sep 13, 2023
9 checks passed
@cfryanr cfryanr deleted the multiple_idps_and_transformations branch September 13, 2023 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants