JAX-RS default security is applied to annotated, inherited endpoints

[Foobartender]

Updated description

I attached a reproducer. Run with ./gradlew test. Please note that all tests pass out-of-the box, all endpoints are annotated with @RolesAllowed, and that unauthenticated requests are covered and result in 401 - it works as it should.

When enabling deny-unannotated-endpoints or default-roles-allowed in application.properties, some tests of the base class fail, but shouldn't.

Also, default-roles-allowed shows some weird behavior on endpoints of the base class. Without @RolesAllowed default roles are applied as expected. But when present, @RolesAllowed is not ignored as when using deny-unannotated-endpoints. It works as expected when the annotated and default role match, but fails when they are different.

Below the key parts of the reproducer, Java variant omitted.

Endpoints:

package org.example.kotlin

import jakarta.annotation.security.RolesAllowed
import jakarta.ws.rs.GET
import jakarta.ws.rs.Path


object Roles {
 const val ROLE1 = "role1"
 const val ROLE2 = "role2"
}



abstract class Base {
 @[GET Path("base/"+Roles.ROLE1)]
 @RolesAllowed(Roles.ROLE1)
 fun baseRole1() = "base/"+Roles.ROLE1

 @[GET Path("base/"+Roles.ROLE2)]
 @RolesAllowed(Roles.ROLE2)
 fun baseRole2() = "base/"+Roles.ROLE2
}


@Path("/kotlin")
class DerivedK : Base() {
 @[GET Path(Roles.ROLE1)]
 @RolesAllowed(Roles.ROLE1)
 fun derivedRole1() = Roles.ROLE1

 @[GET Path(Roles.ROLE2)]
 @RolesAllowed(Roles.ROLE2)
 fun derivedRole2() = Roles.ROLE2
}

Tests:

package org.example

import io.quarkus.test.common.http.TestHTTPEndpoint
import io.quarkus.test.junit.QuarkusTest
import io.quarkus.test.security.TestSecurity
import io.restassured.module.kotlin.extensions.Then
import io.restassured.module.kotlin.extensions.When
import org.example.java.DerivedJ
import org.example.kotlin.DerivedK
import org.example.kotlin.Roles
import org.junit.jupiter.api.Test


interface ResourceTest {
 val myRole: String?

 private fun test(role: String, base: Boolean) {
  When {
   get((if (base) "base/" else "")+role)
  } Then {
   statusCode(
    when (myRole) {
     null -> 401
     role -> 200
     else -> 403
    }
   )
  }
 }

 @Test
 fun `base role1`() = test(Roles.ROLE1, true)
 @Test
 fun `base role2`() = test(Roles.ROLE2, true)
 @Test
 fun `derived role1`() = test(Roles.ROLE1, false)
 @Test
 fun `derived role2`() = test(Roles.ROLE2, false)
}


@QuarkusTest
@TestHTTPEndpoint(DerivedK::class)
class KotlinResourceTestWithNoRole : ResourceTest {
 override val myRole = null
}


@QuarkusTest
@TestHTTPEndpoint(DerivedK::class)
// Note that Kotlin compiles default interfaces methods into the class, so @TestSecurity applies.
@TestSecurity(user = "test1", roles = [Roles.ROLE1])
class KotlinResourceTestWithRole1 : ResourceTest {
 override val myRole = Roles.ROLE1
}


@QuarkusTest
@TestHTTPEndpoint(DerivedK::class)
@TestSecurity(user = "test2", roles = [Roles.ROLE2])
class KotlinResourceTestWithRole2 : ResourceTest {
 override val myRole = Roles.ROLE2
}

q-jds-test.tar.gz

Original description

Describe the bug

Not sure, whether this is now intended behavior or a bug, so I'll keep it short. Below is a rough example. If it's supposed to work (maybe it does, I left out some probably irrelevant details), I'll create a reproducer.

abstract class AbstractFoo {
 @GET
 fun foo() = "Hello world!"
}

@ApplicationScoped
@Path("foo1")
@Authenticated
class Foo1 : AbstractFoo()

@ApplicationScoped
@Path("foo2")
@Authenticated
class Foo2 : AbstractFoo()

Expected behavior

Requests to /foo1 and /foo2 should work when authenticated, because the derived concrete classes have an explicit @Authenticated annotation.

Actual behavior

Requests fail with status 403, although they are authenticated.

[quarkus-bot]

/cc @geoand (kotlin)

[geoand]

cc @michalvavrik @sberyozkin

[michalvavrik]

