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

Since Micronaut Security 4.6.8 route match is randomly null in the SecurityFilter #1683

Open
loicmathieu opened this issue Apr 15, 2024 · 32 comments · Fixed by kestra-io/kestra#3611
Assignees
Labels
priority: high High priority type: bug Something isn't working

Comments

@loicmathieu
Copy link

Expected Behavior

Migrating to 4.3.4 to 4.3.7 should not change the behavior of the Micronaut SecurityFilter.

Actual Behaviour

After migrating from 4.3.4 to 4.3.7, we started to see randomly routeMatch request attribute being null in the Micronaut SecurityFilter.

This has been seen using a debugger, see the following screenshot that show that routeMatch is null for a request GET /api.v1/demo/config but this endpoint is defined in our application so there should be a route match.

Dowgrading to 4.3.4 fixes the issue.

route-match-issue

Steps To Reproduce

No response

Environment Information

Java 17
Linux

Example Application

No response

Version

4.3.7

@loicmathieu
Copy link
Author

The config endpoint is defined as is:

@Slf4j
@Controller("/api/v1")
public class MiscController {
    @Get("{/tenant}/configs")
    @ExecuteOn(TaskExecutors.IO)
    @Operation(tags = {"Misc"}, summary = "Get current configurations")
    public Configuration configuration() throws JsonProcessingException {
        // ...
    }
}

@graemerocher
Copy link
Contributor

don't see anything related to this other than documenting the route match behaviour in the diff micronaut-projects/micronaut-core@v4.3.4...v4.3.7

Do you have steps to reproduce?

@loicmathieu
Copy link
Author

@graemerocher indeed there isn't anything that seems to be able to cause such issue. We are rolling back to the old version of Micronaut, I'll confirm tomorrow if it fixes the issue then I'll try to create a reproducer but as it seems "random" this may be hard to reproduce.

@loicmathieu
Copy link
Author

I can confirm that rolling back to 4.3.4 works in the environment where it causes the issue. I'll try to create a reproducer but as it seems "random" this may be hard to reproduce.

@yawkat
Copy link
Member

yawkat commented Apr 16, 2024

FYI it's likely not core v4.3.4...v4.3.7 you need to look at, you need to look at the equivalent micronaut-core versions for those platform versions instead.

@graemerocher
Copy link
Contributor

so yeah this is the diff so it is between 4.3.9 and 4.3.12 of core

@graemerocher
Copy link
Contributor

@graemerocher
Copy link
Contributor

not sure if there is much relevant there either micronaut-projects/micronaut-core@v4.3.9...v4.3.12 🤔

@sdelamo sdelamo added the type: bug Something isn't working label Apr 16, 2024
@sdelamo
Copy link
Contributor

sdelamo commented Apr 18, 2024

I think I reproduced this in a project. I don't have a reproducer yet. However, upgrading an application that uses session based authentication from 4.3.4 to 4.4.0 I started to unable login in production. Downgraded solved the issue.

not sure if there is much relevant there either v4.3.9...v4.3.12 🤔

Maybe something here: micronaut-projects/micronaut-core@1ab87c5 This commits seems the only one touching the Router.

@sdelamo sdelamo added the priority: high High priority label Apr 18, 2024
@graemerocher
Copy link
Contributor

@sdelamo can you find that commit where the regression exists?

@sdelamo
Copy link
Contributor

sdelamo commented Apr 23, 2024

For my application, the solution was to force the usage of the io.micronaut.http.cookie.DefaultServerCookieEncoder via the creation of a file:

src/main/resources/META-INF/services/io.micronaut.http.cookie.ServerCookieEncoder

With content:

io.micronaut.http.cookie.DefaultServerCookieEncoder

After, #10435, which was part of Micronaut Core 4.1.13 the ServerCookieEncoder, the application was using io.micronaut.http.netty.cookies.NettyServerCookieEncoder. I am still investigating why NettyServerCookieEnconder fails to encode my session cookie.

Thus, I am might be seeing a completely different issues as @loicmathieu

@loicmathieu
Copy link
Author

The last comment of @sdelamo is interesting, we had to switch to the ServerCookieDecoder implementation due to a bug in Micronaut 4 so we configured it to io.micronaut.http.netty.cookies.NettyLaxServerCookieDecoder.

As far as I remember the bug in the default ServerCookieDecoder was parsing issue with domain cookie, using the io.micronaut.http.netty.cookies.NettyLaxServerCookieDecoder fixes the issue.

@loicmathieu
Copy link
Author

The reference issue we had we cookie parsing, it may be related to this issue: micronaut-projects/micronaut-core#10435

@sdelamo
Copy link
Contributor

sdelamo commented Apr 24, 2024

The last comment of @sdelamo is interesting, we had to switch to the ServerCookieDecoder implementation due to a bug in Micronaut 4 so we configured it to io.micronaut.http.netty.cookies.NettyLaxServerCookieDecoder.

Please, note that should not be longer necessary due to micronaut-projects/micronaut-core#10659 if you are using core 4.3.13 or above.

