darkmoonka commented on PR #184:
URL:
https://github.com/apache/santuario-xml-security-java/pull/184#issuecomment-1607276879
> Hmmm. What if I have one thread which preconfigures the `providerName` for
some thread pool? Now they would get `null`. On the other hand you are right
that the class is not thread-safe at all, it is just a global unprotected
variable.
>
> Javadoc says it is just a default and when I take a look on how it is used:
>
> ```
> if (provider == null) {
> String providerId = JCEMapper.getProviderId();
> if (providerId == null) {
> this.signatureAlgorithm =
Signature.getInstance(algorithmID);
>
> } else {
> this.signatureAlgorithm =
Signature.getInstance(algorithmID, providerId);
> }
>
> } else {
> this.signatureAlgorithm =
Signature.getInstance(algorithmID, provider);
> }
> ```
>
> and the same for MessageDigest instead of Signature.
>
> Then I believe that much safer is to use rather
`Signature.getInstance(algorithmID, provider);` and
`MessageDigest.getInstance(algorithmID, provider);` with the `provider` you set
to the `providerName` now, and completely avoid relying on the default
non-final value.
>
> Btw it is also bit weird that the same value is used as a default for
`MessageDigest` and `Signature`, mhm ... seems it is really a "global default
provider".
Hello @dmatej,
thanks for your hints!
However, something feels wrong with the `null` case.
I would use the default `null` provider for signature validation and another
non-null provider for signing.
```
if (provider == null) {
String providerId = JCEMapper.getProviderId();
if (providerId == null) {
this.signatureAlgorithm =
Signature.getInstance(algorithmID);
} else {
this.signatureAlgorithm =
Signature.getInstance(algorithmID, providerId);
}
} else {
this.signatureAlgorithm = Signature.getInstance(algorithmID,
provider);
}
```
This code falls back to the default provider - fetched from the `JCEMapper`
- in case the provider is `null`.
Is there a way to somehow differentiate between the "default" and a "null"?
In my code, I use xmlsec together with OpenSAML4 and it does not pass any
providers to the constructors of the `Signature` objects.
If I have an IDP that can verify the signature of an AuthnRequest from and
SP and sign a SAML Response or Assertion in parallel, then the concurrency
issue appears.
I would solve the root cause, that I think is that the default provider
configuration in `JCEMapper` is global, but 3rd party libraries do not handle
it as a last resort fallback value.
Can it cause any problem if we change the `providerName` to `ThreadLocal`?
Meaning the global would always be thread specific.
If it is a problem, I would have another idea: what if we introduce a
thread-safe provider configuration next to the global default, and the
`getProviderId` method would return the value of the `ThreadLocal` variable if
it is not null, otherwise the global one. This change would be backward
compatible as long the default value of the `ThreadLocal` provider is `null`.
I checked wss4j and OpenSAML that depend on xmlsec, and neither of them call
the `setProviderId` method, so the following would be safe:
```
...
private static String globalProviderName;
private static ThreadLocal<String> threadSpecificProviderName = new
ThreadLocal<>();
...
/**
* Gets the default Provider for obtaining the security algorithms
* @return the default providerId.
*/
public static String getProviderId() {
if (threadSpecificProviderName.get() != null) {
return threadSpecificProviderName.get();
}
return globalProviderName;
}
/**
* Sets the default Provider for obtaining the security algorithms
* @param provider the default providerId.
* @throws SecurityException if a security manager is installed and the
* caller does not have permission to register the JCE algorithm
*/
public static void setProviderId(String provider) {
JavaUtils.checkRegisterPermission();
globalProviderName = provider;
}
public static void setThreadSpecificProviderName(String
threadSpecificProviderName) {
JavaUtils.checkRegisterPermission();
JCEMapper.threadSpecificProviderName.set(threadSpecificProviderName);
}
...
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
