Token refresh process improvements

[sobvan]

The new refresh mechanism in MN 2.0 is really great. It does, however need some improvement to be fully usable out-of-the-box.
The task list below is my opinion only. This was formulated while implementing refresh mechanism for an Angular SPA.

I would like to start a discussion based on the task list bellow. I am also glad to add a PR if Micronaut OGs agree with my view :)

Task List

• JWT_REFRESH_TOKEN cookie max-age should be configurable (default value should be a lot longer than that of JWT, although I am not totally familiar with industry standard refresh token age suggestions.)
• All cookies should have the most secure options by default (`sameSite=strict;secure;)
• logout handler should also clear JWT_REFRESH_TOKEN
• RefreshTokenPersistence should have a new method `invalidateToken(String refreshToken), and JwtClearingLogoutHandler should call this method by default.
• JWT_REFRESH_TOKEN cookie should have it's path set to /oauth/access_token and /logout so that it is not sent with other requests.
• A simple memory based RefreshTokenPersistence implementation would be useful, but I do get, that it would not have production value in a container based environment.

Steps to Reproduce

Not a bug. Current behaviour can easily be observed with settings provided in micronaut openId connect related tutorials.

Expected Behaviour

Token refresh mechanism should follow best practices even more.

Actual Behaviour

Token refresh process is great, but can be improved.

Environment Information

n.a.

Example Application

e.g. https://guides.micronaut.io/micronaut-oauth2-okta/guide/index.html

[graemerocher]

Good suggestions, fancy sending a PR? :)

[graemerocher]

Actually maybe if @jameskleeh can ship in with his thoughts first

[sobvan]

@graemerocher, thanks for the fast feedback :)

I have these things now implemented in kotlin in my controllers that are replacing the default implementations. I think they are relatively easy to port to the default implentations. You guy's code is really easy to read imho :)

One thing I would need some help with is writing some proper unit tests. I do not have much experience with that in MN.

Also, indeed, I would like to validate these ideas with @jameskleeh first.

[jameskleeh]

The first 3 items I agree with for sure

RefreshTokenPersistence should have a new method `invalidateToken(String refreshToken), and JwtClearingLogoutHandler should call this method by default.

I'm a bit weary about adding a method that is only useful for one of the implementations, however it could be useful in general so I guess I'm OK with this. Note it would have to be a default method to not break existing implementations.

JWT_REFRESH_TOKEN cookie should have it's path set to /oauth/access_token and /logout so that it is not sent with other requests.

This should be configurable, and if not set, use the configured value for the controller (the controller path is configurable). If that value is not set, use the default for the controller. We need to support those who have either changed the default path, or replaced the controller with their own implementations that we currently don't know what the path is.

A simple memory based RefreshTokenPersistence implementation would be useful, but I do get, that it would not have production value in a container based environment.

I'm not against this, however it would have to be opt in with configuration (disabled by default)

[sobvan]

Thanks for the feedback. I will try to do it in the beginning of this week.
One more thing that my implementation contains (and which turned out to be more complex than I anticipated before) is the serialization of the access_token. It does not work out-of-the box with any json ser/deser implementation, as the Map<String,Object> type is not easy to serialize (and also has some security implications afaik).

So I would also try to include the json serialization of the access_token claims as an example in the memory based persistence provider, so it would be really straightforward to extend it with any kind of persistence.
I have also checked the existing implementation on how the attributes field of the access_token will be serialized into a JWT token, but that implementation contains more than just the serialization, so I ended up writing my own implementation.

This actually ended up being quite simple, but figuring it out was not totally trivial, so I think, that it might help users of Micronaut.
This is the ser/deser part:

    private fun fromJson(json: String): Map<String, Any> {
        val result = HashMap<String, Any>()
        JSONObjectUtils.parse(json).let {
            it.keys.forEach { key ->
                result[key] = it[key]!!
            }
        }
        return result
    }

    private fun toJson(attributes: Map<String, Any>): String {
        return JSONObject().apply {
            attributes.forEach { attr ->
                this[attr.key] = attr.value
            }
        }.toJSONString(JSONStyle.NO_COMPRESS)
    }

What do you think @jameskleeh, is it okay If I write the RefreshTokenSimplePersistence class in a way, that also includes the access_token serialization code, end is really easily extendable with DB persistance?

[jameskleeh]

@istvanszoboszlai The in memory implementation shouldn't be designed to be extensible and serialization is not necessary. For example:

public class InMemoryRefreshTokenPersistence implements RefreshTokenPersistence {
    
    private final Map<String, UserDetails> tokens = new LinkedHashMap<>();
    
    @Override
    public void persistToken(RefreshTokenGeneratedEvent event) {
        tokens.put(event.getRefreshToken(), event.getUserDetails());
    }

    @Override
    public Publisher<UserDetails> getUserDetails(String refreshToken) {
        return Publishers.just(tokens.get(refreshToken));
    }
}

[sobvan]

@jameskleeh, of course for putting UserDetails in a HashMap we do not need any serialization, but the next step for any user would be to persist UserDetails in some DB, and in that case serialization is hard to avoid.

I get it, however, that you would not like refresh token persistence and serialization to be part of the framework. That is totally fine.

[jameskleeh]

@istvanszoboszlai I don't think the data would be serialized in any case. The idea would be to create a new UserDetails from the database, pulling in data from perhaps multiple places. The existing UserDetails would never be returned because it wouldn't take into account any change of roles or any other user information.

[sobvan]

@jameskleeh I was thinking about some kind of an invalidation mechanism, but thinking a bit further about the issue, I think you are right.
I would just like to avoid having to write the access_token generation logic twice.

I will look into the code more deeply to know where I can hook into the existing UserData generation process in my case of using OpenId Connect.

If I forward every refresh call to the openId provider, I either do not need the Micronaut generated refresh token at all, or I need to cache the refresh token received from the openId provider mapped by the Micronaut generated refresh token. I am still thinking about the pros and cons.

As far as I can tell now, you are not a fan of implementing the full refresh mechanism when an OpenId provider is used within micronaut-security out-of-the-box, right?

[jameskleeh]

@istvanszoboszlai Unfortunately, there is no standard there so we can't implement something generic. If you're using the token directly from the provider its up to you to call the provider to refresh it

[sdelamo]

@istvanszoboszlai

A simple memory based RefreshTokenPersistence implementation would be useful, but I do get, that it would not have production value in a container based environment.

@jameskleeh

I'm not against this, however it would have to be opt in with configuration (disabled by default)

We have already an example with persistence in the JWT guide. I would prefer us to maintain that example with as many good practice as possible and not provide an in-memory implementation. An in-memory implementation will make any app using it not horizontally scalable right? You may be hitting an instance where your refresh token is being saved in memory and an instance which has no recollection of your refresh token thus consider your refresh token invalid.

[sdelamo]

I've moved part of this conversation to its own issue

Limit JWT_REFRESH_TOKEN cookie paths

[sdelamo]

I've created a sub-task for: Logout Handler should also clear JWT_REFRESH_TOKEN cookie

[sdelamo]

@istvanszoboszlai

All cookies should have the most secure options by default (`sameSite=strict;secure;)

