Re: RFR 8242184: CRL generation error with RSASSA-PSS
> On Apr 8, 2020, at 11:46 AM, Valerie Peng wrote:
>
> Hi Max,
>
> Not all of the comments are related to the changes in the webrev, just notice
> some PSS related inconsistency and thought I will ask:
>
>
>
> - For RSASSA-PSS keys, existing code in getDefaultAlgorithmParameterSpec(...)
> (line 1121) decides the default based on key size. I think we should consider
> checking if the key contains PSS parameters. If present, use it as default.
Correct.
>
> - In the checkKeyAndSigAlgMatch(...) method (line 1027), don't we need to add
> checking for RSASSA-PSS signature? RSASSA-PSS sig can take both RSA and
> RSASSA-PSS keys.
>
> - The getEncAlgFromSigAlg(...) method just returns the key algorithm as
> result when the specified signature algorithm does not contain "with". As
> RSASSA-PSS signature can use both RSASSA-PSS and RSA keys, should it still
> return key algorithm?
>
> - The makeSigAlg(...) method does not work for RSASSA-PSS.
These are for JAR file signing. The support for RSASSA-PSS is very poor in this
area. I am thinking about fixing these in "8242068: Signed JAR support for
RSASSA-PSS and EdDSA".
>
>
>
> - The sign() methods of X509CertImpl class do not generate default parameters
> automatically. Have you considered adding a sign() method to X509CRLImpl
> class which has extra signature parameter spec argument and move the
> default parameter call to the caller instead of inside X509CRLImpl? I think
> it's more suitable for the caller to generate the default unless there are
> many callers all need the same functionality.
But this X509CertImpl method is not used anywhere. If only for JDK internal
use, I'd rather sacrifice this flexibility and introduce a method like
public static Signature fromKey(String sigAlg, Key Privatekey, String
provider)
throws NoSuchAlgorithmException, NoSuchProviderException,
InvalidKeyException{
Signature sigEngine = (provider == null || provider.isEmpty())
? Signature.getInstance(sigAlg)
: Signature.getInstance(sigAlg, provider);
AlgorithmParameterSpec params = SignatureUtil
.getDefaultAlgorithmParameterSpec(sigAlg, key);
try {
SignatureUtil.initSignWithParam(sigEngine, key, params,
null);
} catch (InvalidAlgorithmParameterException e) {
throw new AssertionError("getDefaultAlgorithmParameterSpec", e);
}
return s;
}
There are quite some places that need this pattern. If necessary later, we can
add a nullable AlgorithmParameterSpec argument.
Thanks,
Max
>
> Thanks,
>
> Valerie
>
> On 4/6/2020 8:11 PM, Weijun Wang wrote:
>> Please review the fix at
>>
>>
>> http://cr.openjdk.java.net/~weijun/8242184/webrev.00/
>>
>>
>> The major change is inside X509CRLImpl.java to allow params setting and
>> reading.
>>
>> I also take this chance to:
>>
>> 1. Provide a default -sigalg for "keytool -genkeypair -keyalg rsassa-pss".
>>
>> 2. Revert a former change in X509CertImpl.java, which might be a safer call.
>>
>> Thanks,
>> Max
>>
>>
Re: RFR 8241888: Mirror jdk.security.allowNonCaAnchor system property with a security one
Hi Sean, On 4/2/20 6:23 PM, Martin Balao wrote: > Webrev.02: > > * http://cr.openjdk.java.net/~mbalao/webrevs/8241888/8241888.webrev.02 > > On 4/2/20 5:02 PM, Sean Mullan wrote: >> >> In the java.security file might add the sentence "The default value of >> the property is "false"" just to avoid any confusion. >> > > Added. Am I clear to push Webrev.02? CSR has been approved today [1]. Thanks, Martin.- -- [1] - https://bugs.openjdk.java.net/browse/JDK-8241893
Re: RFR 8242184: CRL generation error with RSASSA-PSS
Hi Max, Not all of the comments are related to the changes in the webrev, just notice some PSS related inconsistency and thought I will ask: - For RSASSA-PSS keys, existing code in getDefaultAlgorithmParameterSpec(...) (line 1121) decides the default based on key size. I think we should consider checking if the key contains PSS parameters. If present, use it as default. - In the checkKeyAndSigAlgMatch(...) method (line 1027), don't we need to add checking for RSASSA-PSS signature? RSASSA-PSS sig can take both RSA and RSASSA-PSS keys. - The getEncAlgFromSigAlg(...) method just returns the key algorithm as result when the specified signature algorithm does not contain "with". As RSASSA-PSS signature can use both RSASSA-PSS and RSA keys, should it still return key algorithm? - The makeSigAlg(...) method does not work for RSASSA-PSS. - The sign() methods of X509CertImpl class do not generate default parameters automatically. Have you considered adding a sign() method to X509CRLImpl class which has extra signature parameter spec argument and move the default parameter call to the caller instead of inside X509CRLImpl? I think it's more suitable for the caller to generate the default unless there are many callers all need the same functionality. Thanks, Valerie On 4/6/2020 8:11 PM, Weijun Wang wrote: Please review the fix at http://cr.openjdk.java.net/~weijun/8242184/webrev.00/ The major change is inside X509CRLImpl.java to allow params setting and reading. I also take this chance to: 1. Provide a default -sigalg for "keytool -genkeypair -keyalg rsassa-pss". 2. Revert a former change in X509CertImpl.java, which might be a safer call. Thanks, Max
Re: RFR[15]: 8172404: Tools should warn if weak algorithms are used before restricting them
Everything looks fine, except a very tiny issue:
1332 private String verifyWithWeak(PublicKey key) {
1333 if (DISABLED_CHECK.permits(SIG_PRIMITIVE_SET, key)) {
1334 if (LEGACY_CHECK.permits(SIG_PRIMITIVE_SET, key)) {
1335 int kLen = KeyUtil.getKeySize(key);
1336 if (kLen >= 0) {
1337 return String.format(rb.getString("key.bit"), kLen);
1338 } else {
1339 return rb.getString("unknown.size");
1340 }
1341 } else {
1342 weakPublicKey = key;
1343 legacyAlg |= 8;
1344 return String.format(rb.getString("key.bit.weak"),
KeyUtil.getKeySize(key));
1345 }
1346 } else {
1347disabledAlgFound = true;
1348return String.format(rb.getString("key.bit.disabled"),
KeyUtil.getKeySize(key));
1349 }
1350 }
You can move line 1335 before line 1334 since the size is also used in the else
block on lines 1342-1344.
Thanks,
Max
> On Apr 6, 2020, at 12:51 AM, Hai-May Chao wrote:
>
> Here is the webrev:
>
> http://cr.openjdk.java.net/~weijun/8172404/webrev.00/
>
> Thanks,
> Hai-May
>
>
>> On Apr 4, 2020, at 11:41 PM, Hai-May Chao wrote:
>>
>> Hi,
>>
>> I'd like to request a review for:
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8172404
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8238640
>>
>> It’d be useful to start warning users that certain algorithms and key
>> lengths are becoming weak, so that users could begin transition away from
>> them before they are actually disabled. A new security property named
>> jdk.security.legacyAlgorithms is added to the java.security file to list the
>> legacy algorithms. The keytool and jarsigner tools are enhanced to enforce
>> the new property and to emit the warning messages when legacy algorithms are
>> used.
>>
>> Thanks,
>> Hai-May
>
Re: RFR 8240848: ArrayIndexOutOfBoundsException buf for TextCallbackHandler
I'd removed the change on ConfirmationCallback. Will address it in another fix together with some other similar changes. Webrev updated in-place. Please take a review. Thanks, Max > On Mar 12, 2020, at 9:21 PM, Weijun Wang wrote: > > Please take a review at > > http://cr.openjdk.java.net/~weijun/8240848/webrev.00/ > > The problem is that selection has a different meaning for a specified > optionType (the option value) and an unspecified one (the option index). > > I also take this chance to make ConfirmationCallback more robust. > > Thanks, > Max >
Re: RFR 8242260: Remove customizable ContentSigner from jarsigner
I added my name to the CSR. > On Apr 7, 2020, at 6:41 PM, Weijun Wang wrote: > > Can you please add your name as a CSR reviewer? > > Thanks, > Max > >> On Apr 8, 2020, at 9:25 AM, Xuelei Fan wrote: >> >> +1. >> >> Xuelei >> >>> On 4/7/2020 6:18 PM, Hai-May Chao wrote: >>> Hi Max, >>> Changes look good to me. >>> Is there a man page bug being filed for this? >>> Thanks, >>> Hai-May On Apr 7, 2020, at 1:04 AM, Weijun Wang wrote: I am thinking about removing the `jarsigner -altsigner -altsignerpath` options and underlying classes: JBS : https://bugs.openjdk.java.net/browse/JDK-8242260 Please review everything at: Release note : https://bugs.openjdk.java.net/browse/JDK-8242261 CSR : https://bugs.openjdk.java.net/browse/JDK-8242262 webrev : http://cr.openjdk.java.net/~weijun/8242260/webrev.00/ The CSR "Problem" section has more info on why it's better to remove it now. Thanks, Max >
Re: RFR 8242260: Remove customizable ContentSigner from jarsigner
Can you please add your name as a CSR reviewer? Thanks, Max > On Apr 8, 2020, at 9:25 AM, Xuelei Fan wrote: > > +1. > > Xuelei > > On 4/7/2020 6:18 PM, Hai-May Chao wrote: >> Hi Max, >> Changes look good to me. >> Is there a man page bug being filed for this? >> Thanks, >> Hai-May >>> On Apr 7, 2020, at 1:04 AM, Weijun Wang wrote: >>> >>> I am thinking about removing the `jarsigner -altsigner -altsignerpath` >>> options and underlying classes: >>> >>>JBS : https://bugs.openjdk.java.net/browse/JDK-8242260 >>> >>> Please review everything at: >>> >>> Release note : https://bugs.openjdk.java.net/browse/JDK-8242261 >>>CSR : https://bugs.openjdk.java.net/browse/JDK-8242262 >>> webrev : http://cr.openjdk.java.net/~weijun/8242260/webrev.00/ >>> >>> The CSR "Problem" section has more info on why it's better to remove it now. >>> >>> Thanks, >>> Max >>>
Re: RFR 8242260: Remove customizable ContentSigner from jarsigner
+1. Xuelei On 4/7/2020 6:18 PM, Hai-May Chao wrote: Hi Max, Changes look good to me. Is there a man page bug being filed for this? Thanks, Hai-May On Apr 7, 2020, at 1:04 AM, Weijun Wang wrote: I am thinking about removing the `jarsigner -altsigner -altsignerpath` options and underlying classes: JBS : https://bugs.openjdk.java.net/browse/JDK-8242260 Please review everything at: Release note : https://bugs.openjdk.java.net/browse/JDK-8242261 CSR : https://bugs.openjdk.java.net/browse/JDK-8242262 webrev : http://cr.openjdk.java.net/~weijun/8242260/webrev.00/ The CSR "Problem" section has more info on why it's better to remove it now. Thanks, Max
Re: RFR 8242260: Remove customizable ContentSigner from jarsigner
Hi Max, Changes look good to me. Is there a man page bug being filed for this? Thanks, Hai-May > On Apr 7, 2020, at 1:04 AM, Weijun Wang wrote: > > I am thinking about removing the `jarsigner -altsigner -altsignerpath` > options and underlying classes: > >JBS : https://bugs.openjdk.java.net/browse/JDK-8242260 > > Please review everything at: > > Release note : https://bugs.openjdk.java.net/browse/JDK-8242261 >CSR : https://bugs.openjdk.java.net/browse/JDK-8242262 > webrev : http://cr.openjdk.java.net/~weijun/8242260/webrev.00/ > > The CSR "Problem" section has more info on why it's better to remove it now. > > Thanks, > Max >
Re: RFR 8242184: CRL generation error with RSASSA-PSS
+1. Xuelei On 4/7/2020 5:46 PM, Hai-May Chao wrote: Hi Max, Changes look good to me. Hai-May On Apr 6, 2020, at 8:11 PM, Weijun Wang wrote: Please review the fix at http://cr.openjdk.java.net/~weijun/8242184/webrev.00/ The major change is inside X509CRLImpl.java to allow params setting and reading. I also take this chance to: 1. Provide a default -sigalg for "keytool -genkeypair -keyalg rsassa-pss". 2. Revert a former change in X509CertImpl.java, which might be a safer call. Thanks, Max
Re: RFR 8242184: CRL generation error with RSASSA-PSS
Hi Max, Changes look good to me. Hai-May > On Apr 6, 2020, at 8:11 PM, Weijun Wang wrote: > > Please review the fix at > > http://cr.openjdk.java.net/~weijun/8242184/webrev.00/ > > The major change is inside X509CRLImpl.java to allow params setting and > reading. > > I also take this chance to: > > 1. Provide a default -sigalg for "keytool -genkeypair -keyalg rsassa-pss". > > 2. Revert a former change in X509CertImpl.java, which might be a safer call. > > Thanks, > Max >
Re: RDR: JDK-8242294 JSSE Client does not throw SSLException when an alert occurs during handshaking
Yes, I think you're right. I'll remove that so it just tests for the isInputCloseNotified and rerun the tests just to be sure. Thanks for the feedback, --Jamil On 4/7/20 1:44 PM, Xuelei Fan wrote: The conContext.isBroken condition in line 1124 is duplicated, and could be safely removed, I think. Otherwise, looks good to me. I don't think there's need for another round of RFR. Thanks, Xuelei On 4/7/2020 12:53 PM, Jamil Nimeh wrote: Hello all, This is a fix that brings the JSSE client from JDK 11+ back in line with the JDK 8 behavior of delivering SSLException on subsequent reads after a handshake has failed due to an alert condition. JBS: https://bugs.openjdk.java.net/browse/JDK-8242294 Webrev: https://cr.openjdk.java.net/~jnimeh/reviews/8242294/webrev.01 Thanks, --Jamil
Re: RDR: JDK-8242294 JSSE Client does not throw SSLException when an alert occurs during handshaking
The conContext.isBroken condition in line 1124 is duplicated, and could be safely removed, I think. Otherwise, looks good to me. I don't think there's need for another round of RFR. Thanks, Xuelei On 4/7/2020 12:53 PM, Jamil Nimeh wrote: Hello all, This is a fix that brings the JSSE client from JDK 11+ back in line with the JDK 8 behavior of delivering SSLException on subsequent reads after a handshake has failed due to an alert condition. JBS: https://bugs.openjdk.java.net/browse/JDK-8242294 Webrev: https://cr.openjdk.java.net/~jnimeh/reviews/8242294/webrev.01 Thanks, --Jamil
RDR: JDK-8242294 JSSE Client does not throw SSLException when an alert occurs during handshaking
Hello all, This is a fix that brings the JSSE client from JDK 11+ back in line with the JDK 8 behavior of delivering SSLException on subsequent reads after a handshake has failed due to an alert condition. JBS: https://bugs.openjdk.java.net/browse/JDK-8242294 Webrev: https://cr.openjdk.java.net/~jnimeh/reviews/8242294/webrev.01 Thanks, --Jamil
Re: RFR[jdk] 8237474: Default SSLEngine should create in server role
Looks good to me also Prasad. Trivially, I think you can drop the
'object' word in this implNote:
"for a new {@code SSLEngine} object"
Also, don't forget to create the release note sub-task for this change.
regards,
Sean.
On 02/04/2020 16:56, Prasadrao Koppula wrote:
Thanks for review Xuelei, I will incorporate your suggestions.
Thanks,
Prasad.K
-Original Message-
From: Xuelei Fan
Sent: Thursday, April 2, 2020 9:12 PM
To: security-dev@openjdk.java.net
Subject: Re: RFR[jdk] 8237474: Default SSLEngine should create in server role
Please update copyright year to 2020.
SSLEngine.java
--
@@ -1109,10 +1115,14 @@
+ * @implNote
+ * The JDK SunJSSE provider implementation returns false unless
{@link setUseClientMode}
+ * is used to change the mode to true.
For the link, I may add parameter, and limit the line under 80 characters, and
don't indent the lines.
+ * @implNote
- * The JDK SunJSSE provider implementation returns false unless
{@link setUseClientMode}
- * is used to change the mode to true.
+ * The JDK SunJSSE provider implementation returns false unless
+ * {@link setUseClientMode(boolean)} is used to change the mode
+ * to true.
It's fine to leave the CSR as it is.
Otherwise, looks fine to me.
Xuelei
On 3/30/2020 6:50 AM, Prasadrao Koppula wrote:
Hi,
Added @implnote and updated test changes, here is the new webrev,
please review it.
Webrev: http://cr.openjdk.java.net/~pkoppula/8237474/webrev.01/
issue: https://bugs.openjdk.java.net/browse/JDK-8237474
CSR: https://bugs.openjdk.java.net/browse/JDK-8238593
Thanks,
Prasad.K
*From:* Prasadrao Koppula
*Sent:* Friday, February 7, 2020 5:03 PM
*To:* security-dev@openjdk.java.net
*Subject:* RFR[jdk] 8237474: Default SSLEngine should create in server
role
Hi,
Could you please review this patch. Default server role mode was
flipped in SSLEngine, to client role mode as part of SSL package code
refactoring for TLSv1.3, this patch flips back default client role to
server role in SSLEngine.
webrev: http://cr.openjdk.java.net/~pkoppula/8237474/webrev.00/
issue: https://bugs.openjdk.java.net/browse/JDK-8237474
CSR: https://bugs.openjdk.java.net/browse/JDK-8238593
Thanks,
Prasad.K
RFR 8242260: Remove customizable ContentSigner from jarsigner
I am thinking about removing the `jarsigner -altsigner -altsignerpath` options and underlying classes: JBS : https://bugs.openjdk.java.net/browse/JDK-8242260 Please review everything at: Release note : https://bugs.openjdk.java.net/browse/JDK-8242261 CSR : https://bugs.openjdk.java.net/browse/JDK-8242262 webrev : http://cr.openjdk.java.net/~weijun/8242260/webrev.00/ The CSR "Problem" section has more info on why it's better to remove it now. Thanks, Max
