Open JDoumerc opened 1 week ago
/cc @sberyozkin (security)
cc @michalvavrik as well
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 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.
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:
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.
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:
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).
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 ?
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).
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.
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.
@JDoumerc code does path-specific authentication:
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 :-))
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
Hello again,
Back to confirm that the workaround with the parameter injection of an HttpConfiguration
to build an ImmutablePathMatcher
covers all my needs. And the overhead is less than expected : the simple initialization at construction you proposed (and an instance ImmutablePathMatcher in memory), which are both fine.
Thanks all for a really good reactivy, and thanks @michalvavrik for the ready to use example.
For me the issue can be closed, I let you decide whether if you want to document/add any feature ...
Perhaps speaking about the HttpConfiguration
in the page https://quarkus.io/guides/security-authorize-web-endpoints-reference would be enough, as it might have made me look for in the right direction (meaning public API).
Description
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.
With this method, on a custom HttpAuthenticationMechanism, one can instead of returning a 403, analyze the current path, see if there is any DENY policy on it, and return a 404 (or whatever) before trying any authentication.
Without this method, is gets tricky and heavy to achieve the same goal.
Implementation ideas
The method: public static List findPermissionCheckers(RoutingContext context);
Became private with a new parameter (https://github.com/quarkusio/quarkus/blob/05763506f0fa1a0d2884059381cab8bbbc47e265/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/AbstractPathMatchingHttpSecurityPolicy.java#L177):
But its implementation still exists on (L90-99 https://github.com/quarkusio/quarkus/blob/05763506f0fa1a0d2884059381cab8bbbc47e265/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/AbstractPathMatchingHttpSecurityPolicy.java#L90) of method:
An obviously good practice would be not to remove usefull public methods when their code can still be present.