Re: RFR[15] 8248505: Unexpected NoSuchAlgorithmException when using secure random impl from BCFIPS provider
Thanks! Valerie On 7/7/2020 9:00 AM, Weijun Wang wrote: I see. No more comment. --Max On Jul 7, 2020, at 11:53 PM, Valerie Peng wrote: Hi Max, Thanks for your review. Algorithm is also there, so both work technically. With existing APIs, the only way to check for service registration is to call getService(...) and check for ==. If there were another way to check service registration, then it makes sense to store Service object. Valerie On 7/7/2020 3:44 AM, Weijun Wang wrote: On Jul 7, 2020, at 12:33 AM, Valerie Peng wrote: Hi Max, The suggested fix is not much different than the suggested webrev. I understand they are mostly the same. The essential change is to call getService(...) for the returned service in Provider.getDefaultSecureRandomService(). The only difference here is using the hardcoded type "SecureRandom" vs the one returned by getType() call. Is this what you are referring to? That's not a problem. I re-write the rest to store String instead of Service as it may seem strange why the stored Service is not used but re-queried through getService(...). Also, looks cleaner to me this way. It's good if you like it. I was thinking that the Service object is already there (in parseLegacyPut and implRemoveService) and it's simpler to store them in the map, and maybe later you can detect whether that Service is registered and decide if you can simply return it or a create a new one by calling getService(). Thanks, Max Thanks, Valerie On 7/2/2020 9:05 PM, Weijun Wang wrote: Hi Valerie, How about the suggested fix from the bug reporter? Thanks, Max On Jul 3, 2020, at 4:52 AM, Valerie Peng wrote: Hi Max and Sean, Can you help reviewing this fix for JDK-8248505? This is the followup fix for JDK-8246613 "Choose the default SecureRandom algo based on registration ordering" which you reviewed earlier. Based on the feedback, BCFIPS provider overrides putService/getService() calls which does not work well with the fix for JDK-8246613. Thus, I adapted to store the SecureRandom algorithm names internally and then return the result from getService(...) when Provider.getDefaultSecureRandomService() is called. Updated the regression test to include a custom provider which simulates the BCFIPS provider behavior. Bug: https://bugs.openjdk.java.net/browse/JDK-8248505 Webrev: http://cr.openjdk.java.net/~valeriep/8248505/webrev.00/ Thanks, Valerie
Re: RFR[15] 8248505: Unexpected NoSuchAlgorithmException when using secure random impl from BCFIPS provider
I see. No more comment. --Max > On Jul 7, 2020, at 11:53 PM, Valerie Peng wrote: > > Hi Max, > > Thanks for your review. > > Algorithm is also there, so both work technically. With existing APIs, the > only way to check for service registration is to call getService(...) and > check for ==. If there were another way to check service registration, then > it makes sense to store Service object. > > Valerie > > On 7/7/2020 3:44 AM, Weijun Wang wrote: >>> On Jul 7, 2020, at 12:33 AM, Valerie Peng wrote: >>> >>> Hi Max, >>> >>> The suggested fix is not much different than the suggested webrev. >> I understand they are mostly the same. >> >>> The essential change is to call getService(...) for the returned service in >>> Provider.getDefaultSecureRandomService(). The only difference here is using >>> the hardcoded type "SecureRandom" vs the one returned by getType() call. Is >>> this what you are referring to? >> That's not a problem. >> >>> I re-write the rest to store String instead of Service as it may seem >>> strange why the stored Service is not used but re-queried through >>> getService(...). Also, looks cleaner to me this way. >> It's good if you like it. I was thinking that the Service object is already >> there (in parseLegacyPut and implRemoveService) and it's simpler to store >> them in the map, and maybe later you can detect whether that Service is >> registered and decide if you can simply return it or a create a new one by >> calling getService(). >> >> Thanks, >> Max >> >>> Thanks, >>> Valerie >>> On 7/2/2020 9:05 PM, Weijun Wang wrote: Hi Valerie, How about the suggested fix from the bug reporter? Thanks, Max > On Jul 3, 2020, at 4:52 AM, Valerie Peng wrote: > > Hi Max and Sean, > > Can you help reviewing this fix for JDK-8248505? This is the followup fix > for JDK-8246613 "Choose the default SecureRandom algo based on > registration ordering" which you reviewed earlier. Based on the feedback, > BCFIPS provider overrides putService/getService() calls which does not > work well with the fix for JDK-8246613. Thus, I adapted to store the > SecureRandom algorithm names internally and then return the result from > getService(...) when Provider.getDefaultSecureRandomService() is called. > Updated the regression test to include a custom provider which simulates > the BCFIPS provider behavior. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8248505 > > Webrev: http://cr.openjdk.java.net/~valeriep/8248505/webrev.00/ > > Thanks, > Valerie > >
Re: RFR[15] 8248505: Unexpected NoSuchAlgorithmException when using secure random impl from BCFIPS provider
Hi Max, Thanks for your review. Algorithm is also there, so both work technically. With existing APIs, the only way to check for service registration is to call getService(...) and check for ==. If there were another way to check service registration, then it makes sense to store Service object. Valerie On 7/7/2020 3:44 AM, Weijun Wang wrote: On Jul 7, 2020, at 12:33 AM, Valerie Peng wrote: Hi Max, The suggested fix is not much different than the suggested webrev. I understand they are mostly the same. The essential change is to call getService(...) for the returned service in Provider.getDefaultSecureRandomService(). The only difference here is using the hardcoded type "SecureRandom" vs the one returned by getType() call. Is this what you are referring to? That's not a problem. I re-write the rest to store String instead of Service as it may seem strange why the stored Service is not used but re-queried through getService(...). Also, looks cleaner to me this way. It's good if you like it. I was thinking that the Service object is already there (in parseLegacyPut and implRemoveService) and it's simpler to store them in the map, and maybe later you can detect whether that Service is registered and decide if you can simply return it or a create a new one by calling getService(). Thanks, Max Thanks, Valerie On 7/2/2020 9:05 PM, Weijun Wang wrote: Hi Valerie, How about the suggested fix from the bug reporter? Thanks, Max On Jul 3, 2020, at 4:52 AM, Valerie Peng wrote: Hi Max and Sean, Can you help reviewing this fix for JDK-8248505? This is the followup fix for JDK-8246613 "Choose the default SecureRandom algo based on registration ordering" which you reviewed earlier. Based on the feedback, BCFIPS provider overrides putService/getService() calls which does not work well with the fix for JDK-8246613. Thus, I adapted to store the SecureRandom algorithm names internally and then return the result from getService(...) when Provider.getDefaultSecureRandomService() is called. Updated the regression test to include a custom provider which simulates the BCFIPS provider behavior. Bug: https://bugs.openjdk.java.net/browse/JDK-8248505 Webrev: http://cr.openjdk.java.net/~valeriep/8248505/webrev.00/ Thanks, Valerie
Re: RFR[15] 8248505: Unexpected NoSuchAlgorithmException when using secure random impl from BCFIPS provider
> On Jul 7, 2020, at 12:33 AM, Valerie Peng wrote: > > Hi Max, > > The suggested fix is not much different than the suggested webrev. I understand they are mostly the same. > > The essential change is to call getService(...) for the returned service in > Provider.getDefaultSecureRandomService(). The only difference here is using > the hardcoded type "SecureRandom" vs the one returned by getType() call. Is > this what you are referring to? That's not a problem. > > I re-write the rest to store String instead of Service as it may seem strange > why the stored Service is not used but re-queried through getService(...). > Also, looks cleaner to me this way. It's good if you like it. I was thinking that the Service object is already there (in parseLegacyPut and implRemoveService) and it's simpler to store them in the map, and maybe later you can detect whether that Service is registered and decide if you can simply return it or a create a new one by calling getService(). Thanks, Max > > Thanks, > Valerie > On 7/2/2020 9:05 PM, Weijun Wang wrote: >> Hi Valerie, >> >> How about the suggested fix from the bug reporter? >> >> Thanks, >> Max >> >>> On Jul 3, 2020, at 4:52 AM, Valerie Peng wrote: >>> >>> Hi Max and Sean, >>> >>> Can you help reviewing this fix for JDK-8248505? This is the followup fix >>> for JDK-8246613 "Choose the default SecureRandom algo based on registration >>> ordering" which you reviewed earlier. Based on the feedback, BCFIPS >>> provider overrides putService/getService() calls which does not work well >>> with the fix for JDK-8246613. Thus, I adapted to store the SecureRandom >>> algorithm names internally and then return the result from getService(...) >>> when Provider.getDefaultSecureRandomService() is called. Updated the >>> regression test to include a custom provider which simulates the BCFIPS >>> provider behavior. >>> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8248505 >>> >>> Webrev: http://cr.openjdk.java.net/~valeriep/8248505/webrev.00/ >>> >>> Thanks, >>> Valerie >>> >>>
Re: RFR[15] 8248505: Unexpected NoSuchAlgorithmException when using secure random impl from BCFIPS provider
Updated the webrev to webrev.01 http://cr.openjdk.java.net/~valeriep/8248505/webrev.01/ No changes to source, just test cleanup. Thanks, Valerie On 7/6/2020 12:51 PM, Valerie Peng wrote: Thanks for taking a look, Sean. Agree that we need to get to the root cause for the NSAE which John@Entrust also observed. One possibility is to file another bug for Entrust NSAE and deliver more fix later if this current fix for BCFIPS provider does not resolve Entrust's NSAE. Valerie On 7/6/2020 12:20 PM, Seán Coffey wrote: Your patch looks ok to me Valerie. I note that John mentions an anomaly with your patch - I'm thinking that may need further investigation. regards, Sean. On 06/07/2020 17:33, Valerie Peng wrote: Hi Max, The suggested fix is not much different than the suggested webrev. The essential change is to call getService(...) for the returned service in Provider.getDefaultSecureRandomService(). The only difference here is using the hardcoded type "SecureRandom" vs the one returned by getType() call. Is this what you are referring to? I re-write the rest to store String instead of Service as it may seem strange why the stored Service is not used but re-queried through getService(...). Also, looks cleaner to me this way. Thanks, Valerie On 7/2/2020 9:05 PM, Weijun Wang wrote: Hi Valerie, How about the suggested fix from the bug reporter? Thanks, Max On Jul 3, 2020, at 4:52 AM, Valerie Peng wrote: Hi Max and Sean, Can you help reviewing this fix for JDK-8248505? This is the followup fix for JDK-8246613 "Choose the default SecureRandom algo based on registration ordering" which you reviewed earlier. Based on the feedback, BCFIPS provider overrides putService/getService() calls which does not work well with the fix for JDK-8246613. Thus, I adapted to store the SecureRandom algorithm names internally and then return the result from getService(...) when Provider.getDefaultSecureRandomService() is called. Updated the regression test to include a custom provider which simulates the BCFIPS provider behavior. Bug: https://bugs.openjdk.java.net/browse/JDK-8248505 Webrev: http://cr.openjdk.java.net/~valeriep/8248505/webrev.00/ Thanks, Valerie
Re: RFR[15] 8248505: Unexpected NoSuchAlgorithmException when using secure random impl from BCFIPS provider
Thanks for taking a look, Sean. Agree that we need to get to the root cause for the NSAE which John@Entrust also observed. One possibility is to file another bug for Entrust NSAE and deliver more fix later if this current fix for BCFIPS provider does not resolve Entrust's NSAE. Valerie On 7/6/2020 12:20 PM, Seán Coffey wrote: Your patch looks ok to me Valerie. I note that John mentions an anomaly with your patch - I'm thinking that may need further investigation. regards, Sean. On 06/07/2020 17:33, Valerie Peng wrote: Hi Max, The suggested fix is not much different than the suggested webrev. The essential change is to call getService(...) for the returned service in Provider.getDefaultSecureRandomService(). The only difference here is using the hardcoded type "SecureRandom" vs the one returned by getType() call. Is this what you are referring to? I re-write the rest to store String instead of Service as it may seem strange why the stored Service is not used but re-queried through getService(...). Also, looks cleaner to me this way. Thanks, Valerie On 7/2/2020 9:05 PM, Weijun Wang wrote: Hi Valerie, How about the suggested fix from the bug reporter? Thanks, Max On Jul 3, 2020, at 4:52 AM, Valerie Peng wrote: Hi Max and Sean, Can you help reviewing this fix for JDK-8248505? This is the followup fix for JDK-8246613 "Choose the default SecureRandom algo based on registration ordering" which you reviewed earlier. Based on the feedback, BCFIPS provider overrides putService/getService() calls which does not work well with the fix for JDK-8246613. Thus, I adapted to store the SecureRandom algorithm names internally and then return the result from getService(...) when Provider.getDefaultSecureRandomService() is called. Updated the regression test to include a custom provider which simulates the BCFIPS provider behavior. Bug: https://bugs.openjdk.java.net/browse/JDK-8248505 Webrev: http://cr.openjdk.java.net/~valeriep/8248505/webrev.00/ Thanks, Valerie
Re: RFR[15] 8248505: Unexpected NoSuchAlgorithmException when using secure random impl from BCFIPS provider
Your patch looks ok to me Valerie. I note that John mentions an anomaly with your patch - I'm thinking that may need further investigation. regards, Sean. On 06/07/2020 17:33, Valerie Peng wrote: Hi Max, The suggested fix is not much different than the suggested webrev. The essential change is to call getService(...) for the returned service in Provider.getDefaultSecureRandomService(). The only difference here is using the hardcoded type "SecureRandom" vs the one returned by getType() call. Is this what you are referring to? I re-write the rest to store String instead of Service as it may seem strange why the stored Service is not used but re-queried through getService(...). Also, looks cleaner to me this way. Thanks, Valerie On 7/2/2020 9:05 PM, Weijun Wang wrote: Hi Valerie, How about the suggested fix from the bug reporter? Thanks, Max On Jul 3, 2020, at 4:52 AM, Valerie Peng wrote: Hi Max and Sean, Can you help reviewing this fix for JDK-8248505? This is the followup fix for JDK-8246613 "Choose the default SecureRandom algo based on registration ordering" which you reviewed earlier. Based on the feedback, BCFIPS provider overrides putService/getService() calls which does not work well with the fix for JDK-8246613. Thus, I adapted to store the SecureRandom algorithm names internally and then return the result from getService(...) when Provider.getDefaultSecureRandomService() is called. Updated the regression test to include a custom provider which simulates the BCFIPS provider behavior. Bug: https://bugs.openjdk.java.net/browse/JDK-8248505 Webrev: http://cr.openjdk.java.net/~valeriep/8248505/webrev.00/ Thanks, Valerie
Re: RFR[15] 8248505: Unexpected NoSuchAlgorithmException when using secure random impl from BCFIPS provider
Hi Max, The suggested fix is not much different than the suggested webrev. The essential change is to call getService(...) for the returned service in Provider.getDefaultSecureRandomService(). The only difference here is using the hardcoded type "SecureRandom" vs the one returned by getType() call. Is this what you are referring to? I re-write the rest to store String instead of Service as it may seem strange why the stored Service is not used but re-queried through getService(...). Also, looks cleaner to me this way. Thanks, Valerie On 7/2/2020 9:05 PM, Weijun Wang wrote: Hi Valerie, How about the suggested fix from the bug reporter? Thanks, Max On Jul 3, 2020, at 4:52 AM, Valerie Peng wrote: Hi Max and Sean, Can you help reviewing this fix for JDK-8248505? This is the followup fix for JDK-8246613 "Choose the default SecureRandom algo based on registration ordering" which you reviewed earlier. Based on the feedback, BCFIPS provider overrides putService/getService() calls which does not work well with the fix for JDK-8246613. Thus, I adapted to store the SecureRandom algorithm names internally and then return the result from getService(...) when Provider.getDefaultSecureRandomService() is called. Updated the regression test to include a custom provider which simulates the BCFIPS provider behavior. Bug: https://bugs.openjdk.java.net/browse/JDK-8248505 Webrev: http://cr.openjdk.java.net/~valeriep/8248505/webrev.00/ Thanks, Valerie
Re: RFR[15] 8248505: Unexpected NoSuchAlgorithmException when using secure random impl from BCFIPS provider
Hi Valerie, How about the suggested fix from the bug reporter? Thanks, Max > On Jul 3, 2020, at 4:52 AM, Valerie Peng wrote: > > Hi Max and Sean, > > Can you help reviewing this fix for JDK-8248505? This is the followup fix for > JDK-8246613 "Choose the default SecureRandom algo based on registration > ordering" which you reviewed earlier. Based on the feedback, BCFIPS provider > overrides putService/getService() calls which does not work well with the fix > for JDK-8246613. Thus, I adapted to store the SecureRandom algorithm names > internally and then return the result from getService(...) when > Provider.getDefaultSecureRandomService() is called. Updated the regression > test to include a custom provider which simulates the BCFIPS provider > behavior. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8248505 > > Webrev: http://cr.openjdk.java.net/~valeriep/8248505/webrev.00/ > > Thanks, > Valerie > >
RFR[15] 8248505: Unexpected NoSuchAlgorithmException when using secure random impl from BCFIPS provider
Hi Max and Sean, Can you help reviewing this fix for JDK-8248505? This is the followup fix for JDK-8246613 "Choose the default SecureRandom algo based on registration ordering" which you reviewed earlier. Based on the feedback, BCFIPS provider overrides putService/getService() calls which does not work well with the fix for JDK-8246613. Thus, I adapted to store the SecureRandom algorithm names internally and then return the result from getService(...) when Provider.getDefaultSecureRandomService() is called. Updated the regression test to include a custom provider which simulates the BCFIPS provider behavior. Bug: https://bugs.openjdk.java.net/browse/JDK-8248505 Webrev: http://cr.openjdk.java.net/~valeriep/8248505/webrev.00/ Thanks, Valerie
