Re: RFR: 8244154: Update SunPKCS11 provider with PKCS11 v3.0 header files [v2]
On Mon, 2 Nov 2020 21:33:31 GMT, Weijun Wang wrote: >>> >>> >>> Just curious, can the Java files be generated during the build process? >> >> Hmm, maybe, by the java files, do you just mean PKCS11Constants class or >> more? I am not familiar with how to generate Java files during the build >> process, can you share some pointers? I can look into them for possible >> future enhancement. > >> > Just curious, can the Java files be generated during the build process? >> >> Hmm, maybe, by the java files, do you just mean PKCS11Constants class or >> more? I am not familiar with how to generate Java files during the build >> process, can you share some pointers? I can look into them for possible >> future enhancement. > > Well, any file that was a rewrite of its C sibling. > > SunEC's finite field arithmetic code is generated in the build process. The > generation tool is > https://github.com/openjdk/jdk/blob/fd28aad72d3a13fbc2eb13293d40564e471414f3/make/jdk/src/classes/build/tools/intpoly/FieldGen.java#L35 > and it's called by > https://github.com/openjdk/jdk/blob/1cb7df63e7015da568b67c55a7a4468500ea564d/make/modules/java.base/Gensrc.gmk#L113 > and the files are generated in the `$BUILD_OUTPUT/support/gensrc` directory. > These files will be automatically merged with the human-written sources files > in `src` of the same package at build time. I cannot add comments to unchanged lines in PKCS11Constants.java (there's no + sign on the line numbers), but the class-level comment (starting from line 50) can also be enhanced a little. 1. CK_SESSION_HANDLE appears twice. 2. The following appears in pkcs11t.h and I wonder if they can also be added here: typedef CK_ULONG CK_OTP_PARAM_TYPE; typedef CK_OTP_PARAM_TYPE CK_PARAM_TYPE; /* backward compatibility */ typedef CK_ULONG CK_GENERATOR_FUNCTION; typedef CK_ULONG CK_JAVA_MIDP_SECURITY_DOMAIN; typedef CK_ULONG CK_CERTIFICATE_CATEGORY; typedef CK_ULONG CK_PROFILE_ID; typedef CK_ULONG CK_PRF_DATA_TYPE; typedef CK_MECHANISM_TYPE CK_SP800_108_PRF_TYPE; typedef CK_ULONG CK_SP800_108_DKM_LENGTH_METHOD; typedef CK_ULONG CK_X3DH_KDF_TYPE; typedef CK_ULONG CK_X2RATCHET_KDF_TYPE; typedef CK_ULONG CK_XEDDSA_HASH_TYPE; I also found 2 bugs in pkcs11t.h. `CK_GCM_MESSAGE_PARAMS_PTR` and `CK_CCM_MESSAGE_PARAMS_PTR` are not defined as `CK_PTR` of their corresponding data types. Maybe you can report this to upstream? - PR: https://git.openjdk.java.net/jdk/pull/917
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v3]
On Tue, 3 Nov 2020 01:07:12 GMT, Anthony Scarpino wrote:
>> src/java.base/share/classes/com/sun/crypto/provider/GCTR.java line 153:
>>
>>> 151: while (processed > MAX_LEN) {
>>> 152: encrypt(in, offset, MAX_LEN, out, 0);
>>> 153: dst.get(out, 0, MAX_LEN);
>>
>> Shouldn't this be "put" instead of "get"?
>
> Yeah.. I'm surprised that wasn't caught by the tests. I will look to see
> what case I need to make to check that.
So there is only one calling method that never gives this method it more than
16 bytes. That is why testing never caught this. But it doesn't hurt
performance leaving the ability to call larger input sizes.
-
PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v3]
On Fri, 23 Oct 2020 16:38:01 GMT, Anthony Scarpino
wrote:
>> 8253821: Improve ByteBuffer performance with GCM
>
> Anthony Scarpino has updated the pull request incrementally with six
> additional commits since the last revision:
>
> - style
> - style & comments
> - full update
> - remove old
> - update
> - outputsize
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
541:
> 539: throws IllegalBlockSizeException, ShortBufferException {
> 540: checkDataLength(processed, Math.addExact(len, tagLenBytes));
> 541:
Now that encrypt(byte[], int, int, byte[], int) may also store data into
'ibuffer', shouldn't this encryptFinal() method processes bytes in 'ibuffer'
before processing 'in'? The check here would also needs to be updated with
ibuffer.size()? If this is true, can this be covered in the added regression
tests?
-
PR: https://git.openjdk.java.net/jdk/pull/411
JSSE Debug Log redirection
Hello,
since the backport of the new TLS 1.3 capable JSSE provider the debug
logging of JSSE has changed. This is due to the usage of unified logging,
which is not available on Java 8.
There are some hints in the backport ticket (16 New Debug Logger):
https://bugs.openjdk.java.net/browse/JDK-8248721
- in versions before the backport I could specify -Djavax.net.debug=all to
turn on internal debug logging.
- This was sent to stdout and it DID honor System.setOut() redirection.
This especially means on an application server like JBoss the output is
redirected to the (rolling) application server main logfile, but in the
last two quarterly updates, this is no longer the case.
This looks like a regression to me, the new internal debug loggers used by
JSSE should still write to System.out (and honor redirect)
However, I see that the new JUL logger (and in future the platform logger)
have their benefit, so suppose I want to make changes to my existing code
to accommodate this, how can I actually access and redirect those log
messages? According to the backport request there is some difference
between requesting a JUL logger or creating a internal one dependning on
the value of the system propery.
So I tried to register a handler to see if I can catch those log messages,
but I have not. It works for some messages like the Debug from
X509Certificate, but it does not work for the handshake stuff:
(in the following console output "JUL " is from my handler and
"java.net.ssl|..." is from the internal handling, it will not honor
System.setOut redirects, either.)
java version "1.8.0_261"
Java(TM) SE Runtime Environment (build 1.8.0_261-b12)
Java HotSpot(TM) 64-Bit Server VM (build 25.261-b12, mixed mode)
JUL FINE|1604441741129 started
JUL FINE|1604441741129 ssl started
JUL FINE|1604441741129 ssl started
Connect to https://www.google.com ...
*javax.net.ssl*|FINE|01|main|2020-11-03 23:15:41.270 CET|
*SSLContextImpl.java*:425|System property jdk.tls.client.cipherSuites is
set to 'null'
javax.net.ssl|FINE|01|main|2020-11-03 23:15:41.275
CET|SSLContextImpl.java:425|System property jdk.tls.server.cipherSuites is
set to 'null'
*JUL FINE*|1604441741327 *X509Certificate*: Alg:{0}, Serial:{1},
Subject:{2}, Issuer:{3}, Key type:{4}, Length:{5}, Cert Id:{6}, Valid
from:{7}, Valid until:{8}
JUL FINE|1604441741328 X509Certificate: Alg:{0}, Serial:{1}, Subject:{2},
Issuer:{3}, Key type:{4}, Length:{5}, Cert Id:{6}, Valid from:{7}, Valid
until:{8}
...
javax.net.ssl|FINE|01|main|2020-11-03 23:15:42.358
CET|Finished.java:522|Consuming server Finished handshake message (
"Finished": {
"verify data": {
: 46 40 BF 48 6E 83 A9 7F 6A E0 5A 4D
}'}
)
JUL FINE|1604441742359 TLSHandshake: {0}:{1}, {2}, {3}, {4}
Using TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
-- iterates registered loggers:
log javax.net.ssl null
log sun.net.www.protocol.http.HttpURLConnection null
log jdk.event.security null
log global null
log ALL
JUL close
JUL close
...
I get the same output with -Djava.net.debug=all and -Djava.net.debug. Looks
like the SSLContextImpl or Dinished.java is always using its own logger
with no way to register an handler?
Gruss
Bernd
PS: the sample code is here:
/* SPDX-License-Identifier: apache-2.0 */
package net.eckenfels.test.ssl;
import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.PrintStream;
import java.net.HttpURLConnection;
import java.net.URL;
import java.util.Enumeration;
import java.util.logging.Handler;
import java.util.logging.Level;
import java.util.logging.LogManager;
import java.util.logging.LogRecord;
import java.util.logging.Logger;
import javax.net.ssl.HttpsURLConnection;
import net.eckenfels.test.ssl.UrlInspect.MyHandler;
public class UrlInspect
{
public static class MyHandler extends Handler
{
@Override
public void publish(LogRecord record) {
System.out.println("JUL " + record.getLevel() + "|" +
record.getMillis() + " " + record.getMessage());
}
@Override
public void flush() {
System.out.println("JUL flush");
System.out.flush();
}
@Override
public void close() throws SecurityException {
System.out.println("JUL close");
}
}
public static void main(String[] args) throws IOException {
Handler mh = new MyHandler();
LogManager.getLogManager().reset();
Logger root = LogManager.getLogManager().getLogger("");
root.setLevel(Level.ALL);
root.addHandler(mh);
Logger log = Logger.getLogger("javax.net.ssl");
log.addHandler(mh); // not needed
root.fine("started");
log.fine("ssl started");
//FileOutputStream log = new FileOutputStream(new File("out.log"));
//System.setOut(new PrintStream(log));
//sun.util.logging.PlatformLogger.getLogger("sun.net.www.protocol.http.HttpsURLConnection")
.setLevel(Level.ALL);
URL url = new URL((args.le
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v3]
On Fri, 23 Oct 2020 16:38:01 GMT, Anthony Scarpino wrote: >> 8253821: Improve ByteBuffer performance with GCM > > Anthony Scarpino has updated the pull request incrementally with six > additional commits since the last revision: > > - style > - style & comments > - full update > - remove old > - update > - outputsize src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 746: > 744: len += buffer.remaining(); > 745: } > 746: ct.mark(); This seems redundant given the ct.mark() call on line 732? - PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v3]
On Fri, 23 Oct 2020 16:38:01 GMT, Anthony Scarpino wrote: >> 8253821: Improve ByteBuffer performance with GCM > > Anthony Scarpino has updated the pull request incrementally with six > additional commits since the last revision: > > - style > - style & comments > - full update > - remove old > - update > - outputsize src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 770: > 768: checkDataLength(0, len); > 769: > 770: if ((ibuffer.size() + ct.remaining()) - tagLenBytes > ct is set a new limit (minus the tag length) on line 741. So this check seems incorrect? How about using the 'len' value which seems to be the overall input size which should also be the expected output size? - PR: https://git.openjdk.java.net/jdk/pull/411
Re: GREASE'd ALPN values - a RFC 8701 / RFC 7301 / JEP 244 discussion
On 10/8/2020 9:20 AM, Alexander Scheel wrote:
Hi all,
I saw that ALPN support from JEP 244 was backported to JDK8 and I've
recently had the time to take a closer look at it. For context, I'm
one of the maintainers of JSS, a NSS wrapper for Java. I've been
discussing this with another contributor, Fraser (cc'd).
Hi, thanks for looking it over, and especially thanks for reporting
this. I've filed:
https://bugs.openjdk.java.net/browse/JDK-8254631
to track.
One of the concerns we have with the implementation (and its exposure
in the corresponding SSLEngine/SSLSocket/SSLParameters interface) is
that protocols are passed in as Strings. However, RFC 7301 says in
section 6:
o Identification Sequence: The precise set of octet values that
identifies the protocol. This could be the UTF-8 encoding
[RFC3629] of the protocol name.
This "could be" is probably what the original designer of the ALPN API
went with for API ease-of-use, and it made sense at the time as
everything in the IANA TLS Extensions list was in the ASCII range
(0x00-0x7F). But the GREASE values (0x80-0xFF) invalidated that assumption.
When applied with GREASE'd values from RFC 8701, Strings don't work
well. In particular, most of the registered values [0] are non-UTF-8,
0x0A-0x7A does work, but 0x8A-0xFA won't as you pointed out.
which can't be easily round-tripped in Java. This means that while
precise octet values are specified by IANA, they cannot be properly
specified in Java.
In particular:
byte[] desired = new byte[]{ (byte) 0xFA, (byte) 0xFA };
String encoded = new String(desired, StandardCharsets.UTF_8);
byte[] wire= encoded.getBytes(StandardCharsets.UTF_8);
String round = new String(wire, StandardCharsets.UTF_8);
Right. These 2 values are mapped by the decoder to 2 Object Replacement
Characters ("?" - \ufffd) representing 6 bytes:
0xef, 0xbf, 0xbd, 0xef, 0xbf, 0xbd
https://www.fileformat.info/info/unicode/char/fffd/index.htm
fails, as does choosing US_ASCII for the encoding:
byte[] desired = new byte[]{ (byte) 0xFA, (byte) 0xFA };
String encoded = new String(desired, StandardCharsets.US_ASCII);
byte[] wire= encoded.getBytes(StandardCharsets.UTF_8);
String round = new String(wire, StandardCharsets.UTF_8);
Yes, US_ASCII only uses the first 7 bits, so it also maps to 2
replacement characters ("?"):
0x3f0x3f
Note that we (at the application level) can't control the final (wire
/ round-tripped) encoding to UTF_8 as this is done within the SunJSSE
implementation:
Correct.
and perhaps other files I'm missing.
This decreases interoperability with other TLS implementations.
OpenSSL [1], NSS [2], and GnuTLS [3] support setting opaque blobs as
the ALPN protocol list, meaning the caller is free to supply GREASE'd
values. Go on the other hand still uses its string [4], but that
string class supports round-tripping non-UTF8 values correctly [5].
Additionally, it means that GREASE'd values sent by Java applications
aren't compliant with the RFC 8701/IANA wire values.
Is there some workaround I'm missing?
Nothing is coming to mind.
I believe that setting US_ASCII internally in SunJSSE isn't sufficient
to ensure the right wire encoding gets used. I'm thinking the only
real fix is to deprecate the String methods and provide byte[] methods
for all identifiers.
There is one other option that doesn't introduce a new API but does have
some compatibility risk, and that is to use the ISO_8859_1/LATIN-1
charset instead of UTF-8. This would require folks who use UTF-8 to
update their code, but I haven't yet found any code in the wild which
actually uses anything U+0080 and above. I'm proposing a Security (or
System?) property which would revert the behavior if it becomes a problem.
See the attached file, which is a proposal+code example which will
eventually be turned into a formal CSR barring any significant issue.
I talked to our CSR lead, he felt that in this case, interoperability
probably trumps compatibility for character values that likely aren't
being used anyway, and behavior that was underspecified.
Brad
/*
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Softwa
Re: RFR: 8244154: Update SunPKCS11 provider with PKCS11 v3.0 header files [v2]
On Tue, 3 Nov 2020 16:58:45 GMT, Weijun Wang wrote: > > > https://github.com/openjdk/jdk/blob/0b37b821a10325d9083c23130e4b8921812ed9c5/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/PKCS11Constants.java#L54 > > I cannot add comments to unchanged lines in PKCS11Constants.java (there's no > + sign on the line numbers), but the class-level comment (starting from line > 56) can also be enhanced a little. Sure, I will update them as well. > 1. CK_SESSION_HANDLE appears twice. > > 2. The following appears in pkcs11t.h and I wonder if they can also be > added here: > > > ``` > typedef CK_ULONG CK_OTP_PARAM_TYPE; > typedef CK_OTP_PARAM_TYPE CK_PARAM_TYPE; /* backward compatibility */ > typedef CK_ULONG CK_GENERATOR_FUNCTION; > typedef CK_ULONG CK_JAVA_MIDP_SECURITY_DOMAIN; > typedef CK_ULONG CK_CERTIFICATE_CATEGORY; > typedef CK_ULONG CK_PROFILE_ID; > typedef CK_ULONG CK_PRF_DATA_TYPE; > typedef CK_MECHANISM_TYPE CK_SP800_108_PRF_TYPE; > typedef CK_ULONG CK_SP800_108_DKM_LENGTH_METHOD; > typedef CK_ULONG CK_X3DH_KDF_TYPE; > typedef CK_ULONG CK_X2RATCHET_KDF_TYPE; > typedef CK_ULONG CK_XEDDSA_HASH_TYPE; > ``` > > I also found 2 bugs in pkcs11t.h. `CK_GCM_MESSAGE_PARAMS_PTR` and > `CK_CCM_MESSAGE_PARAMS_PTR` are not defined as `CK_PTR` of their > corresponding data types. Maybe you can report this to upstream? Right, these two looks wrongly defined. I will send a comment about this to the Oasic PKCS11 TC. Thanks~ - PR: https://git.openjdk.java.net/jdk/pull/917
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator)
On Mon, 2 Nov 2020 11:26:51 GMT, Maurizio Cimadamore wrote: >> I looked through the changes in this update. >> >> The shared memory segment support looks sound and the mechanism to close a >> shared memory segment is clever (albeit a bit surprising at first that it >> does global handshake to look for a frame in a scoped region. Also >> surprising that close can cause failure at both ends - it took me a while to >> see that this is pragmatic approach). >> >> The share method specifies NPE if thread == null but there is no thread >> parameter, is this a cut 'n paste error? Another one in registerCleaner >> where it should be NPE if the cleaner is null. >> >> I think the javadoc for the close method needs to be a bit clearer on the >> state of the memory segment when IllegalStateException is thrown. Will it be >> marked "not alive" when it fails? Does this mean there is a resource leak? I >> think an apiNote to explain the rational for why close is not idempotent is >> also needed, or maybe it should be re-visited so that close is a no-op when >> the memory segment is not alive. >> >> Now that MemorySegment is AutoCloseable then maybe the term "alive" should >> be replaced with "open" or "closed" and isAlive replaced with isOpen is >> isClosed. >> >> FileDescriptor can be attraction nuisance and forced reference counting >> everywhere that it is used. Is it needed? Could an isMapped method work >> instead? >> >> mapFromPath was in the second preview but I think the method name should be >> re-examined as it maps a file, the path just locates the file. Naming is >> subjectives but in this case using "map" or "mapFile" would fit beside the >> allocateNative methods. >> >> MappedMemorySegments. The force method specifies a write back guarantee but >> at the same time, the implNote in the class description suggests that the >> methods might be a no-op. You might want to adjust the wording to avoid any >> suggestion that force might be a no-op. >> >> The javadoc for copyFrom isn't changed in this update but I notice it >> specifies IndexOutOfBoundException when the source segment is larger than >> the receiver, have other exceptions been examined? >> >> I don't have any any comments on MemoryAccess except that it's not >> immediately clear why there are "Byte" methods that take a ByteOrder. Make >> sense for the multi-byte types of course. >> >> The updates the java/nio sources look okay but it would be helpful if the >> really long lines could be chopped down as it's just too hard to do >> side-by-side reviews when the lines are so long. A minor nit but the changes >> X-Buffer.java.template mess up the alignment of the parameters to >> copyMemory/copySwapMemory methods. > >> The javadoc for copyFrom isn't changed in this update but I notice it >> specifies IndexOutOfBoundException when the source segment is larger than >> the receiver, have other exceptions been examined? > > This exception is consistent with other uses of this exception throughout > this API (e.g. when writing a segment out of bounds). I assume the CSR needs to be updated so that it's in sync with the API changes in the latest round. - PR: https://git.openjdk.java.net/jdk/pull/548
