[papi] OIDC service signs state with HS256, reusing signing PK - WEB-206 #17328
Conversation
easyCZ
force-pushed
the
mp/papi-oidc-jwt-rsa256
branch
from
dc793e6 to
3b75276
Compare
easyCZ
changed the title
easyCZ
force-pushed
the
mp/papi-oidc-jwt-rsa256
branch
from
793a04c to
74f4fbe
Compare
|
@easyCZ, the reason to use HS256 for state param is that it's used as a URL param and RSA256 signed JWTs are significant longer. Quick stats from JWT.io: HS256 -> ~150 bytes, RSA256 -> 500 bytes The state param is a passthrough we're sending and expect to get back. The rest of the URL is already a lengthy beast, and it may contain various IdP-specific params. We need to be sure this is not breaking the authorize requests to IdPs, or redirects back to Gitpod. |
How do we ensure this? Is there anything about this in the spec? And if not, can we use the RSA symmetric key? |
|
From a quick search, there appears to be nothing in the spec for this. It seems to be largely limited by browser URL max size, which tends to not have a limit, or be on the order of ~5k chars which we'd fit into. Yes, there's also the question whether the auth provider imposes max URL limit but I'd expect most frameworks to keep this reasonable. Given the OIDC tokens are short-lived, and basically only sign-in use, we could go with this change and should we encounter problems with a provider, we can change just the OIDC back to HS256. The benefit is that we reduce the need for another secret for now, making our config (and lives) simpler. |
It's not spec'd. After searching for that topic, it seems there are limitations per provider. You'll find only vague statements on the limitations like this. Numbers existing limits vary depending on search results, 1k, 512, 256, 100, all from different OAuth2 services. Only option to make sure we are not breaking things upfront would be to test some of the major providers (Google, Auth0, Azure AD, maybe more) and see if they all go above 512 bytes. If there is another to reduce length, it would be preferable. |
We should make sure it works with the known providers, before changing this. At least the tree IdPs mentioned above should work. BTW, testing this would be not too hard. Configure the services, prepare the authorize URL containing a lengthy random state param, and navigate the browser to this location. If it doesn't reject, it should be fine. |
In order to get rid of a second secret? Fine with that.
See, that sound easier. But rolling back if it breaks with a customer's in-house IdP service, is something we really want to avoid, especially if we are already aware of it now. |
If we want it signed (and decentralized), then a symmetric cipher is the only way. We can use the RSA private key as the symmetric key, which produces the following: Which is HS256, equivalent to the previous size. |
|
I've refactored the code above to sign with HS256, but use the RSA PK as the key. I'll push some more tests for the HS256 shortly, but see if this now looks okay. |
easyCZ
changed the title
Understood. |
|
Also updated PR description to reflect the change. |
|
Thanks @AlexTugarev for the insights and review! |
|
/unhold |
roboquat
added
deployed: webapp
Description
Related Issue(s)
How to test
Documentation
Preview status
Gitpod was successfully deployed to your preview environment.
Build Options:
Run the build with werft instead of GHA
Run Leeway with
--dont-testPublish Options
Installer Options
Add desired feature flags to the end of the line above, space separated
Preview Environment Options:
If enabled this will build
install/previewIf enabled this will create the environment on GCE infra
Valid options are
all,workspace,webapp,ide,jetbrains,vscode,ssh/hold