Re: RFR 8242260: Remove customizable ContentSigner from jarsigner

[Wang Weijun]

So the classes will be useless but at least old program still compiles. I'll 
modify the CSR and see how Joe thinks of this.

Thanks,
Max

> 在 2020年4月9日，22:58，Sean Mullan  写道：
> 
> On 4/9/20 10:52 AM, Weijun Wang wrote:
>> All info for signing are passed into a ContentSigner through a 
>> ContentSignerParameters object. In order to pass more info, I’ll need to 
>> create new interface methods for it.
> 
> But you can just use your solution in JarSigner in the webrev below where you 
> are calling PKCS7.generateSignedData instead of ContentSigner. Just because 
> the ContentSigner APIs are still there doesn't mean you have to use it in 
> jarsigner (unless I am missing something).
> 
> --Sean
> 
>> —Max
>>>> 在 2020年4月9日，21:27，Sean Mullan  写道：
>>> 
>>> On 4/9/20 3:13 AM, Wang Weijun wrote:
>>>> Oh, I'll then need to add new fields to it to support RSASSA-PSS and 
>>>> EdDSA. Sigh.
>>> 
>>> Why would you need to do that if they are deprecated?
>>> 
>>> --Sean
>>> 
>>>> --Max
>>>>>> 在 2020年4月9日，01:58，Sean Mullan  写道：
>>>>> 
>>>>> We never actually deprecated the com.sun.jarsigner package with a 
>>>>> forRemoval=true flag, so while it may be very low-risk to remove these 
>>>>> APIs, I feel that we should not remove it w/o prior notice.
>>>>> 
>>>>> I would suggest adding the forRemoval=true for this package/APIs instead, 
>>>>> and plan on removing it in JDK 16 or 17.
>>>>> 
>>>>> I'm ok with removing the jarsigner options because the man page already 
>>>>> warned that they may be removed.
>>>>> 
>>>>> --Sean
>>>>> 
>>>>> 
>>>>>> On 4/7/20 4: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

[Wang Weijun]

Valerie in another reply suggested that the default parameters of the default 
sigAlg depends on either the size of the key (if RSA) of the params of the key 
(if RSASSA-PSS). I'll address all of these in another bug.

Thanks,
Max

> 在 2020年4月9日，03:47，Sean Mullan  写道：
> 
> On 4/6/20 11: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".
> 
> I think you should file a CSR for that, since it is a new default, and the 
> default varies based on the size of the key. You should also update the 
> keytool man page section on defaults.
> 
> --Sean
> 
>> 2. Revert a former change in X509CertImpl.java, which might be a safer call.
>> Thanks,
>> Max

Re: RFR 8242260: Remove customizable ContentSigner from jarsigner

[Wang Weijun]

Oh, I'll then need to add new fields to it to support RSASSA-PSS and EdDSA. 
Sigh.

--Max

> 在 2020年4月9日，01:58，Sean Mullan  写道：
> 
> We never actually deprecated the com.sun.jarsigner package with a 
> forRemoval=true flag, so while it may be very low-risk to remove these APIs, 
> I feel that we should not remove it w/o prior notice.
> 
> I would suggest adding the forRemoval=true for this package/APIs instead, and 
> plan on removing it in JDK 16 or 17.
> 
> I'm ok with removing the jarsigner options because the man page already 
> warned that they may be removed.
> 
> --Sean
> 
> 
>> On 4/7/20 4: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: JGSS Enhancements (contribution by Two Sigma Open Source)

[Wang Weijun]

Yes, I am on vacation now. 

When I’m back, I’ll post your changes to cr.openjdk.java.net first.

Thanks
Max

> 在 2018年10月4日，06:13，Valerie Peng  写道：
> 
> Hi Nico,
> 
> Thanks for submitting the patches, Max is off on Chinese holidays til 7th and 
> I am going to be on vacation til 19th.
> 
> I suppose no bugs have been filed for these issues? Max and I will take a 
> look upon our return.
> 
> Regards,
> 
> Valerie
> 
> 
>> On 10/3/2018 2:30 PM, Nico Williams wrote:
>>> On Wed, Oct 03, 2018 at 08:49:28PM +, Nico Williams wrote:
>>> I'll attempt to attach all 25 commits now.  I hope the list will accept
>>> them.  I will post them (and webrev) to cr.openjdk.java.net.
>> And... I forgot to attach them.  Let's try again.
> 

Re: RFR 8210786 : Typo s/overriden/overridden/ in several places

[Wang Weijun]

This makes sense. No other comment.

Thanks
Max

> 在 2018年9月16日，05:10，Ivan Gerasimov  写道：
> 
> Hi Max!
> 
> 
>> On 9/15/18 6:28 AM, Weijun Wang wrote:
>> In the bug description you listed some in jdk/internal/org/objectweb/asm, 
>> but they are not included in the fix. Is it because those are not doc only 
>> but inside source code?
> For the ASM changes, I thought it may be better to fix them in the upstream 
> project.
> I don't know how/if the ASM sources in OpenJDK are updated.  If they are not, 
> then it may make sense to fix these typos too.
> 
>>  There are similar typos in other modules as well. Any reason not to include 
>> them.
> Yep, there are 200+ occurrences in other modules, mostly in java.desktop.  I 
> think I'll create another separate issue and will assign it to someone from 
> the jdk client team.
> 
>> Thanks
>> Max
>> 
>>> On Sep 15, 2018, at 2:03 PM, Ivan Gerasimov  
>>> wrote:
>>> 
>>> Hello!
>>> 
>>> This is a followup of the fix of JDK-8210785 (Trivial typo fix in 
>>> X509ExtendedKeyManager javadoc).
>>> 
>>> A few more occurrences of the typo were found.
>>> 
>>> Would you please help review?
>>> 
>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8210786
>>> WEBREV: http://cr.openjdk.java.net/~igerasim/8210786/00/webrev/
>>> 
>>> -- 
>>> With kind regards,
>>> Ivan Gerasimov
>>> 
>> 
> 
> -- 
> With kind regards,
> Ivan Gerasimov
> 

Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives

[Wang Weijun]

Thinking about this again. Looks like the absolute path is not necessary. Even 
if there are multiple files using the same name, they will be in different 
directories, no matter absolute or relative. Suppose the jarPath info is used 
for debugging purpose mostly like the developer can find out what the current 
working directory is and can find the files. *Matthias*: Is the absolute path 
really necessary? Are you working on actual case?

As for the possible global effect of a security property, maybe we can emphasis 
that this is both a security property and system property, and if it’s just for 
one time use, it’s better to use a system property. 

BTW, does the existing value “hostInfo” of the property have a similar problem?

Thanks
Max

>> 在 2018年9月8日，21:42，Sean Mullan  写道：
>> 
>> On 9/7/18 7:58 PM, Weijun Wang wrote:
>> In my understanding, the author deliberately wants to show the absolute 
>> paths when there are multiple jar files with the same name (Ex: a jar hell).
> 
> If you are very familiar with a particular application and understand the 
> risks associated with running it, then I agree that is ok. But security 
> properties apply to all applications using that JDK, and so I don't always 
> think it is practical for an admin to understand every type of application 
> that may be using that JDK and whether or not enabling this property presents 
> a risk.
> 
>> Maybe we can add some more detail in the java.security so an admin knows 
>> what exact impact it has.
> 
> It would be a slippery slope in my opinion. You would have to say something 
> like: "enabling this property may allow untrusted code running under a 
> SecurityManager to access sensitive information such as the user.home system 
> property even if it has not been granted permission to do so."
> 
> I think we should consider not allowing this property to take effect if a 
> SecurityManager is running.
> 
> --Sean
> 
>> --Max
>>> On Sep 8, 2018, at 3:41 AM, Sean Mullan  wrote:
>>> 
>>> On 8/29/18 10:01 AM, Baesken, Matthias wrote:
 Hi Max, thanks for your input .
 I created another webrev , this contains now   the suggested  
 SecurityProperties   class :
 http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.6/
>>> 
>>> java/util/jar/Attributes.java
>>> 
>>> 469 return AccessController.doPrivileged(new 
>>> PrivilegedAction() {
>>> 470 public String run() {
>>> 471 return file.getAbsolutePath() + ":" + lineNumber;
>>> 472 }
>>> 473 });
>>> 
>>> I have a serious concern with the code above.
>>> 
>>> With this change, untrusted code running under a SecurityManager could 
>>> potentially create a JarFile on a non-absolute path ex: "foo.jar", and 
>>> (somehow) cause an IOException to be thrown which would then reveal the 
>>> absolute path of where the jar was located, and thus could reveal sensitive 
>>> details about the system (ex: the user's home directory). It could still be 
>>> hard to exploit, since it would have to be a known jar that exists, and you 
>>> would then need to cause an IOException to be thrown, but it's still 
>>> theoretically possible.
>>> 
>>> In short, this is a more general issue in that it may allow untrusted code 
>>> to access something it couldn't have previously. new 
>>> JarFile("foo.jar").getName() returns "foo.jar", and not the absolute path.
>>> 
>>> Granted this can only be done if the security property is enabled, but I 
>>> believe this is not something administrators should have to know about 
>>> their environment and account for when enabling this property.
>>> 
>>> The above code should be changed to return only what the caller provides to 
>>> JarFile, which is the name of the file (without the full path).
>>> 
>>> --Sean

Re: RFR[11] JDK-8206171: Signature#getParameters for RSASSA-PSS throws ProviderException when not initialized

[Wang Weijun]

> 在 2018年7月20日，05:18，Valerie Peng  写道：
> 
> Hi Sean,
> 
> Thanks for the suggestion, I like it.

Me too. 

> 
> Max, any objection or concern before I update the webrev and CSR?

No.

Thanks
Max

> 
> Valerie
> 
> 
>> On 7/19/2018 7:28 AM, Sean Mullan wrote:
>> Hi Valerie, Max -
>> 
>> I took a fresh look at this issue this morning. I think we are getting 
>> bogged down by trying to adjust within the current wording, which is 
>> somewhat awkward and hard to understand. So, I think it might be better to 
>> break up the wording into multiple sentences. Here's a cut at the rewording:
>> 
>> /**
>>  * Returns the parameters used with this signature object.
>>  *
>>  *  If this signature has been previously initialized with parameters
>>  * (by calling the {@code setParameter} method), this method returns
>>  * the same parameters. If this signature has not been initialized with
>>  * parameters, this method may return a combination of default and
>>  * randomly generated parameter values if the underlying
>>  * signature implementation supports it and can successfully generate
>>  * them. Otherwise, {@code null} is returned.
>>  *
>>  * @return the parameters used with this signature, or {@code null}
>>  */
>> 
>> In the first sentence of the 2nd paragraph above, I wanted to first list the 
>> case where the signature is initialized with parameters, which is the most 
>> clear-cut case of what the behavior will be. Then I described the case where 
>> an implementation may return default/generated parameters -- and this is 
>> clearly marked "may" since it is optional. All other cases other than those 
>> two always return null, so I think this makes it easier to understand.
>> 
>> --Sean
>> 
>>> On 7/18/18 1:29 PM, Valerie Peng wrote:
>>> Sean,
>>> 
>>> Where do you think that we should add the part about "null must be returned 
>>> ..." paragraph? At the end of first or second paragraph?
>>> 
>>> I will go with majority.
>>> 
>>> Valerie
>>> 
 On 7/17/2018 8:38 PM, Weijun Wang wrote:
 Is it better to append the new lines to the 2nd paragraph?
 
 Thanks
 Max
 
> On Jul 18, 2018, at 9:46 AM, Valerie Peng  wrote:
> 
> 
> Ok, let's use "must" then. I have also added the part about default 
> parameters.
> Hope that all is clear now.
> 
> Latest webrev: http://cr.openjdk.java.net/~valeriep/8206171/webrev.03/
> Latest CSR: https://bugs.openjdk.java.net/browse/JDK-8206864
> 
> Thanks,
> Valerie
> 
> On 7/17/2018 5:50 PM, Weijun Wang wrote:
>>> On Jul 18, 2018, at 8:23 AM, Valerie Peng  
>>> wrote:
>>> 
>>> Hi Max,
>>> 
>>> Thanks for the suggestions. Please find comments inline.
>>> 
 On 7/16/2018 7:38 PM, Weijun Wang wrote:
 CSR at https://bugs.openjdk.java.net/browse/JDK-8206864.
 
 - At the end of the 1st paragraph, you have now
 
>However, for signature algorithm such as "RSASSA-PSS", it requires 
> parameters and the one used for signing must be supplied for 
> verification to succeed. It may be better to return null instead of 
> returning provider-specific default parameters.
 I suggest we don't talk about the reason (params must be the same for 
 signing and verification), we can just say something like "If there is 
 no provider-specific default parameters, this method should return 
 null before user sets one".
>>> Alright, I initially didn't put down the reason. But since Brad asked 
>>> about it, I add it to the CSR in case Joe raise the same question. I 
>>> will update the CSR per your suggestion.
 - null may be returned
 
 How about "{@code null} must be returned"?
>>> How about "should"? Is there a guideline on when to use 
>>> "may/should/must"? Anyone knows?
>> Even if there were guidelines on this for Java, I think we should just 
>> stick to https://tools.ietf.org/html/rfc2119, except that the 
>> capitalization is not necessary.
>> 
>> "Must" is precise here.
>> 
>>> I thought must is mostly used in mandating input arguments must not be 
>>> null. But don't recall it being used much for return values.
>> "must return" appears 39 times in java/ and "should return" 19 (simple 
>> grep, no line break).
>> 
>> --Max
>> 
>>> Thanks,
>>> Valerie
>>> 
 Everything else looks fine.
 
 Thanks
 Max
 
> On Jul 17, 2018, at 9:46 AM, Valerie Peng  
> wrote:
> 
> Hi Max,
> 
> Good catch on the SignatureSpi class and SunMSCAPI provider, I will 
> include them.
> 
> As for the part about "randomly generated", I am leaning toward not 
> having it as I don't see  a value of specifying this.
> 
> Webrev and CSR has been updated.
> 
> New 

Re: RFR 8200101: sun/security/krb5/auto/Renewal.java fails intermittently

[Wang Weijun]

The change looks fine.

Thanks
Max

> 在 2018年4月19日，19:32，bhanu.prakash.gopula...@oracle.com 写道：
> 
> Hi All,
> 
> Please review fix for following issue:
> 
> JBS bug: https://bugs.openjdk.java.net/browse/JDK-8200101
> 
> Webrev: http://cr.openjdk.java.net/~bgopularam/8200101/webrev.00/
> 
> It was found that test failure happened due to test machine slowdown, so 
> fixed the issue by increasing the time interval. I have validated the change 
> with repeated run on multiple platforms and found no issues.
> 
> Thanks,
> Bhanu
> 
> 
> 
> 

Re: RFR 8191137: keytool fails to format resource strings for keys for some languages after JDK-8171319

[Wang Weijun]

> 在 2017年11月14日，18:02，Severin Gehwolf  写道：
> 
> This looks fine, but I wonder if a regression test would be in order.
> E.g. test/sun/security/tools/keytool/WeakAlg.java with
> -Duser.language=fr or some such.

But my change has not really fixed anything. It’s just a hint on how to 
correctly translate the strings. The test you suggested would still fail after 
this change. It might be useful when the French translation is updated. 

Thanks
Max

> 
> Thanks,
> Severin

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

[Wang Weijun]

Some more suggestions on the test:

1. You can use readAllBytes() to ... err ... read all bytes. 

2. I normally don’t remove intermediate files. Jtreg will do an automatic 
cleanup, and it can be configured to retain those files when the test fails. 
They will be very helpful in debugging. 

Thanks
Max 

