-
Notifications
You must be signed in to change notification settings - Fork 66
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
Pinniped server CORS headers missing #2132
Comments
Hi @exolicious, thanks for creating this issue. As you noticed, currently the Supervisor doesn't support single page apps (SPAs). Sorry if the doc did not make that clear. It currently assumes that your webapp has a backend, and that the backend is what will redeem the authcodes at the token endpoint. It still requires PKCE for these apps as well, because it is considered the current best practice for OAuth2. We could take a look at what would be required to also support SPAs. |
@cfryanr Thank you very much for answering so quickly. We would really appreciate if pinniped supported SPAs, since we would like to have a fully state/sessionless backend if possible. |
@cfryanr I looked at the code and this is a quick solution: For example: I can create a PR for a more sophisticated solution tomorrow, if you want. |
Hi @exolicious, thanks for thinking about which CORS headers would be needed. We would need to take care to only apply CORS headers to the appropriate endpoints and only for configured origins. That implies that there would need to be some way to configure the origins. I'm not sure if that should be configured per- |
@cfryanr So for our use case (and I know as a maintainer you want the generic solutions), the cors headers would be enough. Do you think we should fork, until you guys have had time to look at (full fledged) SPA support? |
Hi @exolicious, if your app has a backend then it could be possible to make it work. The backend could have an endpoint which is responsible for receiving the callback (via browser redirect) from the Supervisor and calling the token endpoint to exchange the authcode for tokens. If you need to avoid having session state in the backend, then perhaps you could use some workaround in which that endpoint transmits the tokens to the frontend via the response. Then when a refresh is needed, the frontend could send the refresh token to an endpoint on your backend, which could call the Supervisor's token endpoint to perform the refresh and get all new tokens, and then send them back to the frontend. Keeping tokens on the frontend may not be a best practice, but it's possible. If you're concerned then you could encrypt them using a key known only by your backend, for example. Could that help, as a workaround? |
This is exactly the workaround I implemented and tried to describe in my previous comment, with the only missing link being the CORS headers (since in my current setup the browser initiates the auth code flow). How does Vsphere handle all this? I would assume it does something similar to retrieve TKG related workloads? And thank you very much for taking the time to respond, i really appreciate you being so active and developing this awesome and technically intricate system!! |
What happened?
I want to configure pinniped-oidc for a webapp according to this guide: https://pinniped.dev/docs/howto/configure-auth-for-webapps/
It says: "Clients must use PKCE during the authorization code flow."
However the supervisor does not seem to set CORS related Headers.
Setting them on an httpproxy (contour) is also not an option, since tlspassthrough is recomended.
What did you expect to happen?
I expect the CORS headers to be set on the response, since the guide's title is "Using the Pinniped Supervisor to provide authentication for web applications" and it says "Clients must use PKCE during the authorization code flow."
What is the simplest way to reproduce this behavior?
Connect any SPA as an oidcclient in pinniped supervisor and try to access the wellknown endpoint through a js-fetch.
In what environment did you see this bug?
(other stuff is not relevant)
What else is there to know about this bug?
The text was updated successfully, but these errors were encountered: