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

Resolve Sec. Identity in RESTEasy Reactive when Proactive Auth disabled #26457

Conversation

michalvavrik
Copy link
Member

f ix #23547 for RESTEasy Reactive cases (e.g. the issue reproducer)

Disabling proactive authentication makes accessing the SecurityIdentity the blocking operation. In RR security checks for these of standard security annotations that requires access to SecurityIdentity (e.g. @RollesAllowed) are further up the handler chain then same checks run by Quarkus Security Runtime (io.quarkus.security.deployment.SecurityProcessor#registerSecurityInterceptors). Checks run in RR access SecurityIndentity by getDeferredIdentity (non-blocking), however in Quarkus Security the checks are using getIdentity. That leads to BlockingOperationNotAllowedException when running synchronously on IO thread. The quarkus-security-runtime-spi does not allow to disable security checks (that make sense from security point of view, but f.e io.quarkus.security.runtime.interceptor.check.RolesAllowedCheck is always run twice if successful), but as checks in RR always run first, this PR sets the identity there to make it available later in the chain.

@michalvavrik michalvavrik force-pushed the feature/fix-securityconstrainer-blocking-op-exception branch from 2415439 to f604aee Compare June 29, 2022 21:30
@sberyozkin
Copy link
Member

This looks fine, I'll check tomorrow with a clearer head, but it would be great if @stuartwdouglas could double check as well

@quarkus-bot

This comment has been minimized.

@sberyozkin
Copy link
Member

@michalvavrik Sorry for a delay and thanks for the quality PR, I left a couple of minor comments.

Can you also check the last paragraph in this doc section, will that still be required if an endpoint tries to access the injected SecurityIdentity ? Can you please update the test endpoint and get the name from the injected identity as well ?

Thanks

@michalvavrik michalvavrik force-pushed the feature/fix-securityconstrainer-blocking-op-exception branch 2 times, most recently from 551edd2 to 1519ffa Compare June 30, 2022 23:32
@michalvavrik
Copy link
Member Author

michalvavrik commented Jun 30, 2022

Can you also check the last paragraph in this doc section, will that still be required if an endpoint tries to access the injected SecurityIdentity ?

It's still true except when following conditions are met:

  1. RESTEasy Reactive is used
  2. and SecurityIdentity is accessed from endpoint that is annotated by standard security annotation that also requires SecurityIdentity in order to perform security check (so f.e. not @PermitAll)
    The way I understand the SecurityConstrainer may cause BlockingOperationNotAllowedException  #23547 is that SecurityConstrainer is accessing SecurityIdentity in a blocking manner which leads to an exception in that special case. Judging from implementation (I might be wrong there), there is no other way when SecurityConstrainer may be used from RR, thus this PR fixes the issue for RR.

The article discusses the Quarkus Security in general (not just RR), but I've added explanatory note.

Can you please update the test endpoint and get the name from the injected identity as well ?

Done, good idea, thank you.

@michalvavrik michalvavrik requested a review from sberyozkin June 30, 2022 23:36
@quarkus-bot

This comment has been minimized.

@sberyozkin
Copy link
Member

@michalvavrik Thanks, can you please check the failing RESTEasy Reactve TCK test ? 500 is reported

f ix quarkusio#23547 for RESTEasy Reactive cases (e.g. the issue reproducer)
@michalvavrik michalvavrik force-pushed the feature/fix-securityconstrainer-blocking-op-exception branch from 1519ffa to 513e44c Compare July 1, 2022 11:41
@michalvavrik
Copy link
Member Author

@sberyozkin I fixed typo and rebased onto the latest main - 500 is gone.

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 1, 2022

Failing Jobs - Building 513e44c

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 11 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 Windows #

- Failing: extensions/mongodb-client/deployment 
! Skipped: extensions/liquibase-mongodb/deployment extensions/panache/mongodb-panache-common/deployment extensions/panache/mongodb-panache-kotlin/deployment and 8 more

📦 extensions/mongodb-client/deployment

io.quarkus.mongodb.NamedMongoClientConfigTest. - More details - Source on GitHub

java.lang.RuntimeException: 
java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.mongodb.deployment.DevServicesMongoProcessor#startMongo threw an exception: java.lang.RuntimeException: java.lang.IllegalStateException: Previous attempts to find a Docker environment failed. Will not retry. Please see logs and check configuration

io.quarkus.mongodb.NamedReactiveMongoClientConfigTest. - More details - Source on GitHub

java.lang.RuntimeException: 
java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.mongodb.deployment.DevServicesMongoProcessor#startMongo threw an exception: java.lang.RuntimeException: java.lang.IllegalStateException: Previous attempts to find a Docker environment failed. Will not retry. Please see logs and check configuration

@sberyozkin sberyozkin self-requested a review July 1, 2022 14:40
Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

Thanks @michalvavrik

@sberyozkin sberyozkin merged commit 4491639 into quarkusio:main Jul 1, 2022
@quarkus-bot quarkus-bot bot added this to the 2.11 - main milestone Jul 1, 2022
@michalvavrik michalvavrik deleted the feature/fix-securityconstrainer-blocking-op-exception branch July 1, 2022 15:04
michalvavrik pushed a commit to michalvavrik/quarkus that referenced this pull request Jul 2, 2022
…fix-securityconstrainer-blocking-op-exception"

This reverts commit 4491639.
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.

2 participants