Restore access to HttpSecurityPolicy retriever for current RoutingContext about quarkus

[michalvavrik]

An obviously good practice would be not to remove useful public methods when their code can still be present.

This is very true, however in Quarkus anything under the runtime package of any extension is considered to be part of the implementation and thus subject to any changes. Having said that, I do understand that this policy is not obvious to users, so I'll leave it up to @michalvavrik and @sberyozkin to determine how best to handle your ask.

I'd like to point out there is palpable difference between what this class does in 3.2 and main:

• https://github.com/quarkusio/quarkus/blob/main/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/AbstractPathMatchingHttpSecurityPolicy.java
• https://github.com/quarkusio/quarkus/blob/3.2/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/AbstractPathMatchingHttpSecurityPolicy.java

The fact that one method stayed intact is a pure lack. We need to make clear difference between what is intended to be API what is not. Whenever I make expose new methods / classes etc. I try to head in that direction, but it depends on what I have to change because I don't touch visibility of things that I am not working on.

It would be more useful to provide a feature and possibly document it for users.

[sberyozkin]

May be we can re-open this method in the short term (with note in the Java Docs that it is internal API and as such it can change, be removed, etc), and then later support injecting specific HTTP policies, with more methods added to HttpSecurityPolicy, etc, does it sound reasonable ?

[quarkus-bot]

/cc @sberyozkin (security)

[geoand]

cc @michalvavrik as well

[geoand]

An obviously good practice would be not to remove useful public methods when their code can still be present.

This is very true, however in Quarkus anything under the runtime package of any extension is considered to be part of the implementation and thus subject to any changes.
Having said that, I do understand that this policy is not obvious to users, so I'll leave it up to @michalvavrik and @sberyozkin to determine how best to handle your ask.

[michalvavrik]

I am bit confused by description of this ticket. Please @JDoumerc can you help me out? The description speaks about custom HttpAuthenticationMechanism and title speaks about HttpSecurityPolicy but implementation ideas about /AbstractPathMatchingHttpSecurityPolicy. They are very different things and I need to understand context when you are using it.

Based on your use case, we can expose useful method,. The method you are referring it and class underwent huge changes lately and I don't believe they were ever intended to use publicly. Problem is that if we consider classes in this package part of API, it would limit what we can do. That is one of the reasons when I create new methods (or change them like in this example) I also change their visibility unless it is intended to be used publicly.

Removed between 3.2 to 3.8 with no reason (see implementation ideas), this features is key to easily reduce deny of service risks on the security authentication provider transforming 403 on configured DENY paths into 404 responses.

Can you help me a bit, which feature you are referring to, please? Problem is that I don't understand how you are using it.

[JDoumerc]

Sorry for the confusing description that relies too much on the issue title...

Until 3.2, I was using a custom HttpAuthenticationMechanism to achieve 3 purposes:

1. Return a 404 on "DENY", and allow instantly on "PERMIT" http paths (as per Quarkus configuration)
2. Grant a service account with default rights for unauthenticated users => still works, not a concern
3. Call a custom SecurityIdentityAugmentor => still works, not a concern (and it was a workaround to call it here for a previous issue with GraphQL requests not going throught its accept method)

On 3.8, the 1. seems not possible anymore without duplicating Quarkus code (for a kind of copy of the AbstractPathMatchingHttpSecurityPolicy).
In detail I was using in my custom HttpAuthenticationMechanism :

    private AbstractPathMatchingHttpSecurityPolicy pathMatcher;
    @Override
    public Uni<SecurityIdentity> authenticate(RoutingContext context, IdentityProviderManager identityProviderManager) {
        // Check the configured Http authentication policy for the current request
        final List<HttpSecurityPolicy> permissionCheckers = pathMatcher.findPermissionCheckers(context);
        if (permissionCheckers != null) {
            for (HttpSecurityPolicy hsp : permissionCheckers) {
                if (hsp instanceof DenySecurityPolicy) {
                    context.fail(Status.NOT_FOUND.getStatusCode());
                    return Uni.createFrom().nullItem();
                } else if (hsp instanceof PermitSecurityPolicy) {
                    return Uni.createFrom().nullItem();
                }
            }
        }
        return oidcAuthenticator.authenticate(context, identityProviderManager).onItem().transformToUni(si -> apply(context, si));
    }

In 3.8, findPermissionCheckers was removed (but still present in pure term of functionnal code on L90-99).
I feel it seems important for implementing a custom HttpAuthenticationMechanism to get the configured HttpSecurityPolicy of the current request.
There is still the usefull checkPermission, but it could lead to too much call of a configured authentication system, and DoS issues.

For sure I am missing the final community focus for a global (and documented) emprovment/feature, so please keep ensuring it first ;-)

