-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
OIDC login not work #30797
Comments
/cc @pedroigor (oidc), @sberyozkin (oidc) |
@jie-huang There was a bit of a problem in 2.16.0 related to the loop and the state cookie being same site strict.
|
Hello @sberyozkin With version 2.15.3.Final I am facing no login issues. However, with version 2.16.1.Final the OIDC login is not working anymore. It fails with following message in the browser (chrome):
|
@sberyozkin Thanks for your quick response.
In Firefox console,
It just keep repeat the three URLs util Firefox decide to stop. In server console, it repeats below
Based on firefox request details, when sending request to /test/login, there is no cookie 'q_session' is sent to quarkus, even the previous login-callback response has cookie 'q_session' with httpOnly:true, path: "/", samesite: "Strict". So, the cookie setting has problem so browser does not send it back. That makes quarkus to re-do the auth loop. |
@jie-huang Thanks, I think I know what might be going on, it must be to do with
So you first try to access It's proving to be a very unfortunate update I did. We had good intentions, tests pass, simple demos pass in quickstarts. But it is just causes too many side-effects, any complex application redirecting a lot will now be losing it. So I have no choice but default all OIDC cookies to same site lax - it will remain configurable for the session cookie which will work with simple cases. For now the workaround remains the same,
|
CC @gastaldi @pmlopes Hi, I'll have to now follow #30722 with making the session cookie same site lax as well, otherwise we will be overwhelmed with bug reports. |
@sberyozkin Thanks for the explain. BTW, how about version 2.13.7, the error is 404 "cannot find /oidc/login-callback" even the config can fix it. But the problem seems different. |
@jie-huang Thanks
Right, but the session cookie in your demo is same site, strict must work, the fact that you have
this might be caused by some misconfiguration, this path can be a virtual path, not declared on any endpoint, but as far as OIDC is concerned it has to be recognized as requiring the protection, which is done for such paths with HTTP policy, so if something does not match then it will skip processing it and the request will flow to the JAX-RS chain with 404 to follow. Something like that. If you have identical configurations between 2.13.7 and 2.16.1 with this same site lax config being the only difference and it works in 2.16.1, then it means something has been fixed by now... |
@jie-huang I think if you did not have a custom callback path introduced then the redirect from OIDC would go directly to |
I just wrote a small app to test the sameSite=Strict behavior and test locally, both a.com and b.com are localhost. b.com:8080/d (set cookie 'b'='b') then redirect to b.com:8080/c
So, it seems different browsers handle the sameSite redirect differently when the beginning non-redirect request coming from another domain. That is the case here. All the requests are coming from IdP (a.com in the example) with redirect back to quarkus oidc (b.com in the example), then, quarkus idc redirect to another quarkus url (b.com). How many redirect inside b.com does not matter. The only matter one is, the first non-redirect request is from b.com or a.com. |
Thanks for this analysis, @jie-huang. Right, what it does confirm is that it becomes fairly unpredictable, and it will not something I'd like |
This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [com.diffplug.spotless:spotless-maven-plugin](https://github.com/diffplug/spotless) | build | minor | `2.32.0` -> `2.33.0` | | [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `2.16.1.Final` -> `2.16.2.Final` | | [io.quarkus:quarkus-universe-bom](https://github.com/quarkusio/quarkus-platform) | import | patch | `2.16.1.Final` -> `2.16.2.Final` | --- ### Release Notes <details> <summary>diffplug/spotless</summary> ### [`v2.33.0`](https://github.com/diffplug/spotless/blob/HEAD/CHANGES.md#​2330---2023-01-26) ##### Added - `ProcessRunner` has added some convenience methods so it can be used for maven testing. ([#​1496](diffplug/spotless#1496)) - `ProcessRunner` allows to limit captured output to a certain number of bytes. ([#​1511](diffplug/spotless#1511)) - `ProcessRunner` is now capable of handling long-running tasks where waiting for exit is delegated to the caller. ([#​1511](diffplug/spotless#1511)) - Allow to specify node executable for node-based formatters using `nodeExecutable` parameter ([#​1500](diffplug/spotless#1500)) ##### Fixed - The default list of type annotations used by `formatAnnotations` has had 8 more annotations from the Checker Framework added [#​1494](diffplug/spotless#1494) ##### Changes - **POTENTIALLY BREAKING** Bump minimum JRE from 8 to 11, next release likely to bump bytecode to Java 11 ([#​1514](diffplug/spotless#1514) part 1 of [#​1337](diffplug/spotless#1337)) - Rename `YamlJacksonStep` into `JacksonYamlStep` while normalizing Jackson usage ([#​1492](diffplug/spotless#1492)) - Convert `gson` integration to use a compile-only source set ([#​1510](diffplug/spotless#1510)). - \*\* POTENTIALLY BREAKING\*\* Removed support for KtLint 0.3x and 0.45.2 ([#​1475](diffplug/spotless#1475)) - `KtLint` does not maintain a stable API - before this MR, we supported every breaking change in the API since 2019. - From now on, we will support no more than 2 breaking changes at a time. - NpmFormatterStepStateBase delays `npm install` call until the formatter is first used. This enables better integration with `gradle-node-plugin`. ([#​1522](diffplug/spotless#1522)) - Bump default `ktlint` version to latest `0.48.1` -> `0.48.2` ([#​1529](diffplug/spotless#1529)) - Bump default `scalafmt` version to latest `3.6.1` -> `3.7.1` ([#​1529](diffplug/spotless#1529)) </details> <details> <summary>quarkusio/quarkus</summary> ### [`v2.16.2.Final`](https://github.com/quarkusio/quarkus/releases/tag/2.16.2.Final) [Compare Source](quarkusio/quarkus@2.16.1.Final...2.16.2.Final) ##### Complete changelog - [#​30976](quarkusio/quarkus#30976) - Metrics - check if index contains class before attempting to use it - [#​30965](quarkusio/quarkus#30965) - JandexBeanInfoAdapter.getMetricAnnotationsThroughStereotype is not null safe - [#​30959](quarkusio/quarkus#30959) - Return text from /q/metrics when the Accept header contains html - [#​30953](quarkusio/quarkus#30953) - Fix OIDC capability string - [#​30947](quarkusio/quarkus#30947) - Ignore interface/class without default constructs fields in SB config - [#​30940](quarkusio/quarkus#30940) - Use SchemaType.ARRAY instead of "ARRAY" for native support - [#​30919](quarkusio/quarkus#30919) - Compilation to native fails, when quarkus-smallrye-openapi is included - [#​30916](quarkusio/quarkus#30916) - Add AppCDS documentation - [#​30896](quarkusio/quarkus#30896) - Quarkus spring-boot-properties extension unable to handle complex configuration. - [#​30878](quarkusio/quarkus#30878) - Bump postgresql from 42.5.2 to 42.5.3 - [#​30866](quarkusio/quarkus#30866) - Only run the quickstart compilation for main - [#​30851](quarkusio/quarkus#30851) - Fixed return type typo in smallrye graphQL guide - [#​30844](quarkusio/quarkus#30844) - Fixed greeting in getting started guide - [#​30839](quarkusio/quarkus#30839) - Fix handling of Accept header in graphQL - [#​30833](quarkusio/quarkus#30833) - Update docs to show BuildProducer use as method parameter instead of field - [#​30828](quarkusio/quarkus#30828) - Make OIDC session cookie same site lax by default - [#​30826](quarkusio/quarkus#30826) - Caffeine - Automatically register metrics cache impls if Micrometer is around - [#​30825](quarkusio/quarkus#30825) - Fix comment about Caffeine optimization - [#​30823](quarkusio/quarkus#30823) - Change accept header to valid plain text in micrometer documentation - [#​30821](quarkusio/quarkus#30821) - Packaging type -Dquarkus.package.create-appcds=true isn't documented - [#​30815](quarkusio/quarkus#30815) - Update SmallRye Config to 2.13.2 - [#​30812](quarkusio/quarkus#30812) - Manage the apache-mime4j dependency - [#​30806](quarkusio/quarkus#30806) - */* in Accept header is ignored if not listed as the first item - [#​30805](quarkusio/quarkus#30805) - MailTemplateInstance with attachments - [#​30803](quarkusio/quarkus#30803) - Support file and byte array attachments in `MailTemplateInstance` - [#​30797](quarkusio/quarkus#30797) - OIDC login not work - [#​30783](quarkusio/quarkus#30783) - <artifactId> uses 'quarkus.platform.artifact-id' property - [#​30778](quarkusio/quarkus#30778) - Avoid creating 3 Liquibase MongoDB instances for startup operations - [#​30776](quarkusio/quarkus#30776) - Ensure that AwsProxyRequestContext can be used with [@​Context](https://github.com/Context) in RESTEasy Reactive - [#​30767](quarkusio/quarkus#30767) - Remove duplicate notification of SseBroadcaster's onErrorListeners - [#​30765](quarkusio/quarkus#30765) - Bump postgresql from 42.5.1 to 42.5.2 - [#​30755](quarkusio/quarkus#30755) - Update ForwardedParser to validate the port - [#​30744](quarkusio/quarkus#30744) - \[Quarkus Native] ClassNotFoundException: com.github.benmanes.caffeine.cache.SSSW - [#​30536](quarkusio/quarkus#30536) - munitnyucontextmanager non helpful error reporting - [#​29753](quarkusio/quarkus#29753) - Introduce ConnectionFactoryWrapperBuildItem - [#​29605](quarkusio/quarkus#29605) - Update docs to reflect that injection should not - [#​27774](quarkusio/quarkus#27774) - PLANNER-1709 Avoid deprecated penalize/reward overloads - [#​23442](quarkusio/quarkus#23442) - problem using quarkus-resteasy-reactive-kotlin-serialization with AwsProxyRequestContext </details> <details> <summary>quarkusio/quarkus-platform</summary> ### [`v2.16.2.Final`](quarkusio/quarkus-platform@2.16.1.Final...2.16.2.Final) [Compare Source](quarkusio/quarkus-platform@2.16.1.Final...2.16.2.Final) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
Describe the bug
A simple OIDC app works at version 2.11.1.Final.
But, it fails at version 2.13.7.Final, 2.16.1.Final.
Expected behavior
It should work.
Actual behavior
Firefox has detected that the server is redirecting the request for this address in a way that will never complete.
How to Reproduce?
Please ref https://github.com/jie-huang/quarkus-oidc for example project.
Or, you can generate a simple OIDC web-app.
main config
In firefox, access path /test/login, it should login successfully and redirect to /test/data.
Output of
uname -a
orver
Darwin sd-lmc-1a8902 21.6.0 Darwin Kernel Version 21.6.0: Mon Dec 19 20:46:01 PST 2022; root:xnu-8020.240.18~2/RELEASE_ARM64_T8101 arm64
Output of
java -version
openjdk version "18" 2022-03-22
GraalVM version (if different from Java)
No response
Quarkus version or git rev
2.13.7.Final, 2.16.1.Final
Build tool (ie. output of
mvnw --version
orgradlew --version
)Gradle 7.4.1
Additional information
No response
The text was updated successfully, but these errors were encountered: