RFR 8220513: Wrapper Key may get deleted when closing sessions in SunPKCS11 crypto provider

[Valerie Peng]

Hi, Martin,

Thanks for trying to address this "free-wrapper-key" issue. However, 
changes in the key clean up/free area is difficult to verify and check 
for issues and I am a little concerned that this new model may be hard 
to trace when the key id is mutable and potentially fragile/error prone 
for future changes/fixes.

I still need to think through a few things. Maybe you can help me 
understand the current changes or we can explore alternatives.

My questions/feedbacks on current changes are:

1) line 1283: ref may be null (see line 1220) and this will lead to NPE?

2) It looks like you don't really need to store the "wrapperKeyUsed" 
value in both NativeKeyHolder and SessionKeyRef classes. It can be a 
local variable for NativeKeyHolder class and the increment of 
nativeKeyWrapperRefCount can be moved to SessionKeyRef constructor.  The 
counter nativeKeyWrapperRefCount is incremented inside NativeKeyHolder 
constructor

3) I have not spent as much time as you on tracing this. However, I 
think we should be able to keep the "final keyID" approach and handle 
the NativeKeyWrapperRefCount inc/dec in SessionKeyRef 
constructor/dispose methods. Have you tried this?

4) It may not be enough to just run tests under sun/security/pkcs11, as 
SunPKCS11 provider is used by many other JDK security components such as 
TLS. To be safe, you should submit the test against jdk-tier1,jdk-tier2 
to check for possible regressions. If you can't, please let me know.

Thanks,

Valerie

On 3/20/2019 9:24 PM, Martin Balao wrote:
> Hi Valerie,
>
> On 3/19/19 7:17 PM, Valerie Peng wrote:
>> How about another potential problem - wrapper key may never get deleted?
>> If we don't have a good solution to addressing it, at least add a
>> comment about it?
>> Rest looks fine.
>>
> Thanks for your feedback.
>
> The Wrapper Key was never meant to be destroyed. However, it's a good
> idea and we should do it, in addition to fixing this bug.
>
> Here it's my proposal for getting rid of the Wrapper Key when it's no
> longer needed:
>
> Webrev 01:
>
>   * http://cr.openjdk.java.net/~mbalao/webrevs/8220513/8220513.webrev.01/
>
> Please note that a few changes were required in SessionKeyRef objects
> lifetime. SessionKeyRef objects will live as long as their corresponding
> P11Key object lives -a similar scheme was implemented in the original
> patch-. This allows us to decrement Wrapper Key references -and
> eventually destroy it- while P11Key objects which don't have a native
> key created are deleted. In other words, objects that have wrapped
> native key information but don't have a native key alive can also
> decrement the Wrapper Key reference counter when deleted.
>
> Testing:
>
>   * No regressions found in sun/security/pkcs11.
>
>   * I've verified with the debugger all the new execution flows,
> including the deletion of a Wrapper Key. It would be a bit difficult to
> assert this from an automated test though.
>
> Kind regards,
> Martin.-