Looks better. One question: the bug description mentions *signed* JARs. Will it be covered?
Thanks Max > On Dec 22, 2015, at 2:17 PM, Sibabrata Sahoo <sibabrata.sa...@oracle.com> > wrote: > > Hi Max/Mandy, > > Here is the updated webrev: > http://cr.openjdk.java.net/~asmotrak/siba/8130360/webrev.05/ > > I tried to address all the comments bellow. The major change includes > abstracting the common behavior to ModularTest.java and extend the rest by > each specific test. i.e. SecurityProviderModularTest and > JaasModularClientTest. > > Thanks, > Siba > > -----Original Message----- > From: Wang Weijun > Sent: Monday, December 21, 2015 7:20 AM > To: Sibabrata Sahoo > Cc: Mandy Chung; Valerie Peng; jigsaw-...@openjdk.java.net; > security-dev@openjdk.java.net > Subject: Re: [9] RFR:8130360: Add tests to verify 3rd party security > providers if they are in signed/unsigned modular JARs > > Tests are good. Several comments: > > 1. Try run something like "hg mv -A" to let Mercurial knows that you are > renaming files (SecurityUtils.java and those in login/modules/src) instead of > removing some old and creating some new. The current webrev does not show > this. > > 2. It's not a good practice extending a class just to use its static > fields/methods. Instead, make the util class final. > > public class JaasModularClientTest extends SecurityUtils > public class SecurityProviderModularTest extends SecurityUtils > > In fact, if you do find similarities between SecurityProviderModularTest and > JaasModularClientTest, try abstract them into a ModularTest class. > > 3. Please add enough comments to SecurityUtils since it will be reused. For > example, move the descriptions of EXPLICIT, AUTO and UNNAMED from test > @summary to definition of enum MODULE_TYPE. > > Thanks > Max > > >> On Dec 20, 2015, at 2:10 PM, Sibabrata Sahoo <sibabrata.sa...@oracle.com> >> wrote: >> >> Hi Mandy/Max, >> >> Here is the updated webrev with very minor change over previous: >> http://cr.openjdk.java.net/~asmotrak/siba/8130360/webrev.04/ >> >> Thanks >> Siba >> >> -----Original Message----- >> From: Sibabrata Sahoo >> Sent: Sunday, December 20, 2015 1:34 AM >> To: Mandy Chung; Weijun Wang >> Cc: Valerie Peng; jigsaw-...@openjdk.java.net; >> security-dev@openjdk.java.net >> Subject: RE: [9] RFR:8130360: Add tests to verify 3rd party security >> providers if they are in signed/unsigned modular JARs >> >> Hi Mandy/Max, >> >> Please review the updated webrev: >> http://cr.openjdk.java.net/~asmotrak/siba/8130360/webrev.03/ >> >> "3rd party security providers" Test change includes: >> - Test are converted to use TestNG framework. >> - java/security/modules/JigsawSecurityUtils.java, renamed to >> "SecurityUtils.java". Proper care required while pushing the change because >> "JigsawSecurityUtils.java" is an existing file in JAKE repository. >> - Common reusable operations moved to "SecurityUtils.java" >> - All the following comments associated. >> >> >> Based on the recent comments, I also did some changes to the "JAAS Modular >> test" because both test are very similar to each other. The change includes: >> - Flat folder structure. It places the Test as well as all its dependency in >> a single folder. i.e. "javax/security/auth/login/modules". That also means >> "javax/security/auth/login/modules/src/" and all its subfolders need to be >> removed. >> - Test converted to use TestNG framework. >> >> Thanks, >> Siba >> >> -----Original Message----- >> From: Mandy Chung >> Sent: Wednesday, December 09, 2015 11:55 PM >> To: Sibabrata Sahoo >> Cc: Valerie Peng; jigsaw-...@openjdk.java.net; >> security-dev@openjdk.java.net >> Subject: Re: [9] RFR:8130360: Add tests to verify 3rd party security >> providers if they are in signed/unsigned modular JARs >> >> Some high-level comment: >> 1. I suggest to rename JigsawSecurityUtils.java to SecurityUtils.java. The >> Jigsaw prefix is not needed. >> 2. SecurityProviderModularTest does the setup and launch the test with a set >> of different settings. I suggest to convert it to TestNG and you can >> reference the existing test/jdk/jigsaw tests which are a driver test that >> does the compilation and setup with @BeforeTest and @Test for each run test >> together with @DataProvider. [1] is a simple test for you to get started. >> >> Some comments on JigsawSecurityUtils.java: >> 71 Arrays.asList(options).stream().forEach(o -> command.append(SPACE >> + o)); >> >> this can be replaced with >> Arrays.stream(options).collect(Collectors.joining(SPACE)); >> >> 111 System.out.println(String.format( >> >> You can do: System.out.format(….) instead; >> >> Mandy >> [1] >> http://hg.openjdk.java.net/jigsaw/jake/jdk/file/c1d583efa466/test/jdk/ >> jigsaw/reflect/Proxy/ProxyTest.java >> >>> On Dec 9, 2015, at 10:02 AM, Sibabrata Sahoo <sibabrata.sa...@oracle.com> >>> wrote: >>> >>> Hi Valerie, >>> >>> Here is the updated webrev: >>> http://cr.openjdk.java.net/~ntv/siba/webrev.00/ >>> >>> Changes applied as per previous comments. >>> - File names are modified and do not include the term JCE. >>> - Source folder structure is flat now and all belongs to >>> "java/security/Provider" folder. >>> - Sign jar functionality removed. >>> >>> Thanks, >>> Siba >>> >>> -----Original Message----- >>> From: Valerie Peng >>> Sent: Wednesday, December 09, 2015 5:37 AM >>> To: Sibabrata Sahoo >>> Cc: jigsaw-...@openjdk.java.net; security-dev@openjdk.java.net; Alan >>> Bateman >>> Subject: Re: [9] RFR:8130360: Add tests to verify 3rd party security >>> providers if they are in signed/unsigned modular JARs >>> >>> Hi Siba, >>> >>> Here are some nits: >>> >>> I think it's somewhat misleading to use the term JCE here as what you are >>> testing here is just security provider loading. JCE is more about security >>> providers supporting export-controlled services/algorithms. >>> Since your provider is just an empty one, I don't think u need to sign it >>> (again, it's only for JCE providers). >>> You can remove a lot of code as a result. >>> Also, your directory seems a bit deep, e.g. >>> modules/src/jceprovidermodule/provider/TestJCEProvider.java vs >>> modules/src/jceclientmodule/provider/TestJCEProvider.java. Can all of these >>> files be under the same directory instead of spreading over several level >>> of subdirectories? The file names are different enough to tell which is >>> which. Other regression tests for provider loading functionality are under >>> test/java/security/Provider. Easier to find this way. >>> >>> Can you please make the appropriate changes, e.g. don't use the term JCE, >>> get rid of the signing, and simplify the directory structure if possible? >>> >>> Thanks, >>> Valerie >>> >>> On 12/8/2015 2:04 PM, Valerie Peng wrote: >>>> Right, that'd be my expectation as well. Sounds like everything works. >>>> I will change to look at your latest webrev. >>>> Valerie >>>> >>>> On 12/8/2015 6:09 AM, Sibabrata Sahoo wrote: >>>>> Hi Valerie, >>>>> >>>>> Here is the updated webrev: >>>>> http://cr.openjdk.java.net/~ralexander/8130360/webrev.00/ >>>>> >>>>> Now the modular behavior for the test works as per expectation >>>>> through JAKE build with the following condition. >>>>> If the provider jar is available under ModulePath then the >>>>> "java.security" file should have the provider configuration entry >>>>> as ProviderName while in case of ClassPath the entry should hold >>>>> the full class name. >>>>> >>>>> Thanks, >>>>> Siba >>>>> >>>>> -----Original Message----- >>>>> From: Valerie Peng >>>>> Sent: Tuesday, December 08, 2015 4:46 AM >>>>> To: Sibabrata Sahoo >>>>> Cc: Alan Bateman; security-dev@openjdk.java.net; >>>>> jigsaw-...@openjdk.java.net >>>>> Subject: Re: [9] RFR:8130360: Add tests to verify 3rd party >>>>> security providers if they are in signed/unsigned modular JARs >>>>> >>>>> Siba, >>>>> >>>>> I have just started to review this webrev and not done yet. >>>>> As for your question, the java.security file in OpenJDK9 still uses >>>>> the provider class names instead of provider names. Are you talking >>>>> about the java.security file in Jigsaw? Which build (OpenJDK or >>>>> Jigsaw) have you tested against. I am pretty sure that I tested the >>>>> 3rd party provider on classpath setting when I worked on the >>>>> 7191662 changes. >>>>> >>>>> Supposedly, if the jar files are available on class path, then you >>>>> should specify its full *class name* in the java.security file for >>>>> it to be instantiated. Otherwise, how would it find the class? Only >>>>> the classes discovered by ServiceLoader can be identified using the >>>>> provider name (which is different from the class name referred >>>>> above). So, in your scenario, that would be >>>>> "provider.TestJCEProvider" instead of "TEST". >>>>> >>>>> If you still run into problems, try enable the java security debug >>>>> flag and u should get a good idea why it isn't loaded. Let me know >>>>> if you still have questions. >>>>> >>>>> Thanks, >>>>> Valerie >>>>> >>>>> On 11/30/2015 6:47 AM, Sibabrata Sahoo wrote: >>>>>> I would like to know more about this. As far, I can see the >>>>>> "java.security" provider configuration available with JDK9, it >>>>>> holds provider names instead of provider class names. In that case >>>>>> how it resolve the fall back? >>>>>> >>>>>> Thanks, >>>>>> Siba >>>>>> >>>>>> -----Original Message----- >>>>>> From: Alan Bateman >>>>>> Sent: Monday, November 30, 2015 4:54 PM >>>>>> To: Sibabrata Sahoo; security-dev@openjdk.java.net; >>>>>> jigsaw-...@openjdk.java.net >>>>>> Subject: Re: [9] RFR:8130360: Add tests to verify 3rd party >>>>>> security providers if they are in signed/unsigned modular JARs >>>>>> >>>>>> On 30/11/2015 11:13, Sibabrata Sahoo wrote: >>>>>>> Here is the updated webrev: >>>>>>> http://cr.openjdk.java.net/~asmotrak/siba/8130360/webrev.02/ >>>>>>> >>>>>>> I have one question: >>>>>>> What should be the behavior when the older version of 3rd party >>>>>>> JCE provider jar file(without service descriptor >>>>>>> "META-INF/services/*"& working with<= JDK8) configured by >>>>>>> "java.security" file, will be place in CLASS_PATH, running >>>>>>> through >>>>>>> JDK9 and the client is using Security.getProvider() to look for >>>>>>> the provider? >>>>>>> >>>>>>> Currently the scenario fails to find the JCE provider. Is this >>>>>>> right behavior? If it is, then jdk9 is not backward compatible to >>>>>>> find the security provider provided through older jar files from >>>>>>> CLASS_PATH. >>>>>>> >>>>>> The JCE work in JDK 9 (via JDK-7191662) was meant to address this >>>>>> point by falling back and attempting to load the class name >>>>>> specified via the security.provider.<N> properties in the >>>>>> java.security file. I'm sure Valerie can say more about this. >>>>>> >>>>>> -Alan >> >