> 在 2017年9月24日，23:12，Weijun Wang  写道：
> 
> Great.
> 
> Some suggestions, just my own habit, you are free to decide yourself:
> 
> 1. In ManifestDigester.java, I'd rather define nameBuf inside the if 
> (isNameAttr(bytes, start)) block.
> 
> 2. I normally don't use "import static" unless I can save a lot of keystrokes 
> and still do not confuse anyone. Both in the test and in 
> ManifestDigester.java.
> 
> 3. In the test, there is no need to write "@run main MultibyteUnicodeName" if 
> it is the only action (i.e. no other build/compile) and there is no modifier 
> on main (i.e. no othervm/timeout etc).
> 
> Several tiny problems:
> 
> 1. No need for @modules in the test. Maybe you used some internal classes in 
> your previous version?
> 
> 2. In the new consolidated repo, the test should be inside test/jdk/sun/..., 
> please note the "jdk" after "test".
> 
> 3. Please update the copyright years.
> 
> 4. In the test, there should be no "JDK-" in the @test tag.
> 
> Thanks
> Max
> 
>> On Sep 24, 2017, at 7:51 PM, Philipp Kunz  wrote:
>> 
>> Hi Max and Vincent
>> 
>> Thank you for your suggestions. It sure looks better now. I hope this time I 
>> got the patch added in the right format.
>> 
>> diff -r ddc25f646c2e 
>> src/java.base/share/classes/sun/security/util/ManifestDigester.java
>> --- a/src/java.base/share/classes/sun/security/util/ManifestDigester.java
>> Thu Aug 31 22:21:20 2017 -0700
>> +++ b/src/java.base/share/classes/sun/security/util/ManifestDigester.java
>> Sun Sep 24 10:34:00 2017 +0200
>> @@ -28,6 +28,7 @@
>> import java.security.*;
>> import java.util.HashMap;
>> import java.io.ByteArrayOutputStream;
>> +import static java.nio.charset.StandardCharsets.UTF_8;
>> 
>> /**
>> * This class is used to compute digests on sections of the Manifest.
>> @@ -112,7 +113,7 @@
>>rawBytes = bytes;
>>entries = new HashMap<>();
>> 
>> -ByteArrayOutputStream baos = new ByteArrayOutputStream();
>> +ByteArrayOutputStream nameBuf = new ByteArrayOutputStream();
>> 
>>Position pos = new Position();
>> 
>> @@ -131,50 +132,40 @@
>> 
>>if (len > 6) {
>>if (isNameAttr(bytes, start)) {
>> -StringBuilder nameBuf = new StringBuilder(sectionLen);
>> +nameBuf.reset();
>> +nameBuf.write(bytes, start+6, len-6);
>> 
>> -try {
>> -nameBuf.append(
>> -new String(bytes, start+6, len-6, "UTF8"));
>> +int i = start + len;
>> +if ((i-start) < sectionLen) {
>> +if (bytes[i] == '\r') {
>> +i += 2;
>> +} else {
>> +i += 1;
>> +}
>> +}
>> 
>> -int i = start + len;
>> -if ((i-start) < sectionLen) {
>> -if (bytes[i] == '\r') {
>> -i += 2;
>> -} else {
>> -i += 1;
>> -}
>> +while ((i-start) < sectionLen) {
>> +if (bytes[i++] == ' ') {
>> +// name is wrapped
>> +int wrapStart = i;
>> +while (((i-start) < sectionLen)
>> +&& (bytes[i++] != '\n'));
>> +if (bytes[i-1] != '\n')
>> +return; // XXX: exception?
>> +int wrapLen;
>> +if (bytes[i-2] == '\r')
>> +wrapLen = i-wrapStart-2;
>> +else
>> +wrapLen = i-wrapStart-1;
>> +
>> +nameBuf.write(bytes, wrapStart, wrapLen);
>> +} else {
>> +break;
>>}
>> +}
>> 
>> -while ((i-start) < sectionLen) {
>> -if (bytes[i++] == ' ') {
>> -// name is wrapped
>> -int wrapStart = i;
>> -while (((i-start) < sectionLen)
>> -&& (bytes[i++] != '\n'));
>> -if (bytes[i-1] != '\n')
>> -return; // XXX: exception?
>> -

RFR 8174909: Doc error in SecureRandom

[Wang Weijun]

Please review this doc bug

diff --git 
a/src/java.base/share/classes/java/security/DrbgParameters.java 
b/src/java.base/share/classes/java/security/DrbgParameters.java

--- a/src/java.base/share/classes/java/security/DrbgParameters.java
+++ b/src/java.base/share/classes/java/security/DrbgParameters.java
@@ -53,8 +53,8 @@
  * for CTR_DRBG. Please note that it is not the algorithm used in
  * {@link SecureRandom#getInstance}, which we will call a
  * SecureRandom algorithm below),
- *  optionally features, including prediction resistance
- * and reseeding supports.
+ *  optional features, including prediction resistance
+ * and reseeding supports,
  *  highest security strength.
  * 
  * 
diff --git a/src/java.base/share/classes/java/security/SecureRandom.java 
b/src/java.base/share/classes/java/security/SecureRandom.java

--- a/src/java.base/share/classes/java/security/SecureRandom.java
+++ b/src/java.base/share/classes/java/security/SecureRandom.java
@@ -64,8 +64,8 @@
  * 
  * SecureRandom r1 = new SecureRandom();
  * SecureRandom r2 = SecureRandom.getInstance("NativePRNG");
- * SecureRandom r3 = SecureRandom("DRBG",
- * DrbgParameters.Instantiation(128, RESEED_ONLY, null));
+ * SecureRandom r3 = SecureRandom.getInstance("DRBG",
+ * DrbgParameters.instantiation(128, RESEED_ONLY, null));
  * 
  *
  *  The third statement above returns a {@code SecureRandom} object 
of the


Thanks
Max

RFR: release note for JDK-7004967 SecureRandom should be more explicit about threading

[Wang Weijun]

https://bugs.openjdk.java.net/browse/JDK-8165115

Thanks
Max

Re: Code Review Request JDK-8172217, Need debug log for the intermittent failure of AnonCipherWithWantClientAuth

[Wang Weijun]

Sorry, I didn't notice it.

--Max

On 1/4/2017 7:59 AM, Xuelei Fan wrote:



On 1/3/2017 3:54 PM, Wang Weijun wrote:

This looks good.

On the other hand, it might be better to add exception stack trace or
links to failures to the bug report. Maybe you think they are too
useless?


As this is a sub-tack of JDK-8172005:
https://bugs.openjdk.java.net/browse/JDK-8172005

It's always good to add more details in the bug report.  I just added
the exception stack.

Thanks,
Xuelei


Thanks
Max


On Jan 4, 2017, at 7:23 AM, Xuelei Fan <xuelei@oracle.com> wrote:

Hi,

The intermittent test failure of
test/sun/security/ssl/ServerHandshaker/AnonCipherWithWantClientAuth.java
was fond again.  However, the underlying reason is still not clear.
Need to dump more debug log for further evaluation.

   https://bugs.openjdk.java.net/browse/JDK-8172217

$ hg diff AnonCipherWithWantClientAuth.java

- * @run main/othervm AnonCipherWithWantClientAuth
+ * @run main/othervm -Djavax.net.debug=all AnonCipherWithWantClientAuth

Thanks,
Xuelei

Re: Code Review Request JDK-8172217, Need debug log for the intermittent failure of AnonCipherWithWantClientAuth

[Wang Weijun]

This looks good.

On the other hand, it might be better to add exception stack trace or links to 
failures to the bug report. Maybe you think they are too useless?

Thanks
Max

> On Jan 4, 2017, at 7:23 AM, Xuelei Fan  wrote:
> 
> Hi,
> 
> The intermittent test failure of 
> test/sun/security/ssl/ServerHandshaker/AnonCipherWithWantClientAuth.java was 
> fond again.  However, the underlying reason is still not clear. Need to dump 
> more debug log for further evaluation.
> 
>https://bugs.openjdk.java.net/browse/JDK-8172217
> 
> $ hg diff AnonCipherWithWantClientAuth.java
> 
> - * @run main/othervm AnonCipherWithWantClientAuth
> + * @run main/othervm -Djavax.net.debug=all AnonCipherWithWantClientAuth
> 
> Thanks,
> Xuelei

Re: RFR 8157389: Release Note: New default -sigalg for jarsigner and keytool

[Wang Weijun]

Ping again. Please review https://bugs.openjdk.java.net/browse/JDK-8157389.

JDK-8171190 was already resolved.

Thanks
Max

> On Dec 14, 2016, at 10:04 AM, Wang Weijun <weijun.w...@oracle.com> wrote:
> 
> I copied the exact @implNote from JarSigner into the release note.
> 
> BTW, I noticed NIST 800-57 Part 1 has a new revision 4, which has the same 
> tables as in revision 3. A new bug 
> https://bugs.openjdk.java.net/browse/JDK-8171190 created and I'll create a 
> webrev soon.
> 
> Thanks
> Max
> 
>> On Dec 14, 2016, at 5:09 AM, Sean Mullan <sean.mul...@oracle.com> wrote:
>> 
>> Hi Max,
>> 
>> Could you include more information that shows what signature algorithm is 
>> used based on the key size range and algorithm?
>> 
>> --Sean
>> 
>> On 12/11/16 9:53 PM, Wang Weijun wrote:
>>> Please take a review at the release note at
>>> 
>>>  https://bugs.openjdk.java.net/browse/JDK-8157389
>>> 
>>> Thanks
>>> Max
>>> 
> 

Re: RFR 8170732: GssKrb5Client sends non-zero buffer size when qop is "auth"

[Wang Weijun]

Ping again.

On 12/22/2016 9:52 AM, Wang Weijun wrote:

Please take a review at

http://cr.openjdk.java.net/~weijun/8170732/webrev.00/

According to https://tools.ietf.org/html/rfc4752#section-3.1:

The client then constructs data, with the first octet containing the
bit-mask specifying the selected security layer, the second through
fourth octets containing in network byte order the maximum size
output_message the client is able to receive (which MUST be 0 if the
client does not support any security layer),

A test is modified to check this case. Please note that when there is
no security layer, you cannot call wrap/unwrap anymore.

Thanks Max

Re: RFR 8172017: Two tests sun/security/krb5/auto/ReplayCacheTestProc.java and rcache_usemd5.sh fail on Solaris (Additional pre-authentication required)

[Wang Weijun]

Ping again.

On 12/27/2016 3:25 AM, Wang Weijun wrote:

Please take a review at

http://cr.openjdk.java.net/~weijun/8172017/webrev.00

On Solaris when launched by root, the rcache directory is a little
different.

I've manually tested this on a Solaris machine, and seen rcache
files created at different directories when the test was launched by
root and a normal user.

Thanks Max

RFR 8172017: Two tests sun/security/krb5/auto/ReplayCacheTestProc.java and rcache_usemd5.sh fail on Solaris (Additional pre-authentication required)

[Wang Weijun]

Please take a review at

   http://cr.openjdk.java.net/~weijun/8172017/webrev.00

On Solaris when launched by root, the rcache directory is a little 
different.


I've manually tested this on a Solaris machine, and seen rcache files 
created at different directories when the test was launched by root and 
a normal user.


Thanks
Max

RFR 8170732: GssKrb5Client sends non-zero buffer size when qop is "auth"

[Wang Weijun]

Please take a review at

  http://cr.openjdk.java.net/~weijun/8170732/webrev.00/

According to https://tools.ietf.org/html/rfc4752#section-3.1:

   The client then constructs data, with the first octet containing the
   bit-mask specifying the selected security layer, the second through
   fourth octets containing in network byte order the maximum size
   output_message the client is able to receive (which MUST be 0 if the
   client does not support any security layer),

A test is modified to check this case. Please note that when there is no 
security layer, you cannot call wrap/unwrap anymore.

Thanks
Max

Re: Code Review Request, 6972386 issues with String.toLowerCase/toUpperCase

[Wang Weijun]

Change looks fine.

Thanks for taking care of CtrDrbg.java. I've make quite some changes like this 
and it's a shame I missed it in my own code.

--Max

> On Dec 22, 2016, at 4:05 AM, Xuelei Fan  wrote:
> 
> Hi,
> 
> Please review the fix:
> 
>   http://cr.openjdk.java.net/~xuelei/6972386/webrev.00/
> 
> Change to use the language/country neutral locale for the locale sensitive 
> operations.
> 
> Thanks,
> Xuelei

Re: RFR 8170900: Issue with FilePermission::implies for wildcard flag(-)

[Wang Weijun]

> On Dec 22, 2016, at 8:12 AM, Xuelei Fan <xuelei@oracle.com> wrote:
> 
> I think the note is an example, may not need an additional CCC.

That's always my understanding.

> 
> For easier reading, I may use a contrast example.  For example, "Note that 
> this means "/-" implies "/foo" but not "foo".".

Good advice.

Thanks
Max

> 
> Use the one you like, I'm OK with the either.
> 
> Xuelei
> 
> On 12/21/2016 3:58 PM, Wang Weijun wrote:
>> 
>>> On Dec 22, 2016, at 4:39 AM, Xuelei Fan <xuelei@oracle.com> wrote:
>>> 
>>> I'm trying to understand this update.  Does "/-" imply "/foo"?
>> 
>> Yes.
>> 
>>> 
>>> Does the following spec can be used to explain the new added note?
>>> 
>>>* if the wildcard flag is "-", the simple pathname's path
>>>* must be recursively inside the wildcard pathname's path.
>> 
>> Yes.
>> 
>> But the precise meaning of "recursively inside" is different between the 
>> pre-jdk9 and jdk9 behaviors. The @implNote explains more.
>> 
>> --Max
>> 
>>> 
>>> Xuelei
>>> 
>>> On 12/19/2016 11:25 PM, Wang Weijun wrote:
>>>> Ping again.
>>>> 
>>>>> On Dec 14, 2016, at 1:53 PM, Wang Weijun <weijun.w...@oracle.com> wrote:
>>>>> 
>>>>> An clarification is added to FilePermission::implies:
>>>>> 
>>>>>* @implNote
>>>>>  
>>>>>* a simple {@code npath} is recursively inside a wildcard {@code npath}
>>>>>* if and only if {@code simple_npath.relativize(wildcard_npath)}
>>>>> - * is a series of one or more "..". An invalid {@code 
>>>>> FilePermission} does
>>>>> + * is a series of one or more "..". Note that this means "/-" does 
>>>>> not
>>>>> + * imply "foo". An invalid {@code FilePermission} does
>>>>>* not imply any object except for itself.
>>>>> 
>>>>> The newly added sentence is
>>>>> 
>>>>> Note that this means "/-" does not imply "foo".
>>>>> 
>>>>> JCK has agreed to update their test.
>>>>> 
>>>>> Since this is just a clarification inside an @implNote and no spec is 
>>>>> updated, I suppose no CCC is needed. Please confirm.
>>>>> 
>>>>> Thanks
>>>>> Max
>>>>> 
>>>> 
>> 

Re: RFR 8170900: Issue with FilePermission::implies for wildcard flag(-)

[Wang Weijun]

Ping again.

> On Dec 14, 2016, at 1:53 PM, Wang Weijun <weijun.w...@oracle.com> wrote:
> 
> An clarification is added to FilePermission::implies:
> 
>  * @implNote
>
>  * a simple {@code npath} is recursively inside a wildcard {@code npath}
>  * if and only if {@code simple_npath.relativize(wildcard_npath)}
> - * is a series of one or more "..". An invalid {@code FilePermission} 
> does
> + * is a series of one or more "..". Note that this means "/-" does not
> + * imply "foo". An invalid {@code FilePermission} does
>  * not imply any object except for itself.
> 
> The newly added sentence is
> 
>  Note that this means "/-" does not imply "foo".
> 
> JCK has agreed to update their test.
> 
> Since this is just a clarification inside an @implNote and no spec is 
> updated, I suppose no CCC is needed. Please confirm.
> 
> Thanks
> Max
> 

Re: RFR 8075618: Create tests to check jarsigner work with multi-version jar

[Wang Weijun]

Hi Amanda

Everything is fine. I have no other comment.

Thanks
Max

> On Dec 19, 2016, at 2:43 PM, Amanda Jiang <amanda.ji...@oracle.com> wrote:
> 
> 
> 
> On 12/18/16 10:23 PM, Wang Weijun wrote:
>>> Please see updated webrev : 
>>> http://cr.openjdk.java.net/~amjiang/8075618/webrev.04/ (also attached)
>>> 
>>> I moved the checkPermission() calls into v9/bersion/Version.java, but I did 
>>> not check if the output contains "I'm running on version 9", I think it 
>>> does not make sense to make test fail with later or earlier version of Java 
>>> (10 or 8).
>> This test will run in JDK 10 and your checkPermission(perm,bool) method will 
>> not get called. Suppose a regression is introduced one day that breaks the 
>> access control checking, this test will not be able to catch it.
>> 
>> If you check for the "version 9" string, the test will fail which reminds 
>> you to move the checkPermission call to Version.java of JDK 10 and it will 
>> catch that regression.
>> 
>> Thanks
>> Max
> Version check added to the test 
> :http://cr.openjdk.java.net/~amjiang/8075618/webrev.05
> Thanks,
> Amanda
> 

Re: RFR 8075618: Create tests to check jarsigner work with multi-version jar

[Wang Weijun]

> On Dec 19, 2016, at 5:38 AM, Amanda Jiang <amanda.ji...@oracle.com> wrote:
> 
> Hi Paul, Artem and Max,
> 
> Thanks for your comments.  Please check the new webrev at: 
> http://cr.openjdk.java.net/~amjiang/8075618/webrev.03/  (It looks like 
> "cr.openjdk.java.net" is down, I also attach the webrev with this email).
> 
> In the new webrev, I fixed some syntax issue mentioned by Paul and also 
> simplied the test codes with Artem and Max's suggestions.  Unfortunately I 
> cannot fully apply some functions from 
> http://hg.openjdk.java.net/jdk9/dev/jdk/file/d4d7f1f0d688/test/lib/security/SecurityTools.java
>  . Those functions returns deprecated "OutputAnalyzer " class in 
> http://hg.openjdk.java.net/jdk9/dev/jdk/file/ab164f8b8569/test/lib/testlibrary/jdk/testlibrary/OutputAnalyzer.java
>  ,  which conflicts with the "OubputAnalyzer" class I imported for my test ( 
> http://hg.openjdk.java.net/jdk9/dev/file/5e79c9bac1b5/test/lib/jdk/test/lib/process/OutputAnalyzer.java)

This is a pity. We should move it to the root repo before more people start 
using it.

> 
> Max,
> 
> I also add one test case for permission granted to signed multi-release jar 
> files,

Maybe it's better to move the checkPermission() calls into 
v9/version/Version.java? This would demonstrate that the versioned class itself 
is granted permissions.

In order to make sure it's not another version of Version.java that is running, 
I think you can call assertContains("I am running on version 10").

And if you don't intend to reuse Main.java, I think it's not worth passing into 
arguments.

Thanks
Max

> other functional tests are covered by 
> http://hg.openjdk.java.net/jdk9/dev/jdk/file/ab164f8b8569/test/java/util/jar/JarFile/mrjar/MultiReleaseJarSecurity.java
> 
> Thanks
> Amanda
> On 12/12/16 6:31 PM, Wang Weijun wrote:
>> Hi Amanda
>> 
>> Can you also test the new JarSigner API?
>> 
>>http://hg.openjdk.java.net/jdk9/dev/jdk/rev/ce33c780cfbd
>> 
>> line 2.72 has an example on it.
>> 
>>> On Dec 13, 2016, at 9:22 AM, Artem Smotrakov <artem.smotra...@oracle.com> 
>>> wrote:
>>> 
>>> You can use 
>>> http://hg.openjdk.java.net/jdk9/dev/jdk/file/d4d7f1f0d688/test/lib/security/SecurityTools.java
>>>  which would simplify the code. This lib was added to be used in such tests.
>> Correct. I also wonder if there are existing methods on javac compilation.
>> 
>> Do we have existing tests on checking if a signed multi-version jar works as 
>> expected? Say, permission granted, getCertificates() returning non-null, etc?
>> 
>> Thanks
>> Max
>> 
> 
> 

RFR 8171353: New home for SecurityTools.java test utility

[Wang Weijun]

Hi Artem

I hope you are OK with this change:

diff --git a/test/lib/security/SecurityTools.java 
b/test/lib/testlibrary/jdk/testlibrary/SecurityTools.java
rename from test/lib/security/SecurityTools.java
rename to test/lib/testlibrary/jdk/testlibrary/SecurityTools.java
--- a/test/lib/security/SecurityTools.java
+++ b/test/lib/testlibrary/jdk/testlibrary/SecurityTools.java
@@ -20,13 +20,11 @@
  * or visit www.oracle.com if you need additional information or have any
  * questions.
  */
