quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.8k stars 2.68k forks source link

Implement MP Context Propagation `SECURITY` context #29703

Open knutwannheden opened 1 year ago

knutwannheden commented 1 year ago

Description

The MP Context Propagation specification includes a SECURITY context (see org.eclipse.microprofile.context.ThreadContext#SECURITY), but Quarkus does not directly "implement" this thread context. Instead, the implementation of CurrentIdentityAssociation (which is SecurityIdentityAssociation) is annotated as @RequestScoped and is thus probably implicitly part of the CDI thread context (also specified by MP Thread Context). As a consequence the MP Context Propagation's mechanism to propagate (or clear) individual contexts (using the ThreadContext.Builder and ManagedExecutor.Builder APIs) cannot be used to propagate or clear the SECURITY context independently from the CDI context.

Implementation ideas

Implementing this feature request would probably require removing the @RequestScoped annotation from SecurityIdentityAssociation.

quarkus-bot[bot] commented 1 year ago

/cc @FroMage, @manovotn, @sberyozkin

manovotn commented 1 year ago

IIRC, the decision to implement security via CDI (more precisely via interception) was deliberate and is the reason why we do not implement the MP CP context. I don't think this is subject to change, but @sberyozkin will know more for sure.

sberyozkin commented 1 year ago

I was not architecturing it, but my guess this MP Context Propagation option was not even available back when it was implemented. I'd rather keep it the way it is working now as propagating the security context such that it can be retained after the request which was used to create SecurityIdentity has long gone seems to be a risky idea.

However having an option for the users to switch to the org.eclipse.microprofile.context.ThreadContext#SECURITY security context implementation, via configuration or adding an extra dependency like quarkus-smallrye-propatation-security, can likely be useful for cases where going this way is considered to be secure by users - but it has to be a user's decision IMHO, not a default Quarkus approach.

manovotn commented 1 year ago

I was not architecturing it, but my guess this MP Context Propagation option was not even available back when it was implemented. I'd rather keep it the way it is working now as propagating the security context such that it can be retained after the request which was used to create SecurityIdentity has long gone seems to be a risky idea.

@sberyozkin I think we might have had MP CP in some shape or form already but your second sentence is (I think) the reason why it works the way it does.

@knutwannheden if there is a specific scenario you are after, can you please describe it in more depth?

sberyozkin commented 1 year ago

Hey @manovotn

See the first message in #28742.

IMHO, having Kafka messages be able to pick up security contexts of the requests finished ex 30 mins ago would be a problem if it were done by default - however I realize it may be considered safe in some deployments - so having an extra option to support such cases may indeed be useful