it defaults already defaults to secure:

https://github.com/micronaut-projects/micronaut-security/blob/master/security-jwt/src/main/java/io/micronaut/security/token/jwt/cookie/JwtCookieConfigurationProperties.java#L52

By default we don't specify a same site policy but you could specify it with:

micronaut.security.token.jwt.cookie.cookie-same-site: Strict

Unless, we consider not specifying a same site policy Strict by default a bug if we change the default policy it will be a breaking change. I think, we could create an issue to reconsider this for the next mayor version and maybe improve the documentation around the configuration options already available.

[sdelamo]

I've extracted an issue for: #333

[sobvan]

Hello @sdelamo, thanks for breaking down this issue further and for all Your valuable input.

So in this main issue remains only the task of "RefreshTokenPersistence should have a new method `invalidateToken(String refreshToken), and JwtClearingLogoutHandler should call this method by default.", right?

Do you guys expect PR-s for each subtask?

We have already an example with persistence in the JWT guide. I would prefer us to maintain that example with as many good practice as possible and not provide an in-memory implementation. An in-memory implementation will make any app using it not horizontally scalable right? You may be hitting an instance where your refresh token is being saved in memory and an instance which has no recollection of your refresh token thus consider your refresh token invalid.

I totally agree. And this also realtes to @jameskleeh's comment about only needing to cache some kind of a user ID, and the UserData object should be rebuilt according to the actual state of the user. In that case, however, would it not be better to simply have the user ID part of the refresh token?

I would like to work on this task this week, is it OK if I go for the JWT_REFRESH_TOKEN clearing (including invalidation) task?

[sdelamo]

Do you guys expect PR-s for each subtask?

I think a PR per subtask would be ideal.

[sdelamo]

So in this main issue remains only the task of "RefreshTokenPersistence should have a new method `invalidateToken(String refreshToken), and JwtClearingLogoutHandler should call this method by default.", right?

I am not sure about this this method. But probably there is not harm to add it, specially if we want to enforce that behaviour in logout endpoint.

I see any application using refresh token to need something to revoke refresh tokens. In the guide, what I did was to add a revoked property to the entity used to persist the refresh tokens.

Just to clarify: You want the /logout endpoint to call invalidate refresh token because if we delete only the access token then when the user visits a secured endpoint, it may redirect to the login endpoint and get a new access token due to the refresh token which was not deleted and thus from the user point of view it will be as if the logout did not work.

[sdelamo]

I've added a related task #334

[sobvan]

Just to clarify: You want the /logout endpoint to call invalidate refresh token because if we delete only the access token then when the user visits a secured endpoint, it may redirect to the login endpoint and get a new access token due to the refresh token which was not deleted and thus from the user point of view it will be as if the logout did not work.

Yes this is exactly the reasoning, which actually happened to me, when I implemented the refresh mechanism. I also think that it is a (not to severe, but) security issue if the user's machine possesses a token that grants access to a system after logging out form that system.

[sdelamo]

To sum up:

• JWT_REFRESH_TOKEN cookie max-age should be configurable (default value should be a lot longer than that of JWT, although I am not totally familiar with industry standard refresh token age suggestions.)

Moved to: #333

• All cookies should have the most secure options by default (`sameSite=strict;secure;)

secure already the default. strict can be configured via micronaut.security.token.jwt.cookie.cookie-same-site: Strict. Changing the latter, it will be breaking. I don't think not having a default is a bug.

• logout handler should also clear JWT_REFRESH_TOKEN
  RefreshTokenPersistence should have a new method `invalidateToken(String refreshToken), and JwtClearingLogoutHandler should call this method by default.

Both moved to issue: #332

JWT_REFRESH_TOKEN cookie should have it's path set to /oauth/access_token and /logout so that it is not sent with other requests.

Move to issue: #331

• A simple memory based RefreshTokenPersistence implementation would be useful, but I do get, that it would not have production value in a container based environment.

I think we have consensus we will not be doing this last point.