Commit Graph

738 Commits

Author SHA1 Message Date
Margo Crawford 1c5da35527 Merge remote-tracking branch 'origin' into active-directory-identity-provider 2021-08-18 12:44:12 -07:00
Margo Crawford 8657b0e3e7 Cleanup new group attribute behavior and add test coverage 2021-08-18 10:11:18 -07:00
Margo Crawford 26c47d564f Make new combined sAMAccountName@domain attribute the group name
Also change default username attribute to userPrincipalName
2021-08-17 16:53:26 -07:00
Monis Khan e0901f4fe5
dynamiccert: prevent misuse of NewServingCert
The Kube API server code that we use will cast inputs in an attempt
to see if they implement optional interfaces.  This change adds a
simple wrapper struct to prevent such casts from causing us any
issues.

Signed-off-by: Monis Khan <mok@vmware.com>
2021-08-17 12:58:32 -04:00
Monis Khan 7a812ac5ed
impersonatorconfig: only unload dynamiccert when proxy is disabled
In the upstream dynamiccertificates package, we rely on two pieces
of code:

1. DynamicServingCertificateController.newTLSContent which calls
   - clientCA.CurrentCABundleContent
   - servingCert.CurrentCertKeyContent
2. unionCAContent.VerifyOptions which calls
   - unionCAContent.CurrentCABundleContent

This results in calls to our tlsServingCertDynamicCertProvider and
impersonationSigningCertProvider.  If we Unset these providers, we
subtly break these consumers.  At best this results in test slowness
and flakes while we wait for reconcile loops to converge.  At worst,
it results in actual errors during runtime.  For example, we
previously would Unset the impersonationSigningCertProvider on any
sync loop error (even a transient one caused by a network blip or
a conflict between writes from different replicas of the concierge).
This would cause us to transiently fail to issue new certificates
from the token credential require API.  It would also cause us to
transiently fail to authenticate previously issued client certs
(which results in occasional Unauthorized errors in CI).

Signed-off-by: Monis Khan <mok@vmware.com>
2021-08-16 16:07:46 -04:00
Monis Khan 4a17e1e736
impersonator: update tests for new Impersonate-Uid code
Signed-off-by: Monis Khan <mok@vmware.com>
2021-08-09 19:16:54 -04:00
Monis Khan a027f1ae2c
jwtcachefiller: update to use CAContentProvider
Signed-off-by: Monis Khan <mok@vmware.com>
2021-08-09 19:16:25 -04:00
Matt Moyer 58bbffded4
Switch to a slimmer distroless base image.
At a high level, it switches us to a distroless base container image, but that also includes several related bits:

- Add a writable /tmp but make the rest of our filesystems read-only at runtime.

- Condense our main server binaries into a single pinniped-server binary. This saves a bunch of space in
  the image due to duplicated library code. The correct behavior is dispatched based on `os.Args[0]`, and
  the `pinniped-server` binary is symlinked to `pinniped-concierge` and `pinniped-supervisor`.

- Strip debug symbols from our binaries. These aren't really useful in a distroless image anyway and all the
  normal stuff you'd expect to work, such as stack traces, still does.

- Add a separate `pinniped-concierge-kube-cert-agent` binary with "sleep" and "print" functionality instead of
  using builtin /bin/sleep and /bin/cat for the kube-cert-agent. This is split from the main server binary
  because the loading/init time of the main server binary was too large for the tiny resource footprint we
  established in our kube-cert-agent PodSpec. Using a separate binary eliminates this issue and the extra
  binary adds only around 1.5MiB of image size.

- Switch the kube-cert-agent code to use a JSON `{"tls.crt": "<b64 cert>", "tls.key": "<b64 key>"}` format.
  This is more robust to unexpected input formatting than the old code, which simply concatenated the files
  with some extra newlines and split on whitespace.

- Update integration tests that made now-invalid assumptions about the `pinniped-server` image.

