I am writing a custom Java SecretKey implementation that supports destruction and is thread-safe. Please see my code below:

public class DSecretKey implements SecretKey, KeySpec {

  final String algorithm;
  final byte[] key;
  final AtomicBoolean destroyed = new AtomicBoolean(false);

  public DSecretKey(byte[] key, String algorithm) {
    Asserts.notNull(key);
    this.key = key.clone(); this.algorithm = algorithm;
  }

  @Override
  public String getAlgorithm() { return this.algorithm; }

  @Override
  public String getFormat() { return "RAW"; }

  @Override
  public byte[] getEncoded() {
    if (isDestroyed()) throw new IllegalStateException("The key is destroyed.");
    return this.key.clone();
  }

  @Override
  public boolean isDestroyed() { return this.destroyed.get(); }

  @Override
  public void destroy() throws DestroyFailedException {
    if (this.destroyedpareAndSet(false, true)) {
      CryptoUtils.clear(this.key);
    }
  }
}

As you can see, I am using AtomicBoolean to manage the destroyed state of the key.

Question: Should I not use AtomicBoolean and instead use a boolean data type with synchronized, volatile or a Java Lock in this case? Which method is better?

Note: The key can be used/reused concurrently.

I am writing a custom Java SecretKey implementation that supports destruction and is thread-safe. Please see my code below:

public class DSecretKey implements SecretKey, KeySpec {

  final String algorithm;
  final byte[] key;
  final AtomicBoolean destroyed = new AtomicBoolean(false);

  public DSecretKey(byte[] key, String algorithm) {
    Asserts.notNull(key);
    this.key = key.clone(); this.algorithm = algorithm;
  }

  @Override
  public String getAlgorithm() { return this.algorithm; }

  @Override
  public String getFormat() { return "RAW"; }

  @Override
  public byte[] getEncoded() {
    if (isDestroyed()) throw new IllegalStateException("The key is destroyed.");
    return this.key.clone();
  }

  @Override
  public boolean isDestroyed() { return this.destroyed.get(); }

  @Override
  public void destroy() throws DestroyFailedException {
    if (this.destroyedpareAndSet(false, true)) {
      CryptoUtils.clear(this.key);
    }
  }
}

As you can see, I am using AtomicBoolean to manage the destroyed state of the key.

Question: Should I not use AtomicBoolean and instead use a boolean data type with synchronized, volatile or a Java Lock in this case? Which method is better?

Note: The key can be used/reused concurrently.

• java
• cryptography
• secret-key
• jce
• java-cryptography

1 Answer 1

The problem your implementation is facing is that getEncoded() is not thread safe. During the time between the call to isDestroyed() and key.clone() another thread could call destroy() and the returned key would be all empty, which is not what the implementation is supposed to do.

Since an instance of your class is after construction in an initialized state it can only go to a destroyed state. Then you could implement the method as:

  @Override
  public byte[] getEncoded() {
    byte[] clonedKey = this.key.clone();
    if (isDestroyed()) throw new IllegalStateException("The key is destroyed.");
    return clonedKey;
  }

This way round you can be sure that only a non-destroyed value is returned from the method.