8358451: SunJCE PBEKey impl should throw IllegalStateException when getEncoded() is called #25632
Conversation
…etEncoded() is called
|
👋 Welcome back valeriep! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@valeriepeng The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
There was a problem hiding this comment.
Shall we also throw ISE when getFormat and getAlgorithm are called? Calling these methods after the key is destroyed looks suspicious and may reveal a coding error.
There was a problem hiding this comment.
Well, I see that throw IllegalStateException for almost all methods seems to be the style for the Destroyable impl classes under the javax.security.auth.kerberos package. But I am not sure if this would be too "noisy" for Key objects. At a minimum, we should throw IllegalStateException for method which trying to use the sensitive info which has been cleared out. However, if we throw IllegalStateException for all methods such as getAlgorithm(), then it may lead to even more IllegalStateException being thrown unexpectedly and may make troubleshooting harder.
| @@ -80,6 +81,9 @@ final class PBEKey implements SecretKey { | |||
|
|
|||
| public byte[] getEncoded() { | |||
There was a problem hiding this comment.
I understand this is not a public API class so there is no need to provide @throws in the spec. But, on the other hand, do we need to provide one in its super class java.security.key? I have no opinion.
There was a problem hiding this comment.
Well, I am not sure. java.securityKey is the super interface for all keys, including private, public, and secret keys. The Exception is due to implementing the javax.security.auth.Destroyable interface, so the @throws spec should goes to the class which implements both Destroyable and Key interfaces. PBEKey is not a public API class, so perhaps we can documenting this in the SunJCE provider section as this is implementation-specific?
There was a problem hiding this comment.
Also, PublicKey does not implement (or implement by inheritance) Destroyable.
| @@ -202,6 +205,9 @@ private void readObject(java.io.ObjectInputStream s) | |||
| @java.io.Serial | |||
| private Object writeReplace() throws java.io.ObjectStreamException { | |||
| try { | |||
| if (isDestroyed()) { | |||
| throw new NotSerializableException("key is destroyed"); | |||
There was a problem hiding this comment.
nit: for uniformity, you may want to capitalize key since it is capitalized elsewhere
| @@ -80,6 +81,9 @@ final class PBEKey implements SecretKey { | |||
|
|
|||
| public byte[] getEncoded() { | |||
| try { | |||
| if (isDestroyed()) { | |||
| throw new IllegalStateException("key is destroyed"); | |||
There was a problem hiding this comment.
nit: for uniformity, you may want to capitalize key since it is capitalized elsewhere
JCA SecretKeyFactory class. Changed to throw ISE whenever destroyed key is encountered.
Update the
PBEKeyclass of the SunJCE provider which override thejavax.security.auth.Destroyableinterface toIllegalStateExceptionifgetEncoded()is called after key is destroyedPBEKeyobject will lead to exception.Also update the
PBEKeyFactoryclass of the SunJCE provider to check for destroyed keys and throw exceptions per the method javadoc.Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25632/head:pull/25632$ git checkout pull/25632Update a local copy of the PR:
$ git checkout pull/25632$ git pull https://git.openjdk.org/jdk.git pull/25632/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25632View PR using the GUI difftool:
$ git pr show -t 25632Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25632.diff
Using Webrev
Link to Webrev Comment