Right, I debated about that myself - the changes to the Signature class
is more for 3rd party impls and apps which may support Cloning and rely
on instanceof check. Made the changes as it's harmless at least.
The javadoc wording regarding cloning in MessageDigest class seems more
needed for Signature class especially for callers requesting Signature
with algorithm only which uses the delayed provider selection mechanism.
Pondered for a moment whether to do this as it would involve filing a
CSR. Given the main focus is MessageDigest and none of the JDK provider
support cloneable signature impl, I didn't include this in the 8246077 fix.
Thanks,
Valerie
On 6/15/2020 6:18 PM, Weijun Wang wrote:
Ah yes, you're correct.
But because of this delayed selection, the existence of CloneableDelegate is
not that useful unless user has specified a provider at the beginning. At first
every Signature is a Delegate and thus not a Cloneable, you would have to clone
it to make it Cloneable.
I noticed none of our impls are Cloneable (or did I miss one?). What is the
motivation to update Signature in this code change?
Thanks,
Max
On Jun 16, 2020, at 7:29 AM, Valerie Peng <[email protected]> wrote:
Hmm, on a second thought, I reverted back on this last suggestion. Signature
class has this delayed provider selection mechanism, so the clone() method
should always rely on the chosen signatureSpi obj.
Thanks,
Valerie
On 6/15/2020 12:59 PM, Valerie Peng wrote:
Sure, sounds good. Webrev is updated in place at webrev.01 since the change is
just one-line.
Will proceed with integration once the mach5 tests finish.
Thanks!
Valerie
On 6/14/2020 2:21 AM, Weijun Wang wrote:
Looks fine to me. Maybe you can also use "if (this instanceof Cloneable)" in
Signature$Delegate::clone.
Thanks,
Max
On Jun 11, 2020, at 3:45 AM, Valerie Peng <[email protected]> wrote:
Webrev updated at: http://cr.openjdk.java.net/~valeriep/8246077/webrev.01/
Key changes in this revision are in the Delegate.of() method in
java.security.MessageDigest class.
Comments?
Thanks,
Valerie
On 6/8/2020 1:42 PM, Valerie Peng wrote:
"md instanceof Cloneable && md is not from PKCS11" is not entirely precise.
What I mean is that for MessageDigestSpi impls from PKCS11 provider, we will need to call the
clone() to know for sure whether cloning is supported. If we decide to employ these extra logic
for saving clone() call, it's better to do it inside the MessageDigest.of(...) so the callers
don't have to repeat the same logic. Comments?
Valerie
On 6/8/2020 1:34 PM, Valerie Peng wrote:
Hmm, I checked all MessageDigestSpi impls of JDK providers... The story is more
complicated than we expect.
For SUN provider which implement the digest spi, implementing Cloneable means
it supports clone functionality.
However, for SunPKCS11 provider which wraps native PKCS11 library, it always
implements Cloneable interface, but when clone() is called, it will then
perform the equivalent PKCS11 calls and throw CNSE if any PKCS11 error is
observed.
So, there is a possibility that the instanceof check and the clone() check
leads to different result in this particular scenario.
The chance of 3rd non-PKCS11 party provider whose MessageDigest/MessageDigestSpi impl implements
Cloneable but throws CNSE when clone() is called should be very low? So, I think it should be
sufficient to use "md instanceof Cloneable && md is not from PKCS11"?
Valerie
On 6/6/2020 9:10 AM, Xuelei Fan wrote:
As the the Delegate class takes care of the Cloneable SPI implementation, it should be
sufficient to use "md instanceof Cloneable" only. It is not a expected
behavior that a provider implements Cloneable but does not really support it.
Xuelei
On 6/5/2020 10:54 PM, Weijun Wang wrote:
Is it possible to try "md instanceof Cloneable || md.clone() returns"?
Hopefully this is fast enough in most cases and also has the chance to recognize more
actually-cloneable ones.
I'm also OK with simply using "md instanceof Cloneable".
--Max
On Jun 6, 2020, at 12:02 PM, Valerie Peng <[email protected]> wrote:
I am merely following the spec's recommendation of trying the clone() for
cloneability check.
If you both are ok with it and prefer the instanceof check, I can sure
reverting back the changes in HmacCore and HandshakeHash classes.
Valerie
On 6/5/2020 2:04 AM, Seán Coffey wrote:
I share the same concern. clone() is a heavy weight operation in constructors
that can be called alot during intensive crypto operations.
Now that you've done work on Delegate class - why not also keep the (instanceof
Cloneable) test ? That can serve as your fastpath for the default JDK
configuration AFAIK.
regards,
Sean.
On 05/06/2020 00:16, Weijun Wang wrote:
在 2020年6月5日，03:19，Valerie Peng <[email protected]> 写道：
Can you give an example when these 2 rules have different results? Is this only
true for those implementation that directly subclass MessageDigest?
Before this fix, even the Spi impl implements Cloneable the instanceof check
will always fail because the wrapper class, i.e. MessageDigest.Delegate does
not. However, if you call the clone() (made public by the MessageDigest class),
it will succeed because Delegate.clone() checks to see if the spi object
implements the Cloneable interface, if yes, it will proceed to call the spi
clone(). So, for this scenario, the results are different, e.g. instanceof
returns false, but clone() succeeds. This is just one example. Is this what you
are asking?
No.
I understand this case, but this has already been fixed. Is there any other
example? Or are you only follow the words in the spec? i.e. try clone() to see
if it’s cloneable.
I am worried that try clone() is much heavier than just check instanof
Cloneable.
Thanks,
Max
