Re: RFR: 8301154: SunPKCS11 KeyStore deleteEntry results in dangling PrivateKey entries [v2]
> Could someone help review this PKCS11KeyStore fix regarding the cert chain > removal? > > The proposed fix will not remove the cert if it has a corresponding private > key or is an issuer of other entities in the same keystore. > > Thanks, > Valerie Valerie Peng has updated the pull request incrementally with one additional commit since the last revision: Changed to use keytool to generate keypairs instead of importing from data files. - Changes: - all: https://git.openjdk.org/jdk/pull/13743/files - new: https://git.openjdk.org/jdk/pull/13743/files/f194733c..4c72e3a6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13743=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13743=00-01 Stats: 277 lines in 7 files changed: 82 ins; 137 del; 58 mod Patch: https://git.openjdk.org/jdk/pull/13743.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13743/head:pull/13743 PR: https://git.openjdk.org/jdk/pull/13743
RFR: 8303465: KeyStore of type KeychainStore, provider Apple does not show all trusted certificates
With this PR we try to be better in loading certificates from the MacOS Keychain into a JDK Trust store. The current implementation after JDK-8278449 would only load/trust certificates from an identity (with private key available) and certificates that have explicit trust set in the user domain (as shown by security dump-trust-settings). This, however is not sufficient and does not match the MacOS system behavior, e.g. if you compare with tools like curl or Safari. This change does the following: 1. The native method that reads trust settings will call the API SecTrustSettingsCopyTrustSettings on a certificate for both, User and Admin domain. 2. No trust settings will be reported as "inputTrust" being null. If the certificate is trusted with no specific records, "inputTrust" will be an empty list. 3. The Java Method to add a certificate now checks for "self signed" certificate not only by checking whether it was signed with its own key but it must also not be a root certificate that can be used to sign other certificates. This is done by inspecting the key usage extension. 4. We now trust certificates that are either "real" self-signed certificates or certificates that have an explicit trust entry with no sub-records that would deny the certificate for any purpose. 5. The check for double aliases has been augmented by comparing whether the certificate to be added is the same as the one that is already present. This can happen if a certificate is contained in both, the user and the system keychain, for instance. I have added a test that verifies whether certificates that should be trusted from "security dump-trust-settings" are contained in the keystore and those that should be disallowed are absent. - Commit messages: - JDK-8303465 Changes: https://git.openjdk.org/jdk/pull/13945/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13945=00 Issue: https://bugs.openjdk.org/browse/JDK-8303465 Stats: 272 lines in 3 files changed: 204 ins; 31 del; 37 mod Patch: https://git.openjdk.org/jdk/pull/13945.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13945/head:pull/13945 PR: https://git.openjdk.org/jdk/pull/13945
Re: RFR: 8301991: Convert l10n properties resource bundles to UTF-8 native [v6]
On Thu, 11 May 2023 20:21:57 GMT, Justin Lu wrote: >> This PR converts Unicode sequences to UTF-8 native in .properties file. >> (Excluding the Unicode space and tab sequence). The conversion was done >> using native2ascii. >> >> In addition, the build logic is adjusted to support reading in the >> .properties files as UTF-8 during the conversion from .properties file to >> .java ListResourceBundle file. > > Justin Lu has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 16 commits: > > - Convert the merged master changes to UTF-8 > - Merge master and fix conflicts > - Close streams when finished loading into props > - Adjust CF test to read in with UTF-8 to fix failing test > - Reconvert CS.properties to UTF-8 > - Revert all changes to CurrencySymbols.properties > - Bug6204853 should not be converted > - Copyright year for CompileProperties > - Redo translation for CS.properties > - Spot convert CurrencySymbols.properties > - ... and 6 more: https://git.openjdk.org/jdk/compare/4386d42d...f15b373a I think this is fine, as those properties files are JDK's own. I believe the benefit of moving to UTF-8 outweighs the issue you wrote, which can be remedied by changing the encoding in the IDEs. - PR Comment: https://git.openjdk.org/jdk/pull/12726#issuecomment-1544722480
Re: RFR: 8301991: Convert l10n properties resource bundles to UTF-8 native [v6]
On Thu, 11 May 2023 20:21:57 GMT, Justin Lu wrote: >> This PR converts Unicode sequences to UTF-8 native in .properties file. >> (Excluding the Unicode space and tab sequence). The conversion was done >> using native2ascii. >> >> In addition, the build logic is adjusted to support reading in the >> .properties files as UTF-8 during the conversion from .properties file to >> .java ListResourceBundle file. > > Justin Lu has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 16 commits: > > - Convert the merged master changes to UTF-8 > - Merge master and fix conflicts > - Close streams when finished loading into props > - Adjust CF test to read in with UTF-8 to fix failing test > - Reconvert CS.properties to UTF-8 > - Revert all changes to CurrencySymbols.properties > - Bug6204853 should not be converted > - Copyright year for CompileProperties > - Redo translation for CS.properties > - Spot convert CurrencySymbols.properties > - ... and 6 more: https://git.openjdk.org/jdk/compare/4386d42d...f15b373a Wondering if anyone has any thoughts on the consequences of this PR, in relation to Intellj's (and other IDEs) default encoding for .properties files. Intellj sets the default encoding for .properties files to ISO-8859-1, which would be the wrong encoding if the .properties files are converted to UTF-8 native. This would cause certain key,values to be skewed when represented in the file. Although the default file-encoding for .properties can be switched to UTF-8, it is not the default. Wondering what some solutions/thoughts to this are. - PR Comment: https://git.openjdk.org/jdk/pull/12726#issuecomment-1544708830
Re: RFR: 8297878: KEM: Implementation [v15]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: deterministic randomness - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/c9f1d8f3..128cefe7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13256=14 - incr: https://webrevs.openjdk.org/?repo=jdk=13256=13-14 Stats: 45 lines in 1 file changed: 41 ins; 3 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8155191: Specify that SecureRandom.nextBytes(byte[]) throws NullPointerException when byte array is null [v4]
> Just a trivial change for enforcing consistent NullPointerException behavior > for the SecureRandom.nextBytes(byte[]) method. > > Other similar methods such as Random.nextByte(byte[]) and its other > subclasses all throw NPE for null byte[] argument. Most JDK default > providers' SecureRandom impls also check and throw NPE. Thus, this should be > moved up and enforced by the SecureRandom class to ensure consistency. > > CSR has been filed. > > Thanks, > Valerie Valerie Peng has updated the pull request incrementally with one additional commit since the last revision: minor test update, remove pkcs11 reg test as it's covered by the other test - Changes: - all: https://git.openjdk.org/jdk/pull/13788/files - new: https://git.openjdk.org/jdk/pull/13788/files/3ee76896..a2564259 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13788=03 - incr: https://webrevs.openjdk.org/?repo=jdk=13788=02-03 Stats: 73 lines in 2 files changed: 0 ins; 72 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13788.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13788/head:pull/13788 PR: https://git.openjdk.org/jdk/pull/13788
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v6]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with two additional commits since the last revision: - update copyright - reworking the fix in light of encouragement to change the problematic method signature - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/87b47a03..0017c094 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13466=05 - incr: https://webrevs.openjdk.org/?repo=jdk=13466=04-05 Stats: 54 lines in 2 files changed: 31 ins; 5 del; 18 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8301991: Convert l10n properties resource bundles to UTF-8 native [v6]
> This PR converts Unicode sequences to UTF-8 native in .properties file. > (Excluding the Unicode space and tab sequence). The conversion was done using > native2ascii. > > In addition, the build logic is adjusted to support reading in the > .properties files as UTF-8 during the conversion from .properties file to > .java ListResourceBundle file. Justin Lu has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 16 commits: - Convert the merged master changes to UTF-8 - Merge master and fix conflicts - Close streams when finished loading into props - Adjust CF test to read in with UTF-8 to fix failing test - Reconvert CS.properties to UTF-8 - Revert all changes to CurrencySymbols.properties - Bug6204853 should not be converted - Copyright year for CompileProperties - Redo translation for CS.properties - Spot convert CurrencySymbols.properties - ... and 6 more: https://git.openjdk.org/jdk/compare/4386d42d...f15b373a - Changes: https://git.openjdk.org/jdk/pull/12726/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12726=05 Stats: 28877 lines in 493 files changed: 14 ins; 1 del; 28862 mod Patch: https://git.openjdk.org/jdk/pull/12726.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12726/head:pull/12726 PR: https://git.openjdk.org/jdk/pull/12726
Re: RFR: 8298127: HSS/LMS Signature Verification [v10]
On Thu, 11 May 2023 09:36:17 GMT, Ferenc Rakoczi wrote:
>> Implement support for Leighton-Micali Signatures (LMS) as described in RFC
>> 8554. LMS is an approved software signing algorithm for CNSA 2.0, with
>> SHA-256/192 parameters recommended.
>
> Ferenc Rakoczi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reintroduced Length for HSSPublicKey, added more @Override annotations
src/java.base/share/classes/sun/security/provider/HSS.java line 436:
> 434: int sigArrLen = (12 + n * (p + 1) + m * h);
> 435: if ((q >= (1 << h)) || (inLen < sigArrLen) || (checkExactLen
> && (inLen != sigArrLen))) {
> 436: throw new InvalidParameterException("LMS signature
> length is incorrect");
Should be a `SignatureException`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1191608083
Re: RFR: 8155191: Specify that SecureRandom.nextBytes(byte[]) throws NullPointerException when byte array is null [v3]
On Wed, 10 May 2023 23:07:01 GMT, Valerie Peng wrote: >> Just a trivial change for enforcing consistent NullPointerException behavior >> for the SecureRandom.nextBytes(byte[]) method. >> >> Other similar methods such as Random.nextByte(byte[]) and its other >> subclasses all throw NPE for null byte[] argument. Most JDK default >> providers' SecureRandom impls also check and throw NPE. Thus, this should be >> moved up and enforced by the SecureRandom class to ensure consistency. >> >> CSR has been filed. >> >> Thanks, >> Valerie > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > updated reg test with review comments. Marked as reviewed by mullan (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13788#pullrequestreview-1423299217
Re: RFR: 8155191: Specify that SecureRandom.nextBytes(byte[]) throws NullPointerException when byte array is null [v3]
On Thu, 11 May 2023 17:16:19 GMT, Sean Mullan wrote: >> Valerie Peng has updated the pull request incrementally with one additional >> commit since the last revision: >> >> updated reg test with review comments. > > test/jdk/java/security/SecureRandom/NextBytesNull.java line 29: > >> 27: * @summary check NPE is thrown for various methods of SecureRandom >> class, >> 28: * e.g. SecureRandom(byte[]), nextBytes(byte[]), and setSeed(byte[]). >> 29: * @run main/othervm NextBytesNull > > Does this need to be run in othervm mode? Probably not. Will remove. - PR Review Comment: https://git.openjdk.org/jdk/pull/13788#discussion_r1191603458
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v5]
On Thu, 11 May 2023 16:40:07 GMT, Kevin Driver wrote: >> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) > > Kevin Driver has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains three commits: > > - Update > src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java > >Co-authored-by: Daniel Jelinski > - updated copyright > - fixes JDK-8294985: throw an SSLException wrapping the IAE Not sure if you have more changes coming, but there are still a few other places where IAE could be thrown. I would change `getAuthorities()` (in both CertificateRequest.java and CertificateAuthoritiesExtension.java) to catch `IllegalArgumentException` and rethrow it as an `SSLException`, as this will ensure all existing and future calls to this method are handled consistently. Also, I would change the `toString()` method to also catch IAE but not propagate it, instead print something like "unparseable X500Principal". - PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1423140554
Re: RFR: 8298127: HSS/LMS Signature Verification [v10]
On Thu, 11 May 2023 16:33:25 GMT, Sean Mullan wrote:
>> Ferenc Rakoczi has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Reintroduced Length for HSSPublicKey, added more @Override annotations
>
> src/java.base/share/classes/sun/security/provider/HSS.java line 719:
>
>> 717:
>> 718: @java.io.Serial
>> 719: protected Object writeReplace() throws
>> java.io.ObjectStreamException {
>
> I think the serialized form of an HSSPublicKey should also be specified in
> the CSR since this Key is returned from a standard API. I think you can add a
> simple sentence such as:
>
> "The Keys returned by an "HSS/LMS" `KeyFactory` are `Serializable` and use
> `java.security.KeyRep` as its serialized representation with the fields set
> as follows: type = `KeyRep.Type.PUBLIC`, algorithm = "HSS/LMS", format =
> "X.509", and encoded = the DER encoded bytes ..."
I added a paragraph to the CSR, although it's already approved several days ago.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r119149
Re: RFR: 8155191: Specify that SecureRandom.nextBytes(byte[]) throws NullPointerException when byte array is null [v3]
On Wed, 10 May 2023 23:07:01 GMT, Valerie Peng wrote: >> Just a trivial change for enforcing consistent NullPointerException behavior >> for the SecureRandom.nextBytes(byte[]) method. >> >> Other similar methods such as Random.nextByte(byte[]) and its other >> subclasses all throw NPE for null byte[] argument. Most JDK default >> providers' SecureRandom impls also check and throw NPE. Thus, this should be >> moved up and enforced by the SecureRandom class to ensure consistency. >> >> CSR has been filed. >> >> Thanks, >> Valerie > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > updated reg test with review comments. test/jdk/java/security/SecureRandom/NextBytesNull.java line 29: > 27: * @summary check NPE is thrown for various methods of SecureRandom class, > 28: * e.g. SecureRandom(byte[]), nextBytes(byte[]), and setSeed(byte[]). > 29: * @run main/othervm NextBytesNull Does this need to be run in othervm mode? - PR Review Comment: https://git.openjdk.org/jdk/pull/13788#discussion_r1191480288
Re: RFR: 8298127: HSS/LMS Signature Verification [v10]
On Thu, 11 May 2023 09:36:17 GMT, Ferenc Rakoczi wrote:
>> Implement support for Leighton-Micali Signatures (LMS) as described in RFC
>> 8554. LMS is an approved software signing algorithm for CNSA 2.0, with
>> SHA-256/192 parameters recommended.
>
> Ferenc Rakoczi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reintroduced Length for HSSPublicKey, added more @Override annotations
src/java.base/share/classes/sun/security/provider/HSS.java line 698:
> 696: @Override
> 697: public String toString() {
> 698: HexDumpEncoder encoder = new HexDumpEncoder();
Nit: one space after `HexDumpEncoder`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1191466810
Re: RFR: 8298127: HSS/LMS Signature Verification [v8]
On Tue, 9 May 2023 14:26:46 GMT, Weijun Wang wrote:
>> src/java.base/share/classes/sun/security/provider/HSS.java line 661:
>>
>>> 659:
>>> 660: @SuppressWarnings("deprecation")
>>> 661: HSSPublicKey(byte[] keyArray) throws InvalidKeyException {
>>
>> [I deleted my earlier comment]
>>
>> I am wondering why you don't call `super()` or `decode()` and override
>> `parseKeyBits()` like other `X509Key` subclasses.
>>
>> You should also override `writeReplace` so that serialization uses an
>> alternate form when serializing.
>>
>> It would probably be useful to override `toString()` as well. See other
>> `X509Key` subclasses for more details.
>
> Yes, I also think with `writeReplace` you can make `L` and `lmsPublicKey`
> transient and there is no need to make `LMSPublicKey` serializable.
`L` should be `transient` too as @wangweij noted.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1191465361
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v5]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver 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 three additional commits since the last revision: - Update src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java Co-authored-by: Daniel Jelinski - updated copyright - fixes JDK-8294985: throw an SSLException wrapping the IAE - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/33366412..87b47a03 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13466=04 - incr: https://webrevs.openjdk.org/?repo=jdk=13466=03-04 Stats: 99417 lines in 1498 files changed: 79732 ins; 8161 del; 11524 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8298127: HSS/LMS Signature Verification [v10]
On Thu, 11 May 2023 09:36:17 GMT, Ferenc Rakoczi wrote:
>> Implement support for Leighton-Micali Signatures (LMS) as described in RFC
>> 8554. LMS is an approved software signing algorithm for CNSA 2.0, with
>> SHA-256/192 parameters recommended.
>
> Ferenc Rakoczi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reintroduced Length for HSSPublicKey, added more @Override annotations
src/java.base/share/classes/sun/security/provider/HSS.java line 39:
> 37: /*
> 38: * This class implements the Hierarchical Signature System using the
> 39: * Leighton-Micali Signatures (LMS) as described in RFC 8554 and NIST
> Special publication 800-208
Nit: add period at end of sentence.
src/java.base/share/classes/sun/security/provider/HSS.java line 719:
> 717:
> 718: @java.io.Serial
> 719: protected Object writeReplace() throws
> java.io.ObjectStreamException {
I think the serialized form of an HSSPublicKey should also be specified in the
CSR since this Key is returned from a standard API. I think you can add a
simple sentence such as:
"The Keys returned by an "HSS/LMS" `KeyFactory` are `Serializable` and use
`java.security.KeyRep` as its serialized representation."
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1191420316
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1191436386
Re: RFR: 8298127: HSS/LMS Signature Verification [v10]
On Thu, 11 May 2023 09:36:17 GMT, Ferenc Rakoczi wrote: >> Implement support for Leighton-Micali Signatures (LMS) as described in RFC >> 8554. LMS is an approved software signing algorithm for CNSA 2.0, with >> SHA-256/192 parameters recommended. > > Ferenc Rakoczi has updated the pull request incrementally with one additional > commit since the last revision: > > Reintroduced Length for HSSPublicKey, added more @Override annotations src/java.base/share/classes/sun/security/provider/HSS.java line 622: > 620: var val = new DerValue(new > ByteArrayInputStream(x.getEncoded())); > 621: val.data.getDerValue(); > 622: return new HSSPublicKey(new > DerValue(val.data.getBitString()).getOctetString()); The 2 lines above cannot detect wrong algorithm identifier and garbage data at the end. Now that you already have `parseBits` implementation, you should follow the usual `X509Key` convention to create a new `HSSPublicKey` constructor that takes in the whole encoding and call `decode` to decode it. See `ECKeyFactory` and `ECPublicKeyImpl.java` for an example. - PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1191340263
Re: RFR: 8298127: HSS/LMS Signature Verification [v10]
On Thu, 11 May 2023 09:36:17 GMT, Ferenc Rakoczi wrote:
>> Implement support for Leighton-Micali Signatures (LMS) as described in RFC
>> 8554. LMS is an approved software signing algorithm for CNSA 2.0, with
>> SHA-256/192 parameters recommended.
>
> Ferenc Rakoczi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reintroduced Length for HSSPublicKey, added more @Override annotations
src/java.base/share/classes/sun/security/provider/HSS.java line 620:
> 618: if (keySpec instanceof X509EncodedKeySpec x) {
> 619: try {
> 620: var val = new DerValue(new
> ByteArrayInputStream(x.getEncoded()));
Do not use `ByteArrayInputStream`, just use `new DerValue(x.getEncoded())`.
Otherwise, the encoding might contain extra garbage bytes at the end and we
still accept it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1191332259
Re: RFR: 8298127: HSS/LMS Signature Verification [v10]
On Thu, 11 May 2023 09:36:17 GMT, Ferenc Rakoczi wrote:
>> Implement support for Leighton-Micali Signatures (LMS) as described in RFC
>> 8554. LMS is an approved software signing algorithm for CNSA 2.0, with
>> SHA-256/192 parameters recommended.
>
> Ferenc Rakoczi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reintroduced Length for HSSPublicKey, added more @Override annotations
src/java.base/share/classes/sun/security/provider/HSS.java line 728:
> 726: @Override
> 727: public int length() {
> 728: return getKey().length();
Debatable. `getKey` now contains 2 (OCTET STRING header) + 4 (L) + 4 (LMS type)
+ 4 (LM-OTS type) + 16 (I) + 32 (T) bytes. Should the 2 header bytes be
included in the length? Should all fields other than T be included? The length
is mainly used to compare strength and I suggest we refrain from implementing
this method unless a well-known definition is accepted for HSS/LMS. Table 2 of
[NIST SP 800-57 Part
1](https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-57pt1r5.pdf)
and they are defined by modulus size.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1191214837
Re: RFR: 8298127: HSS/LMS Signature Verification [v9]
On Thu, 11 May 2023 06:02:01 GMT, Ferenc Rakoczi wrote: >> src/java.base/share/classes/sun/security/provider/HSS.java line 571: >> >>> 569: preCandidate[21] = (byte) 0x80; >>> 570: >>> 571: byte[] preZi = hashBuf.clone(); >> >> We can just call `hashbufSha256_32.clone()` here. We'll think about what to >> do when more params are supported in the future, together with the next line. > > hashBuf is assigned at the initialisation of the LMOTSParams object. If > (when) we introduce more algorithms, the initialisation code and the > digestFixedLengthPreprocessed() code needs to be changed only (its first > parameter should be such that it can use the hash algorithm that the object > would be initialised to use). That's OK. I see you already had `SHA2.SHA256 sha256 = new SHA2.SHA256()` as the next line and thought the selection of `hashBuf` and optimized hash impl will be local in the future. - PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1191113228
Re: RFR: 8298127: HSS/LMS Signature Verification [v10]
> Implement support for Leighton-Micali Signatures (LMS) as described in RFC > 8554. LMS is an approved software signing algorithm for CNSA 2.0, with > SHA-256/192 parameters recommended. Ferenc Rakoczi has updated the pull request incrementally with one additional commit since the last revision: Reintroduced Length for HSSPublicKey, added more @Override annotations - Changes: - all: https://git.openjdk.org/jdk/pull/13691/files - new: https://git.openjdk.org/jdk/pull/13691/files/b363ce7f..b3ffdfd2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13691=09 - incr: https://webrevs.openjdk.org/?repo=jdk=13691=08-09 Stats: 13 lines in 1 file changed: 12 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13691.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13691/head:pull/13691 PR: https://git.openjdk.org/jdk/pull/13691
Re: RFR: 8298127: HSS/LMS Signature Verification [v8]
On Wed, 10 May 2023 22:17:52 GMT, Weijun Wang wrote: >> Done. > > There are much more in this class. You are right. I have added many more. I hope I have found all of them. - PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190885719
Re: RFR: 8298127: HSS/LMS Signature Verification [v9]
On Thu, 11 May 2023 06:27:39 GMT, Ferenc Rakoczi wrote: > I had considered that and decided not to use it. In my opinion, Java Enum is > much more complicated than it should be for this case. OK. > Efficiency is not a concern here OK. > but I also don't see how enum could be more efficient. enum switch is O(1), while if-else not. - PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190697557
Re: RFR: 8298127: HSS/LMS Signature Verification [v9]
On Thu, 11 May 2023 06:14:10 GMT, Xue-Lei Andrew Fan wrote:
>> Ferenc Rakoczi has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> serialization fixes, more code shaping
>
> src/java.base/share/classes/sun/security/provider/HSS.java line 165:
>
>> 163: }
>> 164:
>> 165: static class LMSUtils {
>
> It might be simpler and more efficient to use enum for lms and lmots types.
I had considered that and decided not to use it. In my opinion, Java Enum is
much more complicated than it should be for this case. Efficiency is not a
concern here, but I also don't see how enum could be more efficient.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190690480
Re: RFR: 8298127: HSS/LMS Signature Verification [v9]
On Wed, 10 May 2023 15:20:50 GMT, Ferenc Rakoczi wrote:
>> Implement support for Leighton-Micali Signatures (LMS) as described in RFC
>> 8554. LMS is an approved software signing algorithm for CNSA 2.0, with
>> SHA-256/192 parameters recommended.
>
> Ferenc Rakoczi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> serialization fixes, more code shaping
src/java.base/share/classes/sun/security/provider/HSS.java line 165:
> 163: }
> 164:
> 165: static class LMSUtils {
It might be simpler and more efficient to use enum for lms and lmots types.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190680173
Re: RFR: 8298127: HSS/LMS Signature Verification [v9]
On Wed, 10 May 2023 22:11:09 GMT, Weijun Wang wrote: >> Ferenc Rakoczi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> serialization fixes, more code shaping > > src/java.base/share/classes/sun/security/provider/HSS.java line 571: > >> 569: preCandidate[21] = (byte) 0x80; >> 570: >> 571: byte[] preZi = hashBuf.clone(); > > We can just call `hashbufSha256_32.clone()` here. We'll think about what to > do when more params are supported in the future, together with the next line. hashBuf is assigned at the initialisation of the LMOTSParams object. If (when) we introduce more algorithms, the initialisation code and the digestFixedLengthPreprocessed() code needs to be changed only (its first parameter should be such that it can use the hash algorithm that the object would be initialised to use). - PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190671593
