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

feat: add support for public clients and disabling standard auth flow #630

Merged
merged 15 commits into from
Aug 5, 2024

Conversation

Racer159
Copy link
Contributor

@Racer159 Racer159 commented Aug 2, 2024

Description

This enables support for public clients and allows disabling the standard flow in a client to fully support the OAuth device flow.

You can see an example of how this would be implemented here: https://github.com/defenseunicorns/uds-package-sigstore/pull/19/files#diff-84cee30d5e16b64372e9b71dc11e8d2643001f0fdd881c71f76768372ed18edeR10

Related Issue

Fixes #509

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

@Racer159
Copy link
Contributor Author

Racer159 commented Aug 2, 2024

(note - though not directly required this also makes redirectUris optional since they are not needed for device flow clients and are optional in the KC api: https://www.keycloak.org/docs-api/21.1.1/rest-api/#_clientrepresentation)
image

@Racer159 Racer159 marked this pull request as draft August 2, 2024 01:00
@Racer159
Copy link
Contributor Author

Racer159 commented Aug 2, 2024

here is what this looks like for sigstore when tested in that repo:
image

@Racer159 Racer159 marked this pull request as ready for review August 2, 2024 01:11
@Racer159 Racer159 changed the title feat: add support for public clients and disabling standard flow feat: add support for public clients and disabling standard auth flow Aug 2, 2024
@bburky
Copy link
Member

bburky commented Aug 2, 2024

This allows disabling standardFlowEnabled, but doesn't seem to enable the device flow? Or how are you doing that?

publicClient is a risky but does make sense for certain scenarios like device flow. I'd kind of like to protect our usual configurations to prevent a misconfiguration of incorrectly using publicClient, but I'm not sure how to do that. If nothing else we should have a validation rule that you don't use publicClient: true with secretName, secretTemplate, secret, enableAuthserviceSelector, protocol: saml (might be meaningless on SAML, unsure).

Ideally using a public client is like a policy violation that somehow has to actually be marked as allowed. I don't think uds-operator's own policies really map well to this though. If Lula(?) was ready, I guess we could use it to detect all Packages with public clients.

For the device flow, I think redirectUris and webOrigins don't matter/aren't used? I think they're always ignored if standardFlowEnabled: false? I just want to make sure that there's no way to use the normal flow on the client intended for device flow, if we need to throw an error on setting redirectUris and webOrigins with standardFlowEnabled: false too.

@Racer159
Copy link
Contributor Author

Racer159 commented Aug 2, 2024

Enabling the device flow is an attribute you add to the client: https://github.com/defenseunicorns/uds-package-sigstore/pull/19/files#diff-84cee30d5e16b64372e9b71dc11e8d2643001f0fdd881c71f76768372ed18edeR13

The redirectUris, post logout redirectUris, and webOrigins are disabled when standard flow is disabled.

Screenshot from 2024-08-02 11-17-07
Screenshot from 2024-08-02 11-17-24

@Racer159
Copy link
Contributor Author

Racer159 commented Aug 2, 2024

We could disallow a public client for anything but device flow attributed standardAuthFlow disabled clients

@Racer159 Racer159 requested review from bburky and mjnagel August 3, 2024 18:58
bburky
bburky previously approved these changes Aug 5, 2024
Copy link
Member

@bburky bburky left a comment

Choose a reason for hiding this comment

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

This is fine I guess, the allowed configurations are limited enough that it should never be possible to use with a web client.

@Racer159 Racer159 requested a review from a team as a code owner August 5, 2024 19:06
mjnagel
mjnagel previously approved these changes Aug 5, 2024
rjferguson21
rjferguson21 previously approved these changes Aug 5, 2024
@Racer159 Racer159 dismissed stale reviews from rjferguson21 and mjnagel via 0bc8e8e August 5, 2024 19:50
@mjnagel mjnagel enabled auto-merge (squash) August 5, 2024 21:05
@mjnagel mjnagel merged commit 38151d7 into main Aug 5, 2024
11 checks passed
@mjnagel mjnagel deleted the 509-public-client-device-flow branch August 5, 2024 21:26
mjnagel pushed a commit that referenced this pull request Aug 6, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.25.1](v0.25.0...v0.25.1)
(2024-08-06)


### Bug Fixes

* switch metrics-server to optional everywhere
([#641](#641))
([43c5bd5](43c5bd5))


### Miscellaneous

* add debug logs for istio injection logic
([#602](#602))
([9075436](9075436))
* add support for public clients and disabling standard auth flow
([#630](#630))
([38151d7](38151d7))
* **deps:** update dependency defenseunicorns/uds-common to v0.11.0
([#617](#617))
([997cf37](997cf37))
* **deps:** update dependency weaveworks/eksctl to v0.188.0
([#623](#623))
([3081044](3081044))
* **deps:** update uds to v0.14.0
([#612](#612))
([7fe927e](7fe927e))
* update codeowners
([#637](#637))
([eec5017](eec5017))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keycloak support for public clients and device flow (enables sigstore)
4 participants