+package jdk.testlibrary;

 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
-import jdk.testlibrary.JDKToolLauncher;
-import jdk.testlibrary.OutputAnalyzer;
-import jdk.testlibrary.ProcessTools;

 public class SecurityTools {

diff --git a/test/sun/security/tools/keytool/PrintSSL.java 
b/test/sun/security/tools/keytool/PrintSSL.java
--- a/test/sun/security/tools/keytool/PrintSSL.java
+++ b/test/sun/security/tools/keytool/PrintSSL.java
@@ -25,7 +25,6 @@
  * @test
  * @bug 6480981 8160624
  * @summary keytool should be able to import certificates from remote SSL 
server
- * @library /lib/security
  * @library /lib/testlibrary
  * @run main/othervm PrintSSL
  */
@@ -37,6 +36,7 @@
 import javax.net.ssl.SSLServerSocketFactory;
 import javax.net.ssl.SSLSocket;
 import jdk.testlibrary.OutputAnalyzer;
+import jdk.testlibrary.SecurityTools;

 public class PrintSSL {

diff --git a/test/sun/security/tools/keytool/ReadJar.java 
b/test/sun/security/tools/keytool/ReadJar.java
--- a/test/sun/security/tools/keytool/ReadJar.java
+++ b/test/sun/security/tools/keytool/ReadJar.java
@@ -25,7 +25,6 @@
  * @test
  * @bug 6890872 8168882
  * @summary keytool -printcert to recognize signed jar files
- * @library /lib/security
  * @library /lib/testlibrary
  */

@@ -33,6 +32,7 @@
 import java.nio.file.Paths;
 import jdk.testlibrary.JarUtils;
 import jdk.testlibrary.OutputAnalyzer;
+import jdk.testlibrary.SecurityTools;

 public class ReadJar {

Thanks
Max

RFR 8171340: HttpNegotiateServer/java test should not use system proxy on Mac

[Wang Weijun]

Please take a review at

   http://cr.openjdk.java.net/~weijun/8171340/webrev.00/

All "openConnection()" modified to "openConnection(Proxy.NO_PROXY)".

Everything else is whitespace change.

Thanks
Max

Re: RFR 8171190: Bump reference of NIST 800-57 Part 1 Rev 3 to Rev 4 in JarSigner API spec

[Wang Weijun]

7680 itself is 192 bits, and for any bitLength greater than 7680 I treat it as 
in a "higher" level.

--Max

> On Dec 14, 2016, at 5:19 PM, Bernd Eckenfels  wrote:
> 
> Hello,
> 
> I noticed in the existing code: Is the comment "256 bits" referring to the 
> 'comparable strength'?
> 
> # if (bitLength > 7680) { // 256 bits
> 
> If so, it seems misleading, according to table 2 this would be 192 bit. Maybe 
> this can be corrected, removed or the meaning of the comment clarified.
> 
> Gruss
> Bernd
> -- 
> http://bernd.eckenfels.net

Re: RFR 8168979: @implNote for invalid FilePermission

[Wang Weijun]

> On Dec 14, 2016, at 10:11 AM, Xuelei Fan <xuelei@oracle.com> wrote:
> 
> On 12/13/2016 5:45 PM, Wang Weijun wrote:
>> A major behavior change is that <> now implies an invalid 
>> permission, I hope this is good to minimize incompatibility.
> Looks like two sides of the same coin.  If there is an invalid <> 
> (not existing in practice, I think),

Not existing in theory either. As the change says, invalid happens when the 
conversion of string to NIO Path fails. For <>, there will be no 
such conversion.

> it now implies all;  if no change on this point, <> does not imply 
> invalid one.  The existing behavior looks more safe to me.  What's you 
> concern to make this behavior change?

Just safer. Sometimes an app can recover from a FileNotExistException but not a 
SecurityException.

Thanks
Max

> 
> Otherwise, looks fine to me.
> 
> Xuelei

RFR 8171190: Bump reference of NIST 800-57 Part 1 Rev 3 to Rev 4 in JarSigner API spec

[Wang Weijun]

NIST 800-57 Part 1 has a new revision. The lines below are newly introduced in 
jdk9.

diff --git a/src/java.base/share/classes/sun/security/x509/AlgorithmId.java 
b/src/java.base/share/classes/sun/security/x509/AlgorithmId.java
--- a/src/java.base/share/classes/sun/security/x509/AlgorithmId.java
+++ b/src/java.base/share/classes/sun/security/x509/AlgorithmId.java
@@ -1024,7 +1024,7 @@
 }
 }

-// Values from SP800-57 part 1 rev 3 tables 2 and three
+// Values from SP800-57 part 1 rev 4 tables 2 and 3
 private static String ecStrength (int bitLength) {
 if (bitLength >= 512) { // 256 bits of strength
 return "SHA512";
@@ -1035,7 +1035,7 @@
 }
 }

-// same values for RSA and DSA
+// Same values for RSA and DSA
 private static String ifcFfcStrength (int bitLength) {
 if (bitLength > 7680) { // 256 bits
 return "SHA512";
diff --git 
a/src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java 
b/src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java
--- a/src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java
+++ b/src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java
@@ -430,7 +430,7 @@
  * SHA384withECDSA for a 384-bit EC key.
  *
  * @implNote This implementation makes use of comparable strengths
- * as defined in Tables 2 and 3 of NIST SP 800-57 Part 1-Rev.3.
+ * as defined in Tables 2 and 3 of NIST SP 800-57 Part 1-Rev.4.
  * Specifically, if a DSA or RSA key with a key size greater than 7680
  * bits, or an EC key with a key size greater than or equal to 512 
bits,
  * SHA-512 will be used as the hash function for the signature.

Thanks
Max

Re: RFR 8157389: Release Note: New default -sigalg for jarsigner and keytool

[Wang Weijun]

I copied the exact @implNote from JarSigner into the release note.

BTW, I noticed NIST 800-57 Part 1 has a new revision 4, which has the same 
tables as in revision 3. A new bug 
https://bugs.openjdk.java.net/browse/JDK-8171190 created and I'll create a 
webrev soon.

Thanks
Max

> On Dec 14, 2016, at 5:09 AM, Sean Mullan <sean.mul...@oracle.com> wrote:
> 
> Hi Max,
> 
> Could you include more information that shows what signature algorithm is 
> used based on the key size range and algorithm?
> 
> --Sean
> 
> On 12/11/16 9:53 PM, Wang Weijun wrote:
>> Please take a review at the release note at
>> 
>>   https://bugs.openjdk.java.net/browse/JDK-8157389
>> 
>> Thanks
>> Max
>> 

Re: RFR 8168979: @implNote for invalid FilePermission

[Wang Weijun]

Webrev updated at

   http://cr.openjdk.java.net/~weijun/8168979/webrev.01

One comment is moved to its correct position and a typo fixed.

Thanks Daniel for the comment. Can someone with a reviewer hat take a look?

Thanks
Max

> On Dec 12, 2016, at 6:03 PM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:
> 
> Hi Max,
> 
> Don't count me as reviewer - but I see a mismatched comment
> in the file:
> 
> 209 /**
> 210  * Creates FilePermission objects with special internals.
> 211  * See {@link FilePermCompat#newPermPlusAltPath(Permission)} and
> 212  * {@link FilePermCompat#newPermUsingAltPath(Permission)}.
> 213  */
> 214
> 215 private static final Path here = builtInFS.getPath(
> 216 GetPropertyAction.privilegedGetProperty("user.dir"));
> 
> I guess the comment is a left over from some merge or previous fix?
> 
> 
> Also I noticed the following later on:
> 
> 541  * invalid {@code FilePermission}. Even if two {@code FilePermission}
> 542  * are created with the same invalid path, one does imply the other.
> 
> should this be:
> 
>Even if two {@code FilePermission} are created with the same
>invalid path, one does *not* imply the other.
> 
> best regards,
> 
> -- daniel
> 
> On 12/12/16 09:01, Wang Weijun wrote:
>> Please take a review at
>> 
>>   http://cr.openjdk.java.net/~weijun/8168979/webrev.00/
>> 
>> This further clarifies what an invalid FilePermission behaves. A major 
>> behavior change is that <> now implies an invalid permission, I 
>> hope this is good to minimize incompatibility.
>> 
>> Thanks
>> Max
>> 
> 

Re: On 8171135: Include javadoc on JarSigner API in security doc

[Wang Weijun]

Jon

Will there be a dedicated page for all non-SE javadocs? We already had some in 
jdk.security.auth, jdk.security.jgss on the security guide home page [1].

Also, I am not sure if this belongs to the docs or build category if the 
javadoc is meant to be generated automatically. Therefore I let it stay in the 
security-libs at the moment.

Thanks
Max

[1] http://download.java.net/java/jdk9/docs/technotes/guides/security/index.html

> On Dec 14, 2016, at 5:32 AM, Sean Mullan <sean.mul...@oracle.com> wrote:
> 
> I would check with Jon Gibbons to see if there may be a more "official" place 
> for exported, non-SE javadocs; otherwise it seems ok to me.
> 
> --Sean
> 
> On 12/12/16 9:50 PM, Wang Weijun wrote:
>> Hi All
>> 
>> I've create a new bug to include the javadoc of the non-Java SE JarSigner 
>> API into security doc:
>> 
>>   https://bugs.openjdk.java.net/browse/JDK-8171135
>> 
>> If you think this is OK, I'll move the bug to doc.
>> 
>> Thanks
>> Max
>> 

On 8171135: Include javadoc on JarSigner API in security doc

[Wang Weijun]

Hi All

I've create a new bug to include the javadoc of the non-Java SE JarSigner API 
into security doc:

   https://bugs.openjdk.java.net/browse/JDK-8171135

If you think this is OK, I'll move the bug to doc.

Thanks
Max

Re: [PATCH] 8165751: NPE in Signature with java.security.debug=provider

[Wang Weijun]

Hi Adam

The only behavior change is with the debug output, right?

Is this a new pattern that internal optional fields should be defined as an 
Optional?

And, when there is no provider the string form "from: " looks strange, I would 
rather make it "from nowhere". I would also move the space before "from" to 
where the method is called, say, " verification algorithm " + 
getProvidedByString().

Thanks
Max

> On Dec 13, 2016, at 4:05 AM, Adam Petcher  wrote:
> 
> Okay. I changed getProvider() to return this.provider.orElse(null), which 
> will allow this method to return null. For consistency, I allow all other 
> providers (in Instance and Service) to be null using Optional.ofNullable(). 
> Hopefully, I found and fixed all the whitespace issues, too. Here is the 
> corrected diff:
> 
> diff --git a/src/java.base/share/classes/java/security/Signature.java 
> b/src/java.base/share/classes/java/security/Signature.java
> --- a/src/java.base/share/classes/java/security/Signature.java
> +++ b/src/java.base/share/classes/java/security/Signature.java
> @@ -134,7 +134,7 @@
> private String algorithm;
> 
> // The provider
> -Provider provider;
> +Optional provider = Optional.empty();
> 
> /**
>  * Possible {@link #state} value, signifying that
> @@ -266,7 +266,7 @@
> SignatureSpi spi = (SignatureSpi)instance.impl;
> sig = new Delegate(spi, algorithm);
> }
> -sig.provider = instance.provider;
> +sig.provider = Optional.ofNullable(instance.provider);
> return sig;
> }
> 
> @@ -449,13 +449,17 @@
>  */
> public final Provider getProvider() {
> chooseFirstProvider();
> -return this.provider;
> +return this.provider.orElse(null);
> }
> 
> void chooseFirstProvider() {
> // empty, overridden in Delegate
> }
> 
> +private String getProvidedByString() {
> +return provider.map(x -> " from: " + x.getName()).orElse("");
> +}
> +
> /**
>  * Initializes this object for verification. If this method is called
>  * again with a different argument, it negates the effect
> @@ -473,7 +477,7 @@
> 
> if (!skipDebug && pdebug != null) {
> pdebug.println("Signature." + algorithm +
> -" verification algorithm from: " + this.provider.getName());
> +" verification algorithm" + getProvidedByString());
> }
> }
> 
> @@ -522,7 +526,7 @@
> 
> if (!skipDebug && pdebug != null) {
> pdebug.println("Signature." + algorithm +
> -" verification algorithm from: " + this.provider.getName());
> +" verification algorithm" + getProvidedByString());
> }
> }
> 
> @@ -543,7 +547,7 @@
> 
> if (!skipDebug && pdebug != null) {
> pdebug.println("Signature." + algorithm +
> -" signing algorithm from: " + this.provider.getName());
> +" signing algorithm" + getProvidedByString());
> }
> }
> 
> @@ -566,7 +570,7 @@
> 
> if (!skipDebug && pdebug != null) {
> pdebug.println("Signature." + algorithm +
> -" signing algorithm from: " + this.provider.getName());
> +" signing algorithm" + getProvidedByString());
> }
> }
> 
> @@ -1087,7 +1091,7 @@
> }
> try {
> sigSpi = newInstance(s);
> -provider = s.getProvider();
> +provider = Optional.ofNullable(s.getProvider());
> // not needed any more
> firstService = null;
> serviceIterator = null;
> @@ -1132,7 +1136,7 @@
> try {
> SignatureSpi spi = newInstance(s);
> init(spi, type, key, random);
> -provider = s.getProvider();
> +provider = Optional.ofNullable(s.getProvider());
> sigSpi = spi;
> firstService = null;
> serviceIterator = null;
> diff --git a/test/java/security/Signature/NoProvider.java 
> b/test/java/security/Signature/NoProvider.java
> new file mode 100644
> --- /dev/null
> +++ b/test/java/security/Signature/NoProvider.java
> @@ -0,0 +1,99 @@
> +/*
> + * Copyright (c) 2016, 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 

Re: RFR 8168979: @implNote for invalid FilePermission

[Wang Weijun]

> On Dec 12, 2016, at 6:03 PM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:
> 
> Hi Max,
> 
> Don't count me as reviewer - but I see a mismatched comment
> in the file:
> 
> 209 /**
> 210  * Creates FilePermission objects with special internals.
> 211  * See {@link FilePermCompat#newPermPlusAltPath(Permission)} and
> 212  * {@link FilePermCompat#newPermUsingAltPath(Permission)}.
> 213  */
> 214
> 215 private static final Path here = builtInFS.getPath(
> 216 GetPropertyAction.privilegedGetProperty("user.dir"));
> 
> I guess the comment is a left over from some merge or previous fix?

These are 2 different methods: "Plus" and "Using".

> 
> 
> Also I noticed the following later on:
> 
> 541  * invalid {@code FilePermission}. Even if two {@code FilePermission}
> 542  * are created with the same invalid path, one does imply the other.
> 
> should this be:
> 
>Even if two {@code FilePermission} are created with the same
>invalid path, one does *not* imply the other.

Ah, yes.

Thanks
Max

> 
> best regards,
> 
> -- daniel
> 
> On 12/12/16 09:01, Wang Weijun wrote:
>> Please take a review at
>> 
>>   http://cr.openjdk.java.net/~weijun/8168979/webrev.00/
>> 
>> This further clarifies what an invalid FilePermission behaves. A major 
>> behavior change is that <> now implies an invalid permission, I 
>> hope this is good to minimize incompatibility.
>> 
>> Thanks
>> Max
>> 
> 

RFR 8157389: Release Note: New default -sigalg for jarsigner and keytool

[Wang Weijun]

Please take a review at the release note at

   https://bugs.openjdk.java.net/browse/JDK-8157389

Thanks
Max

Re: [9] RFR 8170247: java/security/SecureRandom/ApiTest fails when run with unlimited policy.

[Wang Weijun]

Change looks fine.

One nit: the extra space at the beginning of line 24 looks strange.

Thanks
Max

On 11/30/2016 5:27 PM, Sibabrata Sahoo wrote:

Hi,



Please review the patch for,



JBS: https://bugs.openjdk.java.net/browse/JDK-8170247

Webrev: http://cr.openjdk.java.net/~ssahoo/8170247/webrev.00/



Description:

The Test was failing to handle the expected failure for invalid
parameters, when the SecureRandom parameter size is > algorithm running
with unlimited policy. I have modified the condition to handle the
expected failures correctly. The change is only applicable to the “if”
condition(Line:200). The other 2 lines are minor changes related to
statement performance and renaming a variable.



Thanks,

Siba

Re: [8u-dev] RFR 8170278 : ticket renewal won't happen with debugging turned on

[Wang Weijun]

+1

--Max

> On Nov 29, 2016, at 5:16 PM, Seán Coffey  wrote:
> 
> Looks good.
> 
> regards,
> Sean.
> 
> 
> On 29/11/2016 09:08, Ivan Gerasimov wrote:
>> Hello!
>> 
>> It was reported that there's broken behavior exposed when the debug mode is 
>> turned on, which is due to KerberosTicket.toString() throwing an exception, 
>> if the ticket was already destroyed.
>> 
>> The proposed fix is in fact the backport of a tiny portion of JDK-8043071.
>> 
>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8170278
>> WEBREV: http://cr.openjdk.java.net/~igerasim/8170278/00/webrev/
>> 
>> Would you please help review?
>> 
>> With kind regards,
>> Ivan
> 

Re: RFR(s): JDK-8169866: [TESTBUG] com/sun/security/ tests have undeclared modules dependencies

[Wang Weijun]

Hi Sergei

Looks good to me.

Thanks
Max

On 11/28/2016 9:17 PM, Sergei Kovalev wrote:

R-sending request for review


17.11.16 15:43, Sergei Kovalev wrote:

Hello team,

Please review a small fix for security tests.

BugID: https://bugs.openjdk.java.net/browse/JDK-8169866
Web review: http://cr.openjdk.java.net/~skovalev/8169866/webrev.00/

Issue: Tests from package com/sun/security failing in case of usage
"--limit-modules java.base" command line options due to undeclared
dependencies.
Solution: add declaration of required modules.

Re: RFR 8170364: FilePermission path modified during merge

[Wang Weijun]

Hi Alan

Updated webrev at

   http://cr.openjdk.java.net/~weijun/8170364/webrev.01

Changes since webrev.00:

- a private constructor that can clones 4 fields and modifies 5 others

- using lambda

- test enhancement

Thanks
Max

Re: RFR 8170364: FilePermission path modified during merge

[Wang Weijun]

> On Nov 27, 2016, at 7:13 PM, Wang Weijun <weijun.w...@oracle.com> wrote:
> 
>> 
>> On Nov 27, 2016, at 6:12 PM, Alan Bateman <alan.bate...@oracle.com> wrote:
>> 
>> On 26/11/2016 08:54, Wang Weijun wrote:
>> 
>>> Please take a review at
>>> 
>>>   http://cr.openjdk.java.net/~weijun/8170364/webrev.00/
>>> 
>>> The compatibility layer introduced in the new FilePermission implementation 
>>> requires one FilePermission to imply another with either a relative path or 
>>> an absolute path. This is solved with a private field npath2 inside. When 
>>> this field is set, the permission's name is modified a little (JDK-8168127) 
>>> so that when adding to a FilePermissionCollection, it is not confused with 
>>> one without the field. Unfortunately, this modified name is reused in the 
>>> merge to create a new "merged" FilePermission which contains a malformed 
>>> path now.
>>> 
>>> This fix creates a new non-public method to create this "merged" 
>>> FilePermission.
>>> 
>>> I checked other places and seems the name is not used to create a new 
>>> FilePermission.
>>> 
>> Ideally FilePermission (and all permissions) would have final fields, I hope 
>> that can be fixed some day. 
> 
> Me too, but we have that huge init() method now.
> 
>> In the mean-time then having withNewActions create a new FilePermission and 
>> then mutate it looks a bit ugly, can't you just introduce a new non-public 
>> constructor to create it with the effective mask?
> 
> The private "clone" constructor is now used in 3 places, all with a mutation 
> right after (lines 267, 280, 993). I've taken care that only newly created 
> objects are mutated this way. Creating 3 new constructors looks like too many 
> duplicated codes.

Do you still have a strong opinion?

> 
>> 
>> In passing then maybe the comment at L1063-1065 can be re-examined and it 
>> looks to be stale (the specific bootstrapping issue mentioned was fixed in 
>> 2015).
> 
> I can try.

Changed to lambda. Tests on -testset core most pass. 
java/lang/invoke/lambda/LogGeneratedClassesTest.java still fails but it already 
failed before my change.

> 
>> 
>> The test is one specific scenario and I wonder if you've considered beefing 
>> this up to other scenarios and other targets so that it's more broadly 
>> testing the merging scenarios.
> 
> I can add 2 more lines on checking the absolute path, and maybe with delete, 
> execute and readlink (1+1+1+1, 2+2, 3+1 etc). The merge is only about the 
> name and any name is the same, so it seems unnecessary to try other names.

Test enhanced. Permissions are granted in "read", "write", "delete", "execute", 
or "read,write", "delete,execute", or "read,write,delete", "execute", or 
"read,write,delete,execute", and 2^4-1 combinations of actions for both "x" and 
"/abs/x" checked.

If you are OK with the above, I'll post an updated webrev.

Thanks
Max

> 
> Thanks
> Max
> 
> 
>> 
>> -Alan

RFR 8170364: FilePermission path modified during merge

[Wang Weijun]

Please take a review at

   http://cr.openjdk.java.net/~weijun/8170364/webrev.00/

The compatibility layer introduced in the new FilePermission implementation 
requires one FilePermission to imply another with either a relative path or an 
absolute path. This is solved with a private field npath2 inside. When this 
field is set, the permission's name is modified a little (JDK-8168127) so that 
when adding to a FilePermissionCollection, it is not confused with one without 
the field. Unfortunately, this modified name is reused in the merge to create a 
new "merged" FilePermission which contains a malformed path now.

This fix creates a new non-public method to create this "merged" FilePermission.

I checked other places and seems the name is not used to create a new 
FilePermission.

Thanks
Max

Re: RFR 8169911: Enhanced tests for jarsigner -verbose -verify after JDK-8163304

[Wang Weijun]

Hi Amanda

- There are many duplicates in signWithAlias() and sign(). I think it's better 
to make one based on the other. Maybe signWithAliasAndTsa() and sign().

- checkWeak2() is about combinations of weak/strong algs in a single signer. 
I'd rather it be an individual test case, and not as a step in multiple 
signers. The name is not meaningful enough. Maybe checkHalfWeak()?

- About multiple signers:

  On lines 356 and 364, I suggest using different names for signed jar and 
original jar. This makes debugging easier when there is a failure.

  IMO it's not worth writing different checkComb() and checkComb2(). As long as 
the output contains "weak" we know the weak algorithm is also printed out. What 
we need to cover are:

   1) it's treated as verified if at least one is strong (you already covered 
it)

   2) it has only 1 verified signer, this should be shown with both -verbose 
