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

Token parsing error with Logto OIDC integration #8

Open
fullstackdesign-xyz opened this issue Nov 26, 2024 · 6 comments
Open

Token parsing error with Logto OIDC integration #8

fullstackdesign-xyz opened this issue Nov 26, 2024 · 6 comments

Comments

@fullstackdesign-xyz
Copy link

fullstackdesign-xyz commented Nov 26, 2024

Description

When attempting to use OIDCWarden with Logto as the OIDC provider, the token endpoint fails to parse the response with the following error:

Failed to contact token endpoint: Parse(Error { path: Path { segments: [] }, original: Error("Failed to parse payload JSON: Error("invalid type: null, expected a string", line: 1, column: 371)", line: 1, column: 878) }

The token response appears to be valid JSON containing access_token, expires_in, and id_token fields, but the parser is expecting a string where it's finding a null value.

Environment

  • OIDCWarden version: latest (v2024.10.2-3)
  • OIDC Provider: Logto - https://github.com/logto-io/logto
  • OIDC Redirect URIs: https://MY_OIDCWARDEN.COM/identity/connect/oidc-signin
  • Deployment method: Docker (timshel/oidcwarden:latest)

Configuration

SSO_ENABLED=true
SSO_AUTHORITY=https://MY_LOGTO.com/oidc
SSO_SCOPES=openid profile email offline_access
SSO_PKCE=false
SSO_ALLOW_UNKNOWN_EMAIL_VERIFICATION=true
SSO_DEBUG_TOKENS=true

Steps to reproduce

  • Configure OIDCWarden with Logto as the OIDC provider
  • Attempt to log in via SSO
  • Authentication with Logto succeeds
  • Token exchange fails with parsing error

Related information

This appears to be similar to the Auth0 timestamp parsing issue mentioned in the documentation that requires the accept-rfc3339-timestamps feature flag.

@Timshel
Copy link
Owner

Timshel commented Nov 26, 2024

Hey,
Not sure about the segments path mentioned I'll have to test it.

@fullstackdesign-xyz
Copy link
Author

Good news 🎉 I believe I found the issue and I'm running one final verification test to confirm. Will update with the complete details once confirmed, including steps to reproduce and fix.

In the meantime, can you clarify whether you prefer issues and PRs to be submitted to Timshel/OIDCWarden or Timshel/vaultwarden repository?

@Timshel
Copy link
Owner

Timshel commented Nov 27, 2024

Both project are updated at the moment, so you can submit to whichever you are using.

I'll propagate the fix to the other one :).

fullstackdesign-xyz added a commit to fullstackdesign-xyz/OIDCWarden that referenced this issue Nov 28, 2024
…Claims.

Related to issue: Token parsing error with Logto OIDC integration Timshel#8
@Timshel
Copy link
Owner

Timshel commented Nov 28, 2024

Hey I can see you started to modify the LoginJwtClaims but I'm not sure it will change the situation much.
This part is only used between Vaultwarden and the clients not for SSO.

The claims used during the SSO flow are handled by https://github.com/ramosbugs/openidconnect-rs which is quite strict in it's handling (which is probably the source of the error).

@fullstackdesign-xyz
Copy link
Author

Yes, you're right. When testing with Logto, the SSO login works when the user has a profile picture URL set but fails when the picture field is null. Upon closer inspection, I've realised that this error occurs during the token exchange with the OIDC provider, which is handled by the openidconnect-rs crate, rather than during our internal JWT handling.

Given that openidconnect-rs is quite strict in its parsing, do you have any suggestions on how we can accommodate null values for optional fields like picture within its framework?

@Timshel
Copy link
Owner

Timshel commented Nov 29, 2024

I believe the source of strictness from the lib is that the open-id spec mention that null value should not be included: cf ramosbugs/openidconnect-rs#184 (comment)

But he does mention that he intends to make it more lax, and I believe he has made it so, but it will probably be only included in the next major 4.0.0 release.

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

No branches or pull requests

2 participants