You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@santuario.apache.org by Eric Johnson <er...@tibco.com> on 2012/08/01 23:59:33 UTC
Re: [jira] [Commented] (SANTUARIO-337) ResourceResolver does thread-unsafe
handling of the "secureValidation" flag
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
>
>
Re: [jira] [Commented] (SANTUARIO-337) ResourceResolver does
thread-unsafe handling of the "secureValidation" flag
Posted by Colm O hEigeartaigh <co...@apache.org>.
I would go for option (b).
Colm.
On Thu, Aug 2, 2012 at 8:48 PM, Eric Johnson <er...@tibco.com> 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<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<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<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<https://issues.apache.org/jira/secure/ContactAdministrators%21default.jspa>
>>> For more information on JIRA, see: http://www.atlassian.com/**
>>> software/jira <http://www.atlassian.com/software/jira>
>>>
>>>
>>
>
--
Colm O hEigeartaigh
Talend Community Coder
http://coders.talend.com
Re: [jira] [Commented] (SANTUARIO-337) ResourceResolver does
thread-unsafe handling of the "secureValidation" flag
Posted by Anli Shundi <as...@tibco.com>.
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" <er...@tibco.com> 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
>>>
>>
>
Re: [jira] [Commented] (SANTUARIO-337) ResourceResolver does thread-unsafe
handling of the "secureValidation" flag
Posted by Eric Johnson <er...@tibco.com>.
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
>>
>