and -certs. You can see there is only one certificate shown. Or you can use 
JarFile API to open the jar file and check the length of 
JarEntry::getCodeSigner().

   3) the history shows 2 signers, this can be provide by 2 "Signed by" lines.

  I'd suggest checkComb() be renamed to checkMultiple().

Thanks
Max

> On Nov 19, 2016, at 8:38 AM, Amanda Jiang  wrote:
> 
> HI All, 
> Please help to review following code change:
>   webrev:  http://cr.openjdk.java.net/~amjiang/8169911/webrev.01/
>   bug: https://bugs.openjdk.java.net/browse/JDK-8169911
> 
> This change enhances test case for JDK-8163304 by covering following cases:
>   - Multiple signers (signed by jarsigner twice using different aliases) 
>   - Combinations of weak/strong -digestalg, -tsadigestalg and -sigalg 
>   - Signing with DSA keys
> 
> Thanks,
> Amanda

Re: RFR: 8169335: Add a crypto.policy fallback in case Security Property 'crypto.policy' does not exist

[Wang Weijun]

> On Nov 17, 2016, at 9:33 AM, Bradford Wetmore  
> wrote:
> 
>try (PrintWriter out = new PrintWriter(FILENAME)) {
>Files.lines(path)
>.filter(x -> !x.trim().startsWith("crypto.policy"))
>.forEach(out::println);
>}

Not very sure. Although IMHO a terminate method like forEach *should* close the 
stream, the spec has not said so:

