Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer [v2]
On Sat, 14 May 2022 03:29:14 GMT, Anthony Scarpino wrote: >> Hi, >> >> I need a review of this fix to allow a read-only 'src' buffer to be used >> with SSLEngine.unwrap(). A temporary read-write buffer is created in the >> SSLCipher operation when a read-only buffer is passed. If the 'src' is >> read-write, there is no effect on the current operation >> >> The PR also includes a CSR for an API implementation note to the >> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption >> operation. 'unwrap()' has had this behavior forever, so there is no >> compatibility issue with this note. Using the 'src' buffer for in-place >> decryption was a performance decision. >> >> Tony > > Anthony Scarpino has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - review update > - update some nits > - PR ready > - Initial I think read-only has been allowed by the spec from the start, we just never allowed it to work properly. I don't think this opens up possible complications in the code. - PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer [v2]
On Mon, 16 May 2022 21:08:48 GMT, Anthony Scarpino
wrote:
> There is too much grey area. It says the src buffer maybe modified, which one
> could interpret it cannot be a read-only, but that would still need
> clarification to explicitly say "no read only buffers". And other than these
> internal 'in-place' crypto reason, there is no API reason to not allow
> read-only buffers as input. I did think about closing the CSR as the text was
> already there about the src buffer, even thought it was using a different
> term. But I felt strongly enough that I wanted to prevent that confusion in
> the future.
I think it is good to make the clarification with a CSR.
As the spec says, "The inbound network buffer, {@code src}, may be modified",
applications cannot assume that the inbound network buffer cannot be modified
(read-only) any longer. It does not grant that an application will work with
read-only inbound buffers for another JSSE provider. As a result, an
application that want to support read-only buffers may also need to support
non-read-only buffers. As makes it more complicated in both application layers
and the JSSE implementation layer. It may be simple that applications and JSSE
implementation codes only need to handle non-read-only buffers.
Just my $0.02. I will let you and Brad for the final decision.
-
PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer [v2]
On Sat, 14 May 2022 03:29:14 GMT, Anthony Scarpino wrote: >> Hi, >> >> I need a review of this fix to allow a read-only 'src' buffer to be used >> with SSLEngine.unwrap(). A temporary read-write buffer is created in the >> SSLCipher operation when a read-only buffer is passed. If the 'src' is >> read-write, there is no effect on the current operation >> >> The PR also includes a CSR for an API implementation note to the >> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption >> operation. 'unwrap()' has had this behavior forever, so there is no >> compatibility issue with this note. Using the 'src' buffer for in-place >> decryption was a performance decision. >> >> Tony > > Anthony Scarpino has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains four commits: > > - review update > - update some nits > - PR ready > - Initial There is too much grey area. It says the src buffer maybe modified, which one could interpret it cannot be a read-only, but that would still need clarification to explicitly say "no read only buffers". And other than these internal 'in-place' crypto reason, there is no API reason to not allow read-only buffers as input. I did think about closing the CSR as the text was already there about the src buffer, even thought it was using a different term. But I felt strongly enough that I wanted to prevent that confusion in the future. - PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer [v2]
On Sat, 14 May 2022 03:29:14 GMT, Anthony Scarpino wrote: >> Hi, >> >> I need a review of this fix to allow a read-only 'src' buffer to be used >> with SSLEngine.unwrap(). A temporary read-write buffer is created in the >> SSLCipher operation when a read-only buffer is passed. If the 'src' is >> read-write, there is no effect on the current operation >> >> The PR also includes a CSR for an API implementation note to the >> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption >> operation. 'unwrap()' has had this behavior forever, so there is no >> compatibility issue with this note. Using the 'src' buffer for in-place >> decryption was a performance decision. >> >> Tony > > Anthony Scarpino has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - review update > - update some nits > - PR ready > - Initial As the specification has been indicate that the input buffer could be updated, what do you think if closing the bug as "Not an issue" (or clarify the spec but no implementation update)? I was just wondering if it really worthy the effort to make the code more complicated. - PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer [v2]
> Hi, > > I need a review of this fix to allow a read-only 'src' buffer to be used with > SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher > operation when a read-only buffer is passed. If the 'src' is read-write, > there is no effect on the current operation > > The PR also includes a CSR for an API implementation note to the > SSLEngine.unwrap. The 'src' buffer may be modified during the decryption > operation. 'unwrap()' has had this behavior forever, so there is no > compatibility issue with this note. Using the 'src' buffer for in-place > decryption was a performance decision. > > Tony Anthony Scarpino has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits: - review update - update some nits - PR ready - Initial - Changes: https://git.openjdk.java.net/jdk/pull/8462/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8462=01 Stats: 393 lines in 3 files changed: 291 ins; 20 del; 82 mod Patch: https://git.openjdk.java.net/jdk/pull/8462.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8462/head:pull/8462 PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Wed, 11 May 2022 22:38:04 GMT, Anthony Scarpino wrote: >> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 162: >> >>> 160: statusServer != HandshakeStatus.NOT_HANDSHAKING); >>> 161: >>> 162: // Read NST >> >> What is NST? > > New Session Ticket Duh, of course! Yay, TLAs!!! (Three Letter Acronyms...) Feel free to expand if you're so inclined. ;) - PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Wed, 11 May 2022 23:03:27 GMT, Anthony Scarpino wrote: >> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 172: >> >>> 170: out.clear(); >>> 171: String testString = "ASDF"; >>> 172: in.put(testString.getBytes()).flip(); >> >> If you're going to convert back from UTF_8 later, you should probably >> convert using getBytes(UTF_8) here. > > setting the input to UTF8 isn't a concern. The latter line to decode it > changes it from using the ByteBuffer.toString() to the contents of the > ByteBuffer in a String. You could use the default charsets for encoding and decoding. i.e. in.clear(); receive(server, out.asReadOnlyBuffer(), in); byte[] ba = new byte[in.remaining()]; in.get(ba); String testResult = new String(ba); - PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Wed, 11 May 2022 22:25:43 GMT, Anthony Scarpino
wrote:
>> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 1:
>>
>>> 1: /*
>>
>> Wondering why this is in javax/net/ssl/SSLSession instead of
>> sun/security/ssl/SSLCipher.
>
> I can move it.. I created it from another test which happen to be in that
> directory
Ah. If you can, please do. Thanks.
>> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 157:
>>
>>> 155: // Do TLS handshake
>>> 156: do {
>>> 157: statusClient = doHandshake(client, out, in);
>>
>> It's potentially a little inefficient returning after each wrap/unwrap()
>> instead of doing the task right away, but it works. No need to change.
>>
>> Is this style something you copied from another test? The
>> SSLEngineTemplate in the templates directory is what I often use.
>
> I got it from somewhere that I don't remember off hand. I'm just trying to
> get through the handshake.
It was just an observation, no need to change.
-
PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Wed, 11 May 2022 22:49:02 GMT, Anthony Scarpino
wrote:
>> src/java.base/share/classes/javax/net/ssl/SSLEngine.java line 677:
>>
>>> 675: * @see #unwrap(ByteBuffer, ByteBuffer[], int, int)
>>> 676: *
>>> 677: * @implNote The data in {@code src} may be modified during the
>>> decryption
>>
>> It looks like a note for the API users to me. Is apiNote tag more
>> appropriate here?
>
> The CSR and this code is changing, the note I was adding was already
> documented, but its existing wording should be more clear.
I like the new update as specified in the CSR. I added my name as Reviewer for
the CSR.
-
PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Wed, 11 May 2022 00:31:23 GMT, Bradford Wetmore wrote:
>> Hi,
>>
>> I need a review of this fix to allow a read-only 'src' buffer to be used
>> with SSLEngine.unwrap(). A temporary read-write buffer is created in the
>> SSLCipher operation when a read-only buffer is passed. If the 'src' is
>> read-write, there is no effect on the current operation
>>
>> The PR also includes a CSR for an API implementation note to the
>> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption
>> operation. 'unwrap()' has had this behavior forever, so there is no
>> compatibility issue with this note. Using the 'src' buffer for in-place
>> decryption was a performance decision.
>>
>> Tony
>
> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 172:
>
>> 170: out.clear();
>> 171: String testString = "ASDF";
>> 172: in.put(testString.getBytes()).flip();
>
> If you're going to convert back from UTF_8 later, you should probably convert
> using getBytes(UTF_8) here.
setting the input to UTF8 isn't a concern. The latter line to decode it
changes it from using the ByteBuffer.toString() to the contents of the
ByteBuffer in a String.
> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 188:
>
>> 186: in.clear();
>> 187: System.out.println("2: Server send: " + testString);
>> 188: in.put(testString.getBytes()).flip();
>
> Same
Same :)
> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 189:
>
>> 187: System.out.println("2: Server send: " + testString);
>> 188: in.put(testString.getBytes()).flip();
>> 189: send(server, in, out);
>
> Did you want to try asReadOnlyBuffer() here also?
Yeah, they should have been. I must have undid it to test something
> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 191:
>
>> 189: send(server, in, out);
>> 190: in.clear();
>> 191: receive(client, out, in);
>
> And here?
Same
-
PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Wed, 11 May 2022 05:52:38 GMT, Xue-Lei Andrew Fan wrote:
>> Hi,
>>
>> I need a review of this fix to allow a read-only 'src' buffer to be used
>> with SSLEngine.unwrap(). A temporary read-write buffer is created in the
>> SSLCipher operation when a read-only buffer is passed. If the 'src' is
>> read-write, there is no effect on the current operation
>>
>> The PR also includes a CSR for an API implementation note to the
>> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption
>> operation. 'unwrap()' has had this behavior forever, so there is no
>> compatibility issue with this note. Using the 'src' buffer for in-place
>> decryption was a performance decision.
>>
>> Tony
>
> src/java.base/share/classes/javax/net/ssl/SSLEngine.java line 677:
>
>> 675: * @see #unwrap(ByteBuffer, ByteBuffer[], int, int)
>> 676: *
>> 677: * @implNote The data in {@code src} may be modified during the
>> decryption
>
> It looks like a note for the API users to me. Is apiNote tag more
> appropriate here?
The CSR and this code is changing, the note I was adding was already
documented, but its existing wording should be more clear.
-
PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Mon, 9 May 2022 23:48:24 GMT, Bradford Wetmore wrote:
>> Hi,
>>
>> I need a review of this fix to allow a read-only 'src' buffer to be used
>> with SSLEngine.unwrap(). A temporary read-write buffer is created in the
>> SSLCipher operation when a read-only buffer is passed. If the 'src' is
>> read-write, there is no effect on the current operation
>>
>> The PR also includes a CSR for an API implementation note to the
>> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption
>> operation. 'unwrap()' has had this behavior forever, so there is no
>> compatibility issue with this note. Using the 'src' buffer for in-place
>> decryption was a performance decision.
>>
>> Tony
>
> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 157:
>
>> 155: // Do TLS handshake
>> 156: do {
>> 157: statusClient = doHandshake(client, out, in);
>
> It's potentially a little inefficient returning after each wrap/unwrap()
> instead of doing the task right away, but it works. No need to change.
>
> Is this style something you copied from another test? The SSLEngineTemplate
> in the templates directory is what I often use.
I got it from somewhere that I don't remember off hand. I'm just trying to get
through the handshake.
> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 162:
>
>> 160: statusServer != HandshakeStatus.NOT_HANDSHAKING);
>> 161:
>> 162: // Read NST
>
> What is NST?
New Session Ticket
-
PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Mon, 9 May 2022 23:15:40 GMT, Bradford Wetmore wrote: >> Hi, >> >> I need a review of this fix to allow a read-only 'src' buffer to be used >> with SSLEngine.unwrap(). A temporary read-write buffer is created in the >> SSLCipher operation when a read-only buffer is passed. If the 'src' is >> read-write, there is no effect on the current operation >> >> The PR also includes a CSR for an API implementation note to the >> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption >> operation. 'unwrap()' has had this behavior forever, so there is no >> compatibility issue with this note. Using the 'src' buffer for in-place >> decryption was a performance decision. >> >> Tony > > test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 1: > >> 1: /* > > Wondering why this is in javax/net/ssl/SSLSession instead of > sun/security/ssl/SSLCipher. I can move it.. I created it from another test which happen to be in that directory - PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Fri, 29 Apr 2022 03:58:57 GMT, Anthony Scarpino
wrote:
> Hi,
>
> I need a review of this fix to allow a read-only 'src' buffer to be used with
> SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher
> operation when a read-only buffer is passed. If the 'src' is read-write,
> there is no effect on the current operation
>
> The PR also includes a CSR for an API implementation note to the
> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption
> operation. 'unwrap()' has had this behavior forever, so there is no
> compatibility issue with this note. Using the 'src' buffer for in-place
> decryption was a performance decision.
>
> Tony
src/java.base/share/classes/javax/net/ssl/SSLEngine.java line 677:
> 675: * @see #unwrap(ByteBuffer, ByteBuffer[], int, int)
> 676: *
> 677: * @implNote The data in {@code src} may be modified during the
> decryption
It looks like a note for the API users to me. Is apiNote tag more appropriate
here?
-
PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Fri, 29 Apr 2022 03:58:57 GMT, Anthony Scarpino wrote: > Hi, > > I need a review of this fix to allow a read-only 'src' buffer to be used with > SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher > operation when a read-only buffer is passed. If the 'src' is read-write, > there is no effect on the current operation > > The PR also includes a CSR for an API implementation note to the > SSLEngine.unwrap. The 'src' buffer may be modified during the decryption > operation. 'unwrap()' has had this behavior forever, so there is no > compatibility issue with this note. Using the 'src' buffer for in-place > decryption was a performance decision. > > Tony All of the comments observed are minor. Please consider some of the test changes, but otherwise looks good. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Fri, 29 Apr 2022 03:58:57 GMT, Anthony Scarpino
wrote:
> Hi,
>
> I need a review of this fix to allow a read-only 'src' buffer to be used with
> SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher
> operation when a read-only buffer is passed. If the 'src' is read-write,
> there is no effect on the current operation
>
> The PR also includes a CSR for an API implementation note to the
> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption
> operation. 'unwrap()' has had this behavior forever, so there is no
> compatibility issue with this note. Using the 'src' buffer for in-place
> decryption was a performance decision.
>
> Tony
Finished the SSLEngine/tests, starting on the SSLCipher. Here's the notes so
far.
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 1:
> 1: /*
Wondering why this is in javax/net/ssl/SSLSession instead of
sun/security/ssl/SSLCipher.
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 51:
> 49: public class ReadOnlyEngine {
> 50:
> 51: private static String pathToStores = "../etc";
These 6 can be final if you want.
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 96:
> 94: status = engine.getHandshakeStatus();
> 95: break;
> 96: case FINISHED:
Can combine FINISHED/NOT_HANDSHAKING?
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 157:
> 155: // Do TLS handshake
> 156: do {
> 157: statusClient = doHandshake(client, out, in);
It's potentially a little inefficient returning after each wrap/unwrap()
instead of doing the task right away, but it works. No need to change.
Is this style something you copied from another test? The SSLEngineTemplate
in the templates directory is what I often use.
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 160:
> 158: statusServer = doHandshake(server, in, out);
> 159: } while (statusClient != HandshakeStatus.NOT_HANDSHAKING ||
> 160: statusServer != HandshakeStatus.NOT_HANDSHAKING);
Minor indent problem.
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 162:
> 160: statusServer != HandshakeStatus.NOT_HANDSHAKING);
> 161:
> 162: // Read NST
What is NST?
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 172:
> 170: out.clear();
> 171: String testString = "ASDF";
> 172: in.put(testString.getBytes()).flip();
If you're going to convert back from UTF_8 later, you should probably convert
using getBytes(UTF_8) here.
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 188:
> 186: in.clear();
> 187: System.out.println("2: Server send: " + testString);
> 188: in.put(testString.getBytes()).flip();
Same
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 189:
> 187: System.out.println("2: Server send: " + testString);
> 188: in.put(testString.getBytes()).flip();
> 189: send(server, in, out);
Did you want to try asReadOnlyBuffer() here also?
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 191:
> 189: send(server, in, out);
> 190: in.clear();
> 191: receive(client, out, in);
And here?
-
PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Fri, 29 Apr 2022 03:58:57 GMT, Anthony Scarpino wrote: > Hi, > > I need a review of this fix to allow a read-only 'src' buffer to be used with > SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher > operation when a read-only buffer is passed. If the 'src' is read-write, > there is no effect on the current operation > > The PR also includes a CSR for an API implementation note to the > SSLEngine.unwrap. The 'src' buffer may be modified during the decryption > operation. 'unwrap()' has had this behavior forever, so there is no > compatibility issue with this note. Using the 'src' buffer for in-place > decryption was a performance decision. > > Tony I don't yet count as a reviewer, but your changes look good to me. You might want to let IntelliJ check for problems before you integrate. - PR: https://git.openjdk.java.net/jdk/pull/8462
