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

[Sean Mullan]

On Thu, 18 May 2023 03:51:55 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> wrote:

>> Kevin Driver has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   rework based upon code review comments
>
> src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java line 126:
> 
>> 124:         }
>> 125: 
>> 126:         X500Principal[] getAuthorities() throws IllegalArgumentException {
> 
> IAE is unchecked exception, and should not be throwing explicitly in method signature/statement.  I'm not sure if this throwing is really helpful for caller to check the exception.

Yes, I agree with that comment. I suggest adding a comment above the method to remind callers they may need to catch IAE, something like:

// This method will throw IllegalArgumentException if the X500Principal cannot be parsed.

> src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java line 147:
> 
>> 145:                     builder.append(principal.toString());
>> 146:                 } catch (IllegalArgumentException iae) {
>> 147:                     builder.append("unparseable X500Principal");
> 
> I may use the iae message as well for better debugging.

Yes, suggest:

`builder.append("unparseable X500Principal: " + iae.getMessage());`

> src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java line 289:
> 
>> 287:                 shc.peerSupportedAuthorities = spec.getAuthorities();
>> 288:             } catch (IllegalArgumentException iae) {
>> 289:                 shc.conContext.fatal(Alert.DECODE_ERROR, "X500Principal could not be parsed");
> 
> To easy debugging, I may use the exception with fatal(Alert alert, String diagnostic, Throwable cause).  Considering this point, I may prefer to throw SSLException in getAuthorities() method.
> 
> 
> X500Principal[] getAuthorities() throws SSLException {
> ...
> try {
>     shc.peerSupportedAuthorities = spec.getAuthorities();
> } catch (SSLException ssle) {
>     shc.conContext.fatal(Alert.DECODE_ERROR, "Cannot parse peer supported authorities", ssle);
> }

@XueleiFan Seems like an unnecessary wrapping of IAE when it is going to be rethrown anyway.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197768251
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197771043
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197773420