Signed-off-by: Matt Moyer <moyerm@vmware.com>
2021-08-09 15:05:13 -04:00
Matt Moyer 5f679059d5
Add ClusterIP service to impersonator-config-controller informer.
Prior to this fix, this controller did not correctly react to changes to the ClusterIP service. It would still eventually react with a long delay due to our 5 minute resync interval.

Signed-off-by: Matt Moyer <moyerm@vmware.com>
2021-07-28 11:57:18 -05:00
Monis Khan 8b4ed86071
certs_expirer: be specific about what secret to delete
This change fixes a race that can occur because we have multiple
writers with no leader election lock.

1. TestAPIServingCertificateAutoCreationAndRotation/automatic
   expires the current serving certificate
2. CertsExpirerController 1 deletes expired serving certificate
3. CertsExpirerController 2 starts deletion of expired serving
   certificate but has not done so yet
4. CertsManagerController 1 creates new serving certificate
5. TestAPIServingCertificateAutoCreationAndRotation/automatic
   records the new serving certificate
6. CertsExpirerController 2 finishes deletion, and thus deletes the
   newly created serving certificate instead of the old one
7. CertsManagerController 2 creates new serving certificate
8. TestAPIServingCertificateAutoCreationAndRotation/automatic keeps
   running and eventually times out because it is expecting the
   serving certificate created by CertsManagerController 2 to match
   the value it recorded from CertsManagerController 1 (which will
   never happen since that certificate was incorrectly deleted).

Signed-off-by: Monis Khan <mok@vmware.com>
2021-07-28 09:56:05 -04:00
Matt Moyer 727035a2dc
Fix form_post CSS styling in Firefox and Safari.
This functioned fine, but did not have the intended visual appearance when it came to how the text of the auth code wrapped inside the copy button in the manual flow.

The new styling behaves correctly on at least Chrome, Firefox, and Safari on macOS.

Signed-off-by: Matt Moyer <moyerm@vmware.com>
2021-07-28 08:09:20 -05:00
Margo Crawford 474266f918 Merge branch 'main' of github.com:vmware-tanzu/pinniped into active-directory-identity-provider 2021-07-27 15:06:58 -07:00
Margo Crawford bbaa820278 parsing objectGUID as human-readable string version 2021-07-27 11:08:23 -07:00
Margo Crawford 287a5d225a Change SearchBaseFound condition success reason to be a string constant 2021-07-27 10:23:05 -07:00
Ryan Richard f17f7c0c6a Small refactors in impersonator_config.go suggested by @mattmoyer 2021-07-26 17:46:06 -07:00
Margo Crawford 53b58f65b2 Add integration test for wrong password with ldap 2021-07-26 16:32:46 -07:00
Margo Crawford cc3875f048 PR feedback 2021-07-26 16:03:12 -07:00
Margo Crawford 5d23068690 Removed a todo that was resolved 2021-07-23 13:01:41 -07:00
Margo Crawford 91085e68f9 Refactoring defaulting logic 2021-07-23 13:01:41 -07:00
Margo Crawford f99f7be836 Default values for ad usersearch and groupsearch 2021-07-23 13:01:41 -07:00
Margo Crawford 890d9c3216 resolve some todos about error handling search base discovery results 2021-07-23 13:01:41 -07:00
Margo Crawford cb0ee07b51 Fetch AD search base from defaultNamingContext when not specified 2021-07-23 13:01:41 -07:00
Margo Crawford 8e1d70562d Remove shared variables from ldap upstream observer 2021-07-23 13:01:41 -07:00
Margo Crawford 5d8d7246c2 Refactor active directory and ldap controllers to share almost everything
Signed-off-by: Ryan Richard <richardry@vmware.com>
2021-07-23 13:01:41 -07:00
Margo Crawford e5c8cbb3a4 One line fix for lint error. Forgot a period in a comment.
Signed-off-by: Ryan Richard <richardry@vmware.com>
2021-07-23 13:01:40 -07:00
Margo Crawford 7696f4256d Move defaulting of ad username and uid attributes to controller
Now the controller uses upstreamldap so there is less duplication,
since they are very similar.

