Re: RFR: 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m [v7]
On Mon, 1 Feb 2021 16:51:12 GMT, Weijun Wang wrote: >> This fix covers both >> >> - [[macOS]: Remove JNF dependency from >> libosxsecurity/KeystoreImpl.m](https://bugs.openjdk.java.net/browse/JDK-8257858) >> - [[macOS]: Remove JNF dependency from >> libosxkrb5/SCDynamicStoreConfig.m](https://bugs.openjdk.java.net/browse/JDK-8257860) > > Weijun Wang has updated the pull request incrementally with two additional > commits since the last revision: > > - file attr error > - objc use .m Rest of the Kerberos config handling changes look fine. Thanks! - Marked as reviewed by valeriep (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1845
Re: RFR: 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m [v7]
On Mon, 1 Feb 2021 16:51:12 GMT, Weijun Wang wrote:
>> This fix covers both
>>
>> - [[macOS]: Remove JNF dependency from
>> libosxsecurity/KeystoreImpl.m](https://bugs.openjdk.java.net/browse/JDK-8257858)
>> - [[macOS]: Remove JNF dependency from
>> libosxkrb5/SCDynamicStoreConfig.m](https://bugs.openjdk.java.net/browse/JDK-8257860)
>
> Weijun Wang has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - file attr error
> - objc use .m
src/java.security.jgss/share/classes/sun/security/krb5/SCDynamicStoreConfig.java
line 103:
> 101: ri.put("kdc", kdcs);
> 102: }
> 103: realms.put(nextRealm, ri);
Do you need to check if ri is empty?
-
PR: https://git.openjdk.java.net/jdk/pull/1845
Re: RFR: 8235710: Remove the legacy elliptic curves [v2]
On Tue, 22 Sep 2020 00:18:07 GMT, Anthony Scarpino wrote: >> This change removes the native elliptic curves library code; as well as, and >> calls to that code, tests, and files >> associated with those libraries. The makefiles have been changed to remove >> from all source builds of the ec code. The >> SunEC system property is removed and java.security configurations changed to >> reflect the removed curves. This will >> remove the following elliptic curves from SunEC: secp112r1, secp112r2, >> secp128r1, secp128r2, secp160k1, secp160r1, >> secp160r2, secp192k1, secp192r1, secp224k1, secp224r1, secp256k1, sect113r1, >> sect113r2, sect131r1, sect131r2, >> sect163k1, sect163r1, sect163r2, sect193r1, sect193r2, sect233k1, sect233r1, >> sect239k1, sect283k1, sect283r1, >> sect409k1, sect409r1, sect571k1, sect571r1, X9.62 c2tnb191v1, X9.62 >> c2tnb191v2, X9.62 c2tnb191v3, X9.62 c2tnb239v1, >> X9.62 c2tnb239v2, X9.62 c2tnb239v3, X9.62 c2tnb359v1, X9.62 c2tnb431r1, >> X9.62 prime192v2, X9.62 prime192v3, X9.62 >> prime239v1, X9.62 prime239v2, X9.62 prime239v3, brainpoolP256r1 >> brainpoolP320r1, brainpoolP384r1, brainpoolP512r1 > > Anthony Scarpino has updated the pull request incrementally with one > additional commit since the last revision: > > remove JDKOPT_DETECT_INTREE_EC from configure.ac src/jdk.crypto.ec/share/classes/sun/security/ec/SunEC.java line 219: > 217: > 218: Collection supportedCurves; > 219: supportedCurves = CurveDB.getSupportedCurves(); Shouldn't the supportedCurves be the hardcoded 3 curves? - PR: https://git.openjdk.java.net/jdk/pull/289
Re: RFR 6722928: Support SSPI as a native GSS-API provider
Hi Max, Thanks for the clarification on DecryptMessage. Updated webrev looks fine. Regards, Valerie On 6/9/2019 7:30 PM, Weijun Wang wrote: Updated webrev at http://cr.openjdk.java.net/~weijun/6722928/webrev.08/ @build-dev guys, I realize I've never included you in this code review. Please take a look. @Valerie, All comments accepted except for one (see below). In fact, I think I found a bug in gss_release_context that it might release a cred handle passed in, so I add a isLocalCred flag. However, I test it on my local Windows and it seems the same handle can be FreeCredentialsHandle and then used and then freed again. On Jun 7, 2019, at 10:45 AM, Valerie Peng wrote: Hi, Max, - line 424: the "(used to be const)" comment can now be removed. - line 396-403: on line 338, there is no need to go to err as no memory has been allocated. What happens when jumping to err but the variables, i.e. value and name, have not been declared? Line 400-401 seems not used as there is no more goto err after line 391. - line 528: the size of buffer here is 4*len + 1, but then when calling WideCharToMultiByte, the 6th argument is len. Seems inconsistent? line 534: shouldn't we free "buffer" here? - line 596: free cred allocated on line 588? line 610 and 617: free cred and cred->phCredK? line 638 and 644, 648 and 653: free cred, cred->phCredK and cred->phCredS? - line 829: free the context handle allocated on line 807? line 879: free newCred? line 901: no memory de-allocation before returning error? line 921: seems redundant given line 918. - line 1071: based on gss api doc, context_handle should be set to GSS_C_NO_CONTEXT after deletion. - line 1333: what about secBuff[1].pvBuffer? According to the DecryptMessage spec, "The encrypted message is decrypted in place, overwriting the original contents of its buffer". I've printed out both secBuff[0].pvBuffer and secBuff[1].pvBuffer after the decryption and the latter is indeed inside the former. - line 1390, 1393, 1397: call gss_release_oid_set before returning failure? - line 1471: should we return an error code here when FormatMessage() call failed? Rest looks fine. Thanks, Valerie Thanks, Max [1] https://docs.microsoft.com/en-us/windows/desktop/api/sspi/nf-sspi-decryptmessage On 6/4/2019 2:52 AM, Weijun Wang wrote: I uploaded an updated webrev in place. The only changes are: 1. s/SSPI_TRACE/SSPI_BRIDGE_TRACE/ in sspi.cpp 2. Several copyright year updates. 3. Remove one unused import. Thanks, Max On May 30, 2019, at 11:18 AM, Weijun Wang wrote: Here is the latest webrev http://cr.openjdk.java.net/~weijun/6722928/webrev.07/
Re: RFR 8205445: Add RSASSA-PSS Signature support to SunMSCAPI
Great, that's good then.
Valerie
On 6/22/2018 5:40 PM, Weijun Wang wrote:
On Jun 23, 2018, at 8:35 AM, Valerie Peng wrote:
On 6/22/2018 3:23 PM, Weijun Wang wrote:
On Jun 23, 2018, at 2:30 AM, Valerie Peng wrote:
Max,
Good catch on the SunRsaSign provider bug.
Looking at the changes, I think we may have to fine-grain the check on the
ensureInit() call, i.e.
use ensureInit(boolean sign) instead of ensureInit(), as the current method only
ensures that at least one of the privKey, pubKey or fallbackSignature is non-null, I
think it should check the right one is non-null, i.e. sign -> privKey, verify
-> pubKey/fallbackSignature.
Could anything go wrong? This method just ensures one of initSign() or
initVerify() is called.
Only when the initSign()/initVerify() does not match the subsequent calls of
sign()/verify() I suppose.
I see what you mean.
The Signature class takes care of it:
public final byte[] sign() throws SignatureException {
if (state == SIGN) {
return engineSign();
}
throw new SignatureException("object not initialized for " +
"signing");
}
Thanks
Max
Valerie
In the PSS class engineInitVerify(...) method if the specified key is a MSCAPI
public key, then fallbackSignature is set to null and the native
verifyPssSignedHash method is used, right?
Yes. The native method only fails when trying to import from a blob.
Thanks
Max
Thanks,
Valerie
On 6/21/2018 10:39 PM, Weijun Wang wrote:
Webrev updated at
http://cr.openjdk.java.net/~weijun/8205445/webrev.01
I think I found a bug in SunRsaSign of the RSASSA-PSS signature. Fixed and
added a test.
BTW, I commented out the debug code in security.cpp. Once there is a bug I can
use it.
Thanks
Max
On Jun 21, 2018, at 11:23 PM, Weijun Wang
wrote:
On Jun 21, 2018, at 11:07 PM, Xuelei Fan
wrote:
Hi Weijun,
The release note and the following notes look reasonable to me.
For the implementation part, could it be a little bit more straightforward if
wrapping the new attributes (pss/pssParams/fallbackSignature) and codes (if
pss/fallbackSignature, etc) in the PSS subclass?
Sounds good. I'll try it.
Did you want to remove the debug code in the security.cpp? It seems that they
are not used any more.
Sure I can.
Thanks
Max
Xuelei
On 6/21/2018 4:12 AM, Weijun Wang wrote:
Please take a review on this change
http://cr.openjdk.java.net/~weijun/8205445/webrev.00/
and the release note at
https://bugs.openjdk.java.net/browse/JDK-8205471
The code change adds RSASSA-PSS signature support to the SunMSCAPI provider.
Several notes:
1. CryptoAPI (which SunMSCAPI is based on and now a deprecated technology) does
not support RSASSA-PSS. In fact, CNG [1] is used to perform the signing and
verification. This is certainly not a perfect solution and we are thinking of
support CNG in a more sophisticated way in future releases of JDK.
2. For unknown reason, the newly added verification code for RSASSA-PSS does
not work correctly (precisely, ::NCryptTranslateHandle returns
NTE_INVALID_PARAMETER). A fallback mechanism is added into
mscapi/RSASignature.java. A SunRsaSign Signature object is actually used when a
SunMSCAPI Signature is initialized to verify an RSASSA-PSS signature.
3. It looks like CNG only supports PSSParamterSpec with the same message hash
algorithm and MGF1 hash algorithm, because there is only one algorithm field in
BCRYPT_PSS_PADDING_INFO [2]. This is checked when setting the parameter.
4. It looks like CNG only supports RSASSA-PSS using these hash algorithms: SHA-1,
SHA-256, SHA-384, and SHA-512. This is not checked at parameter setting but sign() will
throw a SignatureException saying "Unrecognised hash algorithm". Since the
verify() side uses a fallback SunRsaSign signature, other hash algorithms are supported.
Thanks
Max
[1]
https://msdn.microsoft.com/en-us/library/windows/desktop/aa376210(v=vs.85).aspx
[2]
https://msdn.microsoft.com/en-us/library/windows/desktop/aa375529(v=vs.85).aspx
[3]
https://msdn.microsoft.com/en-us/library/windows/desktop/aa375534(v=vs.85).aspx
Re: RFR 8205445: Add RSASSA-PSS Signature support to SunMSCAPI
On 6/22/2018 3:23 PM, Weijun Wang wrote: On Jun 23, 2018, at 2:30 AM, Valerie Peng wrote: Max, Good catch on the SunRsaSign provider bug. Looking at the changes, I think we may have to fine-grain the check on the ensureInit() call, i.e. use ensureInit(boolean sign) instead of ensureInit(), as the current method only ensures that at least one of the privKey, pubKey or fallbackSignature is non-null, I think it should check the right one is non-null, i.e. sign -> privKey, verify -> pubKey/fallbackSignature. Could anything go wrong? This method just ensures one of initSign() or initVerify() is called. Only when the initSign()/initVerify() does not match the subsequent calls of sign()/verify() I suppose. Valerie In the PSS class engineInitVerify(...) method if the specified key is a MSCAPI public key, then fallbackSignature is set to null and the native verifyPssSignedHash method is used, right? Yes. The native method only fails when trying to import from a blob. Thanks Max Thanks, Valerie On 6/21/2018 10:39 PM, Weijun Wang wrote: Webrev updated at http://cr.openjdk.java.net/~weijun/8205445/webrev.01 I think I found a bug in SunRsaSign of the RSASSA-PSS signature. Fixed and added a test. BTW, I commented out the debug code in security.cpp. Once there is a bug I can use it. Thanks Max On Jun 21, 2018, at 11:23 PM, Weijun Wang wrote: On Jun 21, 2018, at 11:07 PM, Xuelei Fan wrote: Hi Weijun, The release note and the following notes look reasonable to me. For the implementation part, could it be a little bit more straightforward if wrapping the new attributes (pss/pssParams/fallbackSignature) and codes (if pss/fallbackSignature, etc) in the PSS subclass? Sounds good. I'll try it. Did you want to remove the debug code in the security.cpp? It seems that they are not used any more. Sure I can. Thanks Max Xuelei On 6/21/2018 4:12 AM, Weijun Wang wrote: Please take a review on this change http://cr.openjdk.java.net/~weijun/8205445/webrev.00/ and the release note at https://bugs.openjdk.java.net/browse/JDK-8205471 The code change adds RSASSA-PSS signature support to the SunMSCAPI provider. Several notes: 1. CryptoAPI (which SunMSCAPI is based on and now a deprecated technology) does not support RSASSA-PSS. In fact, CNG [1] is used to perform the signing and verification. This is certainly not a perfect solution and we are thinking of support CNG in a more sophisticated way in future releases of JDK. 2. For unknown reason, the newly added verification code for RSASSA-PSS does not work correctly (precisely, ::NCryptTranslateHandle returns NTE_INVALID_PARAMETER). A fallback mechanism is added into mscapi/RSASignature.java. A SunRsaSign Signature object is actually used when a SunMSCAPI Signature is initialized to verify an RSASSA-PSS signature. 3. It looks like CNG only supports PSSParamterSpec with the same message hash algorithm and MGF1 hash algorithm, because there is only one algorithm field in BCRYPT_PSS_PADDING_INFO [2]. This is checked when setting the parameter. 4. It looks like CNG only supports RSASSA-PSS using these hash algorithms: SHA-1, SHA-256, SHA-384, and SHA-512. This is not checked at parameter setting but sign() will throw a SignatureException saying "Unrecognised hash algorithm". Since the verify() side uses a fallback SunRsaSign signature, other hash algorithms are supported. Thanks Max [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa376210(v=vs.85).aspx [2] https://msdn.microsoft.com/en-us/library/windows/desktop/aa375529(v=vs.85).aspx [3] https://msdn.microsoft.com/en-us/library/windows/desktop/aa375534(v=vs.85).aspx
Re: RFR 8205445: Add RSASSA-PSS Signature support to SunMSCAPI
Max, Good catch on the SunRsaSign provider bug. Looking at the changes, I think we may have to fine-grain the check on the ensureInit() call, i.e. use ensureInit(boolean sign) instead of ensureInit(), as the current method only ensures that at least one of the privKey, pubKey or fallbackSignature is non-null, I think it should check the right one is non-null, i.e. sign -> privKey, verify -> pubKey/fallbackSignature. In the PSS class engineInitVerify(...) method if the specified key is a MSCAPI public key, then fallbackSignature is set to null and the native verifyPssSignedHash method is used, right? Thanks, Valerie On 6/21/2018 10:39 PM, Weijun Wang wrote: Webrev updated at http://cr.openjdk.java.net/~weijun/8205445/webrev.01 I think I found a bug in SunRsaSign of the RSASSA-PSS signature. Fixed and added a test. BTW, I commented out the debug code in security.cpp. Once there is a bug I can use it. Thanks Max On Jun 21, 2018, at 11:23 PM, Weijun Wang wrote: On Jun 21, 2018, at 11:07 PM, Xuelei Fan wrote: Hi Weijun, The release note and the following notes look reasonable to me. For the implementation part, could it be a little bit more straightforward if wrapping the new attributes (pss/pssParams/fallbackSignature) and codes (if pss/fallbackSignature, etc) in the PSS subclass? Sounds good. I'll try it. Did you want to remove the debug code in the security.cpp? It seems that they are not used any more. Sure I can. Thanks Max Xuelei On 6/21/2018 4:12 AM, Weijun Wang wrote: Please take a review on this change http://cr.openjdk.java.net/~weijun/8205445/webrev.00/ and the release note at https://bugs.openjdk.java.net/browse/JDK-8205471 The code change adds RSASSA-PSS signature support to the SunMSCAPI provider. Several notes: 1. CryptoAPI (which SunMSCAPI is based on and now a deprecated technology) does not support RSASSA-PSS. In fact, CNG [1] is used to perform the signing and verification. This is certainly not a perfect solution and we are thinking of support CNG in a more sophisticated way in future releases of JDK. 2. For unknown reason, the newly added verification code for RSASSA-PSS does not work correctly (precisely, ::NCryptTranslateHandle returns NTE_INVALID_PARAMETER). A fallback mechanism is added into mscapi/RSASignature.java. A SunRsaSign Signature object is actually used when a SunMSCAPI Signature is initialized to verify an RSASSA-PSS signature. 3. It looks like CNG only supports PSSParamterSpec with the same message hash algorithm and MGF1 hash algorithm, because there is only one algorithm field in BCRYPT_PSS_PADDING_INFO [2]. This is checked when setting the parameter. 4. It looks like CNG only supports RSASSA-PSS using these hash algorithms: SHA-1, SHA-256, SHA-384, and SHA-512. This is not checked at parameter setting but sign() will throw a SignatureException saying "Unrecognised hash algorithm". Since the verify() side uses a fallback SunRsaSign signature, other hash algorithms are supported. Thanks Max [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa376210(v=vs.85).aspx [2] https://msdn.microsoft.com/en-us/library/windows/desktop/aa375529(v=vs.85).aspx [3] https://msdn.microsoft.com/en-us/library/windows/desktop/aa375534(v=vs.85).aspx
[9] RFR 8079898: Resolve disabled warnings for libj2ucrypto
Anyone can help reviewing this? The fix is straight forward, just renamed the DEBUG to J2UC_DEBUG to address the E_MACRO_REDEFINED warning. In addition, I also updated the nativeCrypto.h to remove the workaround for a Solaris12-specific issue which has now been fixed. Bug: https://bugs.openjdk.java.net/browse/JDK-8079898 Webrev: http://cr.openjdk.java.net/~valeriep/8079898/webrev.00/ JPRT result looks fine. Thanks, Valerie
Re: [9] RFR 8086002: Move apple.security.AppleProvider to a proper module
Ok, thanks! Valerie On 8/18/2015 12:34 AM, Erik Joelsson wrote: One is enough. /Erik On 2015-08-18 02:21, Valerie Peng wrote: Thanks for the review. Is one more reviewer from build team needed? Valerie On 8/14/2015 4:58 PM, Mandy Chung wrote: Looks good. Mandy On Aug 14, 2015, at 4:30 PM, Valerie Peng wrote: Updated the webrev in place to use "osxsecurity" given peer feedbacks. Thanks, Valerie On 8/13/2015 7:31 PM, Valerie Peng wrote: Can someone please help reviewing this change? This is to move Apple provider from jdk.deploy.osx module to java.base module. The native library for Apple provider is separated out from the "osx" one generated in jdk.deploy.osx module and named "osxapple" (sort of following the convention of SunMSCAPI provider whose native library is named "sunmscapi"). webrev: http://cr.openjdk.java.net/~valeriep/8086002/webrev.00/ Thanks, Valerie
Re: [9] RFR 8086002: Move apple.security.AppleProvider to a proper module
Thanks for the review. Is one more reviewer from build team needed? Valerie On 8/14/2015 4:58 PM, Mandy Chung wrote: Looks good. Mandy On Aug 14, 2015, at 4:30 PM, Valerie Peng wrote: Updated the webrev in place to use "osxsecurity" given peer feedbacks. Thanks, Valerie On 8/13/2015 7:31 PM, Valerie Peng wrote: Can someone please help reviewing this change? This is to move Apple provider from jdk.deploy.osx module to java.base module. The native library for Apple provider is separated out from the "osx" one generated in jdk.deploy.osx module and named "osxapple" (sort of following the convention of SunMSCAPI provider whose native library is named "sunmscapi"). webrev: http://cr.openjdk.java.net/~valeriep/8086002/webrev.00/ Thanks, Valerie
Re: [9] RFR 8086002: Move apple.security.AppleProvider to a proper module
Updated the webrev in place to use "osxsecurity" given peer feedbacks. Thanks, Valerie On 8/13/2015 7:31 PM, Valerie Peng wrote: Can someone please help reviewing this change? This is to move Apple provider from jdk.deploy.osx module to java.base module. The native library for Apple provider is separated out from the "osx" one generated in jdk.deploy.osx module and named "osxapple" (sort of following the convention of SunMSCAPI provider whose native library is named "sunmscapi"). webrev: http://cr.openjdk.java.net/~valeriep/8086002/webrev.00/ Thanks, Valerie
[9] RFR 8086002: Move apple.security.AppleProvider to a proper module
Can someone please help reviewing this change? This is to move Apple provider from jdk.deploy.osx module to java.base module. The native library for Apple provider is separated out from the "osx" one generated in jdk.deploy.osx module and named "osxapple" (sort of following the convention of SunMSCAPI provider whose native library is named "sunmscapi"). webrev: http://cr.openjdk.java.net/~valeriep/8086002/webrev.00/ Thanks, Valerie
[9] RFR 8130665 "java/lang/SecurityManager/CheckSecurityProvider.java failing in jake on OS X"
Hi, Sean and build experts, Can you please review the fix for 8130665 "java/lang/SecurityManager/CheckSecurityProvider.java failing in jake on OS X"? The Apple provider fails to be instantiated in Jake due to the missing permission grants. However, it currently only fails in Jake workspace and fix is verified by running JPRT against Jake. Given that Apple provider only presents for Mac OSX, its policy file is under the macosx directory and I have to make minor modifications to pick up that file for MacOSX build. Here is the webrev: http://cr.openjdk.java.net/~valeriep/8130665/webrev.00/ Thanks, Valerie
Re: RFR 7191662: JCE providers should be located via ServiceLoader
It seems that the jimage refresh change may still take some time, so we will end up removing the makefile related changes and then deferring the ServiceLoader related changes to Jake workspace. Here is the latest webrev (Security source/test changes only, no more makefile changes) http://cr.openjdk.java.net/~valeriep/7191662/webrev.04/ Summary of changes from webrev.03: 1) switch back to provider class names for java.security file 2) separated out the java.policy change into its own as SQE has filed a bug for it 3) updated sun.security.jca.Providers class due to 1) 4) fixed sun.security.tools.jarsigner.Main to use the new Provider.configure() api 5) fixed a bug in sun.security.pkcs11.PKCS11Test regarding searching and configuring PKCS11 provider Thanks, Valerie On 6/8/2015 4:44 PM, Valerie Peng wrote: What is the bug id for the image refresh change? Just so I can keep a watch and hold my changes for the time being. My webrev has a new regression test which compares the list of providers found by ServiceLoader and Security.getProviders() call. So, if the META-INF/services/java.security.Provider file content is not correct, the new test would fail and it's a clear indication that ServiceLoader can't find one or more of the providers. Thanks, Valerie On 6/5/2015 10:43 PM, Mandy Chung wrote: On Jun 5, 2015, at 4:32 PM, Valerie Peng wrote: I don't know image builder well enough to answer your question. The current implementation of the image builder sorts the modules by their name that are mapped to the same class loader. That explains why java.naming is the first one containing META-INF/services/java.security.Provider in your current list. There is no guarantee in what particular order a module is processed first. I don’t know if the jimage refresh work will change the ordering either. Since this is temporary, I’m okay if you want to depend on the image build implementation as long as you have a test to catch it and verify java.naming/META-INF/services/java.security.Provider file content. The existing security tests may also catch it but I think it’s not obvious to indicate that the security tests fail because of the issue of the merged service configuration file. Please also hold off until the image refresh change goes into jdk9/dev so that you can verify if your build change still works. If you want to push earlier, another alternative we discussed earlier is to separate the META-INF/services file and make change and java.security to keep using classname. That can be pushed that in a second phase with a proper image builder support. Mandy Based on my own experiment, it seems to pick up the one from java.naming when duplication occurs, so that's why I saved the merged result to there and named the Gensrc makefile with java.naming. The result build work fine. Does this explain what I am trying to do here? If you have better ways to get this done, I am certainly open to that idea. Thanks, Valerie On 6/5/2015 12:21 AM, Erik Joelsson wrote: Hello Valerie, The merging seems ok, but I thought there was non determinism in the image builder regarding which provider would get picked up. Is that resolved or do you really need to override all of those providers with your generated file in gensrc? I can assist in writing that makefile logic if needed. /Erik On 2015-06-04 23:58, Valerie Peng wrote: Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the
Re: RFR 7191662: JCE providers should be located via ServiceLoader
What is the bug id for the image refresh change? Just so I can keep a watch and hold my changes for the time being. My webrev has a new regression test which compares the list of providers found by ServiceLoader and Security.getProviders() call. So, if the META-INF/services/java.security.Provider file content is not correct, the new test would fail and it's a clear indication that ServiceLoader can't find one or more of the providers. Thanks, Valerie On 6/5/2015 10:43 PM, Mandy Chung wrote: On Jun 5, 2015, at 4:32 PM, Valerie Peng wrote: I don't know image builder well enough to answer your question. The current implementation of the image builder sorts the modules by their name that are mapped to the same class loader. That explains why java.naming is the first one containing META-INF/services/java.security.Provider in your current list. There is no guarantee in what particular order a module is processed first. I don’t know if the jimage refresh work will change the ordering either. Since this is temporary, I’m okay if you want to depend on the image build implementation as long as you have a test to catch it and verify java.naming/META-INF/services/java.security.Provider file content. The existing security tests may also catch it but I think it’s not obvious to indicate that the security tests fail because of the issue of the merged service configuration file. Please also hold off until the image refresh change goes into jdk9/dev so that you can verify if your build change still works. If you want to push earlier, another alternative we discussed earlier is to separate the META-INF/services file and make change and java.security to keep using classname. That can be pushed that in a second phase with a proper image builder support. Mandy Based on my own experiment, it seems to pick up the one from java.naming when duplication occurs, so that's why I saved the merged result to there and named the Gensrc makefile with java.naming. The result build work fine. Does this explain what I am trying to do here? If you have better ways to get this done, I am certainly open to that idea. Thanks, Valerie On 6/5/2015 12:21 AM, Erik Joelsson wrote: Hello Valerie, The merging seems ok, but I thought there was non determinism in the image builder regarding which provider would get picked up. Is that resolved or do you really need to override all of those providers with your generated file in gensrc? I can assist in writing that makefile logic if needed. /Erik On 2015-06-04 23:58, Valerie Peng wrote: Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie I'm not sure I followed completely here were this landed. Does this mean that there's currently no need for a build system code review on http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/, and that a new webrev will be posted instead? /Magnus
Re: RFR 7191662: JCE providers should be located via ServiceLoader
I don't know image builder well enough to answer your question. Based on my own experiment, it seems to pick up the one from java.naming when duplication occurs, so that's why I saved the merged result to there and named the Gensrc makefile with java.naming. The result build work fine. Does this explain what I am trying to do here? If you have better ways to get this done, I am certainly open to that idea. Thanks, Valerie On 6/5/2015 12:21 AM, Erik Joelsson wrote: Hello Valerie, The merging seems ok, but I thought there was non determinism in the image builder regarding which provider would get picked up. Is that resolved or do you really need to override all of those providers with your generated file in gensrc? I can assist in writing that makefile logic if needed. /Erik On 2015-06-04 23:58, Valerie Peng wrote: Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie I'm not sure I followed completely here were this landed. Does this mean that there's currently no need for a build system code review on http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/, and that a new webrev will be posted instead? /Magnus . Mandy On May 27, 2015, at 3:29 PM, Valerie Peng wrote: Hi, build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/ This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. The rest of source code changes are reviewed by my team already. Thanks, Valerie (Java Security Team)
Re: RFR 7191662: JCE providers should be located via ServiceLoader
Build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.03 This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. Thanks, Valerie On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote: Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie I'm not sure I followed completely here were this landed. Does this mean that there's currently no need for a build system code review on http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/, and that a new webrev will be posted instead? /Magnus . Mandy On May 27, 2015, at 3:29 PM, Valerie Peng wrote: Hi, build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/ This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. The rest of source code changes are reviewed by my team already. Thanks, Valerie (Java Security Team)
Re: RFR 7191662: JCE providers should be located via ServiceLoader
Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie . Mandy On May 27, 2015, at 3:29 PM, Valerie Peng wrote: Hi, build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/ This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. The rest of source code changes are reviewed by my team already. Thanks, Valerie (Java Security Team)
RFR 7191662: JCE providers should be located via ServiceLoader
Hi, build experts, Can you please review the make file related change, i.e. the new file || *make/gensrc/Gensrc-java.naming.gmk*, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/ This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. The rest of source code changes are reviewed by my team already. Thanks, Valerie (Java Security Team)
Re: RFR 8046002: Move Ucrypto to the open jdk repo
Why is it still using S10u6? Isn't the minimum Solaris version S11? The ucrypto headers can be found on machines with S10u10 and later versions. Thanks, Valerie On 10/20/2014 9:47 PM, David Holmes wrote: On 21/10/2014 2:39 PM, Phil Race wrote: hudson builds are now failing as below. Did this change below break dev ? FYI this was already filed by RE: https://bugs.openjdk.java.net/browse/JDK-8061574 david Compiling ec2_aff.c (for libsunec.so) /localtools/solaris-amd64/SUNWspro/SS12u3-Solaris10u6/SS12u3-slim/bin/cc -DTRACING -DMACRO_MEMSYS_OPS -DBREAKPTS -DcpuIntel -Di586 -Damd64 -D_LITTLE_ENDIAN= -DSOLARIS -DARCH='"amd64"' -Damd64 -DDEBUG -DRELEASE='"1.9.0-ea-fastdebug"' -I/scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/jdk/src/java.base/share/native/include -I/scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/jdk/src/java.base/solaris/native/include -I/scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/jdk/src/java.base/unix/native/include -m64 -D__solaris__ -erroff=E_BAD_PRAGMA_PACK_VALUE -xc99=%none -xCC -errshort=tags -Xa -v -mt -W0,-noglobal -g -xs -KPIC -xstrconst -DMP_API_COMPATIBLE -DNSS_ECC_MORE_THAN_SUITE_B -xO2 -Wu,-O2~yz -xregs=no%frameptr -I/localtools/solaris-amd64/SUNWspro/SS12u3-Solaris10u6/sysroot/usr/include -DTHIS_FILE='"ec2_aff.c"' -c -xMMD -xMF /scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/build/solaris-amd64/jdk/objs/libsunec/ec2_aff.d.tmp -o /scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/build/solaris-amd64/jdk/objs/libsunec/ec2_aff.o /scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/jdk/src/jdk.crypto.ec/share/native/libsunec/impl/ec2_aff.c /usr/bin/find /scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/build/solaris-amd64/jdk/modules/java.naming -type f -a \( -name FILE_NAME_THAT_DOESNT_EXIST -o -name "*.class" -o -name "*.dat" \) | /usr/bin/ggrep -f /scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/build/solaris-amd64/images/lib//_the.charsets.jar_include | /usr/bin/gsed 's|/scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/build/solaris-amd64/jdk/modules/java.naming/|-C /scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/build/solaris-amd64/jdk/modules/java.naming |g' >> /scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/build/solaris-amd64/images/lib//_the.charsets.jar_contents "/scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/jdk/src/jdk.crypto.ucrypto/solaris/native/libj2ucrypto/nativeCrypto.h", line 45: warning: macro redefined: DEBUG "/scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/jdk/src/jdk.crypto.ucrypto/solaris/native/libj2ucrypto/nativeCrypto.c", line 181: undefined symbol: CK_AES_CTR_PARAMS "/scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/jdk/src/jdk.crypto.ucrypto/solaris/native/libj2ucrypto/nativeCrypto.c", line 182: syntax error before or at: ) "/scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/jdk/src/jdk.crypto.ucrypto/solaris/native/libj2ucrypto/nativeCrypto.c", line 185: syntax error before or at: ) "/scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/jdk/src/jdk.crypto.ucrypto/solaris/native/libj2ucrypto/nativeCrypto.c", line 186: syntax error before or at: ) "/scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/jdk/src/jdk.crypto.ucrypto/solaris/native/libj2ucrypto/nativeCrypto.c", line 213: undefined symbol: CK_AES_CTR_PARAMS "/scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/jdk/src/jdk.crypto.ucrypto/solaris/native/libj2ucrypto/nativeCrypto.c", line 213: syntax error before or at: ) "/scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/jdk/src/jdk.crypto.pkcs11/unix/native/libj2pkcs11/j2secmod_md.c", line 61: warning: declaration can not follow a statement cc: acomp failed for /scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/jdk/src/jdk.crypto.ucrypto/solaris/native/libj2ucrypto/nativeCrypto.c Lib-jdk.crypto.ucrypto.gmk:35: recipe for target '/scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/build/solaris-amd64/jdk/objs/libj2ucrypto/nativeCrypto.o' failed gmake[3]: *** [/scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/build/solaris-amd64/jdk/objs/libj2ucrypto/nativeCrypto.o] Error 2 gmake[3]: Leaving directory '/scratch/java_re/builds/workspace/9-2-build-solaris-amd64/jdk9-dev/1534/jdk/make/lib' Main.gmk:182: recipe for target 'jdk.crypto.ucrypto-libs' failed gmake[2]: *** [jdk.
Re: RFR 8046002: Move Ucrypto to the open jdk repo
Ok, I have updated the Copy-java.base.gmk with your suggestion, i.e. folded the ifndef part into the previous ifeq. Webrev updated: http://cr.openjdk.java.net/~valeriep/8046002/webrev.02/ Thanks! Valerie On 10/16/2014 1:00 AM, Erik Joelsson wrote: Hello Valierie, In Copy-java.base.gmk, you could change the findstring construct to a simple "ifeq ($(OPENJDK_TARGET_OS), windows)", perhaps even fold the whole ifndef OPENJDK into the previous ifeq since they are now the same. Otherwise it looks good to me. /Erik On 2014-10-15 20:46, Valerie Peng wrote: Hi, build experts, Could you please review the build-related changes for 8046002: Move Ucrypto to the open jdk repo? The rest (src/test) has been reviewed by my peers in security team. Bug: https://bugs.openjdk.java.net/browse/JDK-8046002 Webrev: http://cr.openjdk.java.net/~valeriep/8046002/webrev.01/ Thanks, Valerie
Re: RFR 8046002: Move Ucrypto to the open jdk repo
Well, it turns out that I do have to update modules.xml with the jdk.crypto.ucrypto module info (Thanks to a tip from Mandy). I have sent the necessary webrev for this to jigsaw alias separately. Thanks, Valerie On 10/15/2014 2:39 PM, Valerie Peng wrote: The /modules.xml already allows jdk.crypto.ucrypto to access sun.nio.cs, sun.security.action, sun.security.internal.spec, sun.security.util packages. The build completes and no module access errors. So, it seems to me that current content of modules.xml is fine. Thanks, Valerie On 10/15/2014 11:54 AM, Alan Bateman wrote: On 15/10/2014 19:46, Valerie Peng wrote: Hi, build experts, Could you please review the build-related changes for 8046002: Move Ucrypto to the open jdk repo? The rest (src/test) has been reviewed by my peers in security team. Bug: https://bugs.openjdk.java.net/browse/JDK-8046002 Webrev: http://cr.openjdk.java.net/~valeriep/8046002/webrev.01/ Is there a webrev for the top-level repo? I assume that at least /modules.xml will need to be updated. -Alan.
Re: RFR 8046002: Move Ucrypto to the open jdk repo
The /modules.xml already allows jdk.crypto.ucrypto to access sun.nio.cs, sun.security.action, sun.security.internal.spec, sun.security.util packages. The build completes and no module access errors. So, it seems to me that current content of modules.xml is fine. Thanks, Valerie On 10/15/2014 11:54 AM, Alan Bateman wrote: On 15/10/2014 19:46, Valerie Peng wrote: Hi, build experts, Could you please review the build-related changes for 8046002: Move Ucrypto to the open jdk repo? The rest (src/test) has been reviewed by my peers in security team. Bug: https://bugs.openjdk.java.net/browse/JDK-8046002 Webrev: http://cr.openjdk.java.net/~valeriep/8046002/webrev.01/ Is there a webrev for the top-level repo? I assume that at least /modules.xml will need to be updated. -Alan.
RFR 8046002: Move Ucrypto to the open jdk repo
Hi, build experts, Could you please review the build-related changes for 8046002: Move Ucrypto to the open jdk repo? The rest (src/test) has been reviewed by my peers in security team. Bug: https://bugs.openjdk.java.net/browse/JDK-8046002 Webrev: http://cr.openjdk.java.net/~valeriep/8046002/webrev.01/ Thanks, Valerie
Re: RFR 8039898: sunpkcs11-solaris.cfg should be in solaris specific directory
Thanks all for the review and tips on webrev. I have re-generated the webrev following all the tips... Thanks again, Valerie On 9/11/2014 3:42 AM, Magnus Ihse Bursie wrote: On 2014-09-11 11:51, Alan Bateman wrote: On 11/09/2014 10:38, Magnus Ihse Bursie wrote: The webrev shows the file as being moved outside the control of mercurial. That is, if you do "hg mv" to move the file, the history of the file will be kept intact. Otherwise it will look like a new file in the repo. (Sometimes this doesn't show up properly in the webrev, apologies if you already did this.) For webrev then I think it depends on whether the changes have been committed or not. If you do a hg mv and then webrev -N before committing then it will show as a move. If you commit and then generate the webrev then it looks like a delete + new file. Maybe there is an opportunity for someone to see if webrev can be made a bit smarter. Aha, that's the reason. I know it "usually" work but not always, but I have not seen the pattern. (I usually create pre-checkin webrevs.) /Magnus
RFR 8039898: sunpkcs11-solaris.cfg should be in solaris specific directory
Could someone please review this build related change for moving sunpkcs11-solaris.cfg file to the pkcs11 module? Webrev: http://cr.openjdk.java.net/~valeriep/8039898/webrev.00/ Thanks, Valerie