* Streams have a {@link #close()} method and implement {@link AutoCloseable}.
* Operating on a stream after it has been closed will throw {@link 
IllegalStateException}.
* Most stream instances do not actually need to be closed after use, as they
* are backed by collections, arrays, or generating functions, which require no
* special resource management. Generally, only streams whose source is an IO 
channel,
* such as those returned by {@link Files#lines(Path)}, will require closing. If 
a
* stream does require closing, it must be opened as a resource within a 
try-with-resources
* statement or similar control structure to ensure that it is closed promptly 
after its
* operations have completed.

I know you use Windows, so if you haven't seen any test failure, maybe it's 
safe.

Thanks
Max

Re: RFR: 8169335: Add a crypto.policy fallback in case Security Property 'crypto.policy' does not exist

[Wang Weijun]

> On Nov 17, 2016, at 6:10 AM, Bradford Wetmore <bradford.wetm...@oracle.com> 
> wrote:
> 
> 
> Current iteration:
> 
>http://cr.openjdk.java.net/~wetmore/8169335/webrev.01
> 
> Changes:
> 
> 1.  Using Debug "jca" instead of "policy"
> 
> 2.  Using debug.println (System.err), as the other jca calls are also using 
> it.

Still have a question:

Why must both (debug != null) and Debug.isOn("jca") be checked? If 
Debug.getInstance("jca") is not null, shouldn't Debug.isOn("jca") always be 
true?

> 
> 3.  Added regression test.  Strips out any crypto.policy entry to create a 
> new file, then uses it.

Looks fine, but I heard you can use some cool jdk8 classes like

   for (in = Files.lines(input); out = new PrintWriter(output)) {
  in.filter(x -> !x.contains("crypto.policy")).forEach(out::println);
   }

--Max


> 
> 4.  Updated webrev with bugid/reviewers.
> 
> Brad
> 
> 
> 
> 
> On 11/16/2016 6:21 AM, Seán Coffey wrote:
>> In the recent jdk8u-dev edits of this file for 8157561, we introduced a
>> debug field based on this key :
>> 
>> Debug.getInstance("jca", "Cipher");
>> 
>> Can we continue to use 'jca' to be consistent for people upgrading ?
>> 
>> for the testcase, I guess you can edit
>> test/javax/crypto/CryptoPermission/TestUnlimited.java but you'll have to
>> launch with a customized java.security file which doesn't have
>> crypto.policy set. (Security.setProperty doesn't allow null values)
>> 
>> Regards,
>> Sean.
>> 
>> On 16/11/16 00:40, Bradford Wetmore wrote:
>>> Never noticed that before!  We have NOT been consistent in whether we
>>> use:
>>> 
>>>System.out.println()
>>> or
>>>debug.println()
>>> 
>>> I knew SeanC wants to rework the JCA/JCE/Security debugging output in
>>> another project, so I will remove the prefix for now.  Thanks for
>>> catching it.
>>> 
>>> I will also add a simple regression Test before I push.  In hindsight,
>>> it's not as trivial a change as I initially thought.  If you want to
>>> review it, I can wait until you are back tomorrow.
>>> 
>>> Brad
>>> 
>>> 
>>> On 11/15/2016 4:12 PM, Wang Weijun wrote:
>>>> You create a debug field with a prefix string and then check both
>>>> debug != null and Debug.isOn("policy") and then use
>>>> System.out.println to print the message. Something must be useless.
>>>> 
>>>> --Max
>>>> 
>>>>> On Nov 16, 2016, at 3:31 AM, Bradford Wetmore
>>>>> <bradford.wetm...@oracle.com> wrote:
>>>>> 
>>>>> Simple codereview:
>>>>> 
>>>>>   http://cr.openjdk.java.net/~wetmore/8169335/webrev.00
>>>>> 
>>>>> The "crypto.policy" Security property is normally defined/configured
>>>>> in the java.security file at build time.  (e.g. "limited" or
>>>>> "unlimited") Rather than currently failing catastrophically if this
>>>>> value doesn't exist, there should be a sensible default if it is
>>>>> undeclared for whatever reason.  We will use a sane fallback value
>>>>> of "limited".
>>>>> 
>>>>> If the distribution has also removed the "limited" policy directory
>>>>> then the VM will still fail to initialize, but we have at least made
>>>>> an effort to recover.
>>>>> 
>>>>> Thanks,
>>>>> 
>>>>> Brad
>>>>> 
>>>> 
>> 

Re: RFR 8043252: Debug of access control is obfuscated - NullPointerException in ProtectionDomain

[Wang Weijun]

> On Nov 16, 2016, at 12:40 PM, Jamil Nimeh <jamil.j.ni...@oracle.com> wrote:
> 
> Oops, good catch. I'll fix that.  I assume you don't need a third review for 
> that?

No. Just push your change.

--Max

> 
> 
> 
> --Jamil
> 
> ---- Original message 
> From: Wang Weijun <weijun.w...@oracle.com>
> Date: 11/15/16 7:48 PM (GMT-08:00)
> To: Jamil Nimeh <jamil.j.ni...@oracle.com>
> Cc: security-dev@openjdk.java.net
> Subject: Re: RFR 8043252: Debug of access control is obfuscated - 
> NullPointerException in ProtectionDomain
> 
> Looks fine.
> 
> Small nit: new test has copyright 2003, 2016.
> 
> --Max
> 
> > On Nov 16, 2016, at 11:45 AM, Jamil Nimeh <jamil.j.ni...@oracle.com> wrote:
> > 
> > Good suggestions, thanks.
> > 
> > Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8043252/webrev.02
> > 
> > --Jamil
> > 
> > 
> > On 11/15/2016 06:18 PM, Wang Weijun wrote:
> >> 41 System.setSecurityManager(new SecurityManager());
> >> 
> >> This is strange. I suppose you only want to trigger a permission check?
> >> 
> >> If so, just change it to Policy.getPolicy(), and you can clean up the 
> >> policy file a little.
> >> 
> >> Thanks
> >> Max
> >> 
> >>> On Nov 16, 2016, at 8:36 AM, Jamil Nimeh <jamil.j.ni...@oracle.com> wrote:
> >>> 
> >>> Hello all,
> >>> 
> >>> This fixes an issue in ProtectionDomain, where Permission classes that 
> >>> take a loose interpretation of the getActions() method and return null 
> >>> cause an NPE to be thrown when a ProtectionDomain's toString method is 
> >>> called (during debugging for instance).
> >>> 
> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8043252
> >>> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8043252/webrev.01/
> >>> 
> >>> Thanks,
> >>> --Jamil
> > 
> 

Re: RFR 8043252: Debug of access control is obfuscated - NullPointerException in ProtectionDomain

[Wang Weijun]

Looks fine.

Small nit: new test has copyright 2003, 2016.

--Max

> On Nov 16, 2016, at 11:45 AM, Jamil Nimeh <jamil.j.ni...@oracle.com> wrote:
> 
> Good suggestions, thanks.
> 
> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8043252/webrev.02
> 
> --Jamil
> 
> 
> On 11/15/2016 06:18 PM, Wang Weijun wrote:
>> 41 System.setSecurityManager(new SecurityManager());
>> 
>> This is strange. I suppose you only want to trigger a permission check?
>> 
>> If so, just change it to Policy.getPolicy(), and you can clean up the policy 
>> file a little.
>> 
>> Thanks
>> Max
>> 
>>> On Nov 16, 2016, at 8:36 AM, Jamil Nimeh <jamil.j.ni...@oracle.com> wrote:
>>> 
>>> Hello all,
>>> 
>>> This fixes an issue in ProtectionDomain, where Permission classes that take 
>>> a loose interpretation of the getActions() method and return null cause an 
>>> NPE to be thrown when a ProtectionDomain's toString method is called 
>>> (during debugging for instance).
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8043252
>>> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8043252/webrev.01/
>>> 
>>> Thanks,
>>> --Jamil
> 

Re: RFR 8169751: sun/security/krb5/auto/rcache_usemd5.sh fails on solaris

[Wang Weijun]

> On Nov 16, 2016, at 11:30 AM, Xuelei Fan <xuelei@oracle.com> wrote:
> 
> Looks like a little bit hack, but fine to me.

Thanks.

> 
> BTW, is it more convenient to have 2 run tags in ReplayCacheTestProc.java?

I'd like to run ReplayCacheTestProc.java manually (still using jtreg) with 
various test.* properties. Having 2 run tags double the time and not what I 
wanted.

--Max

> 
> Xuelei
> 
> On 11/16/2016 11:23 AM, Wang Weijun wrote:
>> Sorry, the webrev here http://cr.openjdk.java.net/~weijun/8169751/webrev.00/
>> 
>> --Max
>> 
>>> On Nov 16, 2016, at 10:40 AM, Wang Weijun <weijun.w...@oracle.com> wrote:
>>> 
>>> Please take a review at
>>> 
>>>  8169751: sun/security/krb5/auto/rcache_usemd5.sh fails on solaris
>>> 
>>> Different service names are used in the 2 tests (rcache_usemd5.sh and 
>>> ReplayCacheTestProc.java) now.
>>> 
>>> Thanks
>>> Max
>>> 
>> 

Re: RFR 8169751: sun/security/krb5/auto/rcache_usemd5.sh fails on solaris

[Wang Weijun]

Sorry, the webrev here http://cr.openjdk.java.net/~weijun/8169751/webrev.00/

--Max

> On Nov 16, 2016, at 10:40 AM, Wang Weijun <weijun.w...@oracle.com> wrote:
> 
> Please take a review at
> 
>   8169751: sun/security/krb5/auto/rcache_usemd5.sh fails on solaris
> 
> Different service names are used in the 2 tests (rcache_usemd5.sh and 
> ReplayCacheTestProc.java) now.
> 
> Thanks
> Max
> 

Re: RFR 7004967: SecureRandom should be more explicit about threading

[Wang Weijun]

Please review the updated webrev at

   http://cr.openjdk.java.net/~weijun/7004967/webrev.02/

Only spec change [1].

This change also covers 8169312.

Thanks
Max

[1] 
http://cr.openjdk.java.net/~weijun/7004967/webrev.02/interdiff.patch.html


On 11/4/2016 10:54 PM, Sean Mullan wrote:

* SecureRandom

 131  * If this attribute is not set or is "false", this class will instead
 132  * synchronize access to each of the methods of the {@code
SecureRandomSpi}
 133  * implementation.

Not all of the methods are synchronized - engineGetParameters is not,
for example. I think to avoid ambiguity, you should list the names of
the methods that are synchronized.

810  * @throws IllegalArgumentException if {@code numBytes} is negative

Since this is a new @throws, you really need to file a new bug.

Please also file a docs bug with a description of the new attribute.

* SecureRandomSpi

lines 63-83, I think the wording could be improved/simplified, how about:

See {@link SecureRandom} for additional details on thread safety. By
default, a SecureRandomSpi implementation is considered to be not safe
for use by multiple concurrent threads and SecureRandom will synchronize
access to each of the applicable engine methods (see SecureRandom for
the list of methods). However, if a SecureRandomSpi implementation is
thread-safe, the service
provider attribute "ThreadSafe" should be set to "true" during its
registration, as follows:

  put("SecureRandom.AlgName ThreadSafe", "true");

  or

  putService(new Service(this, "SecureRandom", "AlgName", className,
 null, Map.of("ThreadSafe", "true")));

{@code SecureRandom} will then call the applicable engine methods
without any synchronization.

--Sean

On 11/2/16 3:27 AM, Wang Weijun wrote:

Ping again.

There is an updated version at
http://cr.openjdk.java.net/~weijun/7004967/webrev.01/ with doc-only
changes.

Thanks
Max


On Aug 25, 2016, at 10:00 AM, Weijun Wang <weijun.w...@oracle.com>
wrote:

Please review the enhancement at

 http://cr.openjdk.java.net/~weijun/7004967/webrev.00/

Basically, we want SecureRandom to be more efficient by removing all
synchronized keywords from its public methods and let an
implementation to take care of thread-safety (We already did some in
JDK-8098581). On the other hand, we need to make sure that existing
implementations that have not synchronized correctly to behave just
as good as before.

Therefore a new Service Attribute "ThreadSafe" is introduced. If you
think your implementation is already thread-safe, set it to "true"
and SecureRandom will be happy. Otherwise, don't set it and
SecureRandom will continuously call your SecureRandomSpi engine
methods in synchronized blocks.

Thanks
Max

RFR 8169751: sun/security/krb5/auto/rcache_usemd5.sh fails on solaris

[Wang Weijun]

Please take a review at

   8169751: sun/security/krb5/auto/rcache_usemd5.sh fails on solaris

Different service names are used in the 2 tests (rcache_usemd5.sh and 
ReplayCacheTestProc.java) now.

Thanks
Max

Re: RFR 8043252: Debug of access control is obfuscated - NullPointerException in ProtectionDomain

[Wang Weijun]

41 System.setSecurityManager(new SecurityManager());

This is strange. I suppose you only want to trigger a permission check?

If so, just change it to Policy.getPolicy(), and you can clean up the policy 
file a little.

Thanks
Max

> On Nov 16, 2016, at 8:36 AM, Jamil Nimeh  wrote:
> 
> Hello all,
> 
> This fixes an issue in ProtectionDomain, where Permission classes that take a 
> loose interpretation of the getActions() method and return null cause an NPE 
> to be thrown when a ProtectionDomain's toString method is called (during 
> debugging for instance).
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8043252
> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8043252/webrev.01/
> 
> Thanks,
> --Jamil

Re: RFR: 8169335: Add a crypto.policy fallback in case Security Property 'crypto.policy' does not exist

[Wang Weijun]

You create a debug field with a prefix string and then check both debug != null 
and Debug.isOn("policy") and then use System.out.println to print the message. 
Something must be useless.

--Max

> On Nov 16, 2016, at 3:31 AM, Bradford Wetmore  
> wrote:
> 
> Simple codereview:
> 
>http://cr.openjdk.java.net/~wetmore/8169335/webrev.00
> 
> The "crypto.policy" Security property is normally defined/configured in the 
> java.security file at build time.  (e.g. "limited" or "unlimited") Rather 
> than currently failing catastrophically if this value doesn't exist, there 
> should be a sensible default if it is undeclared for whatever reason.  We 
> will use a sane fallback value of "limited".
> 
> If the distribution has also removed the "limited" policy directory then the 
> VM will still fail to initialize, but we have at least made an effort to 
> recover.
> 
> Thanks,
> 
> Brad
> 

Re: RFR 7004967: SecureRandom should be more explicit about threading

[Wang Weijun]

> On Nov 9, 2016, at 5:06 AM, Sean Mullan  wrote:
> 
>> In fact, can we
>> deprecate the getSeed() method? It's not unsafe so we don't need to give
>> it a forRemoval value.
> 
> Sounds reasonable. I would file a bug, but I don't think it is critical for 9 
> - we can do it in 10.

https://bugs.openjdk.java.net/browse/JDK-8169437

--Max

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Wang Weijun]

Everything looks fine now.

--Max

> On Nov 8, 2016, at 11:09 AM, Artem Smotrakov  
> wrote:
> 
> Here is final version (I hope)
> 
> http://cr.openjdk.java.net/~asmotrak/8168882/webrev.04/
> 
> Artem

Re: RFR 7004967: SecureRandom should be more explicit about threading

[Wang Weijun]

Accepted.

Please review http://ccc.us.oracle.com/8169312. In fact, can we 
deprecate the getSeed() method? It's not unsafe so we don't need to give 
it a forRemoval value.


Thanks
Max

On 11/4/2016 10:54 PM, Sean Mullan wrote:

* SecureRandom

 131  * If this attribute is not set or is "false", this class will instead
 132  * synchronize access to each of the methods of the {@code
SecureRandomSpi}
 133  * implementation.

Not all of the methods are synchronized - engineGetParameters is not,
for example. I think to avoid ambiguity, you should list the names of
the methods that are synchronized.

810  * @throws IllegalArgumentException if {@code numBytes} is negative

Since this is a new @throws, you really need to file a new CCC (or
withdraw and amend the current one with this @throws).

Please also file a docs bug with a description of the new attribute.

* SecureRandomSpi

lines 63-83, I think the wording could be improved/simplified, how about:

See {@link SecureRandom} for additional details on thread safety. By
default, a SecureRandomSpi implementation is considered to be not safe
for use by multiple concurrent threads and SecureRandom will synchronize
access to each of the applicable engine methods (see SecureRandom for
the list of methods). However, if a SecureRandomSpi implementation is
thread-safe, the service
provider attribute "ThreadSafe" should be set to "true" during its
registration, as follows:

  put("SecureRandom.AlgName ThreadSafe", "true");

  or

  putService(new Service(this, "SecureRandom", "AlgName", className,
 null, Map.of("ThreadSafe", "true")));

{@code SecureRandom} will then call the applicable engine methods
without any synchronization.

--Sean

On 11/2/16 3:27 AM, Wang Weijun wrote:

Ping again.

There is an updated version at
http://cr.openjdk.java.net/~weijun/7004967/webrev.01/ with doc-only
changes.

Thanks
Max


On Aug 25, 2016, at 10:00 AM, Weijun Wang <weijun.w...@oracle.com>
wrote:

Please review the enhancement at

 http://cr.openjdk.java.net/~weijun/7004967/webrev.00/

Basically, we want SecureRandom to be more efficient by removing all
synchronized keywords from its public methods and let an
implementation to take care of thread-safety (We already did some in
JDK-8098581). On the other hand, we need to make sure that existing
implementations that have not synchronized correctly to behave just
as good as before.

Therefore a new Service Attribute "ThreadSafe" is introduced. If you
think your implementation is already thread-safe, set it to "true"
and SecureRandom will be happy. Otherwise, don't set it and
SecureRandom will continuously call your SecureRandomSpi engine
methods in synchronized blocks.

Thanks
Max

Re: Windows LSA cache and Kerberos delegation

[Wang Weijun]

> On Oct 13, 2016, at 3:38 AM, Venkat Iyer (veniyer)  wrote:
> 
> Do you know if the problem of obtaining TGT session key on Windows from LSA 
> credential cache is resolved (see snippet below)?

I am not sure. Sometimes you just cannot get the session key.

One way to check this is to run Microsoft's own klist.exe tool. If "klist tgt" 
shows a non-zero session key, then Java can also get it. Otherwise, we don't 
have a good solution now.

Hope this helps.

--Weijun

> Microsoft has not been able to provide any guidance on steps to make it work. 
> These steps below haven’t helped:
>   • Turn off UAC
>   • Remove Service Account for application from local administrators group
>   • Set the registry key “allowtgtsessionkey” to true
> We need delegation support through the application to underlying Kerberized 
> systems.
> 
> This reference was very useful to troubleshoot the issues. 
> http://cr.openjdk.java.net/~weijun/special/krb5winguide-2/raw_files/new/kwin
> Known Issues
> 
>If an AD account is also added into local administrator group on the
>client PC, Microsoft restricts such client from getting the session key
>for tickets (even if you set the allowtgtsessionkey registry key to 1).
>The workaround is: Just forget you're a logged in user, call kinit.exe.
>Do not depends on LSA credential cache.
> 
>In a recent hotfix ([35]http://support.microsoft.com/kb/942219/en-us,
>should be included in Vista SP1), this restriction is lifted for normal
>service tickets. However, it still applies to TGT. Since Java uses TGT
>to acquire tickets for other services (the standard Kerberos process),
>this update provides no benefit to JGSS programming on Windows.
>Furthermore, even if the implementation of Java is changed to read
>service tickets from the LSA cache, it still cannot perform delegation,
>since a TGT is always needed in that case.
> 
> 
> Appreciate any help.
> Thanks
> Venkat 

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Wang Weijun]

Everything is fine now.

Thanks
Max

On 11/3/2016 9:38 AM, Artem Smotrakov wrote:

My bad, I missed that.

http://cr.openjdk.java.net/~asmotrak/8168882/webrev.02/

Artem


On 11/02/2016 06:30 PM, Wang Weijun wrote:

On 11/01/2016 11:59 PM, Wang Weijun wrote:

>> Main.java:
>>
>> The warning (and the subsequent empty line) should be printed into 
System.err.

This one?

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Wang Weijun]

> On Nov 3, 2016, at 8:27 AM, Artem Smotrakov <artem.smotra...@oracle.com> 
> wrote:
> 
> Hi Max,
> 
> Please see inline.
> 
> 
> On 11/01/2016 11:59 PM, Wang Weijun wrote:
>> Main.java:
>> 
>> The warning (and the subsequent empty line) should be printed into 
>> System.err.

This one?

> 
> Thank you for review Max, please take a look at updated webrev:
> 
> http://cr.openjdk.java.net/~asmotrak/8168882/webrev.01/

Fine except for the System.out one above.

> 
> By the way, I just noticed that we have another version of JarUtils.java 
> which was added by Alan 7 month ago
> 
> http://hg.openjdk.java.net/jdk9/dev/jdk/log/1396fb6d0279/test/lib/testlibrary/JarUtils.java

Looks like it's only used by himself, and has 2 lines of history.

Also, my understanding is that we are switching to test utils in the root repo. 
A lot of same name utils in jdk/test were deprecated.

Thanks
Max

> 
> Artem

Re: RFR 7004967: SecureRandom should be more explicit about threading

[Wang Weijun]

> 在 2016年11月2日，21:50，Xuelei Fan  写道：
> 
> I may change "the service provider attribute" to "the service attribute".

I didn't invent that name, I remember it's named exactly like this in one of 
the security guides. Will check tomorrow, not near a big screen now. 

Thanks
Max

Re: RFR 7004967: SecureRandom should be more explicit about threading

[Wang Weijun]

> On Nov 2, 2016, at 9:18 PM, Xuelei Fan <xuelei@oracle.com> wrote:
> 
> On 11/2/2016 8:47 PM, Wang Weijun wrote:
>> 
>>> On Nov 2, 2016, at 5:34 PM, Xuelei Fan <xuelei@oracle.com> wrote:
>>> 
>>> 1.  More specific
>>> 
>>>  "A SecureRandom service provider can advertise that it is
>>>   thread-safe by setting the service provider attribute
>>>   "ThreadSafe" to "true" when registering the provider."
>>> 
>>> A service provider may contains many services implementations.  May need to 
>>> be more specific to set "ThreadSafe" for SecureRandom only, rather the full 
>>> provider is thread safe.  For example:
>>> 
>>>   map.put("SecureRandom.SHA1PRNG ThreadSafe", "true");
>>> 
>>> Otherwise, a service provider need to make sure all services are thread 
>>> safe, or all services implementation are not thread safe.
>> 
>> How about changing "A SecureRandom service provider" to "A SecureRandom 
>> implementation"?
>> 
> I don't think it is sufficient.  See bellow.
> 
>>> 
>>> 2. "true" is the only true property value.
>>>   "If this attribute is not set or is "false", this class will
>>>instead ..."
>>> 
>>> If the attribute is set to "yes" or "hello, world!", I think it is the same 
>>> as set to "false" per your current implementation.
>>> 
>>>   "Otherwise, this class will ... "
>> 
>> OK.
>> 
>>> 
>>> May need to update the implementation accordingly if you accept the 
>>> comments.
>> 
>> Why update the implementation?
>> 
> If I'm reading correct, you are setting the "ThreadSafe" for provider, but 
> not for SecureRandom implementation.

Sorry but I don't understand what you mean.

On read side, the threadSafe field is for each "SecureRandom." + algorithm, so 
it is for an implementation.

On write side, every call looks like the line you write below.

>  I may use the property similar to:
> 
>   map.put("SecureRandom.SHA1PRNG ThreadSafe", "true");
> 
> That's why I don't think above update is not sufficient.

Can you point out on exactly which line in webrev I'm doing wrong?

Thanks
Max


> 
> Xuelei
> 
>> Thanks
>> Max
>> 
>>> 
>>> Xuelei
>>> 
>>> 
>>> On 11/2/2016 3:27 PM, Wang Weijun wrote:
>>>> Ping again.
>>>> 
>>>> There is an updated version at 
>>>> http://cr.openjdk.java.net/~weijun/7004967/webrev.01/ with doc-only 
>>>> changes.
>>>> 
>>>> Thanks
>>>> Max
>>>> 
>>>>> On Aug 25, 2016, at 10:00 AM, Weijun Wang <weijun.w...@oracle.com> wrote:
>>>>> 
>>>>> Please review the enhancement at
>>>>> 
>>>>> http://cr.openjdk.java.net/~weijun/7004967/webrev.00/
>>>>> 
>>>>> Basically, we want SecureRandom to be more efficient by removing all 
>>>>> synchronized keywords from its public methods and let an implementation 
>>>>> to take care of thread-safety (We already did some in JDK-8098581). On 
>>>>> the other hand, we need to make sure that existing implementations that 
>>>>> have not synchronized correctly to behave just as good as before.
>>>>> 
>>>>> Therefore a new Service Attribute "ThreadSafe" is introduced. If you 
>>>>> think your implementation is already thread-safe, set it to "true" and 
>>>>> SecureRandom will be happy. Otherwise, don't set it and SecureRandom will 
>>>>> continuously call your SecureRandomSpi engine methods in synchronized 
>>>>> blocks.
>>>>> 
>>>>> Thanks
>>>>> Max

Re: RFR 7004967: SecureRandom should be more explicit about threading

[Wang Weijun]

> On Nov 2, 2016, at 5:34 PM, Xuelei Fan <xuelei@oracle.com> wrote:
> 
> 1.  More specific
> 
>   "A SecureRandom service provider can advertise that it is
>thread-safe by setting the service provider attribute
>"ThreadSafe" to "true" when registering the provider."
> 
> A service provider may contains many services implementations.  May need to 
> be more specific to set "ThreadSafe" for SecureRandom only, rather the full 
> provider is thread safe.  For example:
> 
>map.put("SecureRandom.SHA1PRNG ThreadSafe", "true");
> 
> Otherwise, a service provider need to make sure all services are thread safe, 
> or all services implementation are not thread safe.

How about changing "A SecureRandom service provider" to "A SecureRandom 
implementation"?

> 
> 2. "true" is the only true property value.
>"If this attribute is not set or is "false", this class will
> instead ..."
> 
> If the attribute is set to "yes" or "hello, world!", I think it is the same 
> as set to "false" per your current implementation.
> 
>"Otherwise, this class will ... "

OK.

> 
> May need to update the implementation accordingly if you accept the comments.

Why update the implementation?

Thanks
Max