As far as I remember the bug in the default ServerCookieDecoder was parsing issue with domain cookie, using the io.micronaut.http.netty.cookies.NettyLaxServerCookieDecoder fixes the issue.

I get the issue only when deploying to Amazon Elastic Beanstalk instead of locally. Thus, I think the cookie's domain part may be related, but I still need to get to the bottom of it.

@sdelamo sdelamo reopened this Apr 25, 2024
@tchiotludo
Copy link
Contributor

Just to confirm that I've tried with

  • 4.3.8, 4.4.1
  • with or without NettyLaxServerCookieDecoder or DefaultServerCookieEncoder

All failed at approximately 5% of the request, only the 4.3.4 fixed the issue.
My feeling is the security-module is the main reason, but not sure.

@loicmathieu
Copy link
Author

The issue may be in Micronaut Security, there was changes in the JWT token validator and we do use JWT tokens: v4.6.6...v4.6.9

@loicmathieu
Copy link
Author

Don't know if it helps but we have the following cookie after login
image

@loicmathieu
Copy link
Author

We reproduce the issue on one of our preview environment with only the JWT cookie, no other cookie exists. As it's a preview env we can experiment if someone has an idea to dig deeper.

@graemerocher
Copy link
Contributor

could you try with a newer version of Micronaut but forcing an older version of Micronaut Security. If we can narrow it down there we at least know where to look.

@loicmathieu
Copy link
Author

We're testing a bunch of things at the moment; we have a filter that executes before the security filter; if we move it after, it no longer fails.

We also plan to do exactly what you asking for, I'll keep you posted.

@tchiotludo
Copy link
Contributor

@graemerocher, the first version of micronaut-security failing is the 4.6.8
I've update to micronaut-platform at 4.3.8, downgrading to micronaut-security to 4.6.7 at working well, starting from 4.6.8 I start to see errors.
To have error, you need to siege any authenticated endpoint, since it's happen approximately 5%. First felling, this commit could be the reason: afd2f69 (since we are using the JWT implementation)

@graemerocher graemerocher transferred this issue from micronaut-projects/micronaut-core Apr 26, 2024
@graemerocher graemerocher changed the title After upgrade to 4.3.7 route match is randomly null in the SecurityFilter Since Micronaut Security 4.6.8 route match is randomly null in the SecurityFilter Apr 26, 2024
@graemerocher
Copy link
Contributor

thanks for the feedback that helps a great deal

@sdelamo sdelamo moved this to In Progress in 4.4.2 Release Apr 26, 2024
@tchiotludo
Copy link
Contributor

tchiotludo commented Apr 26, 2024

Latest test before weekend: I use micronaut core at 4.3.8, I use this version and replaced the bean from micronaut-security 4.6.10 (see here) and no more error on local.
Will validate after build on our preview env.

@graemerocher
Copy link
Contributor

we can't see that commit

@tchiotludo
Copy link
Contributor

It's just this file with a @Replaces to overload the micronaut bean from latest version

@sdelamo
Copy link
Contributor

sdelamo commented Apr 28, 2024

It's just this file with a @Replaces to overload the micronaut bean from latest version

Thanks @tchiotludo I will try to revert the changes we did lately to that file tomorrow and see if it helps.

@sdelamo
Copy link
Contributor

sdelamo commented Apr 29, 2024

@tchiotludo just to confirm:

If you set, it works ✅:

public class JwtTokenValidator<T> implements TokenValidator<T> {
...
   public Publisher<Authentication> validateToken(String token, @Nullable T request) {
        return validator.validate(token, request)
                .flatMap(jwtAuthenticationFactory::createAuthentication)
                .map(Flux::just)
                .orElse(Flux.empty());
    }
}
}

if you set, as we do currently it fails ❌:

public class JwtTokenValidator<T> implements TokenValidator<T> {
...
   public Publisher<Authentication> validateToken(String token, @Nullable T request) {
return Mono.fromCallable(() -> validator.validate(token, request))
            .flatMap(tokenOptional -> tokenOptional.flatMap(jwtAuthenticationFactory::createAuthentication)
                .map(Mono::just).orElse(Mono.empty())).subscribeOn(scheduler);
}
}

Is that so?

Aside, Do you have io.micrometer:context-propagation in your classpath for Reactor context propagation?

@tchiotludo
Copy link
Contributor

  • Yes it seems that only the first one works (as I see on the diff between both files, this look like the only relevant changes).
  • We don't have any context-propagation on the classpath.

@loicmathieu
Copy link
Author

If I analyze our Gradle dependency graph, context-propagation is a transitive dependency of micronaut-runtime and micronaut-http, so even if we didn't have a direct reference to it, it should be included.

@graemerocher
Copy link
Contributor

are you sure you are not confusing micronaut-context-propagation which is transitive of micronaut-runtime with io.micrometer:context-propagation?

@loicmathieu
Copy link
Author

@graemerocher you're right, I read too quickly. So we're not using it, I confirm.

@sdelamo
Copy link
Contributor

sdelamo commented May 3, 2024

I think the longer term solution is #1693

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high High priority type: bug Something isn't working
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

6 participants