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

PermissionChecker method sometimes receives unaugmented SecurityIdentity even though augmentor did run successfully #44990

Closed
okarmazin opened this issue Dec 8, 2024 · 7 comments · Fixed by #45012
Labels
Milestone

Comments

@okarmazin
Copy link

okarmazin commented Dec 8, 2024

Describe the bug

I'm trying out the new @PermissionChecker mechanism on a method. Every once in a while something happens and the annotated method starts receiving an unaugmented SecurityIdentity argument from Quarkus for all future invocations of that method, even though previously (some minutes ago) it did receive the augmented identity.

After the breakage, the SecurityIdentity received by the PermissionChecker contains the following attributes (as seen by identity.attributes.forEach(::println):

configuration-metadata: io.quarkus.oidc.OidcConfigurationMetadata@1db376bd
io.quarkus.security.identity.AuthenticationRequestContext: io.quarkus.security.runtime.QuarkusIdentityProviderManagerImpl$1@1fd28ed3
io.vertx.ext.web.RoutingContext: io.vertx.ext.web.impl.RoutingContextImpl@5c29dcc
userinfo: io.quarkus.oidc.UserInfo@51da717f
tenant-id: Default

It's authenticated and contains OIDC UserInfo, but my custom attributes are missing.

The behavior happens consistently, but I have not yet been able to find reproduction conditions. For some time after server starts, the secured method receives the correct, augmented identity. I have been able to observe the behavior when I come back to the server after yet-unknown amount of time passes.

In the same controller as this @PermissionsAllowed-annotated method, there are sibling request handlers which perform their own permission checks manually. For these checks they use SecurityIdentity which is constructor-injected into the controller. This injected identity is always correctly augmented.

So far logging shows that the augmentor does run for the affected requests and completes successfully, along with the entire authentication chain.

My current line of inquiry is OIDC userinfo cache expiration or something similar, related to OIDC. The one shared observation so far is that it happens after "some time" passes between consecutive requests made by an authenticated user using a still-valid Bearer token.

I'll keep investigating and will try to find a minimal shareable reproducer project, but I'm currently extremely busy.

Here's my general setup, maybe something can be gleaned from it:

Quarkus: 3.17.2
installed extensions:

implementation(libs.quarkus.arc)
implementation(libs.quarkus.flyway)
implementation(libs.quarkus.hibernate.orm)
implementation(libs.quarkus.hibernate.orm.panache.kotlin)
implementation(libs.quarkus.hibernate.validator)
implementation(libs.quarkus.jdbc.postgresql)
implementation(libs.quarkus.kotlin)
implementation(libs.quarkus.logging.json)
implementation(libs.quarkus.micrometer.registry.prometheus)
implementation(libs.quarkus.oidc)
implementation(libs.quarkus.rest)
implementation(libs.quarkus.rest.kotlinserialization)
implementation(libs.quarkus.rest.qute)
implementation(libs.quarkus.smallrye.health)

OIDC config from application.properties:

quarkus.oidc.auth-server-url=${AUTH0_SERVER_URL}
quarkus.oidc.application-type=service
quarkus.oidc.client-id=${AUTH0_CLIENT_ID}
quarkus.oidc.credentials.secret=${AUTH0_CLIENT_SECRET}
quarkus.oidc.token.verify-access-token-with-user-info=true
quarkus.oidc.authentication.force-redirect-https-scheme=true
# Must enable token cache to not exhaust the Auth0 rate limits!
# If not set, Quarkus would perform UserInfo request on each access,
# leading to immediate UserInfo limit exhaustion
# https://auth0.com/docs/troubleshoot/customer-support/operational-policies/rate-limit-policy/rate-limit-configurations
quarkus.oidc.token-cache.max-size=1000
quarkus.oidc.token-cache.time-to-live=5M
@ApplicationScoped
class PermissionChecker(private val repository: MyRepository) {
    private fun performCheck(identity: SecurityIdentity, arg1: String): Boolean {
        // FAIL: cannot extract expected attribute from the identity!   <================
        val user = identity.extractUser() ?: throw UnauthorizedException()
    }

    @PermissionChecker("permission-check-read")
    @Blocking
    fun decadeRead(identity: SecurityIdentity, arg1: String): Boolean =
        performCheck(identity, arg1)
}

@Path("/api/stuff")
@RolesAllowed("\${myroles.floor-manager-and-above}")
@Blocking
class ApiController(
    private val repo: Repository1,
    private val securityIdentity: SecurityIdentity, // <=== This SecurityIdentity is ALWAYS correct
) {

    @Path("/endpoint")
    @GET
    fun getData(@RestQuery arg1: String, ): ApiDto {
        return constructResponse(arg1)
    }

    @PermissionsAllowed("permission-check-read")
    protected fun constructResponse(arg1: String): ApiDto {
        // ...
    }
    
}

=================================================================================

@ApplicationScoped
class IdentityAugmentor : SecurityIdentityAugmentor {

    @Inject
    internal lateinit var identitySupplierInstance: Instance<IdentitySupplier>

    override fun augment(
        identity: SecurityIdentity,
        context: AuthenticationRequestContext,
        attributes: Map<String, Any>
    ): Uni<SecurityIdentity> = doAugment(identity, context, HttpSecurityUtils.getRoutingContextAttribute(attributes))

    override fun augment(identity: SecurityIdentity, context: AuthenticationRequestContext): Uni<SecurityIdentity> =
        doAugment(identity, context, null)

    private fun doAugment(identity: SecurityIdentity, context: AuthenticationRequestContext, routingContext: RoutingContext?): Uni<SecurityIdentity> {
        val identitySupplier: IdentitySupplier = identitySupplierInstance.get()
        identitySupplier.originalIdentity = identity
        return context.runBlocking(identitySupplier)
    }
}

@Dependent
internal class IdentitySupplier(private val userRepository: UserRepository) : Supplier<SecurityIdentity> {
    lateinit var originalIdentity: SecurityIdentity

    @ActivateRequestContext
    override fun get(): SecurityIdentity = topLevelAugmentIdentity(originalIdentity, userRepository)
}

internal fun topLevelAugmentIdentity(
    originalIdentity: SecurityIdentity,
    userRepository: UserRepository
): SecurityIdentity {
    // Augmentation runs without issues
}

I will keep tinkering and looking for reproduction steps

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

Unfortunately I have not found reproduction steps as this doesn't happen immediately and involves real world 3rd party authentication. I'll post steps if and when I do.

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

3.17.2

Build tool (ie. output of mvnw --version or gradlew --version)

------------------------------------------------------------
Gradle 8.11.1
------------------------------------------------------------

Build time:    2024-11-20 16:56:46 UTC
Revision:      481cb05a490e0ef9f8620f7873b83bd8a72e7c39

Kotlin:        2.0.20
Groovy:        3.0.22
Ant:           Apache Ant(TM) version 1.10.14 compiled on August 16 2023
Launcher JVM:  19.0.2 (Oracle Corporation 19.0.2+7-44)
Daemon JVM:    /Users/ondra/Library/Java/JavaVirtualMachines/openjdk-19.0.2/Contents/Home (no JDK specified, using current Java home)
OS:            Mac OS X 15.1.1 aarch64

Additional information

No response

@okarmazin okarmazin added the kind/bug Something isn't working label Dec 8, 2024
Copy link

quarkus-bot bot commented Dec 8, 2024

/cc @geoand (kotlin), @sberyozkin (security)

@sberyozkin
Copy link
Member

@okarmazin OIDC UserInfo management (in the cache) should not impact the augmentation, the only relationship is that Quarkus OIDC would add a userinfo attribute to the identity once UserInfo is available.

Where is identity.extractUser() coming from ? Quarkus security SecurityIdentity does not have this method...

@okarmazin
Copy link
Author

okarmazin commented Dec 8, 2024

@sberyozkin That's just one of my helper extension functions (it's actually called requireActiveUser, I changed it in the original snippet:

object MyAttributes {
    const val user = "my.user"
}

fun SecurityIdentity.getUser(): User? = getAttribute(MyAttributes.user)

fun SecurityIdentity.requireActiveUser(): User {
    val user = getUser()
    if (user == null) {
        Log.error("SecurityIdentity does not contain User. Principal name: ${principal.name}")
        throw UnauthorizedException()
    }
    if (!user.isActive) {
        Log.error("SecurityIdentity contains User, but it's inactive. User: $user")
        throw InactiveUserException()
    }
    return user
}

fun SecurityIdentity.oidcUserInfo(): UserInfo? = getAttribute("userinfo")

When I post similar code in the future, I'll make sure to write it as performAction(object) instead of the Kotlin syntax sugar object.performAction() to reduce possible confusion

@sberyozkin
Copy link
Member

@okarmazin

When I post similar code in the future, I'll make sure to write it as performAction(object) instead of the Kotlin syntax sugar object.performAction() to reduce possible confusion

Yeah, I'm not working with Kotlin so for someone like myself it can be confusing :-).

@gsmet
Copy link
Member

gsmet commented Dec 9, 2024

/cc @michalvavrik for awareness.

@michalvavrik
Copy link
Member

This sounds like a race between augmentors, we need to make sure that PermissionChecker is only applied when all the augmentors has finished. You don't need to spend time on the reproducer @okarmazin , we use internally augmentors as well an we need to make sure that it runs after all user-defined augmentors. Sounds alright?

@michalvavrik
Copy link
Member

michalvavrik commented Dec 9, 2024

BTW you should be able to @Inject SecurityIdentity and that one will always be up to date. Sorry @okarmazin for time you spend on this. I'll have a look today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants