On Thu, 27 Mar 2025 18:46:02 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> Artur Barashev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix java.security syntax. Remove whitespace.
>
> test/jdk/sun/security/ssl/SignatureScheme/AbstractCheckSignatureSchemes.java
> line 77:
>
>> 75: }
>> 76:
>> 77: protected String getProtocol() {
>
> I'd be more inclined to make this abstract and force subclasses to override
> it.
Will do. I also though about it but since the original `SigSchemePropOrdering`
test had "TLSv1.2" I kept it as default.
> test/jdk/sun/security/ssl/SignatureScheme/DisableSignatureSchemePerScopeTLS12.java
> line 52:
>
>> 50: protected static final String DISABLED_CONSTRAINTS =
>> 51: HANDSHAKE_DISABLED_SIG + " usage HandShakesignature, "
>> 52: + CERTIFICATE_DISABLED_SIG + " usage certificateSignature";
>
> Nit: s/certificateSignature/CertificateSignature/
Actually this is done on purpose to check case-insensitive matching, I use this
approach in other tests as well.
> test/jdk/sun/security/ssl/SignatureScheme/DisableSignatureSchemePerScopeTLS13.java
> line 42:
>
>> 40:
>> 41: public class DisableSignatureSchemePerScopeTLS13
>> 42: extends DisableSignatureSchemePerScopeTLS12 {
>
> It's a little odd this extends *TLS12 - did you consider extending
> `AbstractCheckSignatureSchemes` or was that too complicated?
I've done it for code reusability, we'll have to copy/paste a few methods
otherwise.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23681#discussion_r2017493320
PR Review Comment: https://git.openjdk.org/jdk/pull/23681#discussion_r2017495722
PR Review Comment: https://git.openjdk.org/jdk/pull/23681#discussion_r2017498123
