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

Prioritize OIDC mechanism when inclusive authentication is disabled to simplify using mTLS and OIDC together #45072

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented Dec 11, 2024

@michalvavrik michalvavrik changed the title Prioritize OIDC mechanism when inclusive authentication is disabled so simplify using both mTLS and OIDC Prioritize OIDC mechanism when inclusive authentication is disabled so simplify using mTLS and OIDC together Dec 11, 2024
@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/documentation area/oidc area/vertx labels Dec 11, 2024
@michalvavrik michalvavrik changed the title Prioritize OIDC mechanism when inclusive authentication is disabled so simplify using mTLS and OIDC together Prioritize OIDC mechanism when inclusive authentication is disabled to simplify using mTLS and OIDC together Dec 11, 2024
Copy link

github-actions bot commented Dec 11, 2024

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

@sberyozkin
Copy link
Member

sberyozkin commented Dec 11, 2024

Hey @michalvavrik, thanks very much, it must've been myself who raised the MTLS priority in the old PR, so the breaking side-effect it is definitely not a consequence of your work, thanks in any case...

I'm having some unclear concerns, from one point of view, we are doing the totally right thing, we are restoring what used to work for users combining MTLS and OIDC, which used to work simply because MTLS, before 3.16, is sorted below OIDC.

What the inclusive authentication does it really makes, IMHO, the right thing, MTLS check must run first, to make it clear for users that in such cases, SecurityIdentity represents MTLS, this is the main thing, since the current MTLS authentication mechanism just wraps the client certificate - OIDC may access it directly from RoutingContext.

I guess now what I'm trying to say is that we should update the motivation text in the docs, that the reason it is sorted first when the inclusive authentication is on is to ensure that the injected SecurityIdentity is the one which represents the MTLS authentication, which technically, runs first at the transport level

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/disable-inclusive-auh-refactoring branch from 288df17 to fad46fa Compare December 12, 2024 12:57
@michalvavrik
Copy link
Member Author

Sure Sergey.

@michalvavrik michalvavrik force-pushed the feature/disable-inclusive-auh-refactoring branch from fad46fa to 671ed88 Compare December 12, 2024 13:10
Copy link

quarkus-bot bot commented Dec 12, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 671ed88.

✅ 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.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

quarkus-bot bot commented Dec 12, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 671ed88.

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
Native Tests - Windows support Setup GraalVM ⚠️ Check → Logs Raw logs 🚧

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17 Windows

📦 integration-tests/grpc-hibernate

com.example.grpc.hibernate.BlockingRawTest.shouldAdd - History

  • Condition with Lambda expression in com.example.grpc.hibernate.BlockingRawTestBase was not fulfilled within 30 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in com.example.grpc.hibernate.BlockingRawTestBase was not fulfilled within 30 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at com.example.grpc.hibernate.BlockingRawTestBase.shouldAdd(BlockingRawTestBase.java:59)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)

⚙️ Native Tests - Misc4

📦 integration-tests/gradle

io.quarkus.gradle.nativeimage.CustomNativeTestSourceSetIT.runNativeTests - History

  • Gradle build failed with exit code 1 - java.lang.AssertionError
java.lang.AssertionError: Gradle build failed with exit code 1
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:140)
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:57)
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:52)
	at app//io.quarkus.gradle.nativeimage.QuarkusNativeGradleITBase.runGradleWrapper(QuarkusNativeGradleITBase.java:36)
	at app//io.quarkus.gradle.nativeimage.CustomNativeTestSourceSetIT.runNativeTests(CustomNativeTestSourceSetIT.java:17)
	at [email protected]/java.lang.reflect.Method.invoke(Method.java:569)
	at [email protected]/java.util.ArrayList.forEach(ArrayList.java:1511)

@michalvavrik
Copy link
Member Author

Failures unrelated, fails also here #44642 (comment).

@sberyozkin
Copy link
Member

Thanks @michalvavrik

@sberyozkin sberyozkin merged commit d8069b0 into quarkusio:main Dec 12, 2024
54 of 55 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Dec 12, 2024
@michalvavrik michalvavrik deleted the feature/disable-inclusive-auh-refactoring branch December 12, 2024 18:10
@gsmet gsmet removed this from the 3.18 - main milestone Dec 17, 2024
@gsmet gsmet added this to the 3.17.5 milestone Dec 17, 2024
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.

3 participants