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

quarkus.http.auth.proactive = false and @PermitAll still trigger custom auth #27316

Closed
fediazgon opened this issue Aug 16, 2022 · 9 comments · Fixed by #28061
Closed

quarkus.http.auth.proactive = false and @PermitAll still trigger custom auth #27316

fediazgon opened this issue Aug 16, 2022 · 9 comments · Fixed by #28061
Labels
kind/bug Something isn't working
Milestone

Comments

@fediazgon
Copy link

Describe the bug

I have a custom HttpAuthenticationMechanism that is still triggered even after adding @PermitAll to an endpoint and quarkus.http.auth.proactive = false to application.properties.

Expected behavior

HttpAuthenticationMechanism#authenticate should not be called.

Actual behavior

HttpAuthenticationMechanism#authenticate is called.

How to Reproduce?

This is my custom HttpAuthenticationMechanism:

@ApplicationScoped
public class ApiKeyAuthenticationMechanism implements HttpAuthenticationMechanism {

  private static final String API_KEY_HEADER = "X-API-Key";

  private static final Uni<SecurityIdentity> UNAUTHENTICATED = Uni.createFrom().nullItem();

  @Override
  public Uni<SecurityIdentity> authenticate(
      final RoutingContext context, final IdentityProviderManager identityProviderManager) {
    return Optional.ofNullable(context.request().headers().get(API_KEY_HEADER))
        .map(ApiKeyAuthenticationRequest::new)
        .map(authRequest -> HttpSecurityUtils.setRoutingContextAttribute(authRequest, context))
        .map(identityProviderManager::authenticate)
        .orElse(UNAUTHENTICATED);
  }

  @Override
  public Set<Class<? extends AuthenticationRequest>> getCredentialTypes() {
    return Set.of(ApiKeyAuthenticationRequest.class);
  }

  @Override
  public Uni<ChallengeData> getChallenge(final RoutingContext context) {
    return Uni.createFrom().nullItem();
  }

  @Override
  public Uni<Boolean> sendChallenge(final RoutingContext context) {
    return Uni.createFrom().item(false);
  }
}

The custom IdentityProvider:

@ApplicationScoped
public class APiKeyIdentityProvider implements IdentityProvider<ApiKeyAuthenticationRequest> {

  @Override
  public Class<ApiKeyAuthenticationRequest> getRequestType() {
    return ApiKeyAuthenticationRequest.class;
  }

  @Override
  public Uni<SecurityIdentity> authenticate(
      ApiKeyAuthenticationRequest apiKeyAuthenticationRequest,
      AuthenticationRequestContext authenticationRequestContext) {
    return Uni.createFrom()
        .item(
            QuarkusSecurityIdentity.builder().setAnonymous(true).addRoles(Set.of("USER")).build());
  }
}

This is the endpoint:

@Path("/permit")
public class PermitResource {

  @GET
  @PermitAll
  public Uni<Response> getMethod() {
    return Uni.createFrom().nullItem();
  }
}

Output of uname -a or ver

Darwin C02Z63TTLVDR 21.6.0 Darwin Kernel Version 21.6.0: Sat Jun 18 17:07:25 PDT 2022; root:xnu-8020.140.41~1/RELEASE_X86_64 x86_64

Output of java -version

java version "17.0.3" 2022-04-19 LTS Java(TM) SE Runtime Environment (build 17.0.3+8-LTS-111) Java HotSpot(TM) 64-Bit Server VM (build 17.0.3+8-LTS-111, mixed mode, sharing)

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.11.2.Final

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

Apache Maven 3.8.5 (3599d3414f046de2324203b78ddcf9b5e4388aa0)

Additional information

No response

@fediazgon fediazgon added the kind/bug Something isn't working label Aug 16, 2022
@fediazgon fediazgon changed the title quarkus.http.auth.proactive = false and @PermitAll still triggers custom auth quarkus.http.auth.proactive = false and @PermitAll still trigger custom auth Aug 16, 2022
@sberyozkin
Copy link
Member

@fediazgon #23775 should've resolved it, can you please create a reproducer ?

@fediazgon
Copy link
Author

Thanks for the prompt response @sberyozkin. I've created a reproducer here https://github.com/fediazgon/quarkus-reproducer-27316.

I think the problem is not @PermitAll but quarkus.http.auth.proactive = false. My IdentityProvider is still invoked even when disabling proactive auth.

