quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.73k stars 2.67k forks source link

Micrometer URI templating does not apply for unauthorized requests #24938

Closed ivansenic closed 1 year ago

ivansenic commented 2 years ago

Describe the bug

In case a HTTP request ends up in 401 Unauthorized, the Micrometer URI tag will not be templated, but include the full URI. If the request with the same URI is correctly authenticated, then the URI will be templated.

Expected behavior

Actual behavior

How to Reproduce?

Easy to reproduce with:

@Path("/hello")
public class GreetingResource {

    @GET
    @Path("something/{message}")
    @Produces(MediaType.TEXT_PLAIN)
    public Uni<RestResponse<String>> hello(@PathParam("message") String message) {
        return Uni.createFrom().item(RestResponse.ok(message));
    }
}

and:

%dev.quarkus.security.users.embedded.enabled=true
%dev.quarkus.security.users.embedded.plain-text=true
%dev.quarkus.security.users.embedded.users.scott=reader
%dev.quarkus.security.users.embedded.users.stuart=writer
%dev.quarkus.security.users.embedded.roles.scott=READER
%dev.quarkus.security.users.embedded.roles.stuart=READER,WRITER

quarkus.http.auth.permission.default.paths=/hello/*
quarkus.http.auth.permission.default.policy=authenticated

Results in the following URIs, which provide that 401 requests are not templated and 200 are:

http_server_requests_seconds_count{method="GET",outcome="CLIENT_ERROR",status="401",uri="/hello/something/my",} 3.0
http_server_requests_seconds_sum{method="GET",outcome="CLIENT_ERROR",status="401",uri="/hello/something/my",} 0.013654029
http_server_requests_seconds_count{method="GET",outcome="SUCCESS",status="200",uri="/hello/something/{message}",} 1.0
http_server_requests_seconds_sum{method="GET",outcome="SUCCESS",status="200",uri="/hello/something/{message}",} 0.029346689
http_server_requests_seconds_count{method="GET",outcome="CLIENT_ERROR",status="401",uri="/hello/something/asdsad",} 3.0
http_server_requests_seconds_sum{method="GET",outcome="CLIENT_ERROR",status="401",uri="/hello/something/asdsad",} 0.535294234

Output of uname -a or ver

Linux ise-Precision-5550 5.13.0-39-generic #44~20.04.1-Ubuntu SMP Thu Mar 24 16:43:35 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.8.0.Final

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

Apache Maven 3.8.4 (9b656c72d54e5bacbed989b64718c159fe39b537) Maven home: /home/ise/.m2/wrapper/dists/apache-maven-3.8.4-bin/52ccbt68d252mdldqsfsn03jlf/apache-maven-3.8.4 Java version: 17.0.2, vendor: Private Build, runtime: /usr/lib/jvm/java-17-openjdk-amd64 Default locale: en_US, platform encoding: UTF-8 OS name: "linux", version: "5.13.0-39-generic", arch: "amd64", family: "unix"

Additional information

No response

quarkus-bot[bot] commented 2 years ago

/cc @ebullient

ivansenic commented 2 years ago

Anybody looking into this?

ebullient commented 2 years ago

I have a new person on the team now, and once he's done a few things to get oriented, I was going to have a try with this one with him. It will be a good walk through some finer details.

Does using the matchPattern as a workaround work? I'm guessing I missed a path..

ivansenic commented 2 years ago

Using the match pattern does work and I am using this as the workaround at the moment.

@ebullient I also noticed that it's impossible to customize the response in case of 401 Unauthorized. I followed the instructions here https://quarkus.io/guides/security-built-in-authentication#how-to-customize-authentication-exception-responses, however it does not end up in calling that mapper. Proactive auth on/off does not have any influence as well. Is this somehow correlated? Should I open a separate issue?

sberyozkin commented 2 years ago

@ivansenic,

@ebullient I also noticed that it's impossible to customize the response in case of 401 Unauthorized. I followed the instructions here https://quarkus.io/guides/security-built-in-authentication#how-to-customize-authentication-exception-responses, however it does not end up in calling that mapper. Proactive auth on/off does not have any influence as well. Is this somehow correlated? Should I open a separate issue?

Yes, please create a separate issue with a reproducer

isacandrei commented 2 years ago

I can confirm that this does not only affect status 401, but also 409 and RESET. (Quarkus 2.7.5.Final)

ebullient commented 2 years ago

y.. all the 4xx shortcut the successful path that allows us to retrieve the template. There are some constraints that make build-time discovery of restful paths difficult, and getting the paths correctly affiliated with one of the constructed application paths when full invocation doesn't happen is non-trivial at best. This is why matchPattern exists. ;)

isacandrei commented 2 years ago

Thanks for the update. This is unfortunate though. Although matchPatterns solves the problem, if one would want to use these metrics across a wide set of microservices, each with 5-20 endpoints it would require considerable effort. Is there anything that can be done or I can help with to bring this home, or is it a technical limitation we cannot avoid?

brunobat commented 2 years ago

related to RestClient errors statuscode are not shown in micrometer

brunobat commented 2 years ago

Took a look at this... The only way I see to permanently fix this templating issue is to resolve the template and put it in the context, before the vertx authentication is executed... around here: io.quarkus.vertx.http.runtime.security.HttpAuthorizer:126 Vertx will delegate to resteasy classic only after that. No filters will help here. This is non trivial, though.

geoand commented 1 year ago

There might be a very intricate way to get the required template. I need to look into it, but I can warn you right now that if I can get it to work, it won't be pretty.

geoand commented 1 year ago

As promised, it's ugly but it works (I tested manually).

If we like this, I can add some tests and mark it ready for review

brunobat commented 1 year ago

Will this also work with the OpenTelemetry extension? It suffers from the same condition.

geoand commented 1 year ago

Yes

On Wed, Jul 12, 2023, 16:40 Bruno Baptista @.***> wrote:

Will this also work with the OpenTelemetry extension? It suffers from the same condition.

— Reply to this email directly, view it on GitHub https://github.com/quarkusio/quarkus/issues/24938#issuecomment-1632551193, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBMDPYXGVY2MSONVQN2YA3XP2SMNANCNFSM5TNRYM3Q . You are receiving this because you were assigned.Message ID: @.***>