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

Unable to Overwrite Response using Security-JPA & RestEasy-Classic #27180

Closed
BrainShit opened this issue Aug 8, 2022 · 8 comments · Fixed by #27921
Closed

Unable to Overwrite Response using Security-JPA & RestEasy-Classic #27180

BrainShit opened this issue Aug 8, 2022 · 8 comments · Fixed by #27921
Labels
area/security kind/bug Something isn't working
Milestone

Comments

@BrainShit
Copy link

Describe the bug

I'm currently unable to overwrite the Response with a CustomFormAuthenticationMechanism when using security-jpa with quarkus using RestEasy-Classic. I did not test ReastEasy-Reactive.

The last known version this was possible was using 2.7.5.Final.

If there's anything else you need let me know!

Here's my Implementation of the HttpAuthenticationMechanism

@Alternative
@Priority(1)
@ApplicationScoped
public class CustomFormAuthenticationMechanism implements HttpAuthenticationMechanism {

    private static final Logger LOGGER = LoggerFactory.getLogger(CustomFormAuthenticationMechanism.class);

    @Inject
    FormAuthenticationMechanism delegate;

    @Override
    public Uni<SecurityIdentity> authenticate(RoutingContext context, IdentityProviderManager identityProviderManager) {
        return delegate.authenticate(context, identityProviderManager);
    }

    @Override
    public Uni<ChallengeData> getChallenge(RoutingContext context) {
        Uni<ChallengeData> rValue = delegate.getChallenge(context);
        return rValue.onItem().transform(challengeData -> {
            if (challengeData.status == Response.Status.FOUND.getStatusCode()) {
                return new ChallengeData(Response.Status.UNAUTHORIZED.getStatusCode(), challengeData.headerName, challengeData.headerContent);
            }
            return challengeData;
        });
    }

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

    @Override
    public HttpCredentialTransport getCredentialTransport() {
        return delegate.getCredentialTransport();
    }

    @Override
    public Uni<HttpCredentialTransport> getCredentialTransport(RoutingContext context) {
        return delegate.getCredentialTransport(context);
    }

    @Override
    public Uni<Boolean> sendChallenge(RoutingContext context) {
        return getChallenge(context).map(new ChallengeSender(context));
    }
}

Expected behavior

Changed Challenge Data with changed Response should be returned resulting in a 401 Response

Actual behavior

Default Response (302) is returned

How to Reproduce?

Reproducer: https://github.com/BrainShit/custom-formauth-reproducer

  1. Start the Application
  2. curl the security endpoint with user "admin" and a wrong password

Output of uname -a or ver

No response

Output of java -version

11.0.8 2020-07-14

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)

No response

Additional information

No response

@BrainShit BrainShit added the kind/bug Something isn't working label Aug 8, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Aug 8, 2022

/cc @sberyozkin

@BrainShit
Copy link
Author

Is there any update on this? As this is preventing me from updating further than 2.7.5

@michalvavrik
Copy link
Contributor

Hello, I can reproduce it. I'll have a look and let you know.

@michalvavrik
Copy link
Contributor

michalvavrik commented Sep 12, 2022

Hi @sberyozkin ,
when you are using custom HttpAuthenticationMechanism and call one of:

  • io.quarkus.vertx.http.runtime.security.FormAuthenticationMechanism
  • io.quarkus.smallrye.jwt.runtime.auth.JWTAuthMechanism
  • io.quarkus.oidc.runtime.AbstractOidcAuthenticationMechanism
  • io.quarkus.security.webauthn.WebAuthnAuthenticationMechanism
  • io.quarkus.vertx.http.runtime.security.BasicAuthenticationMechanism
  • io.quarkus.vertx.http.runtime.security.MtlsAuthenticationMechanism
  • has path specific mechanism

