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: 8286423: Destroy password protection in the example code in KeyStore [v3]
On Tue, 10 May 2022 22:09:14 GMT, Weijun Wang wrote: >> Oops, I tried to check but finally forgot about it. Thanks! > > It's probably better to convert these long example code to snippets, maybe > next time. The length is a little bit long, but it is fine to me. It may be nice to revise the example and description, but let's do it later. I modified the example by using one try clause only. - PR: https://git.openjdk.java.net/jdk/pull/8623
Re: RFR: 8286423: Destroy password protection in the example code in KeyStore [v3]
> Hi, > > May I have this simple example update in the KeyStore specification? > > Password protection should be destroyed in the example code in KeyStore > specification. Otherwise, applications may just copy and past the code, and > forget to clean up password protection. > > It's a trivial update, and may not worthy of a CSR. But please let me know > if you would like to have a CSR filed. > > Thanks, > Xuelei Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: More udapte - Changes: - all: https://git.openjdk.java.net/jdk/pull/8623/files - new: https://git.openjdk.java.net/jdk/pull/8623/files/e0bd03d0..442c1fda Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8623=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8623=01-02 Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8623.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8623/head:pull/8623 PR: https://git.openjdk.java.net/jdk/pull/8623
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v2]
On Tue, 10 May 2022 23:01:33 GMT, Roger Riggs wrote:
>> PR#8599 8244681: proposes to add compiler warnings for possible lossy
>> conversions
>> From the CSR:
>>
>> "If the type of the right-hand operand of a compound assignment is not
>> assignment compatible with the type of the variable, a cast is implied and
>> possible lossy conversion may silently occur. While similar situations are
>> resolved as compilation errors for primitive assignments, there are no
>> similar rules defined for compound assignments."
>>
>> This PR anticipates the warnings and adds explicit casts to replace the
>> implicit casts.
>> In most cases, the cast matches the type of the right-hand side to type of
>> the compound assignment.
>> Since these casts are truncating the value, there might be a different
>> refactoring that avoids the cast
>> but a direct replacement was chosen here.
>>
>> (Advise and suggestions will inform similar updates to other OpenJDK
>> modules).
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove stray file
src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java line 128:
> 126: // timed poll interrupted so need to adjust timeout
> 127: long adjust = System.nanoTime() - startTime;
> 128: to -= (int)TimeUnit.MILLISECONDS.convert(adjust,
> TimeUnit.NANOSECONDS);
Can we change this `to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust);` while
we here?
src/java.base/share/classes/jdk/internal/org/objectweb/asm/Frame.java line 615:
> 613: if (outputStackTop >= elements) {
> 614: outputStackTop -= (short)elements;
> 615: } else {
I think we usually try to avoid changing the ASM code if possible but maybe you
have to do this because the compiler option is used for compiling java.base?
src/java.base/unix/classes/sun/nio/ch/PollSelectorImpl.java line 123:
> 121: // timed poll interrupted so need to adjust timeout
> 122: long adjust = System.nanoTime() - startTime;
> 123: to -= (int)TimeUnit.MILLISECONDS.convert(adjust,
> TimeUnit.NANOSECONDS);
This one can also be changed to:
`to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust);`
-
PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v2]
On Wed, 11 May 2022 01:14:00 GMT, Valerie Peng wrote: >> The `core.init(..., cipher)` is actually >> `cipher.init(core.translateKeyAndParams())`. Is it possible we write it this >> way? > > It's possible, more refactoring would be needed and not necessarily less > lines of code. With your suggested change, the caller has to explicitly > destroy the derived key after the cipher.engineInit() call. This would be > repeated in all PKCS12 PBE cipher impl classes, but then there'd be no > casting of the actual classes. I assume this is what you are referring to? > Can code it out and see how it looks. If the returned key-and-iv class implements Closeable, then you can do a try-with-resources to destroy the key, which saves you more lines. - PR: https://git.openjdk.java.net/jdk/pull/8521
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: 8002277: Refactor two PBE classes to simplify maintenance [v2]
On Tue, 10 May 2022 02:27:13 GMT, Weijun Wang wrote: >> Currently, the specified CipherSpi object is one of RC4, RC2, DESede. The >> "else" part is for catching new PKCS12 PBE algorithms support which uses >> other cipher algorithms. >> CipherSpi.engineInit(...) is protected, so that's why we use the instanceof >> to cast the CipherSpi object into the actual impl class in order to call the >> engineInit(...) since the actual impl class is in the same package of this >> class. > > The `core.init(..., cipher)` is actually > `cipher.init(core.translateKeyAndParams())`. Is it possible we write it this > way? It's possible, more refactoring would be needed and not necessarily less lines of code. With your suggested change, the caller has to explicitly destroy the derived key after the cipher.engineInit() call. This would be repeated in all PKCS12 PBE cipher impl classes, but then there'd be no casting of the actual classes. I assume this is what you are referring to? Can code it out and see how it looks. - PR: https://git.openjdk.java.net/jdk/pull/8521
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: 8286428: AlgorithmId should understand PBES2
On Mon, 9 May 2022 21:44:10 GMT, Weijun Wang wrote: > `AlgorithmId.getName` is updated for PBES2 algorithm identifiers so it > directly returns the standard algorithm defined by Java (Ex: > `PBEWithHmacSHA256AndAES_256`), instead of a simple "PBES2". > > Please note I specifically update the javadoc for this method to clarify that > this name is meant to be a name that's recognized by various `getInstance()` > methods. This is how we are actually using this method. > > After this change, the `javax.crypto.EncryptedPrivateKeyInfo` API > automatically works with PBES2 encrypted data. As the spec of its > `getAlgName()` methods says, "Standard name is returned". This is shown by > the newly include regression test. > > Existing security-related tests run fine. Marked as reviewed by valeriep (Reviewer). Changes looks fine. - PR: https://git.openjdk.java.net/jdk/pull/8615
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
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v5]
On Tue, 10 May 2022 22:01:16 GMT, Weijun Wang wrote:
>> Mat Carter has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add test from wangweij
>
> test/jdk/sun/security/mscapi/AllTypes.java line 60:
>
>> 58: return true;
>> 59: } catch (IOException ioe) {
>> 60: if
>> (ioe.getMessage().trim().endsWith("java.security.KeyStoreException: Access
>> is denied.")) {
>
> Could the message be different if the system language is not English?
I checked the API and its not guaranteed to be English. I'm investigating a
better method for detecting admin rights
-
PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v2]
> PR#8599 8244681: proposes to add compiler warnings for possible lossy > conversions > From the CSR: > > "If the type of the right-hand operand of a compound assignment is not > assignment compatible with the type of the variable, a cast is implied and > possible lossy conversion may silently occur. While similar situations are > resolved as compilation errors for primitive assignments, there are no > similar rules defined for compound assignments." > > This PR anticipates the warnings and adds explicit casts to replace the > implicit casts. > In most cases, the cast matches the type of the right-hand side to type of > the compound assignment. > Since these casts are truncating the value, there might be a different > refactoring that avoids the cast > but a direct replacement was chosen here. > > (Advise and suggestions will inform similar updates to other OpenJDK modules). Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: remove stray file - Changes: - all: https://git.openjdk.java.net/jdk/pull/8642/files - new: https://git.openjdk.java.net/jdk/pull/8642/files/e72ce85c..7ff806a5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8642=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8642=00-01 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8642.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8642/head:pull/8642 PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286378: Address possibly lossy conversions in java.base
On Tue, 10 May 2022 21:32:10 GMT, Roger Riggs wrote: > PR#8599 8244681: proposes to add compiler warnings for possible lossy > conversions > From the CSR: > > "If the type of the right-hand operand of a compound assignment is not > assignment compatible with the type of the variable, a cast is implied and > possible lossy conversion may silently occur. While similar situations are > resolved as compilation errors for primitive assignments, there are no > similar rules defined for compound assignments." > > This PR anticipates the warnings and adds explicit casts to replace the > implicit casts. > In most cases, the cast matches the type of the right-hand side to type of > the compound assignment. > Since these casts are truncating the value, there might be a different > refactoring that avoids the cast > but a direct replacement was chosen here. > > (Advise and suggestions will inform similar updates to other OpenJDK modules). IO, math, and NIO changes look fine. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v4]
> This change refactors the PBES2Core and PKCS12PBECipherCore classes in SunJCE > provider as requested in the bug record. Functionality should remain the same > with a clearer and simplified code/control flow with less lines of code. > This should improve readability and maintenance. I enhanced one existing > regression test to test more scenarios. This test would pass before the > proposed change and continues to pass with the proposed changes. Valerie Peng has updated the pull request incrementally with one additional commit since the last revision: Address review feedbacks - Changes: - all: https://git.openjdk.java.net/jdk/pull/8521/files - new: https://git.openjdk.java.net/jdk/pull/8521/files/307d2765..0907114b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8521=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8521=02-03 Stats: 63 lines in 2 files changed: 5 ins; 5 del; 53 mod Patch: https://git.openjdk.java.net/jdk/pull/8521.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8521/head:pull/8521 PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8286378: Address possibly lossy conversions in java.base
On Tue, 10 May 2022 21:32:10 GMT, Roger Riggs wrote: > PR#8599 8244681: proposes to add compiler warnings for possible lossy > conversions > From the CSR: > > "If the type of the right-hand operand of a compound assignment is not > assignment compatible with the type of the variable, a cast is implied and > possible lossy conversion may silently occur. While similar situations are > resolved as compilation errors for primitive assignments, there are no > similar rules defined for compound assignments." > > This PR anticipates the warnings and adds explicit casts to replace the > implicit casts. > In most cases, the cast matches the type of the right-hand side to type of > the compound assignment. > Since these casts are truncating the value, there might be a different > refactoring that avoids the cast > but a direct replacement was chosen here. > > (Advise and suggestions will inform similar updates to other OpenJDK modules). The update in security area looks good to me. - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286423: Destroy password protection in the example code in KeyStore [v2]
On Tue, 10 May 2022 22:03:19 GMT, Xue-Lei Andrew Fan wrote:
>> src/java.base/share/classes/java/security/KeyStore.java line 165:
>>
>>> 163: *}
>>> 164: *} finally {
>>> 165: *protParam.destroy();
>>
>> `KeyStore.ProtectionParameter` does not have a `destroy` method. Only
>> `PasswordProtection` does.
>
> Oops, I tried to check but finally forgot about it. Thanks!
It's probably better to convert these long example code to snippets, maybe next
time.
-
PR: https://git.openjdk.java.net/jdk/pull/8623
Re: RFR: 8286423: Destroy password protection in the example code in KeyStore [v2]
On Tue, 10 May 2022 22:07:47 GMT, Xue-Lei Andrew Fan wrote: >> Hi, >> >> May I have this simple example update in the KeyStore specification? >> >> Password protection should be destroyed in the example code in KeyStore >> specification. Otherwise, applications may just copy and past the code, and >> forget to clean up password protection. >> >> It's a trivial update, and may not worthy of a CSR. But please let me know >> if you would like to have a CSR filed. >> >> Thanks, >> Xuelei > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > Use PasswordProtection Approved. Although that `mySecretKey` is out of nowhere, but this is just example code. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8623
Re: RFR: 8286423: Destroy password protection in the example code in KeyStore [v2]
On Tue, 10 May 2022 13:36:19 GMT, Weijun Wang wrote:
>> Xue-Lei Andrew Fan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Use PasswordProtection
>
> src/java.base/share/classes/java/security/KeyStore.java line 165:
>
>> 163: *}
>> 164: *} finally {
>> 165: *protParam.destroy();
>
> `KeyStore.ProtectionParameter` does not have a `destroy` method. Only
> `PasswordProtection` does.
Oops, I tried to check but finally forgot about it. Thanks!
-
PR: https://git.openjdk.java.net/jdk/pull/8623
Re: RFR: 8286423: Destroy password protection in the example code in KeyStore [v2]
> Hi, > > May I have this simple example update in the KeyStore specification? > > Password protection should be destroyed in the example code in KeyStore > specification. Otherwise, applications may just copy and past the code, and > forget to clean up password protection. > > It's a trivial update, and may not worthy of a CSR. But please let me know > if you would like to have a CSR filed. > > Thanks, > Xuelei Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: Use PasswordProtection - Changes: - all: https://git.openjdk.java.net/jdk/pull/8623/files - new: https://git.openjdk.java.net/jdk/pull/8623/files/12c745d9..e0bd03d0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8623=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8623=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8623.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8623/head:pull/8623 PR: https://git.openjdk.java.net/jdk/pull/8623
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v2]
On Tue, 10 May 2022 00:09:16 GMT, Weijun Wang wrote: >> Oh, the comment about "may be 0" is meant toward the >> pbeKey.getInterationCount() call... Hmm, I will make it clearer. > > I see. Another question, shall we reset `salt` and `iCount` at the beginning? > If `params` is null and `key` is not `PBEKey` and there is an existing > positive `iCount`, it will not be set to `DEFAULT_COUNT`. Yeah, I see PKCS12PBECipherCore does that. Will update PBES2Core with it too. - PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v5]
On Tue, 10 May 2022 18:55:50 GMT, Mat Carter wrote:
>> On Windows you can now access the local machine keystores using the strings
>> "Windows-MY-LOCALMACHINE" and "Windows-ROOT-LOCALMACHINE"; note the
>> application requires admin privileges.
>>
>> "Windows-MY" and "Windows-ROOT" remain unchanged, however given these
>> original keystore strings mapped to the current user, I added
>> "Windows-MY-CURRENTUSER" and "Windows-ROOT-CURRENTUSER" so that a developer
>> can explicitly specify the current user location. These two new strings
>> simply map to the original two strings, i.e. no duplication of code paths etc
>>
>> No new tests added, keystore functionality and API remains unchanged, the
>> local machine keystore types would require the tests to run in admin mode
>>
>> Tested on windows, passes tier1 and tier2 tests
>
> Mat Carter has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add test from wangweij
test/jdk/sun/security/mscapi/AllTypes.java line 60:
> 58: return true;
> 59: } catch (IOException ioe) {
> 60: if
> (ioe.getMessage().trim().endsWith("java.security.KeyStoreException: Access is
> denied.")) {
Could the message be different if the system language is not English?
-
PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: 8286378: Address possibly lossy conversions in java.base
On Tue, 10 May 2022 21:32:10 GMT, Roger Riggs wrote: > PR#8599 8244681: proposes to add compiler warnings for possible lossy > conversions > From the CSR: > > "If the type of the right-hand operand of a compound assignment is not > assignment compatible with the type of the variable, a cast is implied and > possible lossy conversion may silently occur. While similar situations are > resolved as compilation errors for primitive assignments, there are no > similar rules defined for compound assignments." > > This PR anticipates the warnings and adds explicit casts to replace the > implicit casts. > In most cases, the cast matches the type of the right-hand side to type of > the compound assignment. > Since these casts are truncating the value, there might be a different > refactoring that avoids the cast > but a direct replacement was chosen here. > > (Advise and suggestions will inform similar updates to other OpenJDK modules). Looks good. Assuming copyright years will be updated. src/java.base/share/classes/java/time/es line 1: > 1: X Looks like a random file was added by accident? - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8642
RFR: 8286378: Address possibly lossy conversions in java.base
PR#8599 8244681: proposes to add compiler warnings for possible lossy conversions >From the CSR: "If the type of the right-hand operand of a compound assignment is not assignment compatible with the type of the variable, a cast is implied and possible lossy conversion may silently occur. While similar situations are resolved as compilation errors for primitive assignments, there are no similar rules defined for compound assignments." This PR anticipates the warnings and adds explicit casts to replace the implicit casts. In most cases, the cast matches the type of the right-hand side to type of the compound assignment. Since these casts are truncating the value, there might be a different refactoring that avoids the cast but a direct replacement was chosen here. (Advise and suggestions will inform similar updates to other OpenJDK modules). - Commit messages: - 8286378: Address possibly lossy conversions in java.base Changes: https://git.openjdk.java.net/jdk/pull/8642/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8642=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286378 Stats: 57 lines in 33 files changed: 1 ins; 3 del; 53 mod Patch: https://git.openjdk.java.net/jdk/pull/8642.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8642/head:pull/8642 PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v3]
On Tue, 10 May 2022 18:51:07 GMT, Mat Carter wrote: >> @macarte You need to finalize your CSR soon if you want to include this >> change into JDK 19. RDP1 is 2022/06/09, and all enhancements require >> approval after that. >> >> BTW, is it possible to detect whether you have admin privilege inside the >> test? There is a `NTSystem` class inside Java which can find out user info. >> Maybe the `getGroupIDs()` method will tell you if you are an admin? >> >> We can enhance the test before RC. > > @wangweij - I added your test and updated to handle Access Denied errors due > to admin rights missing, in this case we ignore the test > > The CSR has been finalized and I feel we are ready to close out this PR @macarte Can you explain more on when an "access denied" error could happen? I create a normal user in my local AD and didn't see that. We might need to talk about this in the release note. - PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v4]
On Mon, 9 May 2022 18:45:05 GMT, Valerie Peng wrote:
>> This is to update the method javadoc of
>> java.security.Signature.getParameters() with the missing `@throws
>> UnsupportedOperationException`. In addition, the wording on the returned
>> parameters are updated to match those in Cipher and CipherSpi classes.
>>
>> CSR will be filed later.
>>
>> Thanks,
>> Valerie
>
> Valerie Peng 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:
>
> - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8253176
> - Sync'ed w/ the wording in the other Cipher.getParameters() PR.
> - Undo un-intentional changes.
> - 8253176: Signature.getParameters should specify that it can throw
> UnsupportedOperationException
src/java.base/share/classes/java/security/SignatureSpi.java line 399:
> 397: * values used by the underlying signature scheme. If the required
> 398: * parameters were not supplied and can be generated by the
> signature,
> 399: * the generated parameters will be returned. Otherwise, {@code null}
Minor wording nit - change "will be" to "are" to be consistent with other
wording which uses the present tense. Same comment applies to
`Signature.getParameters`.
Also, do we no longer need to mention the part "and the underlying signature
implementation supports returning the parameters as {@code
AlgorithmParameters}"?
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v5]
On Mon, 9 May 2022 18:28:04 GMT, Valerie Peng wrote:
>> Anyone can help review this javadoc update? The main change is the wording
>> for the method javadoc of
>> Cipher.getParameters()/CipherSpi.engineGetParameters(). The original wording
>> is somewhat restrictive and request is to broaden this to accommodate more
>> scenarios such as when null can be returned.
>> The rest are minor things like add {@code } to class name and null, and
>> remove redundant ".".
>>
>> Will file CSR after the review is close to being wrapped up.
>> Thanks~
>
> Valerie Peng 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 five additional
> commits since the last revision:
>
> - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8209038
> - update to address review feedback and minor javadoc cleanup
> - Update for getParameters()
> - Update w/ review feedbacks
> - 8209038: Clarify the javadoc of Cipher.getParameters()
Marked as reviewed by mullan (Reviewer).
src/java.base/share/classes/javax/crypto/CipherSpi.java line 294:
> 292: * values used by the underlying cipher implementation. If the
> required
> 293: * parameters were not supplied and can be generated by the cipher,
> the
> 294: * generated parameters will be returned. Otherwise, {@code null} is
Minor wording nit - change "will be" to "are" to be consistent with other
wording which uses the present tense. Same comment applies to
`Cipher.getParameters`.
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: JDK-6725221 Standardize obtaining boolean properties with defaults
On Fri, 6 May 2022 17:56:44 GMT, Alan Bateman wrote:
>> JDK-6725221 Standardize obtaining boolean properties with defaults
>
> src/java.base/share/classes/java/lang/reflect/AccessibleObject.java line 777:
>
>> 775: if (!printStackPropertiesSet && VM.initLevel() >= 1) {
>> 776: printStackWhenAccessFails = GetBooleanAction.
>> 777:
>> privilegedGetProperty("sun.reflect.debugModuleAccessChecks");
>
> Running with `-Dsun.reflect.debugModuleAccessChecks` should set
> printStackWhenAccessFails to true, not false.
You are right. The old way maps the null string to true, and the new way maps
it to false. I did not notice that. At this point, I see no value in making the
"true".equals and "false".equals changes. Too much can break. I'll reverse
these changes.
-
PR: https://git.openjdk.java.net/jdk/pull/8559
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v4]
On Tue, 10 May 2022 17:23:24 GMT, Mat Carter wrote: >> On Windows you can now access the local machine keystores using the strings >> "Windows-MY-LOCALMACHINE" and "Windows-ROOT-LOCALMACHINE"; note the >> application requires admin privileges. >> >> "Windows-MY" and "Windows-ROOT" remain unchanged, however given these >> original keystore strings mapped to the current user, I added >> "Windows-MY-CURRENTUSER" and "Windows-ROOT-CURRENTUSER" so that a developer >> can explicitly specify the current user location. These two new strings >> simply map to the original two strings, i.e. no duplication of code paths etc >> >> No new tests added, keystore functionality and API remains unchanged, the >> local machine keystore types would require the tests to run in admin mode >> >> Tested on windows, passes tier1 and tier2 tests > > Mat Carter has updated the pull request incrementally with one additional > commit since the last revision: > > Minor formatting and spelling issues addressed Great. After the CSR is approved I will approve this PR as well. Then you will need to type the `/integrate` command as a comment here, and I will type `/sponsor`. Then the Skara bot will integrate the change and close the PR. - PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v3]
On Tue, 10 May 2022 13:07:02 GMT, Weijun Wang wrote: >> @wangweij - regarding the two tests for localmachine, these will throw a >> KeyStore exception "Access denied" if the test is not run as admin, is there >> anyway in the test to make that a requirement? If so we could split into >> two tests, one in admin that does all and one in non-admin that does the >> currentuser tests > > @macarte You need to finalize your CSR soon if you want to include this > change into JDK 19. RDP1 is 2022/06/09, and all enhancements require approval > after that. > > BTW, is it possible to detect whether you have admin privilege inside the > test? There is a `NTSystem` class inside Java which can find out user info. > Maybe the `getGroupIDs()` method will tell you if you are an admin? > > We can enhance the test before RC. @wangweij - I added your test and updated to handle Access Denied errors due to admin rights missing, in this case we ignore the test The CSR has been finalized and I feel we are ready to close out this PR - PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v5]
> On Windows you can now access the local machine keystores using the strings > "Windows-MY-LOCALMACHINE" and "Windows-ROOT-LOCALMACHINE"; note the > application requires admin privileges. > > "Windows-MY" and "Windows-ROOT" remain unchanged, however given these > original keystore strings mapped to the current user, I added > "Windows-MY-CURRENTUSER" and "Windows-ROOT-CURRENTUSER" so that a developer > can explicitly specify the current user location. These two new strings > simply map to the original two strings, i.e. no duplication of code paths etc > > No new tests added, keystore functionality and API remains unchanged, the > local machine keystore types would require the tests to run in admin mode > > Tested on windows, passes tier1 and tier2 tests Mat Carter has updated the pull request incrementally with one additional commit since the last revision: Add test from wangweij - Changes: - all: https://git.openjdk.java.net/jdk/pull/8211/files - new: https://git.openjdk.java.net/jdk/pull/8211/files/c56ce0fd..985378fb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8211=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8211=03-04 Stats: 87 lines in 1 file changed: 87 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8211.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8211/head:pull/8211 PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v4]
> On Windows you can now access the local machine keystores using the strings > "Windows-MY-LOCALMACHINE" and "Windows-ROOT-LOCALMACHINE"; note the > application requires admin privileges. > > "Windows-MY" and "Windows-ROOT" remain unchanged, however given these > original keystore strings mapped to the current user, I added > "Windows-MY-CURRENTUSER" and "Windows-ROOT-CURRENTUSER" so that a developer > can explicitly specify the current user location. These two new strings > simply map to the original two strings, i.e. no duplication of code paths etc > > No new tests added, keystore functionality and API remains unchanged, the > local machine keystore types would require the tests to run in admin mode > > Tested on windows, passes tier1 and tier2 tests Mat Carter has updated the pull request incrementally with one additional commit since the last revision: Minor formatting and spelling issues addressed - Changes: - all: https://git.openjdk.java.net/jdk/pull/8211/files - new: https://git.openjdk.java.net/jdk/pull/8211/files/5b3d4115..c56ce0fd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8211=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8211=02-03 Stats: 18 lines in 1 file changed: 0 ins; 0 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/8211.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8211/head:pull/8211 PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: 8286423: Destroy password protection in the example code in KeyStore
On Tue, 10 May 2022 04:13:43 GMT, Xue-Lei Andrew Fan wrote:
> Hi,
>
> May I have this simple example update in the KeyStore specification?
>
> Password protection should be destroyed in the example code in KeyStore
> specification. Otherwise, applications may just copy and past the code, and
> forget to clean up password protection.
>
> It's a trivial update, and may not worthy of a CSR. But please let me know
> if you would like to have a CSR filed.
>
> Thanks,
> Xuelei
src/java.base/share/classes/java/security/KeyStore.java line 165:
> 163: *}
> 164: *} finally {
> 165: *protParam.destroy();
`KeyStore.ProtectionParameter` does not have a `destroy` method. Only
`PasswordProtection` does.
-
PR: https://git.openjdk.java.net/jdk/pull/8623
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v3]
On Thu, 5 May 2022 16:36:04 GMT, Mat Carter wrote:
>> I'd like to contribute a test. Please modify it as much as you like. You can
>> put it inside `test/jdk/sun/security/mscapi/`.
>>
>> /*
>> * Copyright (c) 2022, 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 Software Foundation,
>> * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>> *
>> * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
>> * or visit www.oracle.com if you need additional information or have any
>> * questions.
>> */
>>
>> import jdk.test.lib.Asserts;
>> import jdk.test.lib.SecurityTools;
>>
>> import java.security.KeyStore;
>> import java.util.Collections;
>> import java.util.List;
>> import java.util.Locale;
>>
>> /*
>> * @test
>> * @bug 6782021
>> * @requires os.family == "windows"
>> * @library /test/lib
>> * @summary More keystore types
>> */
>> public class AllTypes {
>> public static void main(String[] args) throws Exception {
>> var nm = test("windows-my");
>> var nr = test("windows-root");
>> var nmu = test("windows-my-currentuser");
>> var nru = test("windows-root-currentuser");
>> var nmm = test("windows-my-localmachine");
>> var nrm = test("windows-root-localmachine");
>> Asserts.assertEQ(nm, nmu);
>> Asserts.assertEQ(nr, nru);
>> }
>>
>> private static List test(String type) throws Exception {
>> var stdType = "Windows-" +
>> type.substring(8).toUpperCase(Locale.ROOT);
>> SecurityTools.keytool("-storetype " + type + " -list")
>> .shouldHaveExitValue(0)
>> .shouldContain("Keystore provider: SunMSCAPI")
>> .shouldContain("Keystore type: " + stdType);
>> KeyStore ks = KeyStore.getInstance(type);
>> ks.load(null, null);
>> var content = Collections.list(ks.aliases());
>> Collections.sort(content);
>> return content;
>> }
>> }
>
> @wangweij - regarding the two tests for localmachine, these will throw a
> KeyStore exception "Access denied" if the test is not run as admin, is there
> anyway in the test to make that a requirement? If so we could split into two
> tests, one in admin that does all and one in non-admin that does the
> currentuser tests
@macarte You need to finalize your CSR soon if you want to include this change
into JDK 19. RDP1 is 2022/06/09, and all enhancements require approval after
that.
BTW, is it possible to detect whether you have admin privilege inside the test?
There is a `NTSystem` class inside Java which can find out user info. Maybe the
`getGroupIDs()` method will tell you if you are an admin?
We can enhance the test before RC.
-
PR: https://git.openjdk.java.net/jdk/pull/8211