> 
> Xuelei
> 
> 
> On 11/2/2016 3:27 PM, Wang Weijun wrote:
>> Ping again.
>> 
>> There is an updated version at 
>> http://cr.openjdk.java.net/~weijun/7004967/webrev.01/ with doc-only changes.
>> 
>> Thanks
>> Max
>> 
>>> On Aug 25, 2016, at 10:00 AM, Weijun Wang <weijun.w...@oracle.com> wrote:
>>> 
>>> Please review the enhancement at
>>> 
>>> http://cr.openjdk.java.net/~weijun/7004967/webrev.00/
>>> 
>>> Basically, we want SecureRandom to be more efficient by removing all 
>>> synchronized keywords from its public methods and let an implementation to 
>>> take care of thread-safety (We already did some in JDK-8098581). On the 
>>> other hand, we need to make sure that existing implementations that have 
>>> not synchronized correctly to behave just as good as before.
>>> 
>>> Therefore a new Service Attribute "ThreadSafe" is introduced. If you think 
>>> your implementation is already thread-safe, set it to "true" and 
>>> SecureRandom will be happy. Otherwise, don't set it and SecureRandom will 
>>> continuously call your SecureRandomSpi engine methods in synchronized 
>>> blocks.
>>> 
>>> Thanks
>>> Max
>> 

Re: RFR 7004967: SecureRandom should be more explicit about threading

[Wang Weijun]

Ping again.

There is an updated version at 
http://cr.openjdk.java.net/~weijun/7004967/webrev.01/ with doc-only changes.

Thanks
Max

> On Aug 25, 2016, at 10:00 AM, Weijun Wang  wrote:
> 
> Please review the enhancement at
> 
>  http://cr.openjdk.java.net/~weijun/7004967/webrev.00/
> 
> Basically, we want SecureRandom to be more efficient by removing all 
> synchronized keywords from its public methods and let an implementation to 
> take care of thread-safety (We already did some in JDK-8098581). On the other 
> hand, we need to make sure that existing implementations that have not 
> synchronized correctly to behave just as good as before.
> 
> Therefore a new Service Attribute "ThreadSafe" is introduced. If you think 
> your implementation is already thread-safe, set it to "true" and SecureRandom 
> will be happy. Otherwise, don't set it and SecureRandom will continuously 
> call your SecureRandomSpi engine methods in synchronized blocks.
> 
> Thanks
> Max

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Wang Weijun]

Main.java:

The warning (and the subsequent empty line) should be printed into System.err.

Resources.java:

"This tool accepts any algorithm" is a little confusing (sorry that I 
originally suggested it). Maybe "This tool does not attempt to verify a signed 
jar file, please run \"jarsigner -verify\" if you want to."

Also, ever since the 1st time hard coded strings are changed into dot-connected 
resource keys, newly added keys do not necessarily use the exact same string. 
Make it simple so next time if the value needs to be updated you don't need to 
change the key.

Test:

- You can also add -Duser.language=en and -Duser.country=US to keytool.

- With my recent update to JarUtils.createJar(), there is no need to create the 
"test" file.

Everything else looks fine.

Thanks
Max


> On Nov 2, 2016, at 7:35 AM, Artem Smotrakov  
> wrote:
> 
> Hello,
> 
> Please review this small update for keytool.
> 
> "keytool -printcert -jarfile" doesn't work with jars which were signed with 
> algorithms listed in "jdk.jar.disabledAlgorithms" security property.
> 
> The patch below resets "jdk.jar.disabledAlgorithms" security property before 
> reading a jar file, and prints a warning.
> 
> I also re-wrote readjar.sh test, and added SecurityTools class with a couple 
> of re-usable methods for jarsigner and keytool (those methods are based on 
> methods from TimestampCheck.java).
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8168882
> Webrev: http://cr.openjdk.java.net/~asmotrak/8168882/webrev.00/
> 
> Artem

Re: RFR 8168518: rcache interop with krb5-1.15

[Wang Weijun]

Webrev updated at

   http://cr.openjdk.java.net/~weijun/8168518/webrev.02/

The only change in src/ is the system property name from 
"jdk.krb5.rcache.usemd5" to "jdk.krb5.rcache.useMD5".

Test enhanced, now I can launch the test like

jtreg -javacoption:-XDignore.symbol.file \
   -javacoption:-source -javacoption:8 \
   -javacoption:-target -javacoption:8 \
   
-Dtest.libs=J,N,J9=$J9/bin/java,J8=$J8/bin/java,N14=/krb5-1.14.4/build/lib/libgssapi_krb5.dylib,N15=/krb5-1.15-beta1/build/lib/libgssapi_krb5.dylib
 \
   -Dtest.runs \
   test/sun/security/krb5/auto/ReplayCacheTestProc.java

to test interop between all possibilities.

I need to stop spending more time on this one.

Thanks
Max

Re: Code review request, JDK-8168822, Document that algorithm restrictions do not apply to trusted certs

[Wang Weijun]

One question: I thought for TLS, you check twice. First using 
jdk.tls.disabledAlgorithms on cipher suites etc, and second using 
jdk.certpath.disabledAlgorithms on certificates. Why is 
jdk.tls.disabledAlgorithms applied to cert at all?


Thanks
Max

On 10/27/2016 8:30 AM, Wang Weijun wrote:

I don't think this applies to jdk.jar.disabledAlgorithms. While the
private key algorithm and key size are determined by the certificate, I
think they are always checked even if the end-entity cert is trusted
(For example, a trusted self-signed cert).

Thanks
Max

On 10/27/2016 8:04 AM, Xuelei Fan wrote:

Hi,

Please review the simple fix:

http://cr.openjdk.java.net/~xuelei/8168822/webrev/

Algorithm restrictions do not apply to trusted certs as the
application or customer has made the decision to trust the "trusted
cert".  However, this point is not explicit for general developers and
users.  We'd better to clarify this point explicitly.

In the update, I add a short note for each algorithm constraint security
properties:

   Note: Algorithm restrictions do not apply to trusted certificates.

Doc only update, no new regression test.

Thanks,
Xuelei

Re: Code review request, JDK-8168822, Document that algorithm restrictions do not apply to trusted certs

[Wang Weijun]

I don't think this applies to jdk.jar.disabledAlgorithms. While the 
private key algorithm and key size are determined by the certificate, I 
think they are always checked even if the end-entity cert is trusted 
(For example, a trusted self-signed cert).


Thanks
Max

On 10/27/2016 8:04 AM, Xuelei Fan wrote:

Hi,

Please review the simple fix:

http://cr.openjdk.java.net/~xuelei/8168822/webrev/

Algorithm restrictions do not apply to trusted certs as the
application or customer has made the decision to trust the "trusted
cert".  However, this point is not explicit for general developers and
users.  We'd better to clarify this point explicitly.

In the update, I add a short note for each algorithm constraint security
properties:

   Note: Algorithm restrictions do not apply to trusted certificates.

Doc only update, no new regression test.

Thanks,
Xuelei

Re: RFR 8168518: rcache interop with krb5-1.15

[Wang Weijun]

> On Oct 25, 2016, at 4:53 PM, Xuelei Fan <xuelei@oracle.com> wrote:
> 
> >>> Is the "HASH" algorithm a new name?  I'm not sure why introduce this
> >>> none-standard name.
> >>
> >> HASH is actually MD5, it is the label used by MIT krb5 in its
> >> previous releases. In 1.15, it's SHA256. I just want it to the
> >> same with MIT krb5.
> I see.  As the HASH and SHA256 was converted to MD5 and SHA-256 if using 
> (with realAlg()),  and the names are never expose to external, it may be not 
> necessary to use the intermediate variables and realAlg() method.
> 
>   if (the system property is true) {
>  // make a note, it is the same name as HASH in MIT world.
>  set the alg to MD5
>   } else {
>  // make a note, it is the same name as SHA256 in MIT world.
>  set the alg to SHA-56.
>   }

I have to do translation between arg in MessageDigest.getInstance(arg) and the 
label inside the rcache file, and store a field name inside AuthTimeWithHash. 
Do you mean a boolean is enough? Although the value is not fully customizable 
now I still want it to a string instead of boolean so we can be free to change 
later.

> 
> Personally, I'd like to use "useMD5" as the ending part of the system 
> property.

OK.

> 
> Otherwise, the source code looks fine to me.
> 
> For the release note, it compatibility impact on JDK 9/8 are mentioned. What 
> about JDK 7/6?  I guess not.  Would you mind mention the impact on JDK 7/6 
> explicitly?

OK, I remember hash was introduced in jdk7, will double check.

> 
> BTW, can the rcache file can be refreshed to use SHA-256 if MD5 was used 
> previously?

You mean inside the same file? Newly added entries will be SHA-256 and the old 
ones remains. Still interoperable but just ignored.

Thanks
Max