Signed-off-by: Ryan Richard <richardry@vmware.com>
2021-07-23 13:01:40 -07:00
Ryan Richard aaa4861373 Custom API Group overlay for AD
Signed-off-by: Margo Crawford <margaretc@vmware.com>
2021-07-23 13:01:40 -07:00
Margo Crawford b3d0b28bd0 Integration test fixes, fixing objectGUID handling 2021-07-23 13:01:40 -07:00
Margo Crawford be6f9f83ce RBAC rules for activedirectoryidentityprovider 2021-07-23 13:01:40 -07:00
Margo Crawford 8fb35c6569 Active Directory cli options 2021-07-23 13:01:40 -07:00
Margo Crawford 3899292e89 Advertise Active Directory idps 2021-07-23 13:01:40 -07:00
Margo Crawford b06de69f6a ActiveDirectoryIdentityProvider
- Create CRD
- Create implementation of AD-specific user search defaults
2021-07-23 13:01:40 -07:00
Ryan Richard 708164b878 Carefully merge desired annotations into impersonation proxy Service
Don't overwrite annotations that might have come from a human user or
from some other non-Pinniped controller.
2021-07-22 17:09:50 -07:00
Ryan Richard 2bba39d723 TestAgentController unit test is flaky, try to add workaround
TestAgentController really runs the controller and evaluates multiple
calls to the controller's Sync with real informers caching updates.
There is a large amount of non-determinism in this unit test, and it
does not always behave the same way. Because it makes assertions about
the specific errors that should be returned by Sync, it was not
accounting for some errors that are only returned by Sync once in a
while depending on the exact (unpredictable) order of operations.

This commit doesn't fix the non-determinism in the test, but rather
tries to work around it by also allowing other (undesired but
inevitable) error messages to appear in the list of actual error
messages returned by the calls to the Sync function.

