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

[2.13] Perform security checks on inherited endpoints before payload deserialization in the RESTEasy Reactive #38992

Merged

Conversation

michalvavrik
Copy link
Member

backports #38832 with resolved conflicts

Copy link

quarkus-bot bot commented Feb 25, 2024

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the 2-13-backport-rr-inherited-checks branch from b0149d6 to 2eb4659 Compare February 25, 2024 22:09

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the 2-13-backport-rr-inherited-checks branch from 2eb4659 to dd065b6 Compare February 25, 2024 22:23

This comment has been minimized.

@michalvavrik
Copy link
Member Author

It looks like 2.13 CI is seriously unstable, sometimes even initial build fails. I can't reproduce the PolicyEnforcerInGraalITCase locally, just tried it and in the few runs before (I didn't change code, just tests in different module) it passed.

@jmartisk could you re-run the Quarkus CI / Native Tests - Security3 (pull_request) job please? I don't want to waste resources again, I think it you can re-run just that one. In case it will fail again I'll dig in but it seems fine to me :-/

@jmartisk
Copy link
Contributor

@michalvavrik I've triggered a rerun of the failed jobs now

@michalvavrik
Copy link
Member Author

thanks @jmartisk , the test is now green. This PR has no relation to Gradle so I think it is ready for review.

Copy link

quarkus-bot bot commented Feb 26, 2024

Status for workflow Quarkus CI

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

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
Gradle Tests - JDK 11 Build Failures Logs Raw logs 🚧
Gradle Tests - JDK 11 Windows Build Failures Logs Raw logs 🚧
✔️ Native Tests - Security3 Failures Logs Raw logs 🚧

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 #

- Failing: integration-tests/gradle 

📦 integration-tests/gradle

io.quarkus.gradle.BeanInTestSourcesTest.testBasicMultiModuleBuild line 15 - History - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

Expecting value to be true but was false
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at io.quarkus.gradle.BeanInTestSourcesTest.testBasicMultiModuleBuild(BeanInTestSourcesTest.java:15)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

io.quarkus.gradle.ConditionalDependenciesTest.scenarioTwo line 147 - History - More details - Source on GitHub

java.lang.AssertionError: 

Expecting path:
  /home/runner/work/quarkus/quarkus/integration-tests/gradle/target/classes/conditional-test-project/scenario-two/build/quarkus-app/lib/main/org.acme.ext-f-1.0-SNAPSHOT.jar
to exist (symbolic links were followed).
	at io.quarkus.gradle.ConditionalDependenciesTest.scenarioTwo(ConditionalDependenciesTest.java:147)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)

⚙️ Gradle Tests - JDK 11 Windows #

- Failing: integration-tests/gradle 

📦 integration-tests/gradle

io.quarkus.gradle.ConditionalDependenciesTest.scenarioTwo line 147 - History - More details - Source on GitHub

java.lang.AssertionError: 

Expecting path:
  D:\a\quarkus\quarkus\integration-tests\gradle\target\classes\conditional-test-project\scenario-two\build\quarkus-app\lib\main\org.acme.ext-f-1.0-SNAPSHOT.jar
to exist (symbolic links were followed).
	at io.quarkus.gradle.ConditionalDependenciesTest.scenarioTwo(ConditionalDependenciesTest.java:147)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)

io.quarkus.gradle.TestFixtureModuleTest.testTaskShouldUseTestFixtures line 19 - History - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

Expecting value to be true but was false
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at io.quarkus.gradle.TestFixtureModuleTest.testTaskShouldUseTestFixtures(TestFixtureModuleTest.java:19)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

⚙️ Native Tests - Security3 #

- Failing: integration-tests/keycloak-authorization 

📦 integration-tests/keycloak-authorization

io.quarkus.it.keycloak.PolicyEnforcerInGraalITCase.testHttpResponseFromExternalServiceAsClaim - History - More details - Source on GitHub

org.opentest4j.AssertionFailedError: /api/permission/http-response-claim-protected ==> expected: <200> but was: <401>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:560)
	at io.quarkus.it.keycloak.PolicyEnforcerTest.assureGetPath(PolicyEnforcerTest.java:242)
	at io.quarkus.it.keycloak.PolicyEnforcerTest.testHttpResponseFromExternalServiceAsClaim(PolicyEnforcerTest.java:174)

@jmartisk
Copy link
Contributor

Thanks @michalvavrik , awesome job! @geoand could you give this a quick look, as you reviewed the original PR to main?

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

LGTM!

@michalvavrik
Copy link
Member Author

\cc @aloubyansky

@sberyozkin
Copy link
Member

@michalvavrik Can you please check the keycloak-authorization test ? I don't recall it being a flaky test. I've noticed it uses RestEasy Reactive

@michalvavrik
Copy link
Member Author

michalvavrik commented Feb 26, 2024

@michalvavrik Can you please check the keycloak-authorization test ? I don't recall it being a flaky test. I've noticed it uses RestEasy Reactive

@sberyozkin Please note that Quarkus CI / Native Tests - Security3 (pull_request) is now green. I've build this branch locally and tested it.

Also this PR has only relation on standard security annotation checks and there are no such annotations applied on the io.quarkus.it.keycloak.ProtectedResource#httpResponseClaimProtected endpoint, therefore I can not see it being related.

Only suspicious thing I can see in that log is long DNS request, but that is not related:

2024-02-25T23:01:53.5116705Z 2024-02-25 23:01:53,411 WARN  [io.ver.cor.imp.BlockedThreadChecker] (vertx-blocked-thread-checker) Thread Thread[vert.x-eventloop-thread-1,5,main] has been blocked for 7389 ms, time limit is 2000 ms: io.vertx.core.VertxException: Thread blocked
2024-02-25T23:01:53.5120030Z 	at [email protected]/sun.nio.ch.DatagramChannelImpl.<init>(DatagramChannelImpl.java:132)
2024-02-25T23:01:53.5121742Z 	at [email protected]/sun.nio.ch.SelectorProviderImpl.openDatagramChannel(SelectorProviderImpl.java:42)
2024-02-25T23:01:53.5123385Z 	at app//io.netty.channel.socket.nio.NioDatagramChannel.newSocket(NioDatagramChannel.java:89)
2024-02-25T23:01:53.5124838Z 	at app//io.netty.channel.socket.nio.NioDatagramChannel.<init>(NioDatagramChannel.java:120)
2024-02-25T23:01:53.5126217Z 	at app//io.vertx.core.net.impl.transport.Transport.datagramChannel(Transport.java:160)
2024-02-25T23:01:53.5127783Z 	at app//io.vertx.core.impl.resolver.DnsResolverProvider.lambda$new$1(DnsResolverProvider.java:154)
2024-02-25T23:01:53.5129553Z 	at app//io.vertx.core.impl.resolver.DnsResolverProvider$$Lambda$1026/0x00000008008b7840.newChannel(Unknown Source)
2024-02-25T23:01:53.5131144Z 	at app//io.netty.bootstrap.AbstractBootstrap.initAndRegister(AbstractBootstrap.java:310)
2024-02-25T23:01:53.5132576Z 	at app//io.netty.bootstrap.AbstractBootstrap.register(AbstractBootstrap.java:227)
2024-02-25T23:01:53.5133920Z 	at app//io.netty.resolver.dns.DnsNameResolver.<init>(DnsNameResolver.java:517)
2024-02-25T23:01:53.5135387Z 	at app//io.netty.resolver.dns.DnsNameResolverBuilder.build(DnsNameResolverBuilder.java:527)
2024-02-25T23:01:53.5137143Z 	at app//io.netty.resolver.dns.DnsAddressResolverGroup.newNameResolver(DnsAddressResolverGroup.java:114)
2024-02-25T23:01:53.5138937Z 	at app//io.netty.resolver.dns.DnsAddressResolverGroup.newResolver(DnsAddressResolverGroup.java:92)
2024-02-25T23:01:53.5140657Z 	at app//io.netty.resolver.dns.DnsAddressResolverGroup.newResolver(DnsAddressResolverGroup.java:77)
2024-02-25T23:01:53.5142289Z 	at app//io.netty.resolver.AddressResolverGroup.getResolver(AddressResolverGroup.java:70)
2024-02-25T23:01:53.5143702Z 	at app//io.netty.bootstrap.Bootstrap.doResolveAndConnect0(Bootstrap.java:208)
2024-02-25T23:01:53.5144994Z 	at app//io.netty.bootstrap.Bootstrap.doResolveAndConnect(Bootstrap.java:171)
2024-02-25T23:01:53.5146189Z 	at app//io.netty.bootstrap.Bootstrap.connect(Bootstrap.java:148)
2024-02-25T23:01:53.5147548Z 	at app//io.vertx.core.net.impl.ChannelProvider.handleConnect(ChannelProvider.java:155)
2024-02-25T23:01:53.5149065Z 	at app//io.vertx.core.net.impl.ChannelProvider.connect(ChannelProvider.java:106)
2024-02-25T23:01:53.5150505Z 	at app//io.vertx.core.net.impl.ChannelProvider.connect(ChannelProvider.java:92)
2024-02-25T23:01:53.5152029Z 	at app//io.vertx.core.net.impl.NetClientImpl.connectInternal2(NetClientImpl.java:273)
2024-02-25T23:01:53.5153811Z 	at app//io.vertx.core.net.impl.NetClientImpl.lambda$connectInternal$1(NetClientImpl.java:241)
2024-02-25T23:01:53.5155439Z 	at app//io.vertx.core.net.impl.NetClientImpl$$Lambda$1066/0x00000008008fb840.handle(Unknown Source)
2024-02-25T23:01:53.5156979Z 	at app//io.vertx.core.impl.future.FutureImpl$3.onSuccess(FutureImpl.java:141)
2024-02-25T23:01:53.5158448Z 	at app//io.vertx.core.impl.future.FutureBase.lambda$emitSuccess$0(FutureBase.java:54)
2024-02-25T23:01:53.5160011Z 	at app//io.vertx.core.impl.future.FutureBase$$Lambda$1067/0x0000000800908040.run(Unknown Source)
2024-02-25T23:01:53.5162140Z 	at app//io.netty.util.concurrent.AbstractEventExecutor.runTask(AbstractEventExecutor.java:173)
2024-02-25T23:01:53.5163927Z 	at app//io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:166)
2024-02-25T23:01:53.5165822Z 	at app//io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:470)
2024-02-25T23:01:53.5167432Z 	at app//io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:569)
2024-02-25T23:01:53.5168964Z 	at app//io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997)
2024-02-25T23:01:53.5170580Z 	at app//io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
2024-02-25T23:01:53.5172141Z 	at app//io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
2024-02-25T23:01:53.5173822Z 	at [email protected]/java.lang.Thread.run(Thread.java:829)

@sberyozkin
Copy link
Member

@michalvavrik

Only suspicious thing I can see in that log is long DNS request, but that is not related:

That may indeed cause 401 if the remote authorization check failed and agreed it would not be related to this PR

@aloubyansky aloubyansky merged commit ddbd475 into quarkusio:2.13 Feb 27, 2024
45 of 47 checks passed
@michalvavrik michalvavrik deleted the 2-13-backport-rr-inherited-checks branch February 27, 2024 12:00
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.

5 participants