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

Make OIDC session cookie same site lax by default #30828

Merged

Conversation

sberyozkin
Copy link
Member

Fixes #30797

Unfortunately having a same site strict attribute for all of the OIDC session cookies has proven to be non-viable OOB since any application doing a few extra redirects will start failing.

We'll retain though an option to restrict strictly the session cookie to the same path, which will work for simple applications doing no redirects beyond the default OIDC redirects (ex, only one redirect from Keycloak to the Quarkus path which was used to initiate the authentication) and then it is just GET or POST without any other not same root but same site redirects.

Updated tests: Now samesite is lax by defaut for the session cookie (CodeFlowTest#testCodeFlowNoConsent) but if required it can become strict (CodeFlowTest#testCodeFlowForceHttpsRedirectUriAndPkce - its config requires strict)

Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

LGTM

@gastaldi
Copy link
Contributor

gastaldi commented Feb 2, 2023

@sberyozkin don't forget to include this change in https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.0

@quarkus-bot
Copy link

quarkus-bot bot commented Feb 2, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@sberyozkin
Copy link
Member Author

Hi @gastaldi Thanks, This PR, in combination with #30722, are really about restoring the pre-2.16 state of quarkus-oidc when it comes to controlling the same site attribute for OIDC cookies. Before 2.16, there were no SameSite attribute set at all, which defaults to samesite=lax. Now after the 2 last PRs it is back to the same state, but with only difference is that for the session cookie the users will be able to set samesite=strict for the session cookie only if they can verify it is feasible in their setups.

So I'm not sure I'd like to draw much attention to this episode, I'd just like to fix it and pretend it never happened :-).
It is def not a release/noteworhy-feature IMHO, and it is not really a breaking update either, it is more like back-to-good-time label as without the first PR done earlier most users would have to avoid samesite=strict with an extra config and without this PR, quite a few of them will likely have to do the same, see the analysis from the reporter in the linked issue.

I'm sorry for all this mess, I'd only like to say the tests just won't fail, and after the 1st PR I confirmed I could see a working application (I may have been using Chrome, not sure 100% now).

Would you mind if I drop these 2 labels ?
Thanks

Let me merge anyway, we can continue discussing the labels here

@sberyozkin sberyozkin merged commit 7814ea5 into quarkusio:main Feb 2, 2023
@quarkus-bot quarkus-bot bot added this to the 3.0 - main milestone Feb 2, 2023
@sberyozkin sberyozkin deleted the oidc_session_cookie_same_site_lax branch February 2, 2023 23:04
@sberyozkin
Copy link
Member Author

sberyozkin commented Feb 2, 2023

@gastaldi I guess given that it might work for some applications, having session cookie same site strict, you are right, it is worth mentioning it as a breaking change, and I'll update the migration guide now. But yes, I'll just drop the label marking it as a noteworthy feature :-)

@sberyozkin
Copy link
Member Author

@gsmet gsmet modified the milestones: 3.0 - main, 2.16.2.Final Feb 7, 2023
benkard added a commit to benkard/mulkcms2 that referenced this pull request Apr 2, 2023
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#&#8203;2330---2023-01-26)

##### Added