from inside of your methods (see issue description above, the very same is done in docs here https://quarkus.io/guides/security-customization#dealing-with-more-than-one-httpauthenticationmechanism), then custom auth mechanism method Uni<ChallengeData> getChallenge(RoutingContext context) won't ever get called as they set context.put(HttpAuthenticationMechanism.class.getName(), this); with themselves. You can find the problematic line here

HttpAuthenticationMechanism matchingMech = routingContext.get(HttpAuthenticationMechanism.class.getName());

I understand that your intention here #16446 was to do right challenge, not all the challenges. I also realize @BrainShit can do this

    @Override
    public Uni<SecurityIdentity> authenticate(RoutingContext context, IdentityProviderManager identityProviderManager) {
        var authenticated = delegate.authenticate(context, identityProviderManager);
        context.put(HttpAuthenticationMechanism.class.getName(), this);
        return authenticated;
    }

or

    @Override
    public Uni<SecurityIdentity> authenticate(RoutingContext context, IdentityProviderManager identityProviderManager) {
        var authenticated = delegate.authenticate(context, identityProviderManager);
        context.put(HttpAuthenticationMechanism.class.getName(), null);
        return authenticated;
    }

and reproducer is fixed!

From here, these are just my opinions, not facts:

  • it should be better documented, the docs shows very similar example to the issue reproducer and from docs example, you would expect getChallenge to get called
  • setting context.put(HttpAuthenticationMechanism.class.getName(), this); looks like a workaround to me. Are we really expecting custom auth mechanisms to put themselves into the context? (I mean, there should be contract that says do this and xyz happens)

I want to fix the issue (unless you think there is none :-)), but I need to hear what is expected behavior - can you specify algorithm how correct getChallenge is selected?

Thanks a lot!

@michalvavrik
Copy link
Contributor

michalvavrik commented Sep 13, 2022

@sberyozkin I re-read docs now and following paragraph

In some cases such a default logic of selecting the challenge is exactly what is required by a given application, but sometimes it may not meet the requirements. In such cases (or indeed in other similar cases where you'd like to change the order in which the mechanisms are asked to handle the current authentication or challenge request), you can create a custom mechanism and choose which mechanism should create a challenge, for example:

suggests to use an example that won't work as I described above (please see my previous comment), because JwtAuthMechanism sets itself into RouteContext and thus CustomAwareJWTAuthMechanism#getChallenge is not called.

I'd prefer to hear from you once you have time for this (no hurry) as it's not explained why 6 mechanisms I listed above are preferred to a custom mechanism. I read the PRs that submitted these changes and it's not clear, but I can run a few tests to find out.

@michalvavrik
Copy link
Contributor

michalvavrik commented Sep 13, 2022

#22206 #22483 #22404 gave me an idea, I'll try to figure it out on my own.

Originally HttpAuthenticationMechanism.getCredentialTransport was only used to fail when more than one auth mechanism with the same cred transport is used. Now it is also used to find the best candidate for a challenge (for ex, if basic and oidc/web-app are used and basic fails then it is guaranteed basic will return the challenge, even without the priority based sorting). It is also now used for a path-specific authentication: https://quarkus.io/guides/security#path-specific-authentication-mechanism.

HttpAuthenticationMechanism.getCredentialTransport is only called for path specific mechanism. Maybe an easy fix is to just add the mechanism into context when it's part of the path auth mechanism.

@sberyozkin
Copy link
Member

@michalvavrik Apologies, seeing/reading your analysis only now as I've noticed your PR...

@michalvavrik
Copy link
Contributor

np

michalvavrik added a commit to michalvavrik/quarkus that referenced this issue Sep 14, 2022
michalvavrik added a commit to michalvavrik/quarkus that referenced this issue Sep 14, 2022
michalvavrik added a commit to michalvavrik/quarkus that referenced this issue Sep 14, 2022
michalvavrik added a commit to michalvavrik/quarkus that referenced this issue Sep 14, 2022
michalvavrik added a commit to michalvavrik/quarkus that referenced this issue Sep 14, 2022
@quarkus-bot quarkus-bot bot added this to the 2.14 - main milestone Sep 14, 2022
@gsmet gsmet modified the milestones: 2.14 - main, 2.13.0.Final Sep 20, 2022
gsmet pushed a commit to gsmet/quarkus that referenced this issue Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants