Thanks, Claude! This looks great! On Wed, Jun 20, 2018 at 5:50 PM Claude Brisson <cla...@renegat.net.invalid> wrote:
> I reviewed the configuration process of the VelocityView, and it's now > much pickier about files i/o (no more CWD accesses), plus doing sensible > accesses via privileged actions. > > I activated Java Security for the showcase example (ran by the cargo > maven plugin), and it's running with this policy file [1], Christopher, > let me know if it does correspond to what you had in mind. > > [1] https://s.apache.org/fNYb > > Claude > > On 05/07/2018 10:03 PM, Christopher Schultz wrote: > > -----BEGIN PGP SIGNED MESSAGE----- > > Hash: SHA256 > > > > Claude, > > > > On 3/21/18 8:25 PM, Claude Brisson wrote: > >> Yes, it'd be great to soon release the tools since the engine is > >> out. > > Apologies for not replying sooner. > > > >> And yes, autoconfig hasn't to be the default. Why not starting with > >> an empty toolbox by default if it eases things for integrators. But > >> there are two different things here: 1) *default* tools (loaded > >> from tools.xml files inside each velocity-tools jar) 2) *standard* > >> user toolbox configuration files > > I know the world seems to be moving from "explicit configuration" to > > "intelligent defaults", which is okay by me. The only problem is when > > the explicit configuration is provided, those intelligent defaults > > should no longer apply. > > > >> For 1), what about a "load-default-tools" param in the descriptor? > >> (As usual, we would first check init-params for the concerned > >> servlet/filter and then, if not found, the context-params). It can > >> be false by default for 3.0, if you guys think that that's the way > >> to go. As long as we diligently document it when releasing. > > I think if a user wants to explicitly-load their own file, > > explicitly-loading another "defaults" file is not a problem: either > > just saying "yes please load defaults" or "please load this other > > file, too, which happens to be defaults". > > > > JAR-loaded defaults (files) are okay with me. > > > >> For 2), I'm pretty sure no standard configuration file should be > >> ever checked if an explicit configuration is provided, of course > >> (but, by the way, why is it problematic whenever those files aren't > >> there? Oh, I see, the SecurityManager doesn't want me to read files > >> that do not exist, funny but perfectly logical). Would it be > >> harmful to still check for those standard locations whenever no > >> explicit location is given be harmful (other than itching the > >> SecurityManager)? > > Yes, I think that's fine. It was only surprising to see them loaded > > when I was supplying an explicit configuration. > > > >> So if I understand correctly, we *also* need to use > >> PrivilegedActions. > > I think so. It will make the code much more SecurityManager-friendly. > > Otherwise, I have to do ugly things like "trust my whole application" > > instead of only the things that should be trusted. > > > >> Does it boils down to wrapping system calls in > >> AccessController.doPrivileged() calls? Where and how are those > >> calls authorized? Sorry, I never used the PrivilegedActions and > >> SecurityManager paradigm yet. If you have some good pointers or > >> suggestions... > > Basically, things which require privileges should be changed from: > > > > String foo = System.getProperty("bar"); > > > > to this: > > > > String foo; > > > > if(null != System.getSecurityManager()) { > > foo = AccessController.doPrivileged(new PrivilegedAction<String>() { > > @Override > > public String run() { > > return System.getProperty("bar"); > > } > > }); > > } else { > > foo = System.getProperty("bar"); > > } > > > > Yes, I know it's ugly and verbose but there isn't really a better way. > > > > BTW the SecurityManager is initialized at startup so one can easily > > initialize a static boolean in e.g. Velocity.java or whatever and just > > consult that rather than calling System.getSecurityManager() all the tim > > e. > > > >> I don't really understand either why we're speaking of the CWD > >> whereas all the VelocityView knows is the webapp root. But I may > >> have missed something. > > I'll have to look back at the balking done by the SecurityManager. I > > believe it was looking for files with names like "./tools.xml", where > > of course CWD is coming from the "./" part of the path. I think that's > > probably completely unnecessary... the config files ought to come from > > the JAR library itself or from the servlet context's getResource* > > family of methods. > > > > - -chris > > > >> On 21/03/2018 22:23, Nathan Bubna wrote: > >>> If we're talking 2.x, then adding a PrivilegedAction sounds > >>> better. If 3.0 (which, i think needs to happen anyway, right > >>> Claude?), then i'd agree with Michael. The auto config would be > >>> better off as something users need to explicitly turn on, not the > >>> default any longer. > >>> > >>> On Wed, Mar 21, 2018 at 2:03 PM, Michael Osipov > >>> <micha...@apache.org> wrote: > >>> > >>>> Am 2018-03-21 um 06:17 schrieb Christopher Schultz: > >>>> > >>>>> All, > >>>>> > >>>>> Using velocity-tools 2.0. > >>>>> > >>>>> I've been exploring what it takes to get my application > >>>>> working under a SecurityManager and it seems that > >>>>> o.a.v.t.view.VelocityView tries to load a few configuration > >>>>> files from their default locations even when a specific > >>>>> configuration file has been specified by a subclass like > >>>>> VelocityLayoutServlet: > >>>>> > >>>>> <servlet> <servlet-name>velocity</servlet-name> > >>>>> <servlet-class> com.chadis.web.servlet.VelocityLayoutServlet > >>>>> </servlet-class> > >>>>> > >>>>> <init-param> > >>>>> <param-name>org.apache.velocity.tools</param-name> > >>>>> <param-value>/WEB-INF/tools.xml</param-value> </init-param> > >>>>> ... > >>>>> > >>>>> > >>>>> What ends up happening is that VelocityView checks for the > >>>>> default configuration files tools.xml and tools.properties > >>>>> (in the current working directory) and so FilePermissions > >>>>> must be given to the whole JVM -- because VelocityView (or > >>>>> ConfigurationUtils) doesn't make the attempt in a > >>>>> PrivilegedAction. > >>>>> > >>>>> I think this can be done in a more friendly way, but I'm not > >>>>> sure what is best for the community. > >>>>> > >>>>> We could add a PrivilegedAction to the mix when a > >>>>> SecurityManager is present. This way, the velocity library > >>>>> could be granted read access to these specific files (instead > >>>>> of the whole JVM). This would impact the smallest number of > >>>>> users. > >>>>> > >>>>> We could remove the attempts to load these configuration > >>>>> files out of the CWD. This would probably affect the largest > >>>>> number of users (although relying on the CWD to find a > >>>>> default configuration file is ... bad practice). > >>>>> > >>>>> Or we could change the way Velocity*Servlet use VelocityView > >>>>> so that default configuration files are only loaded when > >>>>> there is no explicit configuration file. > >>>>> > >>>>> Thoughts? > >>>>> > >>>> Personally, I was never a huge fan of this autoconfig. It was > >>>> overly comples. A few only understood. This also lead to > >>>> subtile bugs which Maven Doxia leaving them open for years > >>>> which I finally fixed last year. > >>>> > >>>> I would personally expact an empty toolbox present, if nothing > >>>> has been configured by the user. Even if this will break stuff, > >>>> we can do so for 3.0 with ease. See > >>>> https://issues.apache.org/jira/browse/DOXIASITETOOLS-93. > >>>> > >>>> Michael > >>>> > >>>> -------------------------------------------------------------------- > > - - > >>>> > > To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org > >>>> For additional commands, e-mail: dev-h...@velocity.apache.org > >>>> > >>>> > >> > >> --------------------------------------------------------------------- > >> > >> > > To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org > >> For additional commands, e-mail: dev-h...@velocity.apache.org > >> > > -----BEGIN PGP SIGNATURE----- > > Comment: GPGTools - http://gpgtools.org > > Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ > > > > iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlrwsPQACgkQHPApP6U8 > > pFivmhAAoX1h8R7hYKf8d57NACzA2intlXZ99L2p3NlivTecnU6X9l/zd2kh7icU > > N8urW1nG8iPb3p4fARfbmDH6a2n6caosNR3Yx1vpn46a0TiuEbuLUX4dpUu4FBjP > > ZHxtmqL6DISl0YlNKu5Ba/FKXDvwUWyxm8YQDP6Wvxz3cXcvHdt4gIz8h5bQnXwW > > 8SPYhvE7PA1qH6rAKHRa7rVSNxCXrsK7Cr79LjuD1zmxJMAUkpjP2Xf9VvlmwMFG > > +Rd1kj8yv+4fTeFU+Cblp6Y8UmReoddy5rIjotj0o1OOy1tI4Z5Udw+LTQpzwd4h > > hVf1xxUatLej95341S/Cx1Q9wuMnR2dPXn4dcT0ZY9MVpI2qtLw/ALVGDIPVb5J+ > > W9Z6s33fNgirHkkDgKu8sruJJdHNJZwjytejPLZYbqKqsNRPnYBkM/kzPoaZO7rh > > DRZjtNv0v5lbi6BIqZ9aGBeZJB//Mtq2nzeI0MwLXqF9MmljJbCQh4rStMPrGylM > > q+VMBUXqJbLxFPfQmIX62ugmWJnOp674f2UA8ZbFFLC/cWlvhfAC5We6/ApIfbT+ > > ifavyOvmWF+LVBi+jt+wjyWVXrPxPxloTKcdymgkVwXCY1IQItrzqmZWnu1sXxt/ > > ZR0YemOvA49wVuIvXc3SP77vbmOohZANaCG/MEkCAAx0nt17yLM= > > =0vwt > > -----END PGP SIGNATURE----- > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org > > For additional commands, e-mail: dev-h...@velocity.apache.org > > > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org > For additional commands, e-mail: dev-h...@velocity.apache.org > >
