Eric, Performance is very important. Resolvers are typically used multiple times per signature. XML processing currently dwarfs crypto operations by orders (!) of magnitude.
-Anli On Aug 2, 2012, at 9:48 PM, "Eric Johnson" <[email protected]> wrote: > Hmmm. > > I need a little bit of direction. > > In the ResourceResolver class, the "getInstance" method is the one that takes > the flag for "secureValidation". Callers then take the returned "resolver" > instance, and call "resolve", which /doesn't/ take a "secureValidation" > parameter. > > There are two ways to solve this, I think: > > a) always make new copies of the resolver & spi instances - this means that > we can safely set the "secureValidation" flag on the instance of the > ResourceResolverSpi, and it will just work. This moots the > "engineIsThreadSafe" method, since every resolution will be done in a thread > safe manner (although the code still needs to be tweaked so that it only > creates a new resolver and spi instance once an Spi instance has been chosen). > > b) change the parameters to the "resolve" method, so that it takes the > "secureValidation" parameter. Note that this removes the need for the > "secureValidation" field of "ResourceResolverSpi", as it will now always be > passed as state. Older Spi implementations might respect this flag. However, > since it was introduced in 1.5.0, this list of processors is likely to be > short, as it will have to be a processor written in the range of [1.5.0,1.5.3) > > Thoughts? > > -Eric. > > On 8/1/12 2:59 PM, Eric Johnson wrote: >> Hi Colm, >> >> I think I can come up with a patch. It will take me a few days. >> >> I'm inclined to change the resolving code to pass around a "ResolveContext" >> instance which will take all of the existing parameters. This way, >> additional parameters added in the future will not re-break the API. >> >> Do you think that can work? Or do I need to submit a patch before you >> understand what I mean? >> >> I admit, I have reason for such an approach - my port of Santuario to use >> GenXDM has to change the resolver APIs to add a parameter. Combined with the >> "secureValidation" flag, that's two additional pieces of metadata, and that >> suggests the possibility of yet more future changes. >> >> -Eric. >> >> On 8/1/12 3:43 AM, Colm O hEigeartaigh (JIRA) wrote: >>> [ >>> https://issues.apache.org/jira/browse/SANTUARIO-337?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13426502#comment-13426502 >>> ] >>> >>> Colm O hEigeartaigh commented on SANTUARIO-337: >>> ----------------------------------------------- >>> >>> Hi Eric, >>> >>> Could you submit a patch for this issue? One downside to adding the >>> secureValidation flag to the ResourceResolverSpi.engineResolve() method is >>> that it breaks backwards compatibility for any custom ResourceResolverSpi >>> implementations. I don't see a way around this though unfortunately. >>> >>> Colm. >>>> ResourceResolver does thread-unsafe handling of the "secureValidation" flag >>>> --------------------------------------------------------------------------- >>>> >>>> >>>> Key: SANTUARIO-337 >>>> URL: https://issues.apache.org/jira/browse/SANTUARIO-337 >>>> Project: Santuario >>>> Issue Type: Bug >>>> Components: Java >>>> Affects Versions: Java 1.5.2 >>>> Reporter: Eric Johnson >>>> Assignee: Colm O hEigeartaigh >>>> Priority: Minor >>>> >>>> From ResourceResolver.getInstance(), with parts elided for brevity & >>>> clarity: >>>> for (ResourceResolver resolver : resolverList) { >>>> ResourceResolver resolverTmp = resolver; >>>> if (!resolver.resolverSpi.engineIsThreadSafe()) { >>>> // generate a new instances of resolverSpi ... >>>> } >>>> // ... >>>> resolverTmp.resolverSpi.secureValidation = >>>> secureValidation; >>>> if ((resolverTmp != null) && resolverTmp.canResolve(uri, >>>> baseURI)) { >>>> // Check to see whether the Resolver is allowed >>>> // check for certain types .... >>>> return resolverTmp; >>>> } >>>> } >>>> In case you didn't see it, the trouble is the juxtaposition of >>>> "resolverSpi.engineIsThreadSafe()" followed by code that sets >>>> "secureValidation" on the very same instance of the spi, whether or not it >>>> is thread safe. >>>> Meaning, if two threads resolve at the same time, and one thread is >>>> attempting secure resolution while the other is not, all the "thread safe" >>>> resolvers risk a race condition where they will now be in an uncertain >>>> state. Of course, it turns out that all the included resolvers declare >>>> themselves thread-safe, so this potentially magnifies the problem. >>>> In practice, this is not likely ever to occur, because any given >>>> application will likely share the same notion of "secureValidation". >>>> Three observations >>>> #1 - the secureValidation flag needs only be set for the resolver that is >>>> actually chosen >>>> #2 - the secureValidation flag should instead be passed as a parameter >>>> ResourceResolverSpi.engineResolve() method, not stored as data in the SPI >>>> instance. >>>> #3 - if there were ever a resolver to show up that isn't thread safe, and >>>> it also has properties, the logic above which creates a new instance of >>>> the SPI would discard the properties set on the registered spi. It turns >>>> out only the HTTP resolver uses these properties for anything, so this may >>>> not be problematic in the real world. >>>> Originally reported via email: >>>> http://article.gmane.org/gmane.text.xml.security.devel/7647 >>> -- >>> This message is automatically generated by JIRA. >>> If you think it was sent incorrectly, please contact your JIRA >>> administrators: >>> https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa >>> For more information on JIRA, see: http://www.atlassian.com/software/jira >>> >> >
