On Mon, 22 May 2023 22:18:13 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>> We discussed this change with @franferrax and have some concerns. The method >> Key::getEncoded does not document that a copy will be returned, and this >> would change the current behavior and affect non-PBE cases. In practical >> terms, it would mean that a key directly or indirectly converted to a P11Key >> would be destroyed if it does not return a clone in its getEncoded method. >> We suggest to make the caller responsible and keep the existing behavior. >> I.e.: if we call with a SecretKeySpec —whose ::getEncoded returns a clone—, >> the caller will need to (optionally) clear the key. What do you think? > > Hmm, so you are aware of a provider whose Key.getEncoded() impl returns the > internal key bytes directly? Although the javadoc does NOT state a copy is > being returned, it's very likely because an "encoding" is returned. If > internal key bytes are returned, it seems bad programming practice, e.g. no > protection for internal states/values? Thanks for your feedback. We've discussed this further and will move forward with the change but Just for the record let me document our conclusions here: - This affect non-PBE scenarios and will change [previous JDK behavior](https://git.openjdk.org/jdk/blob/jdk-21%2B23/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java#L190). - We have found one type of key in OpenJDK whose getEncoded method does not return a clone [here](https://git.openjdk.org/jdk/blob/jdk-21%2B23/src/jdk.crypto.mscapi/windows/classes/sun/security/mscapi/CPublicKey.java#L86). While we acknowledge that it's unlikely, there can be more in non-OpenJDK security providers. - We find that making the callee potentially mutate the arguments without documentation explicitly stating so can be confusing. While the clone pattern is known in Java to overcome limitations in the language and keep object immutability, it's inefficient as copies of objects need to be generated. We hope that the removal of the Security Manager and the untrusted code model provides more incentives for a different approach to this problem in the future. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/12396#discussion_r1202859208
