Thanks for your review Weijun. I'll update with your comments [1] and
push shortly.
regards,
Sean.
[1]
com/sun/crypto/provider/KeyProtector.java:
(reverted to as was)
187: KeyFactory kFac = KeyFactory.getInstance(oidName);
return kFac.generatePrivate(new
PKCS8EncodedKeySpec(plain));
======
JavaKeyStore.java:
@@ -133,18 +133,20 @@
throw new UnrecoverableKeyException("Password must not be
null");
}
- KeyProtector keyProtector = new KeyProtector(password);
+ byte[] passwordBytes = convertToBytes(password);
+ KeyProtector keyProtector = new KeyProtector(passwordBytes);
Regards,
Sean.
On 03/08/18 10:05, Weijun Wang wrote:
com/sun/crypto/provider/KeyProtector.java:
187 Key k = kFac.generatePrivate(new PKCS8EncodedKeySpec(plain));
188 return k;
No need to create a local variable "k" anymore.
JavaKeyStore.java:
128 byte[] passwordBytes = null;
No need to define so early and assigned null.
No other comments.
On Aug 3, 2018, at 12:37 AM, Seán Coffey <sean.cof...@oracle.com> wrote:
All valid comments Max. Changes made. Webrev at
http://cr.openjdk.java.net/~coffeys/webrev.8208583.v4/webrev/index.html
I wonder why DestroyedFailedException was a checked exception, what can we do
if it's thrown?
I guess we could log a message, but given the limited usage case here, I don't
see a need.
I was not clear, I meant I cannot find a proper action to do in this case, thus
it might be better to defined as an unchecked exception.
Thanks
Max
regards,
Sean.
On 02/08/2018 17:05, Weijun Wang wrote:
KeyProtector.java:
113 pbeKeySpec.clearPassword();
You can also put this into the finally block.
189 Arrays.fill(plain, (byte) 0x00);
Can this be in finally?
JavaKeyStore.java:
149 Arrays.fill(passwordBytes, (byte) 0x00);
In other cases, you call it in a finally block. Unnecessary here?
(Oops, every comment is about finally.)
KeyProtector.java:
123 public KeyProtector(byte[] password)
How about just "public KeyProtector(byte[] passwordBytes)"?
On Aug 2, 2018, at 7:38 PM, Seán Coffey <sean.cof...@oracle.com> wrote:
No - no problem at all. Some extra exception handling but probably best for the
long run.
I wonder why DestroyedFailedException was a checked exception, what can we do
if it's thrown?
Thanks
Max
http://cr.openjdk.java.net/~coffeys/webrev.8208583.v3/webrev/index.html
regards,
Sean.
On 02/08/2018 02:13, Weijun Wang wrote:
1.
I wasn't able to rename to destroy since that method is reserved for the
Destroyable interface. I've gone with destroyKey.
Sorry I wasn't clear but this is exactly what I meant. SecretKey implements
Destroyable so you don't need to define sKey as PBEKey. Does it make any
problem?
