On Mon, 31 Jan 2022 20:24:47 GMT, Xue-Lei Andrew Fan <xue...@openjdk.org> wrote:
>> This update is to support signature schemes customization for individual >> (D)TLS connection. Please review the CSR as well: >> CSR: https://bugs.openjdk.java.net/browse/JDK-8280495 >> RFE: https://bugs.openjdk.java.net/browse/JDK-8280494 > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > Rollback to use captialized S A few more comments on the API. src/java.base/share/classes/javax/net/ssl/SSLParameters.java line 727: > 725: * Note that the underlying provider may define the default signature > 726: * schemes for each SSL/TLS/DTLS connection. Applications may also > use > 727: * System Property, "jdk.tls.client.SignatureSchemes" and/or Use the javadoc annotation @systemProperty when referring to the system properties. Look at other code for examples. src/java.base/share/classes/javax/net/ssl/SSLParameters.java line 734: > 732: * pre-populated objects. > 733: * > 734: * @return a non-null, possibly zero-length array of signature scheme The other methods that return arrays like `getCipherSuites` and `getProtocols` return `null` if none have been set. While one could argue whether returning `null` or an empty array is better, there is already a precedent in this API of returning `null`, and with this API programmers will have to check for two different ways of checking for parameters that are not set. I'm not really sure if the inconsistency is worth it. src/java.base/share/classes/javax/net/ssl/SSLParameters.java line 765: > 763: * System Property, "jdk.tls.client.SignatureSchemes" and/or > 764: * "jdk.tls.server.SignatureSchemes", to customize the > provider-specific > 765: * default signature schemes. If the {@code signatureSchemes} array > is In this sentence does `signatureSchemes` mean the "*.SignatureSchemes" property or the `signatureSchemes` parameter? I don't think I understand this sentence. I think you should explain when the system property overrides the parameter in this API, and/or vice-versa. src/java.base/share/classes/javax/net/ssl/SSLParameters.java line 774: > 772: * is empty (zero-length), the provider-specific default > signature > 773: * schemes will be used for the SSL/TLS/DTLS connection. > 774: * @throws IllegalArgumentException if signatureSchemes is null, or > if The other methods that accept arrays like `setCipherSuites` and `setProtocols` accept `null` as a parameter. Like my previous comment, it seems more important to keep this behavior consistent in the API and allow `null` as an acceptable value, which is also useful for clearing the current value of the parameter. ------------- Changes requested by mullan (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7252