Signed-off-by: Margo Crawford <margaretc@vmware.com>
2021-07-15 13:41:31 -07:00
Matt Moyer 71d4e05fb6
Add custom response_mode=form_post HTML template.
This is a new pacakge internal/oidc/provider/formposthtml containing a number of static files embedded using the relatively recent Go "//go:embed" functionality introduced in Go 1.16 (https://blog.golang.org/go1.16).

The Javascript and CSS files are minifiied and injected to make a single self-contained HTML response. There is a special Content-Security-Policy helper to calculate hash-based script-src and style-src rules.

This new code is covered by a new integration test that exercises the JS/HTML functionality in a real browser outside of the rest of the Supervisor.

Signed-off-by: Matt Moyer <moyerm@vmware.com>
2021-07-09 12:08:43 -05:00
Matt Moyer 674cd4a88c
Adjust our securityheader pkg to support form_post.
Our Supervisor callback handler now needs to load JS and CSS from the provider endpoint, and this JS needs to make a `fetch()` call across origins (to post the form to the CLI callback). This requires a custom Content-Security-Policy compared to other pages we render.

Signed-off-by: Matt Moyer <moyerm@vmware.com>
2021-07-09 12:08:43 -05:00
Matt Moyer 2823d4d1e3
Add "response_modes_supported" to Supervisor discovery response.
Signed-off-by: Matt Moyer <moyerm@vmware.com>
2021-07-09 12:08:43 -05:00
Matt Moyer 6d83ecb420
Unit test response_mode=form_post in internal/oidc/callback.
Signed-off-by: Matt Moyer <moyerm@vmware.com>
2021-07-09 12:08:43 -05:00
Matt Moyer c27eb17f23
Add "response_mode=form_post" to CLI client.
Signed-off-by: Matt Moyer <moyerm@vmware.com>
2021-07-09 12:08:42 -05:00
Matt Moyer 9f91c6c884
Merge branch 'main' into oidc-upstream-watcher-supports-proxy 2021-07-09 07:24:52 -07:00
Ryan Richard e130da6daa Add unit test assertion for new OIDC client request timeout 2021-07-08 11:47:49 -07:00
Ryan Richard f0d120a6ca Fix broken upstream OIDC discovery timeout added in previous commit
After noticing that the upstream OIDC discovery calls can hang
indefinitely, I had tried to impose a one minute timeout on them
by giving them a timeout context. However, I hadn't noticed that the
context also gets passed into the JWKS fetching object, which gets
added to our cache and used later. Therefore the timeout context
was added to the cache and timed out while sitting in the cache,
causing later JWKS fetchers to fail.

This commit is trying again to impose a reasonable timeout on these
discovery and JWKS calls, but this time by using http.Client's Timeout
field, which is documented to be a timeout for *each* request/response
cycle, so hopefully this is a more appropriate way to impose a timeout
for this use case. The http.Client instance ends up in the cache on
the JWKS fetcher object, so the timeout should apply to each JWKS
request as well.

Requests that can hang forever are effectively a server-side resource
leak, which could theoretically be taken advantage of in a denial of
service attempt, so it would be nice to avoid having them.
2021-07-08 09:44:02 -07:00
Ryan Richard f1e63c55d4 Add `https_proxy` and `no_proxy` settings for the Supervisor
- Add new optional ytt params for the Supervisor deployment.
- When the Supervisor is making calls to an upstream OIDC provider,
  use these variables if they were provided.
- These settings are integration tested in the main CI pipeline by
  sometimes setting them on deployments in certain cases, and then
  letting the existing integration tests (e.g. TestE2EFullIntegration)
  provide the coverage, so there are no explicit changes to the
  integration tests themselves in this commit.
2021-07-07 12:50:13 -07:00
Matt Moyer 562951b77a
Merge branch 'main' into oidc-upstream-watcher-supports-proxy 2021-07-06 11:30:09 -07:00
Ryan Richard 629bf61655 Extract some trivial helpers for identical code usages 2021-06-30 15:02:14 -07:00
Guangyuan Wang 76dc39ac2d Use hostname instead of host and split on ":"
Co-authored-by: Christian Ang <angc@vmware.com>
Co-authored-by: Tyler Schultz <tschultz@vmware.com>
2021-06-28 23:03:05 +00:00
Guangyuan Wang d19d63ad7d Set Proxy on oidc upstream watcher transport
- this allows the oidc upsream watcher to honor the
HTTP_PROXY,HTTPS_PROXY,NO_PROXY environment variables

Co-authored-by: Christian Ang <angc@vmware.com>
2021-06-24 22:35:16 +00:00
Monis Khan 5ff2be973c
credentialrequest: use safer approximation for ExpirationTimestamp
We want the value of time.Now() to be calculated before the call to
IssueClientCertPEM to prevent the ExpirationTimestamp from being
later than the notAfter timestamp on the issued certificate.

Signed-off-by: Monis Khan <mok@vmware.com>
2021-06-23 11:07:00 -04:00
Monis Khan d78b845575
Fix bad test package name
Signed-off-by: Monis Khan <mok@vmware.com>
2021-06-22 11:23:19 -04:00
Matt Moyer 551249fb69
Use a custom type for our static CLI client (smaller change).
Before this change, we used the `fosite.DefaultOpenIDConnectClient{}` struct, which implements the  `fosite.Client` and `fosite.OpenIDConnectClient` interfaces. For a future change, we also need to implement some additional optional interfaces, so we can no longer use the provided default types. Instead, we now use a custom `clientregistry.Client{}` struct, which implements all the requisite interfaces and can be extended to handle the new functionality (in a future change).

There is also a new `clientregistry.StaticRegistry{}` struct, which implements the `fosite.ClientManager` and looks up our single static client. We could potentially extend this in the future with a registry backed by Kubernetes API, for example.

This should be 100% refactor, with no user-observable change.

Signed-off-by: Matt Moyer <moyerm@vmware.com>
2021-06-15 15:31:48 -05:00