Add RoutingContext to Vert.x duplicated context local data so that it is accessible in the SecurityIdentityAugmentor

[michalvavrik]

• fixes currentVertxRequest.getCurrent in a SecurityIdentityAugmentor became null since 3.2.9 with GraphQL #37457
• fixes Add RoutingContext to SecurityIdentity for mTLS authentication #28326
• we have seen other requests to make RoutingContext accessible from the SecurityIdentityAugmentor like in Support for AuthenticationRequest enrichment / agumentation #16861

[github-actions]

🙈 The PR is closed and the preview is expired.

[michalvavrik]

I've seen both of these CI failures in my other PR.s

[franz1981]

This will blindly set it always, even if present already: this is what we need? or we need avoiding setting it again if already present, and failing if what's present is different (or any other behaviour we want to enforce).

Sadly ctx::putLocal is a ConcurrentHashMap::put which can become way cheaper if it doesn't always happen (unless is the semantic we requires, or we know that is always transitioning the value from being absent to the new value).

[michalvavrik]

I think relation of RoutingContext to duplicated context is 1:N (but at this point it will most likely be 1:1, just in case someone create a new one later?) so it will always be the same. And it is not there otherwise we would not bother to add it there. I can't presume if situation can change in the future.

Sorry I don't have a good answer for this, but I appreciate your input. I'd like to always add it there because right now we are talking augmentors, but users can't access it anywhere during authentication and authorization (that is simplified - list of cases is also complicated)

[michalvavrik]

Quick check @franz1981 - do you think it makes sense to always run get != null before adding it even though it won't be the case unless something changes in the future? I can add it.

[franz1981]

To me it's fine as it is as long as the semantic for who use it is clear and doc somewhere 👍

[michalvavrik]

Well I thought that would be answer on not executing unnecessary ConcurrentHashMap::put, but as said, unless someone starts to add it, this code runs pretty early so it won't be there.

[michalvavrik]

Okay, so I have added extra condition to address your comment.

[franz1981]

it's just ignored? we don't want to throw an exception here?

[michalvavrik]

That's a good question. Situation is that I expect this always to be true at this moment because in VertxHttpRecorder the duplicated context is enforced, however I won't risk an exception and I think it is safer to always check.

So no, I acknowledge there can be extensions that for some reason will try to use root context and then feature that relies on duplicated context won't work. Please note that documentation explicitly mention this feature depends on duplicated context so users can check https://quarkus.io/guides/duplicated-context#context-local-data and see if they can use it or not.

[franz1981]

If setRoutingContextAttribute is exposed to external users, to me, would be better to check it and fail/report somehow what's going on.
if the users are "internals" - still other known extensions we have control of and test coverage, you can just use an assert which should be enabled on tests IIRC (@geoand can confirm, I'm not sure...and TBH not everyone love asserts, so it's a matter of personal preferences...)

[michalvavrik]

But test coverage won't help here, we can't control if some user code or maybe Quarkiverse extension will mark

quarkus/extensions/vertx/runtime/src/main/java/io/quarkus/vertx/core/runtime/context/VertxContextSafetyToggle.java

Line 101 in 4470678

public static void setCurrentContextSafe(final boolean safe) {

as unsafe or will enforce root context.

This feature makes sense because duplicated context should be used in most cases and only some rare cases will not have RoutingContext available.

Is this maybe problem of documentation, do you think I didn't highlight it enough that this feature depends on duplicated context? Please let me know.

[michalvavrik]

If setRoutingContextAttribute is exposed to external users, to me, would be better to check it and fail/report somehow what's going on.

Okay, I've added extra logging for this situation, but please note this is best effort feature which as far as my knowledge of duplicated context goes, will be available for most users. Does it look better @franz1981 ?

[sberyozkin]

Hey Francesco, @franz1981, thanks for the feedback to Michal's PR, and apologies I used the first 5 letters of your github alias when asking for your feedback in the now resolved comment, I switched off for 1 sec thinking it was the right name :-)

[michalvavrik]

HttpSecurityPolicySecurityEventTest.testRolesPolicy I didn't wrote correctly I can see now, need little tweak on condition to unmake it flaky, but it's not related to this PR.

VertxIOHangTestCase seems flaky, can't think of a way it could be related to this PR.

[michalvavrik]

I'll look if I can fix HTTP Sec policy in this PR, it should be tiny fix.

[gsmet]

We need to make sure that this doesn't have a cost that is too important either from an allocation standpoint or a memory usage standpoint but I see discussions are already going on.

Let's also make sure that we have @cescoffier 's blessing before merging this.

[franz1981]

I would like @geoand as well, cause I have no idea if this is a fixed cost we always expect to pay or is just if a specific feature is enabled.

Having an additional concurrent atomic ops in the hot path is never nice, but at least the local CHM should be already populated with 16 entries, by default, which leave the cost of the atomic ops as the one which we should be worried about (given that's in the hot path)

[geoand]

I need to go through this, so far I was only paying very loose attention

[michalvavrik]

It should be noted because RoutingContext is not available in general, we keep adding it to authentication request attributes and SecurityIdentity attributes.

fixed cost we always expect to pay or is just is a specific feature is enabled.

It is always, sure we can have some detection of augmentors if that is required, which will make this feature related directly only to augmentors and not making the context generally available during auth, but I'd like to first understand why there are so worries about one instance stored in the map. Can you help me to understand what is the cost you are referring to? Can you compare it with other Quarkus code when anything put in the map is investigated this way? Thank you

[franz1981]

but I'd like to first understand why there are so worries about one instance stored in the map. Can you help me to understand what is the cost you are referring to?

Context::putLocal is performing this under the hood: https://github.com/eclipse-vertx/vert.x/blob/296a325c99bf77a56de1a49dd8d32f400cd78fc1/src/main/java/io/vertx/core/impl/DuplicatedContext.java#L123's put by using synchronized to retrieve the CHM field.
Sadly synchronized, now that biased locking is gone, are performed using 2 atomic volatile ops to enter/exit the monitor (not sure about the exit, but it should be like that) AND ConcurrentHashMap::put, under the hood require to both allocate an entry (to store the key and the value) AND perform at least an atomic op the same (see https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java#L1019).

The problem with atomic operations is while performed in the hot path and near to each others (without meaningful work in-between) can be quite costy because they forcibly "await" the so called store buffer of the cpu cache they run to be fully drained: the operation per-se isn't costy, but prevent the cpu to fully use its instruction-level-parallelism, causing cpu stalls which the cpu could have used to perform entire methods, instead, or just meaningful work.

More info about it at #30024, including another problem due to conditional-card-marking (explained in the issue).

Can you compare it with other Quarkus code when anything put in the map is investigated this way? Thank you

The PR I've shown should report some of the analysis performed to evaluate the cost, although it refer, as usual, to synthetic use cases, mostly CPU bound: in the "real" world this cost maybe won't be as bad, but the inherent overhead it brings, if made wrong, could still bite us for some real users and 100% in public benchmarks, which we cannot ignore.

[sberyozkin]

Hi All, and @franz1981 @geoand @cescoffier @gsmet

I'd like to clarify as well, in the original code this PR had there was a new handler created to set a RoutingContext property on the duplicated context, but now it has gone.
The only action which happens now is that this property is added to the context at
https://github.com/quarkusio/quarkus/pull/37795/files#diff-931a1532759e6852f492ef461181032a1249b0a5eb4d3eb594fc8f6350814be5R37

and this is it, there are just about 2 very small source updates to make it happen - IMHO now that the extra handler has gone this PR has no any performance related impact.

The reason this is done is to workaround limitations of some security API, such as SecurityIdentity, since users often need access to the RoutingContext in security identity augmentors. It can be expanded later but only if it will be necessary to support access to RoutingContext in various non-security handlers.

[sberyozkin]

LGTM, but awaiting for one more approval

[sberyozkin]

IMHO now that the extra handler has gone this PR has no any performance related impact.

and then I saw the analysis from Francesco @franz1981 :-).

Michal @michalvavrik Can we just do it conditionally, HTTP security authentication mechanisms set this property anyway, so may be it should only be done when the duplicated contexts is accessed there ?

[michalvavrik]

Thanks very much @sberyozkin , but I simply can't have answer to @franz1981 . I love performance, I'm just not sure whether every other Quarkus addition is analyzed with such a scrutiny in regards to performance. If that is going to be new standard, I think it's fantastic and I'm really happy.

Michal @michalvavrik Can we just do it conditionally, HTTP security authentication mechanisms set this property anyway, so may be it should only be done when the duplicated contexts is accessed there ?

We don't work with duplicated context I think. @sberyozkin what about we just change SecurityIdentityAugmentor API and pass down to augmentor io.quarkus.security.identity.request.AuthenticationRequest#getAttributes?

[michalvavrik]

So I think I'm going to close this PR it is only related to duplicated context local data and that was rejected. I'll open PR In Quarkus Security project with proposal.

[michalvavrik]

thanks @franz1981

[sberyozkin]

@michalvavrik

what about we just change SecurityIdentityAugmentor API

This will be a significant breaking change.

What about https://github.com/quarkusio/quarkus/blob/main/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/AbstractOidcAuthenticationMechanism.java#L41 ?

Can we use a more generic property enabling such a propagation ?

[sberyozkin]

Hi @michalvavrik

I'm just not sure whether every other Quarkus addition is analyzed with such a scrutiny in regards to performance

This is probably the hottest request execution path in Quarkus so I guess we have to accept that only the most critical updates can be allowed to add something to the overall processing time here, given all the public attention to the performance of Quarkus. We can try limit it at the security authentication path only

[geoand]

Was this closed by accident?

[geoand]

By the way, this looks good to me.

[sberyozkin]

@michalvavrik Maybe we can simply consider doing what OIDC does and update every other authentication mechanism and identity provider we ship to have it added a security identity attribute, won't cover custom identity providers though but we will just doc how custom identity providers can do it, that will be simple and I'm nearly 100% sure, have no negative performance impact.

@geoand thanks, I believe Michal got concerned he could not rework it to address the performance concerns, so he closed it, though maybe a little bit too fast

[geoand]

As for the performance concerns, we would need to measure. There is just know way to guess with this kind of thing.

[michalvavrik]

@michalvavrik Maybe we can simply consider doing what OIDC does and update every other authentication mechanism and identity provider we ship to have it added a security identity attribute,

No, this won't help because usecase were also for anonymous identity and re-creating anonymous identity wouldn't be the best for performance we can do.

won't cover custom identity providers though but we will just doc how custom identity providers can do it, that will be simple and I'm nearly 100% sure, have no negative performance impact.

I agree, custom identity providers will have to do it themselves, but I suggest to pass down the map where we already have the RoutingContext as I suggested earlier. I'll open separate PR in Quarkus Security where we can discuss it (and you can come with alternative proposals). Too many people that might not be interested in it are subscribed here.

@geoand thanks, I believe Michal got concerned he could not rework it to address the performance concerns, so he closed it, though maybe a little bit too fast

I just accepted @franz1981 arguments, I opened this PR because I thought it's not a big deal to add one more item to contextual local data, but there are ways to completely avoid it now that I understand problem. Sorry to get so many people involved, but this was very informative, thanks.

[michalvavrik]

Let's continue here quarkusio/quarkus-security#40 @sberyozkin .