What I was previously achieving (custom return code on specific HttpSecurityPolicy) seems not possible by configuration, and neither was a default authentication for anonymous users (it was not the best idea, a dedicated gateway seems a better framework).

[JDoumerc]

It sounds more than reasonable for me since it would cover my use-case.

It is up to your teammate to see if you can spend time for this kind of slight adjustement (with no risk except for more questions/issuers if changed later, and with not much effort : refactor a method).

[michalvavrik]

May be we can re-open this method in the short term (with note in the Java Docs that it is internal API and as such it can change, be removed, etc),

That method signature has changed, so this is not a simple case of "re-openinig" (changing visibility), see https://github.com/quarkusio/quarkus/blob/main/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/AbstractPathMatchingHttpSecurityPolicy.java#L177.

and then later support injecting specific HTTP policies, with more methods added to HttpSecurityPolicy, etc, does it sound reasonable ?

AFAICT @JDoumerc (please correct me if I am wrong) injects either PathMatchingHttpSecurityPolicy or ManagementPathMatchingHttpSecurityPolicy and than wants to send 404 for paths where quarkus.http.auth.permission."permissions".policy=deny. I think proper solution is this:

example of application.properties:

quarkus.http.auth.permission."permissions".policy=not-found-policy

example of a custom policy:

import io.quarkus.security.identity.SecurityIdentity;
import io.smallrye.mutiny.Uni;
import io.vertx.ext.web.RoutingContext;
import jakarta.enterprise.context.ApplicationScoped;

@ApplicationScoped
public class NotFoundHttpSecurityPolicy implements HttpSecurityPolicy {

    @Override
    public Uni<CheckResult> checkPermission(RoutingContext context, Uni<SecurityIdentity> identity, AuthorizationRequestContext requestContext) {
        context.fail(404);

        // not sure what's next...
        return Uni.createFrom().failure(new IllegalStateException("I don't think this will cause an issue, but I don't like it. If you can see this anywhere, don't use this solution"));
    }

    @Override
    public String name() {
        return "not-found-policy";
    }
}

For sure I am missing the final community focus for a global (and documented) emprovment/feature, so please keep ensuring it first ;-)

Maybe we could improve this. I don't like failing routing context and than returning something, though it is no different to what you already do in your authenticate method.

Probably there are other options, like you can fail with an exception and define Vert.x route failure handler that handles it. Having ability to return specific status on denied in CheckResult would be IMHO best.

[michalvavrik]

Yeah, so there is also "do not authenticate on permit all" thingy. Let me think about it for a bit and get back to you.

[michalvavrik]

@JDoumerc code does path-specific authentication:

• permit all -> let others authenticate or anonymous
• deny all -> 404
• others: oidc auth

If authorization is the first thing that happens (disabled proactive auth), you can use custom path-specific policies that does that (and authenticates for other paths - just return augmented identity). Other options is to stick to your authentication mechanism and do this:

import io.quarkus.security.identity.IdentityProviderManager;
import io.quarkus.security.identity.SecurityIdentity;
import io.quarkus.vertx.http.runtime.HttpConfiguration;
import io.smallrye.mutiny.Uni;
import io.vertx.ext.web.RoutingContext;
import jakarta.enterprise.context.ApplicationScoped;

@ApplicationScoped
public class PathSpecificAuthMechanism implements HttpAuthenticationMechanism {

    private enum Action {
        PERMIT_ALL, DENY_ALL, OTHER;

        static Action of(String policy) {
            return switch (policy) {
                case "permit" -> PERMIT_ALL;
                case "deny" -> DENY_ALL;
                default -> OTHER;
            };
        }
    }

    private final ImmutablePathMatcher<Action> pathMatcher;

    public PathSpecificAuthMechanism(HttpConfiguration httpConfig) {
       // if you have things like multiple policies applied on single path, you might need to set accumulator to the builder
        var builder = ImmutablePathMatcher.<Action> builder();
        httpConfig.auth.permissions.values().stream()
                .filter(p -> p.enabled.orElse(Boolean.TRUE))
                .forEach(p -> p.paths.ifPresent(listOfPaths -> {
                    listOfPaths.forEach(path -> builder.addPath(path, Action.of(p.policy)));
                }));
        pathMatcher = builder.build();
    }

    @Override
    public Uni<SecurityIdentity> authenticate(RoutingContext context, IdentityProviderManager identityProviderManager) {
        var action = pathMatcher.match(context.normalizedPath());
        // do what you did previously
    }

    // do other things, maybe tune priority etc....
}

I didn't test any of this, so better be careful (also it's Friday :-))

[michalvavrik]

Hey @sberyozkin , if you think there is some feature we should provide related to use case described in this issue, please write down suggestion. Suggestion above should provide same capabilities as was provided previously. I prefer to not expose some methods on AbstractPathMatchingHttpSecurityPolicy. Thanks