On Tue, 29 Oct 2024 19:35:54 GMT, Artur Barashev <[email protected]> wrote:
>> Check for unexpected plaintext alert message during TLSv1.3 handshake. This
>> can happen if client doesn't receive ServerHello due to network timeout and
>> tries to close the connection by sending an alert message.
>
> Artur Barashev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove logging
One minor issue, a few nits to consider, otherwise it looks reasonable to me.
The opening of permissions for inheritance feels stylistically weird to me, but
can live with it.
I'll review/sponsor it as soon as I see them.
src/java.base/share/classes/sun/security/ssl/SSLCipher.java line 1864:
> 1862: // Check for unexpected plaintext alert.
> 1863: if (contentType == ContentType.ALERT.id
> 1864: && bb.remaining() == 2) {
Minor nit. `if` continuation lines should be 8 spaces. e.g.
if (this > that
&& big > little) {
doSomethingElse();
}
([Oracle Java Code Style
Guidelines](https://www.oracle.com/technetwork/java/codeconventions-150003.pdf)
Section 4: `Line wrapping for if statements should generally use the 8-space
rule, since conventional (4
space) indentation makes seeing the body difficult.`)
src/java.base/share/classes/sun/security/ssl/SSLCipher.java line 1865:
> 1863: if (contentType == ContentType.ALERT.id
> 1864: && bb.remaining() == 2) {
> 1865: throw new
> GeneralSecurityException(String.format(
Ok, I am grudgingly ok with throwing a `GeneralSecurityException` here, but
only because it is then being rewrapped inside a `SSLProtocolException` in
`decodeInputRecord`.
`SSLProtocolException` isn't my first choice of exceptions, as it feels to me
like it should be a `SSLHandshakeException` which is when the client/server
were unable to negotiate the desired level of security for some reason. But I
see it's used throughout this class, so ok.
>From `SSLProtocolException`
> Normally this indicates a flaw in one of the protocol implementations.
I wouldn't really consider it a flaw in one of the protocol implementations, as
the implementation is only acting on the data is had available at the time it
decided to shutdown.
src/java.base/share/classes/sun/security/ssl/SSLCipher.java line 1868:
> 1866: "Unexpected plaintext alert received: " +
> 1867: "Level: %s; Alert: %s",
> 1868: Alert.Level.nameOf(bb.get(bb.position())),
Take or leave this comment: you could dump the raw values as well.
Unexpected plaintext alert received: Level: 1(warning); Alert: 90(user_canceled)
Again, not critical, but might be nice.
test/jdk/sun/security/ssl/SSLCipher/SSLEngineNoServerHelloClientShutdown.java
line 1:
> 1: /*
As you used the templates as the basis for these tests, if you note any errors
or see improvements that should be added to the templates please let's discuss
and file a bug. The template is meant to be clean enough that people writing
tests can just add any-issue specific test code and everything should just work.
test/jdk/sun/security/ssl/SSLCipher/SSLEngineNoServerHelloClientShutdown.java
line 67:
> 65: "Level: warning; Alert: user_canceled";
> 66:
> 67: protected SSLEngine clientEngine; // client Engine
I finally figured out why you had tweaked/removed the `final`s/`private`s
protections in this file: it's because you are having `SSLSocketNoServer...`
reuse some of this code. It was very surprising on first read.
I probably would have just duplicated in the other file to have the test be
standalone.
test/jdk/sun/security/ssl/SSLCipher/SSLSocketNoServerHelloClientShutdown.java
line 52:
> 50:
> 51: /**
> 52: * To reproduce @bug 8331682 (client sends an unencrypted TLS alert during
We used to have a SSLSocketSSLEngineTemplate.java which did exactly this
(Socket on client/Engine on Server by default, but could be switched IIRC), I
wonder what happened to it.
test/lib/jdk/test/lib/security/SecurityUtils.java line 130:
> 128: }
> 129:
> 130: public static void inspectTlsBuffer(ByteBuffer buffer) throws
> IOException {
Another take/leave minor nit: maybe `dumpTlsPacketsBuffer` as that's all
you're doing.
-------------
PR Review: https://git.openjdk.org/jdk/pull/21043#pullrequestreview-2410960880
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1826351875
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1826368022
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1826370835
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1826382738
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1826381328
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1826400074
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1826435828