For example, for this resource (included in the reproducer):

 @GET
  @Produces(MediaType.TEXT_PLAIN)
  @PermitAll
  @Path("/unprotected")
  public String unprotectedRes() {
    System.out.println("Inside GreetingResource/unprotected");
    System.out.println("===================================");
    return "Hello from RESTEasy Reactive (unprotected)";
  }

The following call:

curl -o - -I http://localhost:8080/api/repr/protected -H 'X-API-Key: 123abc' 

has the following trace:

ApiKeyAuthenticationMechanism called
ApiKeyAuthenticationRequest created
APiKeyIdentityProvider called
Inside GreetingResource/unprotected

I would not have expected APiKeyIdentityProvider to be called since it can do some validation and return an error in that case.

@fediazgon
Copy link
Author

I could use Uni.createFrom().deferred in my APiKeyIdentityProvider to avoid errors in validation/hitting the database but it looks more like a workaround.

@sberyozkin
Copy link
Member

@fediazgon Apologies for a delay. @michalvavrik Hi Michal, would you be interested to check this issue ?

@michalvavrik
Copy link
Contributor

@sberyozkin yes, this is interesting, I'll have a look. thank you

@sberyozkin
Copy link
Member

@michalvavrik Thanks Michal

@michalvavrik
Copy link
Contributor

This is super easy to fix, but I need to be sure there is an agreement it's a bug (for me this is expected behavior, but let see about you :-)).

We call your APIKeyIdentityProvider from here

Uni<SecurityIdentity> potentialUser = authenticator.attemptAuthentication(event).memoize().indefinitely();
, the stack looks like this

-> io.quarkus.vertx.http.runtime.security.HttpSecurityRecorder#authenticationMechanismHandler
-> (matchingMechUni == null) io.quarkus.vertx.http.runtime.security.HttpAuthenticator#attemptAuthentication
-> io.quarkus.vertx.http.runtime.security.HttpAuthenticator#createSecurityIdentity
-> org.issue.ApiKeyAuthenticationMechanism#authenticate
-> io.quarkus.security.runtime.QuarkusIdentityProviderManagerImpl#authenticate
-> io.quarkus.security.runtime.QuarkusIdentityProviderManagerImpl#handleSingleProvider
-> org.issue.APiKeyIdentityProvider#authenticate

Why do we do it? We basically say If someone needs the identity, he can subscribe to the identity we put into the routing context, so we need to create the Uni. We can just wrap the method call that creates your Uni with something like this:

Uni<SecurityIdentity> potentialUser = Uni.createFrom().nullItem().flatMap(n -> authenticator.attemptAuthentication(event).memoize().indefinitely());

then when you call your reproducer

curl -o - -I http://localhost:8080/api/repr/unprotected -H 'X-API-Key: 123abc' 

you get just Inside GreetingResource/unprotected. For

curl -o - -I http://localhost:8080/api/repr/protected -H 'X-API-Key: 123abc'

you would still get

ApiKeyAuthenticationMechanism called
ApiKeyAuthenticationRequest created
APiKeyIdentityProvider called
SecurityIdentityAugmentor called

as then we have to subscribe to identity in order to determine your roles. Needless to say I can't guarantee some other extension won't call io.quarkus.security.identity.IdentityProviderManager#authenticate for their own reasons without subscribing there in the future.

My point is that the identity is correctly requested (subscribed) only when we really need it, but we need Uni every time. I believe you should only do business logic (that's in your words validation/hitting the database, or for the reproducer System.out.println("APiKeyIdentityProvider called");) when the Uni is subscribed, not before. But if I get it right, Fernando (@fediazgon) don't agree. WDYT @sberyozkin?

@sberyozkin
Copy link
Member

sberyozkin commented Sep 19, 2022

Hi @michalvavrik Thanks for the analysis and sorry for missing your ping.

IMHO though it is not providing a resolution as such, quarkus.http.auth.proactive=false is a message to the security runtime that the security identity creation should be enforced only if it is required, so the identity providers should not be called.

@michalvavrik
Copy link
Contributor

michalvavrik commented Sep 19, 2022

@sberyozkin IMHO it goes against reactive approach (if you are doing logic when Uni is created, there is a little reason for using Uni at all; I mean, what extra value does using Uni add you then?). I'll create PR and we can discuss it there (as I have resolution - reproducer is fixed and I understand why the security is called [but I respect you think it shouldn't be called]).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants