RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v3]

[Sean Mullan]

On Tue, 9 May 2023 16:30:22 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> wrote:

>> As for other examples of the `X500Principal(byte[] ..)` constructor being called in TLS packages, here are the ones that don't seem to be handled nicely currently: 
>> 
>> - `consume` in `CertificateAuthoritiesExtension.CRCertificateAuthoritiesConsumer` (could throw IAE, which is an uncaught RuntimeException)
>> - `toString` in `CertificateAuthoritiesExtension.CertificateAuthoritiesSpec` (could throw IAE, which is an uncaught RuntimeException)
>> - `consume` in `CertificateRequest.T10CertificateRequestConsumer` (could throw IAE, which is an uncaught RuntimeException)
>> - `toString` in `CertificateRequest.T10CertificateRequestMessage` (could throw IAE, which is an uncaught RuntimeException)
>> - `consume` in `CertificateRequest.T12CertificateRequestConsumer` (could throw IAE, which is an uncaught RuntimeException)
>> - `toString` in `CertificateRequest.T12CertificateRequestMessage` (could throw IAE, which is an uncaught RuntimeException)
>> 
>> I will look at making related changes in these spots as well. 
>> 
>> @XueleiFan wrt your other question about updating the `getAuthorities` method, I considered this, but it looks like it would involve changing a method signature for that method. This may be fine, but similar changes would need to be made in all the above places anyway, I suspect, unless we can pass information about the context in order to throw an `SSL(Protocol)Exception` and have that bubble-up to where `IOException`s are usually checked. 
>> 
>> @seanjmullan @XueleiFan thoughts on that?
>
>> I will look at making related changes in these spots as well.
>> 
> OK.
> 
>> @XueleiFan wrt your other question about updating the `getAuthorities` method, I considered this, but it looks like it would involve changing a method signature for that method.
> 
> Changing the signature should be fine as it is a internal method.  But I'm fine if the calls to getAuthorities() have considered the impact of illegal values of X500Principal.
> 
> Anyway, this is a typical example to show how hard to use runtime exception.  From the viewpoint of X500Principal, and unchecked exception should be thrown for invalid input values.  But for the caller, it may need to check the input values for sure everything is good.  However, an unchecked exception cannot be detected by Java compiler and thus the checking of unchecked exception could be missed.

I would suggest catching the IAE and rethrowing as an IOE (with the IAE as the cause) if the data is coming from the network. If it is not coming from the network, then don't necessarily rethrow as the IAE could indicate a bug in the JSSE code. I agree with @XueleiFan that it is ok to change internal method signatures, by adding more parameters or throwing exceptions.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/13466#issuecomment-1542441464