Re: RFR: 8281567: Remove @throws IOException from X509CRLImpl::getExtension docs
On Thu, 10 Feb 2022 06:18:28 GMT, John Jiang wrote: > In class sun.security.x509.X509CRLImpl, method getExtension(ObjectIdentifier) > doesn't declare that IOException would be thrown, so the @throws IOException > doc should be removed. Looks good and trivial. - Marked as reviewed by jiefu (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7419
Re: RFR: 8281567: Remove @throws IOException from X509CRLImpl::getExtension docs
On Thu, 10 Feb 2022 06:18:28 GMT, John Jiang wrote: > In class sun.security.x509.X509CRLImpl, method getExtension(ObjectIdentifier) > doesn't declare that IOException would be thrown, so the @throws IOException > doc should be removed. Marked as reviewed by xuelei (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7419
RFR: 8281567: Remove @throws IOException from X509CRLImpl::getExtension docs
In class sun.security.x509.X509CRLImpl, method getExtension(ObjectIdentifier) doesn't declare that IOException would be thrown, so the @throws IOException doc should be removed. - Commit messages: - 8281567: Remove @throws IOException from X509CRLImpl::getExtension docs Changes: https://git.openjdk.java.net/jdk/pull/7419/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7419&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281567 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7419/head:pull/7419 PR: https://git.openjdk.java.net/jdk/pull/7419
Re: RFR: 8280494: (D)TLS signature schemes [v16]
> This update is to support signature schemes customization for individual > (D)TLS connection. Please review the CSR as well: > CSR: https://bugs.openjdk.java.net/browse/JDK-8280495 > RFE: https://bugs.openjdk.java.net/browse/JDK-8280494 > Release-note: https://bugs.openjdk.java.net/browse/JDK-8281290 Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: spec re-wording - Changes: - all: https://git.openjdk.java.net/jdk/pull/7252/files - new: https://git.openjdk.java.net/jdk/pull/7252/files/c0018c67..40163778 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7252&range=15 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7252&range=14-15 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/7252.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7252/head:pull/7252 PR: https://git.openjdk.java.net/jdk/pull/7252
Re: RFR: 8280494: (D)TLS signature schemes [v15]
On Wed, 9 Feb 2022 21:38:22 GMT, Sean Mullan wrote: >> Xue-Lei Andrew Fan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Spec update > > src/java.base/share/classes/javax/net/ssl/SSLParameters.java line 749: > >> 747: * @implNote >> 748: * Note that the underlying provider may define the default >> signature >> 749: * schemes for each SSL/TLS/DTLS connection. Applications may also >> use > > I think you can remove the first sentence that starts with "Note ..." as you > already talk about provider-specific defaults before this. Also @implNotes > are usually about the JDK implementation and not any provider. In the second > sentence I would be more specific that this applies to the SunJSSE provider > (see changes in italics), ex: "Applications ... system properties _with the > SunJSSE provider_ to ..." Sounds good to me. Updated the CSR and PR. Thank you for the review! @jnimeh Jamil, do you have any comment about the spec and implementation? Otherwise, I will finalize the CSR. - PR: https://git.openjdk.java.net/jdk/pull/7252
Re: RFR: 8274524: SSLSocket.close() hangs if it is called during the ssl handshake [v4]
On Wed, 3 Nov 2021 17:02:40 GMT, Alexey Bakhtin wrote: >> Please review the patch for JDK-8274524 >> >> The fix just adds locks around InputStream read and skip operations to >> prevent concurrent read from socket. >> sun/security/ssl jtreg tests passed >> api/javax_net/ssl/SSLSocket/setUseClientMode jck test passed > > Alexey Bakhtin has updated the pull request incrementally with one additional > commit since the last revision: > > Fix jcheck issues With the patch, the readLock is placed in handshakeLock, and the handshake may require lock write lock. These nested lock is pretty risk and will run into deadlocks. That's the underlying reason why readHandshakeRecord() is not placed within read lock and write lock. It looks like the problem is cause by close() method. Maybe, improving the close() implementation could be an alternative solution. - PR: https://git.openjdk.java.net/jdk/pull/5760
Re: RFR: 8280494: (D)TLS signature schemes [v15]
On Wed, 9 Feb 2022 18:24:56 GMT, Xue-Lei Andrew Fan wrote: >> This update is to support signature schemes customization for individual >> (D)TLS connection. Please review the CSR as well: >> CSR: https://bugs.openjdk.java.net/browse/JDK-8280495 >> RFE: https://bugs.openjdk.java.net/browse/JDK-8280494 >> Release-note: https://bugs.openjdk.java.net/browse/JDK-8281290 > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > Spec update If you agree with my last couple of comments, I think the API looks good now that you can finalize the CSR. I have not really reviewed the impl code and tests previously and may not have time to do a thorough review although I think Jamil may have done that - if he is ok with the rest of the code, then I think you can integrate once the CSR is approved. Thanks for the patience. src/java.base/share/classes/javax/net/ssl/SSLParameters.java line 749: > 747: * @implNote > 748: * Note that the underlying provider may define the default signature > 749: * schemes for each SSL/TLS/DTLS connection. Applications may also > use I think you can remove the first sentence that starts with "Note ..." as you already talk about provider-specific defaults before this. Also @implNotes are usually about the JDK implementation and not any provider. In the second sentence I would be more specific that this applies to the SunJSSE provider (see changes in italics), ex: "Applications ... system properties _with the SunJSSE provider_ to ..." - PR: https://git.openjdk.java.net/jdk/pull/7252
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v2]
On Tue, 8 Feb 2022 18:06:04 GMT, Lance Andersen wrote:
>>> ze can't be null here.
>>
>> Actually it can be: Consider the following:
>>
>>
>> try (JarFile jf = new JarFile(SIGNED_VALID_ENTRY_NAME_JAR.toFile(),
>> true)) {
>> var ze = new ZipEntry("org/gotham/Batcave.class");
>> var ex= expectThrows(ZipException.class,
>> () -> jf.getInputStream(ze) );
>> // Validate that we receive the expected message from
>> // JarFile::verifiableEntry when ZipEntry::getName returns null
>> assertTrue( ex != null && ex.getMessage().equals("Error:
>> ZipEntry should not be null!"));
>> }
>>
>>
>> The above code does generate the error.
>
>> Nit, add space after "if"
>
> will fix
So a bit more on this. If the ZipEntry passed to `ZipFile::getInputStream`
does not represent an entry within the current Zip/Jar,
`ZipFile::getInputStream` will return a null for the InputStream:
@Test
public static void ZipFileZipEntryNullGetInputStreamTest() throws Exception
{
try (ZipFile zf = new ZipFile(VALID_ENTRY_NAME_JAR.toFile())) {
var ze = new ZipEntry("org/gotham/Batcave.class");
var is = zf.getInputStream(ze);
// As the ZipEntry cannot be found, the returned InpuStream is null
assertNull(is);
}
}
JarFile::getInputStream will also return null when the jar is not signed or
we are not verifying the jar:
@Test
public static void JarFileZipEntryNullGetInputStreamTest() throws Exception
{
try (JarFile jf = new JarFile(VALID_ENTRY_NAME_JAR.toFile())) {
var ze = new ZipEntry("org/gotham/Batcave.class");
var is = jf.getInputStream(ze);
// As the ZipEntry cannot be found, the returned InpuStream is null
assertNull(is);
}
try (JarFile jf = new JarFile(SIGNED_INVALID_ENTRY_NAME_JAR.toFile(),
false)) {
var ze = new ZipEntry("org/gotham/Batcave.class");
var is = jf.getInputStream(ze);
// As the ZipEntry cannot be found, the returned InpuStream is null
assertNull(is);
}
}
This behavior dates back to at least JDK 1.3
So I think we should return null instead of throwing an Exception when the
resulting ZipEntry is null that is returned from the call
to`JarFile::getJarEntry` (which calls `ZipFile::getEntry`) for consistency with
ZipFile and when the Jar is not signed or not verified.
I also noticed that `ZipFile::getInputStream` and `JarFile::getInputStream` do
not mention that a null will be returned if the specified ZipEntry is not found
within the Jar/Zip. I guess I could open a CSR as part of this fix to clarify
this 20+ year behavior.
-
PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280494: (D)TLS signature schemes [v13]
On Wed, 9 Feb 2022 14:33:11 GMT, Sean Mullan wrote:
>> Basically, the suggestion captures the implementation behaviors correctly.
>> To make it more accuracy, if we want to use it, we may need to consider more
>> cases:
>> 1. _explicitly set by application_, with null, empty or 1+ schemes.
>> 2. _specified by system property_, with unset, empty or 1+ schemes.
>>
>> I think the description could be in the following logic:
>> A. the system properties are used to customize the provider default schemes.
>> B. If the API set it to null, use the the provider default schemes
>> (including system properties customized default schemes).
>> C. If the API set it to empty, don't use any schemes.
>> D. if the API set it to 1+ scheme, use the API set schemes.
>>
>> With logic A, we don't have to describe both _system properties_ and the
>> _provider default schemes_ each time when we talk about the interaction
>> behaviors of them. And then we can simplify the 3 interactive factors into 2
>> factors.
>>
>> Here is the general 3 interactive factors:
>> 1. the API.
>> 2. the System properties.
>> 3. the provider default.
>>
>> Here is set 1 of the 2 interactive factors:
>> 1. The System properties;
>> 2. the provider default.
>>
>> Here is set 2 of the 2 interactive factors:
>> 1. the API
>> 2. the provider default.
>>
>> Then, we could describe set 1 and set 2 individually. I think it is easier
>> to understand.
>>
>> Then, we could have the sections accordingly.
>> For A, we have (for set 1 of the 2 interactive factors):
>>
>> * @implNote
>> * Note that the underlying provider may define the default signature
>> * schemes for each SSL/TLS/DTLS connection. Applications may also use
>> * the {@systemProperty jdk.tls.client.SignatureSchemes} and/or
>> * {@systemProperty jdk.tls.server.SignatureSchemes} system properties to
>> * customize the provider-specific default signature schemes.
>> ```
>>
>> For B, we have (for set 2 of the 2 interactive factors):
>>
>> * If the returned array is {@code null}, then the underlying
>> * provider-specific default signature schemes will be used over the
>> * SSL/TLS/DTLS connections.
>>
>>
>> For C, we have (for set 2 of the 2 interactive factors):
>>
>> * If the returned array is empty (zero-length), then the signature
>> scheme
>> * negotiation mechanism is turned off for SSL/TLS/DTLS protocols, and
>> * the connections may not be able to be established if the negotiation
>> * mechanism is required by a certain SSL/TLS/DTLS protocol.
>>
>>
>> For D, we have (for set 2 of the 2 interactive factors):
>>
>> *
>> * If the returned array is not {@code null} or empty (zero-length),
>> * then the signature schemes in the returned array will be used over
>> * the SSL/TLS/DTLS connections.
>>
>>
>> That's the current description we have. That's, in order to explain
>> behavior of the null, empty and 1+ schemes setting, I cut the sentence into
>> 4 sections above.
>>
>> I understand it could be confusing when multiple facts are involved. It may
>> be clear if re-org the description. What do you think of the following
>> descriptions?
>>
>> In the get method:
>>
>> *
>> * Note that the underlying provider may define the default signature
>> schemes for
>> * each SSL/TLS/DTLS connection. Applications may also use the
>> {@systemProperty
>> * jdk.tls.client.SignatureSchemes} and/or {@systemProperty
>> * jdk.tls.server.SignatureSchemes} system properties to customize the
>> * provider-specific default signature schemes.
>> *
>> * The set of signature schemes that will be used over the SSL/TLS/DTLS
>> * connections is determined by the returned array of this method and
>> the
>> * underlying provider-specific default signature schemes.
>> *
>> * If the returned array is {@code null}, then the underlying
>> * provider-specific default signature schemes will be used over the
>> * SSL/TLS/DTLS connections.
>> *
>> * If the returned array is empty (zero-length), then the signature
>> scheme
>> * negotiation mechanism is turned off for SSL/TLS/DTLS protocols, and
>> * the connections may not be able to be established if the negotiation
>> * mechanism is required by a certain SSL/TLS/DTLS protocol.
>> *
>> * If the returned array is not {@code null} or empty (zero-length),
>> * then the signature schemes in the returned array will be used over
>> * the SSL/TLS/DTLS connections.
>>
>>
>> and similarly, in the set method:
>>
>> *
>> * Note that the underlying provider may define the default signature
>> * schemes for each SSL/TLS/DTLS connection. Applications may also use
>> * the {@systemProperty jdk.tls.client.SignatureSchemes} and/or
>> * {@systemProperty jdk.tls.server.SignatureSchemes} system properties to
>> * custom
Re: RFR: 8280494: (D)TLS signature schemes [v15]
> This update is to support signature schemes customization for individual > (D)TLS connection. Please review the CSR as well: > CSR: https://bugs.openjdk.java.net/browse/JDK-8280495 > RFE: https://bugs.openjdk.java.net/browse/JDK-8280494 > Release-note: https://bugs.openjdk.java.net/browse/JDK-8281290 Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: Spec update - Changes: - all: https://git.openjdk.java.net/jdk/pull/7252/files - new: https://git.openjdk.java.net/jdk/pull/7252/files/8302c592..c0018c67 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7252&range=14 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7252&range=13-14 Stats: 31 lines in 1 file changed: 7 ins; 16 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/7252.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7252/head:pull/7252 PR: https://git.openjdk.java.net/jdk/pull/7252
Integrated: 8265765: DomainKeyStore may stop enumerating aliases if a constituting KeyStore is empty
On Tue, 8 Feb 2022 17:13:53 GMT, Hai-May Chao wrote: > This is to fix `DomainKeyStore::engineAliases` to take into account that > there may be empty keystore(s) within the collection of keystores of a domain > keystore. This pull request has now been integrated. Changeset: 178b962e Author:Hai-May Chao URL: https://git.openjdk.java.net/jdk/commit/178b962e01cc6c150442bf41dc6bd199caff0042 Stats: 136 lines in 2 files changed: 129 ins; 2 del; 5 mod 8265765: DomainKeyStore may stop enumerating aliases if a constituting KeyStore is empty Reviewed-by: weijun - PR: https://git.openjdk.java.net/jdk/pull/7386
Re: RFR: 8280494: (D)TLS signature schemes [v13]
On Tue, 8 Feb 2022 23:36:05 GMT, Xue-Lei Andrew Fan wrote:
>> Ok, I get it now, the API wins if both are set. But I could not discern that
>> from the current text. I think it is ok to be more clear about this. I
>> suggest adding something like the following:
>>
>> "The set of signature schemes that will be used is determined in this order
>> of preference: 1. explicitly set by application; 2. specified by system
>> property; 3. specified by provider defaults. For example, setting the
>> signature schemes in your application overrides settings specified in
>> jdk.tls.client.SignatureSchemes or jdk.tls.server.SignatureSchemes, as well
>> as JDK provider defaults."
>>
>> Does that accurately capture the implementation behavior?
>
> Basically, the suggestion captures the implementation behaviors correctly.
> To make it more accuracy, if we want to use it, we may need to consider more
> cases:
> 1. _explicitly set by application_, with null, empty or 1+ schemes.
> 2. _specified by system property_, with unset, empty or 1+ schemes.
>
> I think the description could be in the following logic:
> A. the system properties are used to customize the provider default schemes.
> B. If the API set it to null, use the the provider default schemes (including
> system properties customized default schemes).
> C. If the API set it to empty, don't use any schemes.
> D. if the API set it to 1+ scheme, use the API set schemes.
>
> With logic A, we don't have to describe both _system properties_ and the
> _provider default schemes_ each time when we talk about the interaction
> behaviors of them. And then we can simplify the 3 interactive factors into 2
> factors.
>
> Here is the general 3 interactive factors:
> 1. the API.
> 2. the System properties.
> 3. the provider default.
>
> Here is set 1 of the 2 interactive factors:
> 1. The System properties;
> 2. the provider default.
>
> Here is set 2 of the 2 interactive factors:
> 1. the API
> 2. the provider default.
>
> Then, we could describe set 1 and set 2 individually. I think it is easier
> to understand.
>
> Then, we could have the sections accordingly.
> For A, we have (for set 1 of the 2 interactive factors):
>
> * @implNote
> * Note that the underlying provider may define the default signature
> * schemes for each SSL/TLS/DTLS connection. Applications may also use
> * the {@systemProperty jdk.tls.client.SignatureSchemes} and/or
> * {@systemProperty jdk.tls.server.SignatureSchemes} system properties to
> * customize the provider-specific default signature schemes.
> ```
>
> For B, we have (for set 2 of the 2 interactive factors):
>
> * If the returned array is {@code null}, then the underlying
> * provider-specific default signature schemes will be used over the
> * SSL/TLS/DTLS connections.
>
>
> For C, we have (for set 2 of the 2 interactive factors):
>
> * If the returned array is empty (zero-length), then the signature scheme
> * negotiation mechanism is turned off for SSL/TLS/DTLS protocols, and
> * the connections may not be able to be established if the negotiation
> * mechanism is required by a certain SSL/TLS/DTLS protocol.
>
>
> For D, we have (for set 2 of the 2 interactive factors):
>
> *
> * If the returned array is not {@code null} or empty (zero-length),
> * then the signature schemes in the returned array will be used over
> * the SSL/TLS/DTLS connections.
>
>
> That's the current description we have. That's, in order to explain behavior
> of the null, empty and 1+ schemes setting, I cut the sentence into 4 sections
> above.
>
> I understand it could be confusing when multiple facts are involved. It may
> be clear if re-org the description. What do you think of the following
> descriptions?
>
> In the get method:
>
> *
> * Note that the underlying provider may define the default signature
> schemes for
> * each SSL/TLS/DTLS connection. Applications may also use the
> {@systemProperty
> * jdk.tls.client.SignatureSchemes} and/or {@systemProperty
> * jdk.tls.server.SignatureSchemes} system properties to customize the
> * provider-specific default signature schemes.
> *
> * The set of signature schemes that will be used over the SSL/TLS/DTLS
> * connections is determined by the returned array of this method and the
> * underlying provider-specific default signature schemes.
> *
> * If the returned array is {@code null}, then the underlying
> * provider-specific default signature schemes will be used over the
> * SSL/TLS/DTLS connections.
> *
> * If the returned array is empty (zero-length), then the signature scheme
> * negotiation mechanism is turned off for SSL/TLS/DTLS protocols, and
> * the connections may not be able to be established if the negotiation
> * mechanism is required by a certain SSL/TLS/DTLS protocol.
>
Re: RFR: 8265765: DomainKeyStore may stop enumerating aliases if a constituting KeyStore is empty [v2]
On Wed, 9 Feb 2022 06:34:40 GMT, Hai-May Chao wrote: >> This is to fix `DomainKeyStore::engineAliases` to take into account that >> there may be empty keystore(s) within the collection of keystores of a >> domain keystore. > > Hai-May Chao has updated the pull request incrementally with one additional > commit since the last revision: > > Testcase updated Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7386
