> On May 17, 2018, at 2:42 AM, Sean Mullan <sean.mul...@oracle.com> wrote: > > The while(true) in PKCS12KeyStore.java seems like it isn't really necessary, > since you are calling the code inside it at most twice. I think a better > approach would be to move lines 2134-2146 into a utility method and call it > again if you get an Exception and the password is empty.
The while block looks a little strange, but there are too many local variables here and extracting the lines into a method means we need to pass all of them as arguments. This is also just a single step in a long process and taking it out as a separate method breaks the code reading. Finally, such while blocks appear 3 times and we will have to create multiple methods like tryDecryptKey/tryDecryptCert/tryVerifyMac. It will be nice if Java allows a method defined inside a method. What do you think if I extract the lines into a lambda? interface RetryWithZero<T> { T tryOnce(char[] password) throws Exception; static <S> S run(RetryWithZero<S> f, char[] password) throws Exception { try { return f.tryOnce(password); } catch (Exception e) { if (password.length == 0) { return f.tryOnce(new char[1]); } throw e; } } } byte[] keyInfo = RetryWithZero.run(pass -> { SecretKey skey = getPBEKey(pass); Cipher cipher = Cipher.getInstance( mapPBEParamsToAlgorithm(algOid, algParams)); cipher.init(Cipher.DECRYPT_MODE, skey, algParams); return cipher.doFinal(encryptedKey); }, password); byte[] rawData = safeContentsData; try { safeContentsData = RetryWithZero.run(pass -> { SecretKey skey = getPBEKey(pass); Cipher cipher = Cipher.getInstance(algOid.toString()); cipher.init(Cipher.DECRYPT_MODE, skey, algParams); return cipher.doFinal(rawData); }, password); } catch (Exception e) { throw new IOException("keystore password was incorrect", new UnrecoverableKeyException( "failed to decrypt safe contents entry: " + e)); } RetryWithZero.run(pass -> { SecretKey key = getPBEKey(pass); m.init(key, params); m.update(authSafeData); byte[] macResult = m.doFinal(); if (debug != null) { debug.println("Checking keystore integrity " + "(" + m.getAlgorithm() + " iterations: " + ic + ")"); } if (!MessageDigest.isEqual(macData.getDigest(), macResult)) { throw new UnrecoverableKeyException("Failed PKCS12" + " integrity checking"); } return (Void)null; }, password); If we code this way, password will NOT be updated permanently (see p.p.s of my previous mail). Thanks Max > > Looks fine otherwise. > > --Sean > > On 4/27/18 12:56 PM, Weijun Wang wrote: >> Please take a look at >> http://cr.openjdk.java.net/~weijun/8202299/webrev.00/ >> Turns out we have to retry [0] other than [] in all 3 locations: decrypting >> keys, decrypting certs, and verifying the mac. >> Thanks >> Max >> p.s. You might wonder why suddenly in Windows Server 2016, Microsoft starts >> using [0] to generate the Mac. In fact, they have been doing this all the >> time. However, before 2016, they also encrypted the certificates, and to >> decrypt them, Java has already changed password from [] to [0]. >> p.p.s. But is this correct? Should the certificate decryption code only >> temporarily retries [0] but not changing password itself? Well, maybe. But >> unless a weird software sometimes uses [] and sometimes [0], this will not >> be a problem, and changing password itself saves us some cycles from always >> trying twice.