Introduce ValidationPackageOpener to relay open directives to validation providers (JPMS package opens -> validation provider) - #340
Conversation
| } | ||
|
|
||
| @Override | ||
| public void openPackage(MethodHandles.Lookup providerLookup, Module targetModule, String packageName) { |
There was a problem hiding this comment.
Would it make sense to ensure this method is only ever called from the module of the provider?
We could use StackWalker to find all the stack frames until the provider module is reached and assert that there are no stack frames from other modules, which are not reflection related. That way, we'd ensure that only the provider can call this, as an additional safety net.
There was a problem hiding this comment.
Would it make sense to ensure this method is only ever called from the module of the provider
+1 , that definitely was the intention, yes! I thought that I got it addressed by:
if ( !providerLookup.lookupClass().getModule().equals( providerModule ) ) {the idea being that we pass the lookup created at a callsite opener.openPackage(MethodHandles.lookup(), ...) and as this carries the info of who called the lookup + it cannot be "faked" we get that safty net ? Or did you mean to add the stack walker as an extra check to this one?
There was a problem hiding this comment.
I was thinking about adding the StackWalker code as an extra safety, but it seems like your proposed solution is good enough, assuming safe usage from the provider.
yrodiere
left a comment
yrodiere
left a comment
There was a problem hiding this comment.
This looks good to me overall, but the devil being in the details... it would be great to have the opinion of an expert such as @dmlloyd, who has been working extensively on using/hijacking the Java module system for Quarkus.
David, could you please tell us what you think of this? Of course this is outside of the context of Quarkus and I suppose native compilation will through a lot of it outside the window, but at least we'd want this to work / be safe in a generic "Java with modulepath" application.
Another question I have, but for @marko-bekhta: is the module opening transitive? I.e. once the provider got a module opened to it, can it open that module to another module (e.g. hibernate-validator opening user modules to a hypothetical hibernate-validator-extras module)?
yes, but one must do it "manually". At least, that's my understanding and what I've seen from my "experiments", if a package is opened to the current module, then this module can open that package to some other module. It would mean that we'd either have move package openers in our projects, or, for example, like with accessors, we could just open the package to us (HV core) and right away open that same package to the accessor lib, assuming that HV itslef won't be doing actual work and just delegate to the accessor lib, we need that inital "open" (form the package opener) to get the ability to pass it over. Also from the javadoc (https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/Module.html#addOpens(java.lang.String,java.lang.Module)):
|
|
On the surface it seems... OK. I can't see any obvious flaw. I'm speaking as someone who's only (very) casually familiar with the validation spec.... but my understanding is that you have two possible usage cases:
So this improvement mainly does stuff to make case 1 easier, right? In my honest opinion, the extra SPI, spec, and docs surface, plus the risk of an implementation error causing a vector for some kind of exploit, makes it not seem worthwhile. But if you all are OK with potentially having to mitigate a worst-case scenario, then I can't see anything obviously wrong with the code itself. |
|
@sebersole can you also chime in on this one please |
Because @marko-bekhta is proposing to do a very similar thing in JPA itself. |
|
I've played with this concept a bit and never had much luck getting it to work properly. But that might just be from a lack of understanding. But in principle it is a good thing in my opinion. |
| if ( !providerLookup.hasFullPrivilegeAccess() ) { | ||
| throw new PackageAccessException( | ||
| "ValidationPackageOpener requires a full-privilege Lookup (obtained via MethodHandles.lookup())" | ||
| ); | ||
| } | ||
|
|
||
| if ( !providerLookup.lookupClass().getModule().equals( providerModule ) ) { | ||
| throw new PackageAccessException( | ||
| "Lookup class module " + providerLookup.lookupClass().getModule().getName() | ||
| + " does not match the bound provider module " + providerModule.getName() | ||
| ); | ||
| } | ||
|
|
||
| if ( !targetModule.isNamed() ) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| if ( !targetModule.isOpen( packageName, providerModule ) ) { | ||
| targetModule.addOpens( packageName, providerModule ); | ||
| } | ||
| } | ||
| catch (IllegalCallerException e) { | ||
| throw new PackageAccessException( | ||
| "Cannot open package " + packageName + " in module " + targetModule.getName() | ||
| + " to provider module " + providerModule.getName() | ||
| + ". Ensure the module has 'opens " + packageName + " to jakarta.validation'", | ||
| e | ||
| ); | ||
| } |
There was a problem hiding this comment.
Hey @mkouba @Sanne, you expressed concerns about the security aspects of https://in.relation.to/2018/03/21/spec-api-modularity-patterns/ a (long) while ago.
I believe @marko-bekhta successfully addressed these concerns here by making sure the package opener can only ever be used to open a package to the provider (in this case, Hibernate Validator) and not to a random malicious module.
Do you see any obvious flaw with this solution?
There was a problem hiding this comment.
It's been too long ;-). But @manovotn might have some good observations.
There was a problem hiding this comment.
I am by no means a JPMS expert but we are just now having a similar conversation over on the CDI specification (jakartaee/cdi#1015).
The approach you have here seems OK to me in terms on security/giving access. Whether it is "the way" to handle JPMS, that I do not know :)
I wonder - and this is likely because of my utter ignorance of validation so sorry for that - what scenarios are you covering with this? Because for CDI, we have this conversation mainly because of CDI SE - in Jakarta EE, existing servers either have their own approximation of modular approach already or they can introduce a module layer and sidestep the whole problem.
There was a problem hiding this comment.
It's been too long ;-)
Right sorry it's been too long for me as well.
At the time I was working actively with various POCs of modules and trying out things... JPA was nowhere ready for it and I remember suggesting changes but it took eons to get them integrated. It's now 8 years later and I'm just very rusty with it all - I barely remember what I worked on last month :D
Perhaps @gunnarmorling would remember some details and would want to chime in?
Has anyone here actually managed to make it work in practice? @marko-bekhta ? |
this thing here, yes. I think what Steve was referring to was the experiment with hibernate-models, where we thought it would be possible to programmatically export some packages to providers, but since providers needed to use the classes (direct-in-code references), they needed compile-time access and not something given programmatically later on. Here, though, the difference is that we are never using direct references and hence do not need compile-time "permissions". |
9 participants
Following the idea described in https://in.relation.to/2018/03/21/spec-api-modularity-patterns/
Asking users to open packages with their constrained classes only to the Validation API module, without specifying the provider modules, and placing that responsibility on the API/provider to share the opened packages makes the applications more portable and not tied to a specific validation provider.
The idea is that the package opener is tied to a specific validation provider, and such an opener would only work from inside the provider module -- we require a lookup tied to the provider module. Since lookups are supposed to be caller sensitive, even if the opener "leaks" to another module, it will be useless, as the other module's lookup won't match the one from the provider (to which the opener is bound).
Opening this one to discuss the idea (and potentially include it in 4.0).
cc: @yrodiere @beikov