Standard security annotations are not supposed to be inherited. I remember I read some discussion in some old opened issues in Quarkus where @stuartwdouglas wrote that, I hope I do not misinterpret him. Please comment Stuart if I do.

Anyway, I think we should treat @Autheticated similarly to how we treat https://jakarta.ee/specifications/platform/9/apidocs/jakarta/annotation/security/package-summary.html so we should go by https://jakarta.ee/specifications/annotations/2.1/annotations-spec-2.1.pdf section 3.1. General Guidelines for Inheritance of Annotations says . Class-level annotations only affect the class they annotate and its members, that is, its methods and fields.. They never affect a member declared by a superclass, even if it is not hidden or overridden by the class in question.

Therefore I vote for not a bug. Interesting it worked before though. I'd be interested in a reproducer, but I do not plan to fix it.

Please comment if you disagree @sberyozkin

[michalvavrik]

also @geoand could have opinion as this is related to inheritance of endpoints..

[sberyozkin]

@michalvavrik The inheritance works in general but I agree it is not really a bug, at least in the JAX-RS classic, the inheritance is not partial, i.e, all JAX-RS annotations must be inherited, so I'd rather not treat this issue as a breaking fix side-effect. IMHO, if the users want to continue using the deny all feature they should align the annotations. Worth giving it a migration doc notice though

[geoand]

I agree with @michalvavrik

[michalvavrik]

agreed @sberyozkin , thanks

[Foobartender]

I generally agree, too, but the example below also doesn't work (added @Authenticated to both base class and base method). There should be a way to both have deny-unannotated-endpoints enabled and leverage inheritance.

@Authenticated
abstract class AbstractFoo {
 @GET
 @Authenticated
 fun foo() = "Hello world!"
}

@ApplicationScoped
@Path("foo1")
@Authenticated
class Foo1 : AbstractFoo()

@ApplicationScoped
@Path("foo2")
@Authenticated
class Foo2 : AbstractFoo()

[sberyozkin]

@Foobartender Can you please align JAX-RS annotations, the problem is, you have @GET on the abstract class, while @Path on concrete classes, the partial inheritance does not work with the security

[Foobartender]

The example below works, even though I didn't annotate the overridden foo methods. That doesn't really make sense to me. I expected 403 unless I'd annotate the overridden foo methods with @Authenticated, too.

@Authenticated
abstract class AbstractFoo {
 @GET
 @Authenticated
 fun foo() = "Hello world!"
}

@ApplicationScoped
@Path("foo1")
@Authenticated
class Foo1 : AbstractFoo() {
 override fun foo() = super.foo()
}

@ApplicationScoped
@Path("foo2")
@Authenticated
class Foo2 : AbstractFoo() {
 override fun foo() = super.foo()
}

[michalvavrik]

The example below works, even though I didn't annotate the overridden foo methods. That doesn't really make sense to me. I expected 403 unless I'd annotate the overridden foo methods with @Authenticated, too.

The example you provided has AbstractFoo secured with @Authenticated and Foo2 and Foo1 secured with @Authenticated. Can you suggest which of your actual endpoints requires default JAX-RS security? The overriden foo gets security from a class level.

[sberyozkin]

@Foobartender My understanding is that given

@Path("service")
abstract class AbstractFoo {
 @GET
 @Path("foo1")
 fun foo1() = "Hello world!"

 @GET
 @Path("foo2")
 fun foo2() = "Hello world!"
}

@ApplicationScoped
class Foo : AbstractFoo() {
 @Authenticated
 override fun foo1() = super.foo()
}

Only access to foo2 will be denied

[Foobartender]

Yes, that makes sense, because the overridden method is now a method of the concrete class and the class annotation applies.

@michalvavrik: I don't really need default JAX-RS security (i.e. deny-unannotated-endpoints), I only use it as a fail-safe for missing security annotations on JAX-RS beans. I'm very cautious about not creating information leakage/privacy vulnerabilities by forgetting them. When I was using WildFly, I even put security annotations on repository beans to guard data access at the lowest level. I already had to compromise on that, because there are @Scheduled methods that need to call repository bean methods and Quarkus doesn't support @RunAs. With disabling deny-unannotated-endpoints I'm compromising even further.

[michalvavrik]

okay @Foobartender , I need to divide answer into 3 topics, I think second point is of interest for you:

1. I don't say you can't be right about inheritance impl. but so far I do not comprehend where we differ from that Jakarta spec
2. Current implementation does one thing - secures endpoints, so whatever ends up actual endpoint should either have RBAC annotation or it's class should. This should mean you have a safety measure - if endpoint is not annotated or its class, nothing will get in. If it doesn't work, it would be a bug.
3. This discussion makes me to think we should document current situation better. However I'd prefer if users wouldn't rely on complex inheritance rules.

[Foobartender]

In reference to my example from #38754 (comment) ...

2. ...whatever ends up actual endpoint...

AbstractFoo#foo() is the actual endpoint.

2. ...should either have RBAC annotation or it's class should. ...

Both AbstractFoo (the class of the actual endpoint) and AbstractFoo#foo() (the actual endpoint) are annotated with @Authenticated.

Thus I'd argue it's a bug, even though @Authenticated doesn't require a particular role and thus might not qualify as an RBAC annotation. I don't want to nitpick here, sorry for doing it anyway. 😜

Regarding 1. and 3.: relying on complex inheritance rules is something that happened when I actually wanted to do composition, e.g. implementing my own repository pattern or something like rest-data-panache. While doing so, I often checked the specs, which usually left plenty of room for implementation, and didn't cover new language features like default interface methods.

[michalvavrik]

Both AbstractFoo (the class of the actual endpoint) and AbstractFoo#foo() (the actual endpoint) are annotated with @authenticated. reference #38754 (comment)
Thus I'd argue it's a bug,

AbstractFoo#foo() is annotated with @Authenticated, therefore default JAX-RS should not be applied. I think we have tests, but if that is not a case, please provide a reproducer and I'll fix it.

even though @authenticated doesn't require a particular role and thus might not qualify as an RBAC annotation.

I used RBAC as a shortcut, sure, @Authenticated is not RBAC.

Regarding 1. and 3.: relying on complex inheritance rules is something that happened when I actually wanted to do composition, e.g. implementing my own repository pattern or something like rest-data-panache. While doing so, I often checked the specs, which usually left plenty of room for implementation, and didn't cover new language features like default interface methods.

Sorry, I think current implementation is safer and easier. I'll keep documentation improvements in mind. Feel free to open new issue (not a bug) that suggests changing it. Maybe others will agree...

[Foobartender]

AbstractFoo#foo() is annotated with @Authenticated, therefore default JAX-RS should not be applied. I think we have tests, but if that is not a case, please provide a reproducer and I'll fix it.

I attached a reproducer. Run with ./gradlew test. Please note that all tests pass out-of-the box, all endpoints are annotated with @RolesAllowed, and that unauthenticated requests are covered and result in 401 - it works as it should.

When enabling deny-unannotated-endpoints or default-roles-allowed in application.properties, some tests of the base class fail, but shouldn't.

Also, default-roles-allowed shows some weird behavior on endpoints of the base class. Without @RolesAllowed default roles are applied as expected. But when present, @RolesAllowed is not ignored as when using deny-unannotated-endpoints. It works as expected when the annotated and default role match, but fails when they are different.

Below the key parts of the reproducer, Java variant omitted.

Endpoints:

package org.example.kotlin

import jakarta.annotation.security.RolesAllowed
import jakarta.ws.rs.GET
import jakarta.ws.rs.Path


object Roles {
 const val ROLE1 = "role1"
 const val ROLE2 = "role2"
}



abstract class Base {
 @[GET Path("base/"+Roles.ROLE1)]
 @RolesAllowed(Roles.ROLE1)
 fun baseRole1() = "base/"+Roles.ROLE1

 @[GET Path("base/"+Roles.ROLE2)]
 @RolesAllowed(Roles.ROLE2)
 fun baseRole2() = "base/"+Roles.ROLE2
}


@Path("/kotlin")
class DerivedK : Base() {
 @[GET Path(Roles.ROLE1)]
 @RolesAllowed(Roles.ROLE1)
 fun derivedRole1() = Roles.ROLE1

 @[GET Path(Roles.ROLE2)]
 @RolesAllowed(Roles.ROLE2)
 fun derivedRole2() = Roles.ROLE2
}

Tests:

package org.example

import io.quarkus.test.common.http.TestHTTPEndpoint
import io.quarkus.test.junit.QuarkusTest
import io.quarkus.test.security.TestSecurity
import io.restassured.module.kotlin.extensions.Then
import io.restassured.module.kotlin.extensions.When
import org.example.java.DerivedJ
import org.example.kotlin.DerivedK
import org.example.kotlin.Roles
import org.junit.jupiter.api.Test


interface ResourceTest {
 val myRole: String?

 private fun test(role: String, base: Boolean) {
  When {
   get((if (base) "base/" else "")+role)
  } Then {
   statusCode(
    when (myRole) {
     null -> 401
     role -> 200
     else -> 403
    }
   )
  }
 }

 @Test
 fun `base role1`() = test(Roles.ROLE1, true)
 @Test
 fun `base role2`() = test(Roles.ROLE2, true)
 @Test
 fun `derived role1`() = test(Roles.ROLE1, false)
 @Test
 fun `derived role2`() = test(Roles.ROLE2, false)
}


@QuarkusTest
@TestHTTPEndpoint(DerivedK::class)
class KotlinResourceTestWithNoRole : ResourceTest {
 override val myRole = null
}


@QuarkusTest
@TestHTTPEndpoint(DerivedK::class)
// Note that Kotlin compiles default interfaces methods into the class, so @TestSecurity applies.
@TestSecurity(user = "test1", roles = [Roles.ROLE1])
class KotlinResourceTestWithRole1 : ResourceTest {
 override val myRole = Roles.ROLE1
}


@QuarkusTest
@TestHTTPEndpoint(DerivedK::class)
@TestSecurity(user = "test2", roles = [Roles.ROLE2])
class KotlinResourceTestWithRole2 : ResourceTest {
 override val myRole = Roles.ROLE2
}

q-jds-test.tar.gz

[michalvavrik]

I'll look at it in the next few days (ideally this weekend).

[michalvavrik]

I can confirm there is a bug, I don't think it's vulnerability (as it does opposite, harden unsolicitously), so I'll go ahead here. In short: before the CVE fix there was a bug in the EagerSecurityHandler which for your use case caused security checks performed after serialization (by CDI interceptors). My fix hardened it and made sure nothing passes, however I didn't know about this bug in matching of annotation with actually declared class, so instead of allowing request to pass in the ServerRestHandler and let it to CDI interceptors (which was a bad thing to do), we now apply default JAX-RS security.

Fix is quite easy, I'll create it tomorrow. We will need to backport it to all the streams though. cc @sberyozkin @gsmet

Thank you @Foobartender

[michalvavrik]

I've reopened it although the issue description is not up to date with your latest comment, if you could replace the issue description with this comment #38754 (comment) it would be nice.

[Foobartender]

Done, also updated the title, hope that fits.

Thank you, and please enjoy your weekend, it's not urgent. 😉

[sberyozkin]

Thanks @michalvavrik @Foobartender

The extra hardening causing some side-effects causing the denial of access is unfortunate but not critical IMHO, so Michal, please do take your time, do not spend weekends.

I have to admit I'm not exactly sure where a side-effect is, sorry.

The Base class has all the annotations, then the DerivedK class has the same annotations but overrides the path, so the inheritance is not working already (all the annotations are on the DerivedK methid), and no non-annotated methods exist.
What has to be fixed here ?

[michalvavrik]

Hey @sberyozkin, agreed. Your question is good, but it's difficult to catch this in words, it should be clear from the fix here: #38832

[Foobartender]

@sberyozkin: I'll try to explain.

First, please note the difference between security annotations (@RolesAllowed, @PermitAll, etc.) and JAX-RS annotations (@Path, @GET, etc), and their different inheritance rules (I'll get back to them). This bug is about the former, and I think you got some details wrong about the latter.

The documentation for deny-unannotated-endpoints and default-roles-allowed says that they only apply to endpoints without security annotations. But in case of inherited endpoints with security annotations, they are applied anyway. That's the bug.

My original example was indeed wrong (which might have caused some confusion), and I was a bit shocked about my own misconception that @Authenticated and the other security annotations apply to the resource and all its sub-resources like @Path or @Produces, not only to methods of the declaring class. Luckily, Keycloak authorization still got me covered. And I noticed it thanks to the CVE fix. However, after adding @Authenticated to the base class in this comment it should have worked, but still didn't.

There's some confusion about JAX-RS annotations. I think all JAX-RS annotions in my examples are correct. I didn't understand what you meant with "aligning them". Their inheritance is described here. You put @Path on the base class here, but that doesn't work, because "inheritance of class or interface annotations is not supported" in JAX-RS (see linked documentation). Also, no @Paths are overridden in the test resource as you say in your last comment. They are generally chained along resources and sub-resources.

[sberyozkin]

@Foobartender

The documentation for deny-unannotated-endpoints and default-roles-allowed says that they only apply to endpoints without security annotations. But in case of inherited endpoints with security annotations, they are applied anyway. That's the bug.

OK

Also, no @paths are overridden in the test resource as you say in your last comment.

I missed you had a different method name in DerivedK, thought it was the same