Skip to content

8358451: SunJCE PBEKey impl should throw IllegalStateException when getEncoded() is called #25632

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@valeriepeng valeriepeng Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ final class PBEKey implements SecretKey {

public byte[] getEncoded() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, PublicKey does not implement (or implement by inheritance) Destroyable.

try {
if (isDestroyed()) {
throw new IllegalStateException("Key is destroyed");
}
return key.clone();
} finally {
// prevent this from being cleaned for the above block
Expand Down Expand Up @@ -139,7 +142,6 @@ public boolean equals(Object obj) {

/**
* Clears the internal copy of the key.
*
*/
@Override
public void destroy() {
Expand Down Expand Up @@ -202,6 +204,9 @@ private void readObject(java.io.ObjectInputStream s)
@java.io.Serial
private Object writeReplace() throws java.io.ObjectStreamException {
try {
if (isDestroyed()) {
throw new IllegalStateException("Key is destroyed");
}
return new KeyRep(KeyRep.Type.SECRET,
getAlgorithm(),
getFormat(),
Expand Down
14 changes: 14 additions & 0 deletions src/java.base/share/classes/javax/crypto/SecretKeyFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -372,9 +372,16 @@ public final SecretKey generateSecret(KeySpec keySpec)
* key), or the given key cannot be dealt with
* (e.g., the given key has an algorithm or format not supported by this
* secret key factory).
* @exception IllegalStateException if the given key is already destroyed.
*/
public final KeySpec getKeySpec(SecretKey key, Class<?> keySpec)
throws InvalidKeySpecException {
if (key == null) {
throw new InvalidKeySpecException("Key is null");
}
if (key.isDestroyed()) {
throw new IllegalStateException("Key is destroyed");
}
if (serviceIterator == null) {
return spi.engineGetKeySpec(key, keySpec);
}
Expand Down Expand Up @@ -407,9 +414,16 @@ public final KeySpec getKeySpec(SecretKey key, Class<?> keySpec)
*
* @exception InvalidKeyException if the given key cannot be processed
* by this secret key factory.
* @exception IllegalStateException if the given key is already destroyed.
*/
public final SecretKey translateKey(SecretKey key)
throws InvalidKeyException {
if (key == null) {
throw new InvalidKeyException("Key is null");
}
if (key.isDestroyed()) {
throw new IllegalStateException("Key is destroyed");
}
if (serviceIterator == null) {
return spi.engineTranslateKey(key);
}
Expand Down
38 changes: 35 additions & 3 deletions test/jdk/com/sun/crypto/provider/KeyFactory/PBEKeyDestroyTest.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2023, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand All @@ -23,11 +23,17 @@

/*
* @test
* @bug 8312306
* @bug 8312306 8358451
* @summary Check the destroy()/isDestroyed() of the PBEKey impl from SunJCE
* @library /test/lib
* @run testng/othervm PBEKeyDestroyTest
*/
import java.io.ByteArrayOutputStream;
import java.io.NotSerializableException;
import java.io.ObjectOutputStream;
import java.security.InvalidKeyException;
import java.security.spec.InvalidKeySpecException;
import java.util.Arrays;
import javax.crypto.*;
import javax.crypto.spec.*;
import java.nio.charset.StandardCharsets;
Expand All @@ -36,6 +42,16 @@

public class PBEKeyDestroyTest {

private static final Class<IllegalStateException> ISE =
IllegalStateException.class;

private static void printKeyInfo(SecretKey k, String name) {
System.out.println(name);
System.out.println("algo: " + k.getAlgorithm());
System.out.println("format: " + k.getFormat());
System.out.println("hashCode: " + k.hashCode());
}

@Test
public void test() throws Exception {
PBEKeySpec keySpec = new PBEKeySpec("12345678".toCharArray(),
Expand All @@ -47,18 +63,34 @@ public void test() throws Exception {
SecretKey key1 = skf.generateSecret(keySpec);
SecretKey key2 = skf.generateSecret(keySpec);

// should be equal
printKeyInfo(key1, "key1");

// both keys should be equal
Assert.assertFalse(key1.isDestroyed());
Assert.assertFalse(key2.isDestroyed());
Assert.assertTrue(key1.equals(key2));
Assert.assertTrue(key2.equals(key1));
Assert.assertTrue(key1.hashCode() == key2.hashCode());

// destroy key1
key1.destroy();

// make sure no exception when retrieving algo, format, hashCode
printKeyInfo(key1, "destroyed key1");

Assert.assertTrue(key1.isDestroyed());
Assert.assertFalse(key1.equals(key2));
Assert.assertFalse(key2.equals(key1));

Assert.assertThrows(ISE, () -> key1.getEncoded());

// serialization should fail
ObjectOutputStream oos = new ObjectOutputStream(
new ByteArrayOutputStream());
Assert.assertThrows(ISE, () -> oos.writeObject(key1));
Assert.assertThrows(ISE, () -> skf.translateKey(key1));
Assert.assertThrows(ISE, () -> skf.getKeySpec(key1, PBEKeySpec.class));

// also destroy key2
key2.destroy();
Assert.assertTrue(key2.isDestroyed());
Expand Down
7 changes: 4 additions & 3 deletions test/jdk/javax/crypto/SecretKeyFactory/FailOverTest.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2006, 2007, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2006, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand All @@ -23,13 +23,14 @@

/*
* test
* @bug 6370923
* @bug 6370923 8358451
* @summary SecretKeyFactory failover does not work
* @author Brad R. Wetmore
*/

import java.security.*;
import javax.crypto.*;
import javax.crypto.spec.SecretKeySpec;

public class FailOverTest {

Expand All @@ -41,7 +42,7 @@ public static void main(String args[]) throws Exception {
Security.insertProviderAt(p2, 2);

SecretKeyFactory skf = SecretKeyFactory.getInstance("DUMMY");
skf.translateKey((SecretKey)null);
skf.translateKey(new SecretKeySpec("any".getBytes(), "DUMMY"));

if (skf.getProvider() != p2) {
throw new Exception("Should have gotten Provider 2");
Expand Down