RFR: 8150530:Improve javax.crypto.BadPaddingException messages

[Seán Coffey]

Thanks for the review Xuelei. OK - I'll drop the 'password' reference 
then and push the changes.

!                     throw new BadPaddingException("Given final block 
not " +
!                     "properly padded. Such issues could arise if a bad 
key " +
!                     "is used during decryption.");

Regards,
Sean.

On 24/08/16 15:55, Xuelei Fan wrote:
> On 8/24/2016 7:02 PM, Seán Coffey wrote:
>>
>> On 22/08/16 11:06, Xuelei Fan wrote:
>>> Minor comments:
>>>
>>> CipherCore.java
>>> ---------------
>>> "... could arise if a bad key or password is used during decryption."
>>> "password" may be confusing for some user cases.  This could also
>>> happen if bad key used for encryption.  I may just say "could arise if
>>> a bad key used."
>> The exception message modified only gets thrown in a decryption call.
>> Isn't it true that a password fed to an application may be used to
>> generate a key ? For that reason, I was trying to suggest that other
>> variables (from user) could be at fault for the BadPaddingException.
>> Would it be ok to keep the password reference (as a hint?)
>>
> Password is just one way of many to generate a key.  There is no 
> password in the context of this class.  Password based implementation 
> should take care of to show the right message. Adding password word to 
> this message may be confusing to both users and developers, I think.
>
> For example, I want to login with password but the key used to encrypt 
> and decrypt the communication is not actually generated by the 
> password (it's a very common case for HTTPS based login in practice).  
> This exception message may be very confusing.
>
> Not a big issue, I'm OK if you want to keep the password reference.
>
>> I've implemented the rest of your suggested edits. Makes sense - thanks.
>> new webrev :
>> http://cr.openjdk.java.net/~coffeys/webrev.8150530.v2/webrev/index.html
>>
> Looks fine to me.
>
> Thanks,
> Xuelei
>
>> regards,
>> Sean.
>>
>>>
>>> RSAPadding.java
>>> ---------------
>>> I may prefer to use a sentence for the exception message.  For example:
>>>    "Data must be shorter than ... bytes, but received ... bytes"
>>>    "The pad array length (padded.length) is not the specified pad size
>>> (paddedSize)  "
>>>
>>> CipherBox.java
>>> --------------
>>> I may not use the internal variable name in the exception message.  It
>>> might be easier to read:
>>>
>>> 496/580: "The padding removed text (newLen bytes) should be bigger
>>> than <blockSize> as explicit IV used."
>>>
>>> 763/810: "The padding length (padLen) of SSLv3 message should not
>>> bigger than the block size (blockSize)."
>>>
>>> 934: "Insufficient buffer for AEAD cipher fragment, needs more than
>>> (recordIvSize + tagSize) bytes, but only (bb.remaining()) remains in
>>> the buffer"
>>>
>>> P11RSACipher.java
>>> -----------------
>>> 360: "The output buffer (outLen bytes) is too small to hold the
>>> produced data (tmpBuffer.length bytes)"
>>>
>>> Thanks,
>>> Xuelei
>>>
>>> On 8/22/2016 3:56 PM, Seán Coffey wrote:
>>>> Looking to improve some of the messages used in generation of
>>>> BadPaddingException messages. The 'Given final block not properly
>>>> padded' one in particular has caused confusion for some users in the
>>>> past.
>>>>
>>>> JBS report : https://bugs.openjdk.java.net/browse/JDK-8150530
>>>> webrev : http://cr.openjdk.java.net/~coffeys/webrev.8150530/webrev/
>>>>
>>