> 
> Thanks,
> Xuelei
> 
> On 10/25/2016 3:36 PM, Wang Weijun wrote:
>> Hi Xuelei
>> 
>> I rethink about it. Here is the updated webrev:
>> 
>>   http://cr.openjdk.java.net/~weijun/8168518/webrev.01/
>> 
>> Changes from webrev.00:
>> 
>> 1. Default value is now SHA256. This is also the same default for latest MIT 
>> krb5.
>> 
>> 2. User can only revert it to HASH with a boolean system property. No more 
>> free setting.
>> 
>> 3. Test fixed. I forgot to pass the system property to child process.
>> 
>> Please also review the release note at 
>> https://bugs.openjdk.java.net/browse/JDK-8168635.
>> 
>> Thanks
>> Max
>> 
>>> On Oct 25, 2016, at 1:46 PM, Wang Weijun <weijun.w...@oracle.com> wrote:
>>> 
>>> 
>>> 
>>> On 10/25/2016 1:40 PM, Xuelei Fan wrote:
>>>> Is the "HASH" algorithm a new name?  I'm not sure why introduce this
>>>> none-standard name.
>>> 
>>> HASH is actually MD5, it is the label used by MIT krb5 in its previous 
>>> releases. In 1.15, it's SHA256. I just want it to the same with MIT krb5.
>>> 
>>>> 39 // The hash algorithm can be "HASH" or "SHA256".
>>>> What if the algorithm is not one of the two?  Why not use the standard
>>>> names?  Do you want to support SHA3?
>>> 
>>> No.
>>> 
>>> The system property is not meant to be fully customizable. The only usage 
>>> is that you can set it to SHA256 if you want full krb5-1.15 
>>> interoperability. I mainly created it so I can test on it.
>>> 
>>>> 
>>>> As the DEFAULT_HASH_ALG system property is customizable, the value may
>>>> be not valid.  Therefore, the AssertionError may be too strong to use in
>>>> line 310 of KrbApReq.java, and the message can have more information.
>>> 
>>> OK, I'll change the message.
>>> 
>>> 
>>>> 
>>>> Otherwise, looks fine to me.
>>>> 
>>>> Xuelei
>>>> 
>>>> On 10/25/2016 12:18 PM, Wang Weijun wrote:
>>>>> http://cr.openjdk.java.net/~weijun/8168518/webrev.00/
>>>>> 
>>>>> Please read https://bugs.openjdk.java.net/browse/JDK-8168518 for the
>>>>> reason. This code change includes:
>>>>> 
>>>>> 1. Add a hashAlg field in AuthTimeWithHash.java.
>>>>> 
>>>>> 2. Add AuthTimeWithHash.DEFAULT_HASH_ALG so we can change it later.
>>>>> 
>>>>> 3. The fix of the bug is inside DflCache.java:
>>>>> 
>>>>> @@ -300,7 +302,7 @@
>>>>> 
>>>>>  if (time.equals(a)) {
>>>>>  // E

Re: RFR 8168518: rcache interop with krb5-1.15

[Wang Weijun]

Hi Xuelei

I rethink about it. Here is the updated webrev:

   http://cr.openjdk.java.net/~weijun/8168518/webrev.01/

Changes from webrev.00:

1. Default value is now SHA256. This is also the same default for latest MIT 
krb5.

2. User can only revert it to HASH with a boolean system property. No more free 
setting.

3. Test fixed. I forgot to pass the system property to child process.

Please also review the release note at 
https://bugs.openjdk.java.net/browse/JDK-8168635.

Thanks
Max

> On Oct 25, 2016, at 1:46 PM, Wang Weijun <weijun.w...@oracle.com> wrote:
> 
> 
> 
> On 10/25/2016 1:40 PM, Xuelei Fan wrote:
>> Is the "HASH" algorithm a new name?  I'm not sure why introduce this
>> none-standard name.
> 
> HASH is actually MD5, it is the label used by MIT krb5 in its previous 
> releases. In 1.15, it's SHA256. I just want it to the same with MIT krb5.
> 
>>  39 // The hash algorithm can be "HASH" or "SHA256".
>> What if the algorithm is not one of the two?  Why not use the standard
>> names?  Do you want to support SHA3?
> 
> No.
> 
> The system property is not meant to be fully customizable. The only usage is 
> that you can set it to SHA256 if you want full krb5-1.15 interoperability. I 
> mainly created it so I can test on it.
> 
>> 
>> As the DEFAULT_HASH_ALG system property is customizable, the value may
>> be not valid.  Therefore, the AssertionError may be too strong to use in
>> line 310 of KrbApReq.java, and the message can have more information.
> 
> OK, I'll change the message.
> 
> 
>> 
>> Otherwise, looks fine to me.
>> 
>> Xuelei
>> 
>> On 10/25/2016 12:18 PM, Wang Weijun wrote:
>>> http://cr.openjdk.java.net/~weijun/8168518/webrev.00/
>>> 
>>> Please read https://bugs.openjdk.java.net/browse/JDK-8168518 for the
>>> reason. This code change includes:
>>> 
>>> 1. Add a hashAlg field in AuthTimeWithHash.java.
>>> 
>>> 2. Add AuthTimeWithHash.DEFAULT_HASH_ALG so we can change it later.
>>> 
>>> 3. The fix of the bug is inside DflCache.java:
>>> 
>>> @@ -300,7 +302,7 @@
>>> 
>>>   if (time.equals(a)) {
>>>   // Exact match, must be a replay
>>>   throw new KrbApErrException(Krb5.KRB_AP_ERR_REPEAT);
>>> 
>>> -  } else if (time.isSameIgnoresHash(a)) {
>>> +  } else if (time.sameTimeDiffHash((AuthTimeWithHash)a)) {
>>> 
>>>  // Two different authenticators in the same second.
>>>  // Remember it
>>>  seeNewButNotSame = true;
>>> 
>>> When a AuthTimeWithHash is seen with a different hash, we believe it's
>>> a new one. Before this fix, we simply compare the HASH string. Now
>>> that the algorithm can be different, we only treat it a new one if the
>>> algorithm is the same and the hash value is different.
>>> 
>>> This code change has not tried to understand what a different hashAlg
>>> means and try to re-calculate with it. It is just treated as unknown.
>>> 
>>> Tests updated. ReplayCacheTestProc.java enhanced so it can be called
>>> with some special system properties to test interop between a
>>> non-system-default native library or even between 2 different native
>>> libraries.
>>> 
>>> Thanks
>>> Max
>>> 
>>> 
>>> 
>>> 
>>> 

Re: RFR 8168518: rcache interop with krb5-1.15

[Wang Weijun]

On 10/25/2016 1:40 PM, Xuelei Fan wrote:

Is the "HASH" algorithm a new name?  I'm not sure why introduce this
none-standard name.


HASH is actually MD5, it is the label used by MIT krb5 in its previous 
releases. In 1.15, it's SHA256. I just want it to the same with MIT krb5.



  39 // The hash algorithm can be "HASH" or "SHA256".
What if the algorithm is not one of the two?  Why not use the standard
names?  Do you want to support SHA3?


No.

The system property is not meant to be fully customizable. The only 
usage is that you can set it to SHA256 if you want full krb5-1.15 
interoperability. I mainly created it so I can test on it.




As the DEFAULT_HASH_ALG system property is customizable, the value may
be not valid.  Therefore, the AssertionError may be too strong to use in
line 310 of KrbApReq.java, and the message can have more information.


OK, I'll change the message.




Otherwise, looks fine to me.

Xuelei

On 10/25/2016 12:18 PM, Wang Weijun wrote:

http://cr.openjdk.java.net/~weijun/8168518/webrev.00/

Please read https://bugs.openjdk.java.net/browse/JDK-8168518 for the
reason. This code change includes:

1. Add a hashAlg field in AuthTimeWithHash.java.

2. Add AuthTimeWithHash.DEFAULT_HASH_ALG so we can change it later.

3. The fix of the bug is inside DflCache.java:

@@ -300,7 +302,7 @@

   if (time.equals(a)) {
   // Exact match, must be a replay
   throw new KrbApErrException(Krb5.KRB_AP_ERR_REPEAT);

-  } else if (time.isSameIgnoresHash(a)) {
+  } else if (time.sameTimeDiffHash((AuthTimeWithHash)a)) {

  // Two different authenticators in the same second.
  // Remember it
  seeNewButNotSame = true;

When a AuthTimeWithHash is seen with a different hash, we believe it's
a new one. Before this fix, we simply compare the HASH string. Now
that the algorithm can be different, we only treat it a new one if the
algorithm is the same and the hash value is different.

This code change has not tried to understand what a different hashAlg
means and try to re-calculate with it. It is just treated as unknown.

Tests updated. ReplayCacheTestProc.java enhanced so it can be called
with some special system properties to test interop between a
non-system-default native library or even between 2 different native
libraries.

Thanks
Max

RFR 8168518: rcache interop with krb5-1.15

[Wang Weijun]

http://cr.openjdk.java.net/~weijun/8168518/webrev.00/

Please read https://bugs.openjdk.java.net/browse/JDK-8168518 for the reason. 
This code change includes:

1. Add a hashAlg field in AuthTimeWithHash.java.

2. Add AuthTimeWithHash.DEFAULT_HASH_ALG so we can change it later.

3. The fix of the bug is inside DflCache.java:

@@ -300,7 +302,7 @@

   if (time.equals(a)) {
   // Exact match, must be a replay
   throw new KrbApErrException(Krb5.KRB_AP_ERR_REPEAT);

-  } else if (time.isSameIgnoresHash(a)) {
+  } else if (time.sameTimeDiffHash((AuthTimeWithHash)a)) {

  // Two different authenticators in the same second.
  // Remember it
  seeNewButNotSame = true;

When a AuthTimeWithHash is seen with a different hash, we believe it's a new 
one. Before this fix, we simply compare the HASH string. Now that the algorithm 
can be different, we only treat it a new one if the algorithm is the same and 
the hash value is different.

This code change has not tried to understand what a different hashAlg means and 
try to re-calculate with it. It is just treated as unknown.

Tests updated. ReplayCacheTestProc.java enhanced so it can be called with some 
special system properties to test interop between a non-system-default native 
library or even between 2 different native libraries.

Thanks
Max

Re: [8u-dev] Request for Review & Approval - 8163304: jarsigner -verbose -verify should print the algorithms used to sign the jar

[Wang Weijun]

On Oct 21, 2016, at 10:31 AM, Rob McKenna  wrote:Hi folks,Looking for a codereview and push approval for the following:bug: https://bugs.openjdk.java.net/browse/JDK-81633049 changeset: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/7a25dbe45e618 webrev: http://cr.openjdk.java.net/~robm/8163304/webrev.02/The code change looks good to me.ThanksMaxThanks,	-Rob

RFR 8167646: Better invalid FilePermission

[Wang Weijun]

Please review the code change at

   http://cr.openjdk.java.net/~weijun/8167646/webrev.00/

A new flag invalid is added so invalid FilePermissions (invalid Path) do not 
equal or imply or is implied by anything else except for itself.

Thanks
Max

RFR 8168374: TsacertOptionTest.java fails on all platforms

[Wang Weijun]

Please review this test change:

diff --git a/test/sun/security/tools/jarsigner/TsacertOptionTest.java 
b/test/sun/security/tools/jarsigner/TsacertOptionTest.java
--- a/test/sun/security/tools/jarsigner/TsacertOptionTest.java
+++ b/test/sun/security/tools/jarsigner/TsacertOptionTest.java
@@ -31,6 +31,7 @@
  * @library /lib/testlibrary warnings
  * @modules java.base/sun.security.pkcs
  *  java.base/sun.security.timestamp
+ *  java.base/sun.security.tools.keytool
  *  java.base/sun.security.util
  *  java.base/sun.security.x509
  *  java.management

TsacertOptionTest.java references another file TimestampCheck.java which uses a 
class in this private package, and compilation fails.

Unfortunately JPRT has not caught this because TimestampCheck.java was executed 
earlier (which itself is also a @test and includes the property @modules line).

Noreg-self.

Thanks
Max

RFR 8168127: FilePermissionCollection merges incorrectly

[Wang Weijun]

Please review the code change at

   http://cr.openjdk.java.net/~weijun/8168127/webrev.00/

Two changes:

1. npath2 is considered in equals and hashCode of FilePermission, so 2 objects 
with different npath2 can be added to a map and different entries.

2. special name for newPermUsingAltPath and newPermPlusAltPath results, so 
FilePermissionCollection::add will not merge one with the original.

Thanks
Max

Re: RFR 8163304: jarsigner -verbose -verify should print the algorithms used to sign the jar

[Wang Weijun]

Updated at

   http://cr.openjdk.java.net/~weijun/8163304/webrev.02/

changes to webrev.01 is at

  http://cr.openjdk.java.net/~weijun/8163304/webrev.02/interdiff.patch.html

Thanks
Max

Re: RFR 8163304: jarsigner -verbose -verify should print the algorithms used to sign the jar

[Wang Weijun]

> Am Wed, 19 Oct 2016 16:13:24 -0400
> schrieb Sean Mullan <
> sean.mullan at oracle.com
> >:
> 
> >
>  150 "The jar will be treated as unsigned, because it
> 
> >
>  is signed with a weak algorithm that is now disabled.\n\nRe-run
> 
> >
>  jarsigner with the -verbose option for more details."},
> 
> 
> I also wondered: what if there are multiple signatures. So a "because
> it is signed only with weak algorithms" might be better?

This is more precise.

But probably not more helpful. This warning only shows when all algorithms are 
weak and saying one algorithm is weak is not misleading.

IMO, people will only get confused when one signature is weak and the other is 
not. In this case, the history prints out 2 signatures but "jarsigner -verify 
-verbose -certs" only shows one for the entries. I hope the weak label there 
could be meaningful.

Thanks
Max

> 
> Gruss
> Bernd

Re: RFR 8163304: jarsigner -verbose -verify should print the algorithms used to sign the jar

[Wang Weijun]

> On Oct 20, 2016, at 4:13 AM, Sean Mullan <sean.mul...@oracle.com> wrote:
> 
> * Main.java
> 
>  98 private static final DisabledAlgorithmConstraints SIGN_CHECK =
>  99 new DisabledAlgorithmConstraints(
> 100 DisabledAlgorithmConstraints.PROPERTY_CERTPATH_DISABLED_ALGS);
> 
> This should be changed to PROPERTY_JAR_DISABLED_ALGS now that the fix for 
> 8167594 is in 9.

Yes.

> 
> * Resources.java
> 
> 150 "The jar will be treated as unsigned, because it is 
> signed with a weak algorithm that is now disabled.\n\nRe-run jarsigner with 
> the -verbose option for more details."},
> 
> Should this also have "WARNING:" at the beginning like the other 2 unsigned 
> warning messages?

You suggested this some time ago:

   I think we should say "WARNING: Signature not parsable or verifiable. ..."

   Without the word "WARNING", the impact (to me) seems to get lost in the 
verbose output. The "WARNING" part is not needed if -verbose is not specified.


> 
> * JarUtils.java
> 
> 45  * a new jar entry will be created with the file name itself the 
> content.
> 70  * with the file name itself the content.
> 
> These 2 lines would be more understandable if you changed "itself the 
> content" to "itself as the content".

Yes.

> 
> * TimestampCheck.java
> 
> You will need to update this test based on the new MD5 restrictions added in 
> 8167594.

Yes.

Thanks
Max

> 
> --Sean
> 
> On 10/19/2016 03:36 AM, Wang Weijun wrote:
>> Please review the code change at
>> 
>>   http://cr.openjdk.java.net/~weijun/8163304/webrev.01/
>> 
>> With this change, "jarsigner -verify -verbose" will print out how a jar was 
>> signed.
>> 
>> For example, a jar which was signed and timestamped with many weak 
>> algorithms will show
>> 
>> - Signed by "CN=old"
>>Digest algorithm: MD2 (weak)
>>Signature algorithm: MD2withRSA (weak), 2048-bit key
>>  Timestamped by "CN=tsbad1" on Wed Oct 19 07:32:22 UTC 2016
>>Timestamp digest algorithm: MD2 (weak)
>>Timestamp signature algorithm: SHA1withRSA, 512-bit key (weak)
>> 
>> WARNING: The jar will be treated as unsigned, because it is signed with a 
>> weak algorithm that is now disabled by the security property:
>> 
>>  jdk.jar.disabledAlgorithms=MD2, RSA keySize < 1024, DSA keySize < 1024
>> 
>> Thanks
>> Max
>> 

RFR 8163304: jarsigner -verbose -verify should print the algorithms used to sign the jar

[Wang Weijun]

Please review the code change at

   http://cr.openjdk.java.net/~weijun/8163304/webrev.01/

With this change, "jarsigner -verify -verbose" will print out how a jar was 
signed.

For example, a jar which was signed and timestamped with many weak algorithms 
will show

- Signed by "CN=old"
Digest algorithm: MD2 (weak)
Signature algorithm: MD2withRSA (weak), 2048-bit key
  Timestamped by "CN=tsbad1" on Wed Oct 19 07:32:22 UTC 2016
Timestamp digest algorithm: MD2 (weak)
Timestamp signature algorithm: SHA1withRSA, 512-bit key (weak)

WARNING: The jar will be treated as unsigned, because it is signed with a weak 
algorithm that is now disabled by the security property:

  jdk.jar.disabledAlgorithms=MD2, RSA keySize < 1024, DSA keySize < 1024

Thanks
Max

RFR 8167647: Copy-and-paste bug in javax.security.auth.kerberos.KerberosTicket.toString()

[Wang Weijun]

Please take a review on this change:

diff --git 
a/src/java.security.jgss/share/classes/javax/security/auth/kerberos/KerberosTicket.java 
b/src/java.security.jgss/share/classes/javax/security/auth/kerberos/KerberosTicket.java
--- 
a/src/java.security.jgss/share/classes/javax/security/auth/kerberos/KerberosTicket.java
+++ 
b/src/java.security.jgss/share/classes/javax/security/auth/kerberos/KerberosTicket.java

@@ -694,7 +694,7 @@
 "Proxy Ticket " + flags[PROXY_TICKET_FLAG] + "\n" +
 "Postdated Ticket " + flags[POSTDATED_TICKET_FLAG] + 
"\n" +
 "Renewable Ticket " + flags[RENEWABLE_TICKET_FLAG] + 
"\n" +

-"Initial Ticket " + flags[RENEWABLE_TICKET_FLAG] + "\n" +
+"Initial Ticket " + flags[INITIAL_TICKET_FLAG] + "\n" +
 "Auth Time = " + String.valueOf(authTime) + "\n" +
 "Start Time = " + String.valueOf(startTime) + "\n" +
 "End Time = " + endTime.toString() + "\n" +

Noreg-trivial.

Thanks
Max

RFR 8167181: Exported elements referring to inaccessible types in jdk.security.jgss

[Wang Weijun]

Please review the code change for jdk9/dev/jdk repo at

   http://cr.openjdk.java.net/~weijun/8167181/webrev.00

and for jdk9/dev which is simply

diff --git a/make/CompileJavaModules.gmk b/make/CompileJavaModules.gmk
--- a/make/CompileJavaModules.gmk
+++ b/make/CompileJavaModules.gmk
@@ -452,10 +452,6 @@




-jdk.security.jgss_ADD_JAVAC_FLAGS := -Xlint:-exports
-
-
-
 jdk.vm.ci_ADD_JAVAC_FLAGS := -Xlint:-exports




After the change the *Impl classes disappeared from javadoc [1]. I wish 
I had noticed them before.


Thanks
Max

[1] 
http://download.java.net/java/jdk9/docs/jre/api/security/jgss/spec/com/sun/security/jgss/package-summary.html

Re: [9] RFR: JDK-8164322: sun/security/pkcs11/PKCS11Test.java shall be updated to run on ARM platforms

[Wang Weijun]

> On Sep 28, 2016, at 7:59 AM, Xuelei Fan  wrote:
> 
>> Currently the tests silently quit which looks like they pass. This makes
>> people think that everything went smoothly, but actually nothing was
>> tested.
>> 
> I did not get the idea.

I think what Artem meant is that without the platform name in osMap, the 
platform is not tested at all. If the platform has NSS libs, we won't be able 
to know whether it works.

--Max

Re: [9] RFR: JDK-8164322: sun/security/pkcs11/PKCS11Test.java shall be updated to run on ARM platforms

[Wang Weijun]

Looking at the webrev, it looks we've never tested on "Linux-arm-32" and 
"Linux-aarch64-64" before and we only realized it now. This is a true problem.

On the other hand, I also agree with Xuelei's concern. If a new platform is 
added and it does not have NSS libs tests will fail.

How about this? If there is such a new platform called "mhos-arch-32", we can 
add

   osMap("mhos-arch-32", new String[]{});

and, make nssLibDirs == null a failure, but nssLibDirs.length == 0 can return 
null.

Is this good?

Thanks
Max


> On Sep 27, 2016, at 7:25 PM, Xuelei Fan  wrote:
> 
> I think, it is the expected behavior to ignore the test if a platform does 
> not support it.  If showing failures, every testing on unsupported platform 
> will fail, and additional effort MUST be paid to evaluate the root cause of 
> the failure.  We should try to avoid that.
> 
> Xuelei
> 
> On 9/27/2016 6:32 PM, Tim Du wrote:
>> Hi All:
>> 
>> Would you help to review the patch for sun/security/pkcs11/PKCS11Test.java?
>> The test keep pass on not supported platforms, it will make nobody
>> notice the test was skipped,which is not our expected. Update case to
>> show failure, when platform not supported. And add the support for
>> Linux-arm-32 and Linux-arm-64 platforms. Thanks.
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8164322
>> Webrev: http://cr.openjdk.java.net/~tidu/8164322/webrev.00/
>> 
>> 
>> Regards
>> Tim

Re: RFR: JDK-8049516: sun.security.provider.SeedGenerator throws ArrayIndexOutOfBoundsException

[Wang Weijun]

I am OK with your fix, but I found "(latch + 1) % Integer.MAX_VALUE" a little 
difficult to understand. Does rndTab[latch & 0xff] also work?

Thanks
Max


> On Sep 21, 2016, at 11:57 PM, Jamil Nimeh  wrote:
> 
> Hi Max and Xuelei,
> 
> Yesterday I also reached out to the SQE engineer who submitted the bug, 
> asking if this is an issue he's seen going forward from the original instance 
> in 8u20.  He said that he hasn't seen this issue come up since the original 
> bug submission.  Since the simple overflow condition is easily solved with my 
> fix, and the code has been otherwise stable I'd like to suggest that we keep 
> the fix as-is.  The loop timing as it stands now is not the source of the 
> bug, other than that latch can overflow and that is solved in one line.  If 
> we want to revisit this and improve the overall performance (though I haven't 
> seen evidence that there is a perf issue with this at all) then maybe an RFE 
> is in order.  What do you think?
> 
> --Jamil

Re: RFR(s): 8166450: SMARTCARDIO related tests failed on compilation

[Wang Weijun]

Change looks fine.

One question: When you say the --limit-modules option, is it a new option for 
jtreg that allows it to automatically choose modules described in the @modules 
tags? Or it's just the standard VM option and you still need to provide the 
module names yourself?

Thanks
Max

> On Sep 21, 2016, at 8:08 PM, Sergei Kovalev  wrote:
> 
> Hello team,
> 
> Could you please review small fix for regression tests related to smartcardio 
> API?
> 
> BugID: https://bugs.openjdk.java.net/browse/JDK-8166450
> WebRev: http://cr.openjdk.java.net/~skovalev/8166450/webrev.00/
> 
> Issue: unable to run smartcardio related tests bu jtreg by default.
> Root cause: missed module declaration in the tests header.
> Solution: Adding missed module declaration.
> Testing done: Fix tested locally using jtreg and "--limit-modules" command 
> line option.
> 
> -- 
> With best regards,
> Sergei
> 

Re: RFR: JDK-8049516: sun.security.provider.SeedGenerator throws ArrayIndexOutOfBoundsException

[Wang Weijun]

> On Sep 21, 2016, at 9:58 AM, Xuelei Fan  wrote:
> 
>  359   while (System.nanoTime() - startTime < 25000) {
>  360   synchronized(this){};
> - 361   latch++;
> + 361   latch = (latch + 1) % Integer.MAX_VALUE;
>  362   }
> 
> This block may be not CPU friendly as it may loop a large amount of times in 
> a very short period (250milli).

To get a <255 index I think we only need to loop for <66536 times.

How about we stop at every millisecond and see if it's enough? Something like 
this:

long next = startTime + 100;
while (next < startTime + 25000) {
while (System.nanoTime() < next) {
synchronized(this){};
latch++;
}
if (latch > 65535 || latch < 0) break;
next += 100;
}

> 
> What's the usage of line 360?  Just for some computation?
> 
> 367   counter += latch;
> The counter variable may be overflow too.

I find this strange. Were computers so slow in 1996 that within 250ms latch 
cannot exceed 64000?

--Max

> 
> Xuelei
> 
> On 9/21/2016 8:57 AM, Jamil Nimeh wrote:
>> Hello all,
>> 
>> This fixes a bug found in stress testing where on faster CPUs the latch
>> can overflow resulting in a negative array index.  The fix avoids the
>> overflow by resetting the latch to 0 when it reaches Integer.MAX_VALUE -
>> 1 and will continue increasing from there.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8049516
>> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8049516/webrev.01/
>> 
>> Thanks,
>> --Jamil

Re: Code Review Request, JDK-8146600 AVA Normalizer.Form issue

[Wang Weijun]

> On Sep 20, 2016, at 8:58 AM, Xuelei Fan  wrote:
>> 
>> I this case, a comma appears but then it is escaped. You might say it is
>> unexpected, but at least after escaping, it becomes a legal string.
>> 
> I did not get the point.  A comma (",") should be escaped and it does get 
> escaped and the string is legal.  Do you mean "，" (double bytes comma) should 
> be converted to ","?  Can you have more details?

I'll write double bytes comma as ,, below.

Current code, "Hello,,world" is not modified at escaping, and becomes 
"Hello,world" after normalization. This is illegal.

With my fix, "Hello,,world" becomes "Hello,world" after normalization, and then 
"Hello\,world" after escaping. This is legal.

With your fix, "Hello,,world" is not modified after both steps, and it's legal.

So both your and my fixes will make it legal and the test will succeed.

> 
>>> It is something I want to avoid, so that it is fixed to use NFD
>>> instead.  I think if we are moving to use NFD, it is does not matter
>>> to escaping first or normalization first if I understand the UTF-8
>>> correctly.
>> 
>> Maybe, but IMO this is not the correct fix. The ultimate reason of the
>> bug is not the form chosen, but the order.
>> 
> I'm not with you for this bug. The bug is complain about the escaping issue, 
> but actually the character should not be escaped.  So it is not an issue of 
> escaping.  So this fix is not trying to fix the escaping issue, but trying to 
> fix the normalization issue.

Yes it is complaining about escaping, but there are 2 ways to amend it. 1) 
escape it. 2) make it not necessary to escape.

I just prefer my fix, because I think that's where the bug is. Even if we 
switch to NFD, I would still like to put normalization before escaping, even if 
practically it makes no difference.

Thanks
Max

> 
> Thanks,
> Xuelei
> 
>> --Max
>> 
>>> 
>>> Thanks,
>>> Xuelei
>>> 
 Thanks
 Max
 
> On Sep 19, 2016, at 10:32 AM, Xuelei Fan  > wrote:
> 
>> 4. Is it possible to perform normalization before escaping special
>> characters?
>> 
> Yes.  I though about this case.  The current fix comes from the fact
> that UTF-8 "Hello, world!" and "Hello， world!" should be different.
> Parsing them as the same thing may result in unexpected serious issues.
 

Re: Code Review Request, JDK-8146600 AVA Normalizer.Form issue

[Wang Weijun]

Sorry. Whenever I wrote NFC, I meant NFD. Typo. 

> 在 2016年9月19日，23:16，Xuelei Fan <xuelei@oracle.com> 写道：
> 
>> On 9/19/2016 11:03 PM, Wang Weijun wrote:
>> After some thinking, my current opinion is.
>> 
>> 1. Maybe NFC is better than NFKD, but I am not a Unicode expert.
>> 
> It is updated from NFKD to NFD.  I did not get the point.  Do you mean NFC is 
> better than NFD?
> 
>> 2. I think the real bug is the order of escaping and normalization. The 
>> normalization (if a must) should be performed earlier right after valStr is 
>> created and only performed on valStr. Otherwise the NFKD normalization would 
>> generate new chars that need to be escaped. Again I am not a Unicode expert 
>> and I don't know if NFC will also do the same.
>> 
> I don't get the point.  The update is moving from NFKD to NFD.  No NFKD 
> normalization any more.
> 
>> If 2) is fixed, whatever is correct in 1) does not matter much.
>> 
> If we continue to use NFKD, normalization before escaping would result in 
> unexpected string as we talked for the hello-world example.  

