Re: RFR 8250582: Revert Principal Name type to NT-UNKNOWN when requesting TGS Kerberos tickets
Looks fine to me. Thanks for the quick fix. —Max > 在 2020年7月25日，12:36，Martin Balao 写道： > > Hello Max, > > I'd like to propose a patch for "8250582: Revert Principal Name type to > NT-UNKNOWN when requesting TGS Kerberos tickets" [1]. > > Webrev.00: > > * http://cr.openjdk.java.net/~mbalao/webrevs/8250582/8250582.webrev.00/ > > This change is trivial and we are reverting to a previous (and known > state). I've run tests under sun/security/krb5 and did not find any > regression (125 passed). I've also run my internal Kerberos-referrals > test -based on a real Active Directory environment- and worked fine. > > Thanks, > Martin.- > > -- > [1] - https://bugs.openjdk.java.net/browse/JDK-8250582 >
RFR 8250582: Revert Principal Name type to NT-UNKNOWN when requesting TGS Kerberos tickets
Hello Max, I'd like to propose a patch for "8250582: Revert Principal Name type to NT-UNKNOWN when requesting TGS Kerberos tickets" [1]. Webrev.00: * http://cr.openjdk.java.net/~mbalao/webrevs/8250582/8250582.webrev.00/ This change is trivial and we are reverting to a previous (and known state). I've run tests under sun/security/krb5 and did not find any regression (125 passed). I've also run my internal Kerberos-referrals test -based on a real Active Directory environment- and worked fine. Thanks, Martin.- -- [1] - https://bugs.openjdk.java.net/browse/JDK-8250582
Re: RFR 8247960: jarsigner says "signer errors" for some normal warnings when -strict is set
> On Jul 24, 2020, at 6:54 AM, Weijun Wang wrote:
>
> I'd suggest changing
>
> +result = isSigning ? rb.getString("jar.signed.") :
> rb.getString("jar.verified.");
> +if ((strict) && (!errors.isEmpty())) {
> +result = isSigning ?
> rb.getString("jar.signed.with.signer.errors.")
> + : rb.getString("jar.verified.with.signer.errors.");
> +}
>
> to
>
> +if ((strict) && (!errors.isEmpty())) {
> +result = isSigning ?
> rb.getString("jar.signed.with.signer.errors.")
> + : rb.getString("jar.verified.with.signer.errors.");
> +} else {
> +result = isSigning ? rb.getString("jar.signed.") :
> rb.getString("jar.verified.");
> +}
>
Webrev updated as suggested.
> Everything else looks fine.
>
> Also, I remember you meant to fix 2 bugs with a single changeset. What should
> the full commit message be?
Fix in a single changeset, so use this bug as the commit message please.
Thanks,
Hai-May
>
> Thanks,
> Max
>
>> On Jul 24, 2020, at 4:25 PM, Hai-May Chao wrote:
>>
>>
>>
>>> On Jul 15, 2020, at 12:16 AM, Weijun Wang wrote:
>>>
>>> The following lines are useless now:
>>>
>>
>> Ok, this is a separate problem from the original bug to be addressed.
>> Cleanup/refactoring of the checking on flags is also included in this
>> changeset.
>>
>>> 1053 if (badKeyUsage || badExtendedKeyUsage || badNetscapeCertType
>>> ||
>>> 1054 notYetValidCert || chainNotValidated || hasExpiredCert
>>> ||
>>> 1055 hasUnsignedEntry || signerSelfSigned || (legacyAlg !=
>>> 0) ||
>>> 1056 (disabledAlg != 0) || aliasNotInStore ||
>>> notSignedByAlias ||
>>> 1057 tsaChainNotValidated ||
>>> 1058 (hasExpiredTsaCert && !signerNotExpired)) {
>>>
>>> 1198 }
>>>
>>> 1205 if (hasExpiringCert ||
>>> 1206 (hasExpiringTsaCert && expireDate != null) ||
>>> 1207 (noTimestamp && expireDate != null) ||
>>> 1208 (hasExpiredTsaCert && signerNotExpired)) {
>>>
>>> 1245 }
>>>
>>> I would even suggest you remove the "result" variable and move the
>>> "System.out.println(result)" line into branches of the if-else block on
>>> lines 1254-1272.
>>
>> Current change has the checking for sign and verify. Keep it as-is that you
>> agreed.
>>
>> https://cr.openjdk.java.net/~hchao/8247960/webrev.01/
>>
>> Thanks,
>> Hai-May
>>
>>
>>>
>>> No other comments.
>>>
>>> Thanks,
>>> Max
>>>
>>>
>>>
On Jul 15, 2020, at 4:09 AM, Hai-May Chao wrote:
Hi,
I’d like to request a review for:
JBS: https://bugs.openjdk.java.net/browse/JDK-8247960
Webrev: https://cr.openjdk.java.net/~hchao/8247960/webrev.00/
Jarsigner is changed to emit “with signer errors” only when there are
errors detected during sign and verify with -strict specified.
Thanks,
Hai-May
>
Re: JDK 16 RFR of JDK-8250246: Address reliance on default constructors in security libs
On 7/24/2020 10:32 AM, Sean Mullan wrote:
On 7/24/20 1:18 PM, Joe Darcy wrote:
-
src/jdk.security.jgss/share/classes/com/sun/security/jgss/GSSUtil.java
37 /**
38 * Do not call.
39 */
40 @Deprecated(since="16", forRemoval=true)
41 public GSSUtil() {}
Is your concern that there may be some code out there that might be
erroneously using the default constructor so you want to give some
warning first before making this a private ctor after it is removed?
(I think I agree, but I want to make sure I understand your thinking).
That is my concern here, yes. We've had a number of other cases in
the JDK where the default constructor appeared in an static-only
class as an attractive nuisance. Before static imports, subclassing
such a class was a way to get its names in the namespace of the
subclass, but that is an anti-pattern that should not be encouraged.
Ok, sounds good.
src/jdk.security.auth/share/classes/com/sun/security/auth/module/NTLoginModule.java
87 * Creates a {@code NTLoginModule}.
s/a/an/
Hmm. If the this is pronounced as "N T Login Module", "N" would be a
consonant sound so should be prefixed by the indefinite article "a", no?
This might be a question for one of the grammar experts amongst us.
But isn't "N" a vowel sound: "en"?
I suppose looked at that way it is :-)
I'll make the adjustments, copyrights, etc. and push this later today.
Thanks,
-Joe
Re: JDK 16 RFR of JDK-8250246: Address reliance on default constructors in security libs
On 7/24/20 1:18 PM, Joe Darcy wrote:
- src/jdk.security.jgss/share/classes/com/sun/security/jgss/GSSUtil.java
37 /**
38 * Do not call.
39 */
40 @Deprecated(since="16", forRemoval=true)
41 public GSSUtil() {}
Is your concern that there may be some code out there that might be
erroneously using the default constructor so you want to give some
warning first before making this a private ctor after it is removed?
(I think I agree, but I want to make sure I understand your thinking).
That is my concern here, yes. We've had a number of other cases in the
JDK where the default constructor appeared in an static-only class as an
attractive nuisance. Before static imports, subclassing such a class was
a way to get its names in the namespace of the subclass, but that is an
anti-pattern that should not be encouraged.
Ok, sounds good.
src/jdk.security.auth/share/classes/com/sun/security/auth/module/NTLoginModule.java
87 * Creates a {@code NTLoginModule}.
s/a/an/
Hmm. If the this is pronounced as "N T Login Module", "N" would be a
consonant sound so should be prefixed by the indefinite article "a", no?
This might be a question for one of the grammar experts amongst us. But
isn't "N" a vowel sound: "en"?
--Sean
src/jdk.security.auth/share/classes/com/sun/security/auth/module/LdapLoginModule.java
359 * Creates a {@code LdapLoginModule}.
s/a/an/
Okay.
Thanks,
-Joe
Re: JDK 16 RFR of JDK-8250246: Address reliance on default constructors in security libs
Hi Sean,
On 7/24/2020 4:52 AM, Sean Mullan wrote:
Hi Joe,
On 7/24/20 1:17 AM, Joe Darcy wrote:
Hello,
Please review a set of changes to add explicit constructors to
replace default (implicit) constructors in various classes in
security libs across several modules:
JDK-8250246: Address reliance on default constructors in
security libs
http://cr.openjdk.java.net/~darcy/8250246.0/
CSR: https://bugs.openjdk.java.net/browse/JDK-8250248
(This is a companion to similar changes being made across other
libraries in preparation for creating a new javac lint warning.)
One of the classes seems to only accidentally have a constructor, so
I added that one as terminally deprecated.
You have not updated the copyright dates; not sure if that is
necessary for this type of change.
Right; I run a script that handles the copyright updates before a push
-- less clutter in a review like this where the per-file changes is
small and reduces the possibility of spurious merge conflicts for the
copyright line during the review period.
- src/jdk.security.jgss/share/classes/com/sun/security/jgss/GSSUtil.java
37 /**
38 * Do not call.
39 */
40 @Deprecated(since="16", forRemoval=true)
41 public GSSUtil() {}
Is your concern that there may be some code out there that might be
erroneously using the default constructor so you want to give some
warning first before making this a private ctor after it is removed?
(I think I agree, but I want to make sure I understand your thinking).
That is my concern here, yes. We've had a number of other cases in the
JDK where the default constructor appeared in an static-only class as an
attractive nuisance. Before static imports, subclassing such a class was
a way to get its names in the namespace of the subclass, but that is an
anti-pattern that should not be encouraged.
-
src/jdk.security.auth/share/classes/com/sun/security/auth/module/NTLoginModule.java
87 * Creates a {@code NTLoginModule}.
s/a/an/
Hmm. If the this is pronounced as "N T Login Module", "N" would be a
consonant sound so should be prefixed by the indefinite article "a", no?
-
src/jdk.security.auth/share/classes/com/sun/security/auth/module/LdapLoginModule.java
359 * Creates a {@code LdapLoginModule}.
s/a/an/
Okay.
Thanks,
-Joe
Re: RFR 8247960: jarsigner says "signer errors" for some normal warnings when -strict is set
I'd suggest changing
+result = isSigning ? rb.getString("jar.signed.") :
rb.getString("jar.verified.");
+if ((strict) && (!errors.isEmpty())) {
+result = isSigning ? rb.getString("jar.signed.with.signer.errors.")
+ : rb.getString("jar.verified.with.signer.errors.");
+}
to
+if ((strict) && (!errors.isEmpty())) {
+result = isSigning ? rb.getString("jar.signed.with.signer.errors.")
+ : rb.getString("jar.verified.with.signer.errors.");
+} else {
+result = isSigning ? rb.getString("jar.signed.") :
rb.getString("jar.verified.");
+}
Everything else looks fine.
Also, I remember you meant to fix 2 bugs with a single changeset. What should
the full commit message be?
Thanks,
Max
> On Jul 24, 2020, at 4:25 PM, Hai-May Chao wrote:
>
>
>
>> On Jul 15, 2020, at 12:16 AM, Weijun Wang wrote:
>>
>> The following lines are useless now:
>>
>
> Ok, this is a separate problem from the original bug to be addressed.
> Cleanup/refactoring of the checking on flags is also included in this
> changeset.
>
>> 1053 if (badKeyUsage || badExtendedKeyUsage || badNetscapeCertType ||
>> 1054 notYetValidCert || chainNotValidated || hasExpiredCert
>> ||
>> 1055 hasUnsignedEntry || signerSelfSigned || (legacyAlg !=
>> 0) ||
>> 1056 (disabledAlg != 0) || aliasNotInStore ||
>> notSignedByAlias ||
>> 1057 tsaChainNotValidated ||
>> 1058 (hasExpiredTsaCert && !signerNotExpired)) {
>>
>> 1198 }
>>
>> 1205 if (hasExpiringCert ||
>> 1206 (hasExpiringTsaCert && expireDate != null) ||
>> 1207 (noTimestamp && expireDate != null) ||
>> 1208 (hasExpiredTsaCert && signerNotExpired)) {
>>
>> 1245 }
>>
>> I would even suggest you remove the "result" variable and move the
>> "System.out.println(result)" line into branches of the if-else block on
>> lines 1254-1272.
>
> Current change has the checking for sign and verify. Keep it as-is that you
> agreed.
>
> https://cr.openjdk.java.net/~hchao/8247960/webrev.01/
>
> Thanks,
> Hai-May
>
>
>>
>> No other comments.
>>
>> Thanks,
>> Max
>>
>>
>>
>>> On Jul 15, 2020, at 4:09 AM, Hai-May Chao wrote:
>>>
>>> Hi,
>>>
>>> I’d like to request a review for:
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8247960
>>> Webrev: https://cr.openjdk.java.net/~hchao/8247960/webrev.00/
>>>
>>> Jarsigner is changed to emit “with signer errors” only when there are
>>> errors detected during sign and verify with -strict specified.
>>>
>>> Thanks,
>>> Hai-May
Re: JDK 16 RFR of JDK-8250246: Address reliance on default constructors in security libs
Hi Joe,
On 7/24/20 1:17 AM, Joe Darcy wrote:
Hello,
Please review a set of changes to add explicit constructors to replace
default (implicit) constructors in various classes in security libs
across several modules:
JDK-8250246: Address reliance on default constructors in security libs
http://cr.openjdk.java.net/~darcy/8250246.0/
CSR: https://bugs.openjdk.java.net/browse/JDK-8250248
(This is a companion to similar changes being made across other
libraries in preparation for creating a new javac lint warning.)
One of the classes seems to only accidentally have a constructor, so I
added that one as terminally deprecated.
You have not updated the copyright dates; not sure if that is necessary
for this type of change.
- src/jdk.security.jgss/share/classes/com/sun/security/jgss/GSSUtil.java
37 /**
38 * Do not call.
39 */
40 @Deprecated(since="16", forRemoval=true)
41 public GSSUtil() {}
Is your concern that there may be some code out there that might be
erroneously using the default constructor so you want to give some
warning first before making this a private ctor after it is removed? (I
think I agree, but I want to make sure I understand your thinking).
-
src/jdk.security.auth/share/classes/com/sun/security/auth/module/NTLoginModule.java
87 * Creates a {@code NTLoginModule}.
s/a/an/
-
src/jdk.security.auth/share/classes/com/sun/security/auth/module/LdapLoginModule.java
359 * Creates a {@code LdapLoginModule}.
s/a/an/
--Sean
Re: RFR 8247960: jarsigner says "signer errors" for some normal warnings when -strict is set
> On Jul 15, 2020, at 12:16 AM, Weijun Wang wrote:
>
> The following lines are useless now:
>
Ok, this is a separate problem from the original bug to be addressed.
Cleanup/refactoring of the checking on flags is also included in this changeset.
> 1053 if (badKeyUsage || badExtendedKeyUsage || badNetscapeCertType ||
> 1054 notYetValidCert || chainNotValidated || hasExpiredCert ||
> 1055 hasUnsignedEntry || signerSelfSigned || (legacyAlg != 0)
> ||
> 1056 (disabledAlg != 0) || aliasNotInStore ||
> notSignedByAlias ||
> 1057 tsaChainNotValidated ||
> 1058 (hasExpiredTsaCert && !signerNotExpired)) {
>
> 1198 }
>
> 1205 if (hasExpiringCert ||
> 1206 (hasExpiringTsaCert && expireDate != null) ||
> 1207 (noTimestamp && expireDate != null) ||
> 1208 (hasExpiredTsaCert && signerNotExpired)) {
>
> 1245 }
>
> I would even suggest you remove the "result" variable and move the
> "System.out.println(result)" line into branches of the if-else block on lines
> 1254-1272.
Current change has the checking for sign and verify. Keep it as-is that you
agreed.
https://cr.openjdk.java.net/~hchao/8247960/webrev.01/
Thanks,
Hai-May
>
> No other comments.
>
> Thanks,
> Max
>
>
>
>> On Jul 15, 2020, at 4:09 AM, Hai-May Chao wrote:
>>
>> Hi,
>>
>> I’d like to request a review for:
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8247960
>> Webrev: https://cr.openjdk.java.net/~hchao/8247960/webrev.00/
>>
>> Jarsigner is changed to emit “with signer errors” only when there are errors
>> detected during sign and verify with -strict specified.
>>
>> Thanks,
>> Hai-May
>>
>