-   `ProcessRunner` has added some convenience methods so it can be used for maven testing. ([#&#8203;1496](diffplug/spotless#1496))
-   `ProcessRunner` allows to limit captured output to a certain number of bytes. ([#&#8203;1511](diffplug/spotless#1511))
-   `ProcessRunner` is now capable of handling long-running tasks where waiting for exit is delegated to the caller. ([#&#8203;1511](diffplug/spotless#1511))
-   Allow to specify node executable for node-based formatters using `nodeExecutable` parameter ([#&#8203;1500](diffplug/spotless#1500))

##### Fixed

-   The default list of type annotations used by `formatAnnotations` has had 8 more annotations from the Checker Framework added [#&#8203;1494](diffplug/spotless#1494)

##### Changes

-   **POTENTIALLY BREAKING** Bump minimum JRE from 8 to 11, next release likely to bump bytecode to Java 11 ([#&#8203;1514](diffplug/spotless#1514) part 1 of [#&#8203;1337](diffplug/spotless#1337))
-   Rename `YamlJacksonStep` into `JacksonYamlStep` while normalizing Jackson usage ([#&#8203;1492](diffplug/spotless#1492))
-   Convert `gson` integration to use a compile-only source set ([#&#8203;1510](diffplug/spotless#1510)).
-   \*\* POTENTIALLY BREAKING\*\* Removed support for KtLint 0.3x and 0.45.2 ([#&#8203;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`. ([#&#8203;1522](diffplug/spotless#1522))
-   Bump default `ktlint` version to latest `0.48.1` -> `0.48.2` ([#&#8203;1529](diffplug/spotless#1529))
-   Bump default `scalafmt` version to latest `3.6.1` -> `3.7.1` ([#&#8203;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

-   [#&#8203;30976](quarkusio/quarkus#30976) - Metrics - check if index contains class before attempting to use it
-   [#&#8203;30965](quarkusio/quarkus#30965) - JandexBeanInfoAdapter.getMetricAnnotationsThroughStereotype is not null safe
-   [#&#8203;30959](quarkusio/quarkus#30959) - Return text from /q/metrics when the Accept header contains html
-   [#&#8203;30953](quarkusio/quarkus#30953) - Fix OIDC capability string
-   [#&#8203;30947](quarkusio/quarkus#30947) - Ignore interface/class without default constructs fields in SB config
-   [#&#8203;30940](quarkusio/quarkus#30940) - Use SchemaType.ARRAY instead of "ARRAY" for native support
-   [#&#8203;30919](quarkusio/quarkus#30919) - Compilation to native fails, when quarkus-smallrye-openapi is included
-   [#&#8203;30916](quarkusio/quarkus#30916) - Add AppCDS documentation
-   [#&#8203;30896](quarkusio/quarkus#30896) - Quarkus spring-boot-properties extension unable to handle complex configuration.
-   [#&#8203;30878](quarkusio/quarkus#30878) - Bump postgresql from 42.5.2 to 42.5.3
-   [#&#8203;30866](quarkusio/quarkus#30866) - Only run the quickstart compilation for main
-   [#&#8203;30851](quarkusio/quarkus#30851) - Fixed return type typo in smallrye graphQL guide
-   [#&#8203;30844](quarkusio/quarkus#30844) - Fixed greeting in getting started guide
-   [#&#8203;30839](quarkusio/quarkus#30839) - Fix handling of Accept header in graphQL
-   [#&#8203;30833](quarkusio/quarkus#30833) - Update docs to show BuildProducer use as method parameter instead of field
-   [#&#8203;30828](quarkusio/quarkus#30828) - Make OIDC session cookie same site lax by default
-   [#&#8203;30826](quarkusio/quarkus#30826) - Caffeine - Automatically register metrics cache impls if Micrometer is around
-   [#&#8203;30825](quarkusio/quarkus#30825) - Fix comment about Caffeine optimization
-   [#&#8203;30823](quarkusio/quarkus#30823) - Change accept header to valid plain text in micrometer documentation
-   [#&#8203;30821](quarkusio/quarkus#30821) - Packaging type -Dquarkus.package.create-appcds=true isn't documented
-   [#&#8203;30815](quarkusio/quarkus#30815) - Update SmallRye Config to 2.13.2
-   [#&#8203;30812](quarkusio/quarkus#30812) - Manage the apache-mime4j dependency
-   [#&#8203;30806](quarkusio/quarkus#30806) - */* in Accept header is ignored if not listed as the first item
-   [#&#8203;30805](quarkusio/quarkus#30805) - MailTemplateInstance with attachments
-   [#&#8203;30803](quarkusio/quarkus#30803) - Support file and byte array attachments in `MailTemplateInstance`
-   [#&#8203;30797](quarkusio/quarkus#30797) - OIDC login not work
-   [#&#8203;30783](quarkusio/quarkus#30783) - <artifactId> uses 'quarkus.platform.artifact-id' property
-   [#&#8203;30778](quarkusio/quarkus#30778) - Avoid creating 3 Liquibase MongoDB instances for startup operations
-   [#&#8203;30776](quarkusio/quarkus#30776) - Ensure that AwsProxyRequestContext can be used with [@&#8203;Context](https://github.com/Context) in RESTEasy Reactive
-   [#&#8203;30767](quarkusio/quarkus#30767) - Remove duplicate notification of SseBroadcaster's onErrorListeners
-   [#&#8203;30765](quarkusio/quarkus#30765) - Bump postgresql from 42.5.1 to 42.5.2
-   [#&#8203;30755](quarkusio/quarkus#30755) - Update ForwardedParser to validate the port
-   [#&#8203;30744](quarkusio/quarkus#30744) - \[Quarkus Native] ClassNotFoundException: com.github.benmanes.caffeine.cache.SSSW
-   [#&#8203;30536](quarkusio/quarkus#30536) - munitnyucontextmanager non helpful error reporting
-   [#&#8203;29753](quarkusio/quarkus#29753) - Introduce ConnectionFactoryWrapperBuildItem
-   [#&#8203;29605](quarkusio/quarkus#29605) - Update docs to reflect that injection should not
-   [#&#8203;27774](quarkusio/quarkus#27774) - PLANNER-1709 Avoid deprecated penalize/reward overloads
-   [#&#8203;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-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OIDC login not work
3 participants