I this case, a comma appears but then it is escaped. You might say it is 
unexpected, but at least after escaping, it becomes a legal string. 

> It is something I want to avoid, so that it is fixed to use NFD instead.  I 
> think if we are moving to use NFD, it is does not matter to escaping first or 
> normalization first if I understand the UTF-8 correctly.

Maybe, but IMO this is not the correct fix. The ultimate reason of the bug is 
not the form chosen, but the order. 

--Max

> 
> Thanks,
> Xuelei
> 
>> Thanks
>> Max
>> 
>>>> On Sep 19, 2016, at 10:32 AM, Xuelei Fan <xuelei@oracle.com> wrote:
>>>> 
>>>> 4. Is it possible to perform normalization before escaping special 
>>>> characters?
>>>> 
>>> Yes.  I though about this case.  The current fix comes from the fact that 
>>> UTF-8 "Hello, world!" and "Hello， world!" should be different. Parsing them 
>>> as the same thing may result in unexpected serious issues.
>> 

Re: Code Review Request, JDK-8146600 AVA Normalizer.Form issue

[Wang Weijun]

After some thinking, my current opinion is.

1. Maybe NFC is better than NFKD, but I am not a Unicode expert.

2. I think the real bug is the order of escaping and normalization. The 
normalization (if a must) should be performed earlier right after valStr is 
created and only performed on valStr. Otherwise the NFKD normalization would 
generate new chars that need to be escaped. Again I am not a Unicode expert and 
I don't know if NFC will also do the same.

If 2) is fixed, whatever is correct in 1) does not matter much.

Thanks
Max

> On Sep 19, 2016, at 10:32 AM, Xuelei Fan  wrote:
> 
>> 4. Is it possible to perform normalization before escaping special 
>> characters?
>> 
> Yes.  I though about this case.  The current fix comes from the fact that 
> UTF-8 "Hello, world!" and "Hello， world!" should be different. Parsing them 
> as the same thing may result in unexpected serious issues.

Re: Code Review Request, JDK-8146600 AVA Normalizer.Form issue

[Wang Weijun]

I am not sure of this change for several reasons:

1. I cannot find anywhere in RFC 2253 (or its new versions) mentioning 
normalizations. Do you know elsewhere?

2. It's not obvious to say "Hello, world!" and "Hello， world!" should be 
different if NFKD thinks they are.

3. Why not NFC? Although I did't find normalization on X500 names in RFC 5280, 
I do see in several other cases NFV is used.

4. Is it possible to perform normalization before escaping special characters?

5. Why is normalization necessary? At least in RFC 5280 4.1.2.6, it says

   When the subject of the certificate is a CA, the subject
   field MUST be encoded in the same way as it is encoded in the
   issuer field (Section 4.1.2.4 ) in all certificates issued by
   the subject CA.

which implies comparison should be on encoding instead of toString.

Thanks
Max

> On Sep 15, 2016, at 8:09 AM, Xuelei Fan  wrote:
> 
> Hi,
> 
> Please review this fix:
>http://cr.openjdk.java.net/~xuelei/8146600/webrev.00/
> 
> The Normalizer.Form.NFKD is used to normalize attribute-value assertion in 
> X.509 certificate processing.  The normalizer may convert some UTF-8 
> character into ASCII code.  For example, "，"(two bytes) will be converted to 
> ","(one byte), and "Hello， world!" is normalize to "Hello, world!".  However, 
> "Hello, world!" and "Hello， world!" should be different because of the comma 
> code.  This conversion may result in unexpected weird behaviors for name 
> comparing and conversions.
> 
> This fix will update to use "Normalizer.Form.NFD".
> 
> Thanks,
> Xuelei

Re: Intended behavior of the -providerName option of keytool

[Wang Weijun]

> On Sep 15, 2016, at 11:53 PM, Michael Wang  wrote:
> 
> Hi,
> 
> I'm trying to understand what the -providerName option of keytool does. The 
> documentation for -providerName just says:
> 
> "Used to identify a cryptographic service provider's name when listed in the 
> security properties file."
> 
> Which doesn't really say anything about how it should be used and the 
> resulting behavior.
> 
> I looked at the latest Java 9 source code for keytool, the only 2 places that 
> I see that uses providerName are
> 
> a. Getting an instance of the keystore, with:
> KeyStore.getInstance(storetype, providerName);
> 
> b. Getting an instance of key pair generator, with:
> new CertAndKeyGen(keyAlgName, sigAlgName, providerName);
> 
> It looks like all other calls in keytool that requires the services of a 
> provider does not use providerName, so it defaults to looking up the matching 
> provider from the providers list.
> 
> This behavior doesn't seem very clear cut to me.
> I think -providerName should used to either:
> 
> 1. Specify the provider of the keystore only. All other services used by 
> keytool that requires a provider will look up the provider using the default 
> providers list.
> 
> 2. Specify the provider of all services used by keytool that requires a 
> provider, including keystore.

This is the intended behavior. However, we only use providerName when it's 
necessary:

The reason why you see it is not used everywhere can be due to several reasons:

1. storetype is already different for different providers so it's not necessary.

2. some object is smart enough to switch provider even after getInstance. For 
example, if you call sig = Signature.getInstance("SHA1withRSA") to get a 
signature object it is not actually fully initialized. Next when you call 
sig.initSign(key), it will use the provider of key as its provider if necessary.

Thanks
Max


> 
> I just want to understand what the intended behavior should be.
> 
> Thanks,
> Michael Wang

Re: Do your NSS DLLs on Windows have execute permission?

[Wang Weijun]

> On Sep 16, 2016, at 2:30 AM, Bradford Wetmore  
> wrote:
> 
> As of today's JDK 9 code, look at the "prep" target" in the jdk/test/Makefile 
> test directory.  The "prep" target updates the DLLs as needed if this is not 
> a repository (which I think is how our build/test system JPRT works).

I didn't noticed that before. This explains everything.

Thanks
Max

Re: X.509 Certificate Testing...

[Wang Weijun]

I don't know a single place including all these things. In fact, in most cases 
we avoid including a certificate directly in a test if it can be created on the 
fly.

--Max

> On Sep 16, 2016, at 7:19 AM, Milton Smith  wrote:
> 
> Hi All,
> 
> I'm looking for a set of certificates, self-signed certs, cross-signed certs, 
> small chains, large chains, different critical and non-critical sections, 
> revoked certs, blacklisted certs, invalid, not yet valid, time stamped, etc.  
> I realize it's difficult to be comprehensive but is there anything anyone can 
> recommend for unit tests or CD/CI builds?  Trying to avoid creating all this 
> if it exists already.  Thanks!
> 
> Regards,
> Milton

Re: RFR(M): 8166032: Fix module dependencies for javax.SSL tests

[Wang Weijun]

I see. But krb5 cipher suites is just a very small part of TLS. Maybe we can 
tweak the tests a little so that they can work without JGSS.

I am fine with the change at the moment.

Thanks
Max

> On Sep 15, 2016, at 7:39 AM, Xuelei Fan <xuelei@oracle.com> wrote:
> 
> I did some research yesterday.  The "/javax/net/ssl/TLSCommon" lib depends on 
> krb5.  For example, BufferOverflowUnderflowTest need to set up KDC and test 
> krb5 cipher suites.
> 
> Xuelei
> 
> On 9/15/2016 6:21 AM, Wang Weijun wrote:
>> Confusing. This does not show why jgss is needed. I'll do some
>> investigation.
>> 
>> Thanks
>> Max
>> 
>> 在 2016年9月14日，23:57，Sergei Kovalev <sergei.kova...@oracle.com
>> <mailto:sergei.kova...@oracle.com>> 写道：
>> 
>>> java.lang.module.ResolutionException: Module java.naming not found,
>>> required by java.security.jgss

Re: RFR(M): 8166032: Fix module dependencies for javax.SSL tests

[Wang Weijun]

Confusing. This does not show why jgss is needed. I'll do some investigation. 

Thanks
Max

> 在 2016年9月14日，23:57，Sergei Kovalev  写道：
> 
> java.lang.module.ResolutionException: Module java.naming not found, required 
> by java.security.jgss

Re: RFR(S): 8165689: Fix module dependencies for sun/security/pkcs11/* tests

[Wang Weijun]

Sorry I was thinking about another bug on TLS tests -- 8166032. What exception 
is thrown there if the jgss module is missing?

Thanks
Max

> 在 2016年9月14日，23:44，Wang Weijun <weijun.w...@oracle.com> 写道：
> 
> The example shows jdk.crypto.pkcs11 is needed, but I still don't know why 
> java.security.jgss is. Can you give another example where jgss must be 
> present?
> 
> Thanks
> Max
> 
>> 在 2016年9月14日，23:26，Sergei Kovalev <sergei.kova...@oracle.com> 写道：
>> 
>> Hi Sean,
>> 
>> I'm working for testing minimal JRE image. If I create custom JRE with 
>> java.base only - the tests fail. To emulate such behavior we can use 
>> "--limit-modules java.base" option. In case if we have no module declaration 
>> in tests header the test fails with, e.g. ClassNotFound exception (see 
>> example in attached log). In case we declare appropriate modules in jtreg 
>> header then jtreg suite skip the test and mark it "not run" in final report. 
>> This help me to clean out all "false positive" error during testing and 
>> reduce time that I spend to failures analysis.
>> 
>> 
>> 14.09.16 18:20, Sean Mullan wrote:
>>> Looks fine to me, but can you explain in more detail why the extra 
>>> dependencies are needed, or an example using --limit-modules? These tests 
>>> are not failing regularly now, so when do the missing dependencies cause 
>>> failures?
>>> 
>>> Thanks,
>>> Sean
>>> 
>>>> On 09/13/2016 08:34 AM, Sergei Kovalev wrote:
>>>> Hello team,
>>>> 
>>>> This is re-request for review of very small changes. Could somebody take
>>>> a look?
>>>> 
>>>> 
>>>> 08.09.16 17:03, Sergei Kovalev wrote:
>>>>> Hello team,
>>>>> 
>>>>> Could you please review the fix for below CR:
>>>>> 
>>>>> Bug ID: https://bugs.openjdk.java.net/browse/JDK-8165689
>>>>> WebRev: http://cr.openjdk.java.net/~skovalev/8165689/webrev.00/
>>>>> 
>>>>> Goal: make test possible to run with "--limit-modules" flag.
>>>>> Summary: added @modules tag into jtreg header if applicable.
>>>>> 
>>>> 
>> 
>> -- 
>> With best regards,
>> Sergei
>> 
>> 

Re: RFR(S): 8165689: Fix module dependencies for sun/security/pkcs11/* tests

[Wang Weijun]

The example shows jdk.crypto.pkcs11 is needed, but I still don't know why 
java.security.jgss is. Can you give another example where jgss must be present?

Thanks
Max

> 在 2016年9月14日，23:26，Sergei Kovalev  写道：
> 
> Hi Sean,
> 
> I'm working for testing minimal JRE image. If I create custom JRE with 
> java.base only - the tests fail. To emulate such behavior we can use 
> "--limit-modules java.base" option. In case if we have no module declaration 
> in tests header the test fails with, e.g. ClassNotFound exception (see 
> example in attached log). In case we declare appropriate modules in jtreg 
> header then jtreg suite skip the test and mark it "not run" in final report. 
> This help me to clean out all "false positive" error during testing and 
> reduce time that I spend to failures analysis.
> 
> 
> 14.09.16 18:20, Sean Mullan wrote:
>> Looks fine to me, but can you explain in more detail why the extra 
>> dependencies are needed, or an example using --limit-modules? These tests 
>> are not failing regularly now, so when do the missing dependencies cause 
>> failures?
>> 
>> Thanks,
>> Sean
>> 
>>> On 09/13/2016 08:34 AM, Sergei Kovalev wrote:
>>> Hello team,
>>> 
>>> This is re-request for review of very small changes. Could somebody take
>>> a look?
>>> 
>>> 
>>> 08.09.16 17:03, Sergei Kovalev wrote:
 Hello team,
 
 Could you please review the fix for below CR:
 
 Bug ID: https://bugs.openjdk.java.net/browse/JDK-8165689
 WebRev: http://cr.openjdk.java.net/~skovalev/8165689/webrev.00/
 
 Goal: make test possible to run with "--limit-modules" flag.
 Summary: added @modules tag into jtreg header if applicable.
 
>>> 
> 
> -- 
> With best regards,
> Sergei
> 
> 

Re: RFR(M): 8166032: Fix module dependencies for javax.SSL tests

[Wang Weijun]

I also have no idea why these are needed:

+ * @modules java.security.jgss
+ *  jdk.security.auth

Do the test uses KRB5-related cipher suites?

Thanks
Max

> On Sep 14, 2016, at 8:54 PM, Xuelei Fan  wrote:
> 
> Hi Sergei,
> 
> Thanks for the update.  The fix looks fine to me.  However, I'm not sure why 
> some tests need the "/sun/security/krb5/auto" libraries and the related 
> modules.  As you were already there, would you mind help to test whether the 
> "/sun/security/krb5/auto" library can be removed for some tests?
> 
> Thanks,
> Xuelei
> 
> On 9/14/2016 8:22 PM, Sergei Kovalev wrote:
>> Hello Team,
>> 
>> Could you please review below fix for
>> 
>> Bug ID: https://bugs.openjdk.java.net/browse/JDK-8166032
>> Code review: http://cr.openjdk.java.net/~skovalev/8166032/webrev.00/
>> 
>> Issue: The test should be able to pass or skipped by JTreg test suite if
>> required modules have not been included in custom JRE.
>> Solution: added '@modules' tags and appropriate module list to tests
>> header.
>> Testing done locally using JTregt test suite.
>> 

Re: RFR: 8061842: Package jurisdiction policy files as something other than JAR

[Wang Weijun]

Before this change, you require default policy in neither export nor import to 
be empty but do not care about the getMinimum() result. In this change, you 
make sure the final result is not empty. I assume this is a fix?

283 // Did we find a default perms?

What does this line mean?

296 // This should never happen

But you can still print out the file name.


Can you rename policydir-tbd to something else? I am afraid it will be confused 
with policy.url.1 etc.

The original README.TXT in unlimited says "are exportable from the United 
States." and you have "is exportable." now. Is this intended? (IANAL)

TestUnlimited.java:
45 "// Use the AES are the test Cipher", you mean "Use AES as the test Cipher"?
51 "throw new Exception ("Unlimited policy is NOT active");". No space before 
"(".

No other comment.

--Max

> On Aug 18, 2016, at 7:22 AM, Bradford Wetmore  
> wrote:
> 
> New webrev:
> 
> https://bugs.openjdk.java.net/browse/JDK-8061842
> http://cr.openjdk.java.net/~wetmore/8061842/webrev.01/

Re: RFR : 8147772, 8163104

[Wang Weijun]

The changes look fine.

Thanks
Max

> On 2016年8月10日, at 下午9:48, Seán Coffey  wrote:
> 
> Looking to backport the following two bug fixes to jdk8u-dev :
> 
> JDK-8147772 Update KerberosTicket to describe behavior if it has been 
> destroyed and fix NullPointerExceptions
> JDK-8163104 Unexpected NPE still possible on some Kerberos ticket calls
> 
> The changes are similar to JDK 9 expect that the @implNote and javadoc edits 
> have been omitted from jdk8u for the 8147772 port.
> 
> webrev : 
> http://cr.openjdk.java.net/~coffeys/webrev.8147772.8163104.jdk8u/webrev/
> 
> -- 
> Regards,
> Sean.
>