Re: Code review request: 6755701 SecretKeySpec DES
I updated the webrev to reflect the simple style change you mention below. I'm going to proceed with the pre-push jar signing procedures figuring the discussion regarding InvalidKeyException is not related to this exact fix. http://cr.openjdk.java.net/~ascarpino/6755701/webrev.01/ Tony On 07/02/2013 02:20 PM, Brad Wetmore wrote: It's not common to use this style: 74 throw new InvalidKeySpecException 75 (Inappropriate key specification); but rather: throw new InvalidKeySpecException( Inapp...); Also, what happens in the case that the size doesn't match up with what DESKey's constructor needs? For example, if you provide 7 bytes, won't that throw a InvalidKeyException and thus you get a null back from engineGenerateSecret? The SecretKeyFactory.generateSecret() API doesn't mention anything about possibly getting a null back. I know that's the existing behavior, but that seems fishy to me. Bug in API? Brad On 6/28/2013 5:33 PM, Xuelei Fan wrote: Looks fine to me. Xuelei On 6/29/2013 1:40 AM, Anthony Scarpino wrote: ping... On 06/13/2013 05:08 PM, Anthony Scarpino wrote: Hi all, I'm requesting a code review for the below bug 6755701 SunJCE DES/DESede SecretKeyFactory.generateSecret throws InvalidKeySpecExc if passed SecretKeySpec http://cr.openjdk.java.net/~ascarpino/6755701/webrev.00/ Thanks Tony
Re: Code review request: 6755701 SecretKeySpec DES
As I mentioned in my earlier email, I think you should add a check to ensure that the result from theSecretKeySpec.getEncoded() has the right length (i.e. 8 for DES, 24 for DESede) before passing them to DESKey/DESedeKey. Valerie On 07/03/13 11:46, Anthony Scarpino wrote: I updated the webrev to reflect the simple style change you mention below. I'm going to proceed with the pre-push jar signing procedures figuring the discussion regarding InvalidKeyException is not related to this exact fix. http://cr.openjdk.java.net/~ascarpino/6755701/webrev.01/ Tony On 07/02/2013 02:20 PM, Brad Wetmore wrote: It's not common to use this style: 74 throw new InvalidKeySpecException 75 (Inappropriate key specification); but rather: throw new InvalidKeySpecException( Inapp...); Also, what happens in the case that the size doesn't match up with what DESKey's constructor needs? For example, if you provide 7 bytes, won't that throw a InvalidKeyException and thus you get a null back from engineGenerateSecret? The SecretKeyFactory.generateSecret() API doesn't mention anything about possibly getting a null back. I know that's the existing behavior, but that seems fishy to me. Bug in API? Brad On 6/28/2013 5:33 PM, Xuelei Fan wrote: Looks fine to me. Xuelei On 6/29/2013 1:40 AM, Anthony Scarpino wrote: ping... On 06/13/2013 05:08 PM, Anthony Scarpino wrote: Hi all, I'm requesting a code review for the below bug 6755701 SunJCE DES/DESede SecretKeyFactory.generateSecret throws InvalidKeySpecExc if passed SecretKeySpec http://cr.openjdk.java.net/~ascarpino/6755701/webrev.00/ Thanks Tony
Re: Code review request: 6755701 SecretKeySpec DES
Just to follow up, we discussed the change offline and I added the checks.. Tony On 07/03/2013 12:13 PM, Valerie (Yu-Ching) Peng wrote: As I mentioned in my earlier email, I think you should add a check to ensure that the result from theSecretKeySpec.getEncoded() has the right length (i.e. 8 for DES, 24 for DESede) before passing them to DESKey/DESedeKey. Valerie On 07/03/13 11:46, Anthony Scarpino wrote: I updated the webrev to reflect the simple style change you mention below. I'm going to proceed with the pre-push jar signing procedures figuring the discussion regarding InvalidKeyException is not related to this exact fix. http://cr.openjdk.java.net/~ascarpino/6755701/webrev.01/ Tony On 07/02/2013 02:20 PM, Brad Wetmore wrote: It's not common to use this style: 74 throw new InvalidKeySpecException 75 (Inappropriate key specification); but rather: throw new InvalidKeySpecException( Inapp...); Also, what happens in the case that the size doesn't match up with what DESKey's constructor needs? For example, if you provide 7 bytes, won't that throw a InvalidKeyException and thus you get a null back from engineGenerateSecret? The SecretKeyFactory.generateSecret() API doesn't mention anything about possibly getting a null back. I know that's the existing behavior, but that seems fishy to me. Bug in API? Brad On 6/28/2013 5:33 PM, Xuelei Fan wrote: Looks fine to me. Xuelei On 6/29/2013 1:40 AM, Anthony Scarpino wrote: ping... On 06/13/2013 05:08 PM, Anthony Scarpino wrote: Hi all, I'm requesting a code review for the below bug 6755701 SunJCE DES/DESede SecretKeyFactory.generateSecret throws InvalidKeySpecExc if passed SecretKeySpec http://cr.openjdk.java.net/~ascarpino/6755701/webrev.00/ Thanks Tony
Re: Code review request: 6755701 SecretKeySpec DES
It's not common to use this style: 74 throw new InvalidKeySpecException 75 (Inappropriate key specification); but rather: throw new InvalidKeySpecException( Inapp...); Also, what happens in the case that the size doesn't match up with what DESKey's constructor needs? For example, if you provide 7 bytes, won't that throw a InvalidKeyException and thus you get a null back from engineGenerateSecret? The SecretKeyFactory.generateSecret() API doesn't mention anything about possibly getting a null back. I know that's the existing behavior, but that seems fishy to me. Bug in API? Brad On 6/28/2013 5:33 PM, Xuelei Fan wrote: Looks fine to me. Xuelei On 6/29/2013 1:40 AM, Anthony Scarpino wrote: ping... On 06/13/2013 05:08 PM, Anthony Scarpino wrote: Hi all, I'm requesting a code review for the below bug 6755701 SunJCE DES/DESede SecretKeyFactory.generateSecret throws InvalidKeySpecExc if passed SecretKeySpec http://cr.openjdk.java.net/~ascarpino/6755701/webrev.00/ Thanks Tony
Re: Code review request: 6755701 SecretKeySpec DES
On 07/02/2013 02:20 PM, Brad Wetmore wrote: It's not common to use this style: 74 throw new InvalidKeySpecException 75 (Inappropriate key specification); but rather: throw new InvalidKeySpecException( Inapp...); That was preexisting code. I have no problem fixing the style, I'm just not taking the fall :) Also, what happens in the case that the size doesn't match up with what DESKey's constructor needs? For example, if you provide 7 bytes, won't that throw a InvalidKeyException and thus you get a null back from engineGenerateSecret? The SecretKeyFactory.generateSecret() API doesn't mention anything about possibly getting a null back. I know that's the existing behavior, but that seems fishy to me. Bug in API? It does seem a bit strange to not be throwing a InvalidKeyException. Looks like a bug in the API. Brad On 6/28/2013 5:33 PM, Xuelei Fan wrote: Looks fine to me. Xuelei On 6/29/2013 1:40 AM, Anthony Scarpino wrote: ping... On 06/13/2013 05:08 PM, Anthony Scarpino wrote: Hi all, I'm requesting a code review for the below bug 6755701 SunJCE DES/DESede SecretKeyFactory.generateSecret throws InvalidKeySpecExc if passed SecretKeySpec http://cr.openjdk.java.net/~ascarpino/6755701/webrev.00/ Thanks Tony
Re: Code review request: 6755701 SecretKeySpec DES
Well, I don't think there is anything wrong with the SecretKeyFactory.generateSecret API. DESKeySpec/DESedeKeySpec do check the length of the key bytes when constructed. If we were to accept the SecretKeySpec object as generic key spec, then we should add additional checkings and throw InvalidKeySpec exception before passing the key bytes to the underlying key impl classes, i.e. DESKey/DESedeKey. Valerie On 07/02/13 14:48, Anthony Scarpino wrote: On 07/02/2013 02:20 PM, Brad Wetmore wrote: It's not common to use this style: 74 throw new InvalidKeySpecException 75 (Inappropriate key specification); but rather: throw new InvalidKeySpecException( Inapp...); That was preexisting code. I have no problem fixing the style, I'm just not taking the fall :) Also, what happens in the case that the size doesn't match up with what DESKey's constructor needs? For example, if you provide 7 bytes, won't that throw a InvalidKeyException and thus you get a null back from engineGenerateSecret? The SecretKeyFactory.generateSecret() API doesn't mention anything about possibly getting a null back. I know that's the existing behavior, but that seems fishy to me. Bug in API? It does seem a bit strange to not be throwing a InvalidKeyException. Looks like a bug in the API. Brad On 6/28/2013 5:33 PM, Xuelei Fan wrote: Looks fine to me. Xuelei On 6/29/2013 1:40 AM, Anthony Scarpino wrote: ping... On 06/13/2013 05:08 PM, Anthony Scarpino wrote: Hi all, I'm requesting a code review for the below bug 6755701 SunJCE DES/DESede SecretKeyFactory.generateSecret throws InvalidKeySpecExc if passed SecretKeySpec http://cr.openjdk.java.net/~ascarpino/6755701/webrev.00/ Thanks Tony
Re: Code review request: 6755701 SecretKeySpec DES
ping... On 06/13/2013 05:08 PM, Anthony Scarpino wrote: Hi all, I'm requesting a code review for the below bug 6755701 SunJCE DES/DESede SecretKeyFactory.generateSecret throws InvalidKeySpecExc if passed SecretKeySpec http://cr.openjdk.java.net/~ascarpino/6755701/webrev.00/ Thanks Tony
Re: Code review request: 6755701 SecretKeySpec DES
Looks fine to me. Xuelei On 6/29/2013 1:40 AM, Anthony Scarpino wrote: ping... On 06/13/2013 05:08 PM, Anthony Scarpino wrote: Hi all, I'm requesting a code review for the below bug 6755701 SunJCE DES/DESede SecretKeyFactory.generateSecret throws InvalidKeySpecExc if passed SecretKeySpec http://cr.openjdk.java.net/~ascarpino/6755701/webrev.00/ Thanks Tony
Code review request: 6755701 SecretKeySpec DES
Hi all, I'm requesting a code review for the below bug 6755701 SunJCE DES/DESede SecretKeyFactory.generateSecret throws InvalidKeySpecExc if passed SecretKeySpec http://cr.openjdk.java.net/~ascarpino/6755701/webrev.00/ Thanks Tony
