RFR: JDK-8140466: ChaCha20-Poly1305 TLS cipher suites
Hello all, This change will add ChaCha20-Poly1305 cipher suites to our TLS 1.2 and TLS 1.3 implementations. A few test cases had to be updated to reflect the new suites as well. JBS: https://bugs.openjdk.java.net/browse/JDK-8140466 CSR: https://bugs.openjdk.java.net/browse/JDK-8204192 Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8140466/webrev.01/ Thanks, --Jamil
Re: RFR: 8210432: Add additional TeliaSonera root certificate
Looks good. --Sean On 9/5/18 6:33 PM, Rajan Halade wrote: Please review this fix to add new TeliaSonera root certificate to the cacert store. Webrev: http://cr.openjdk.java.net/~rhalade/8210432/webrev.00/ Thanks, Rajan
RFR: 8210432: Add additional TeliaSonera root certificate
Please review this fix to add new TeliaSonera root certificate to the cacert store. Webrev: http://cr.openjdk.java.net/~rhalade/8210432/webrev.00/ Thanks, Rajan
Re: RFR 6913047: SunPKCS11 memory leak
On 9/4/2018 9:59 PM, Valerie Peng wrote: These sun.security.pkcs11.wrapper classes are internal and subject to changes without notice. No arguments there. But that interface has been stable since the initial contribution and to be blunt - the PKCS11 provider only works well if you use the keys you created through the provider. There's a set of idiosyncratic choices for how to map keys to certs to keys that make keys created by non-provider calls (e.g. C code or other non-java libs) to the token difficult to use through the provider and vice versa. That led to us using the wrapper classes directly. Maybe its time to provide a PKCS11AttributeSpec of some sort for key creation and for looking things up? The current model is literally 12-15 years old AFAICT. For the time being, maybe we can leave these method intact and add a comment about calling the new methods which use P11Key argument instead of the key handle argument. That should work. You may want to think about deprecating those methods and target removing them for a later release in a couple of years. Thanks - Mike Regards, Valerie On 9/1/2018 11:18 AM, Michael StJohns wrote: Hi Valerie - I may be speaking only for myself, but there are probably a number of folk using sun.security.pkcs11.wrapper.* in lieu of the PKCS11 provider for a number of reasons including an ability to manage the key storage and wrapping efficiently. Looking at this I'm thinking there will be a large number of things breaking because of the way you refactored things. While I understand that none of the sun.security.* classes are "public" - these were mostly taken directly from the IAIK contribution way back when and the folks I worked with used them in lieu of external libraries to have a consistent PKCS11 interface java-to-java. Perhaps instead you'd consider doing something like leaving the Java to Native public methods intact? Mike On 8/31/2018 7:53 PM, Valerie Peng wrote: Hi Martin, With the new model of "creating the key handle as needed", I think we should not allow the direct access of keyID field outside of P11Key class. This field should be made private and accessed through methods. In addition, existing PKCS11.C_XXX() methods with P11 keyID arguments can be made private and we can add wrapper methods with P11Key object argument to ensure proper usage of the P11Key object and its keyID field. I have also refactored the keyID management code into a separate holder class. The prototype code can be found at: http://cr.openjdk.java.net/~valeriep/6913047Exp/webrev.00/ The main changes that I made are in P11Key.java and PKCS11.java. The other java classes are updated to call the wrapper methods in PKCS11.java. Thanks & have a great Labor day weekend, Valerie On 8/16/2018 5:28 PM, Valerie Peng wrote: Hi Martin, Since there is no regression test for this test, you will need to add a "noreg-xxx" label to the bug record. For this issue, it'll probably be "noreg-hard". To understand the changes better, please find my questions and some review comments below: - line 397: It's incorrect to change initialize() to ensureInitialized(). If the cipher object were to be initialized twice with two different keys, you need to re-initialize again. - line 471: Why do you change it to Throwable from PKCS11Exception? - line 99: comment is somewhat unclear. typo: "temporal" should be "temporary". - line 148-149: JDK-8165996 has been fixed. This does not apply now, does it? - You changed the constructors of all the P11Key-related classes to take an additional boolean argument "tmpNativeKey". However, this boolean argument is only used when the underlying token is "NSS". Why limiting to NSS only? It seems that all callers except for PKCS11 KeyStore pass true for "tmpNativeKey". When "tmpNativeKey" is true, the keyID handle first destroyed in constructor and later created again (ref count == 1) and destroyed (when ref count drops to 0). This replaced the PhantomReference approach in SessionKeyRef class, right? Now both approaches seem to be used and key destruction is taking places at different methods. I think we should clarify the cleanup model and perhaps refactor the code so it's easier which approach is in use. Or grouping all these cleanup-related fields/variables into a separate class for readability/clarity. - line 157-175: nativeKeyWrapperKeyID is a static variable, shouldn't it be synchronized on a class level object? - line 1343: the impl doesn't look right. Why do you call both removeObject() and addObject() here with the same check? - line 1363: the change seems incorrect, you should still call session.removeObject(). the call setKeyIDAndSession(0, null) does not lower the session object count. Thanks, Valerie On 8/7/2018 5:41 PM, Valerie Peng wrote: Hi Martin, Thanks for the update, I will resume the review on this one with your latest webrev. BTW, there is no
Re: Code Review Request, JDK-8210334, TLS 1.3 server fails if ClientHello doesn't have pre_shared_key and psk_key_exchange_modes
Looks fine > On Sep 5, 2018, at 11:01 AM, Xuelei Fan wrote: > > Hi, > > Please review: >http://cr.openjdk.java.net/~xuelei/8210334/webrev.00/ > > Simple update, no new regression test. > > Thanks, > Xuelei
Code Review Request, JDK-8210334, TLS 1.3 server fails if ClientHello doesn't have pre_shared_key and psk_key_exchange_modes
Hi, Please review: http://cr.openjdk.java.net/~xuelei/8210334/webrev.00/ Simple update, no new regression test. Thanks, Xuelei
Re: RFR 8171279: Support X25519 and X448 in TLS 1.3
On 9/5/2018 10:09 AM, Adam Petcher wrote: Updated webrev: http://cr.openjdk.java.net/~apetcher/8171279/webrev.01/ On 9/4/2018 3:25 PM, Xuelei Fan wrote: I have no finished the full code review. So far, I have a few question about the struct of the code. 1. XECParameters I can see the reason to dynamic parameters for something other than X25519/X448. But for JSSE, currently, only named curves are used. It would be nice to use the name for the static parameters. Sorry, I don't think I understand what you are suggesting here. As far as I know, nothing in sun.security.ssl uses XECParameters directly. Okay, it is used indirectly. It is used indirectly through ECUtil to encode and decode keys, and in this case the name is used to look up the parameters. Can the public NamedParameterSpec be used, instead of the private XECParameters? I think XECParameters may be better in the EC provider, please try to mitigate the dependence between modules. If one day the sun.security.util.XECParameters get updated, the EC implementation get impacted a lot. I prefer to use public APIs if possible. Is there some place in the code where JSSE is doing something too complicated related to these parameters? Yes, the algorithm name is sufficient, I'm not sure the necessary to use XECParameters in sun.security.util. 2. "TlsPremasterSecret" only XDH key agreement It would be nice the JCE implementation can support key agreement other than TLS protocols and providers other than SunJSSE provider. It would be nice if the JCE implementation does not bind to the SunJSSE provider private algorithm name. We used to use SunJSSE private name in the JCE crypto implementation. But there are some known problems with this dependence. Is there a problem to support generic key agreement? I simply continued the pattern that was used in DH and ECDH. We can use a different string here, but I worry that people will be surprised if DH and ECDH support "TlsPreMasterSecret" but XDH doesn't. It could support "TlsPreMasterSecret", right? My concern is about not limited to this one only. I would rather continue to use "TlsPreMasterSecret" for now, and then add support for a standard, TLS-independent name (that means the same thing) in a separate ticket. But if you feel strongly that XDH should not support "TlsPreMasterSecret", then perhaps we can use a different string now. In this case, let me know if you have any suggestions for what string to use. See above. 3. NamedGroupFunctions It might be more straightforward to define these functions in NamedGroup. If comparing nameGroup.getFunctions().getParameterSpec() and namedGroup.getParameterSpec(), I'd prefer the latter. A simple way to accomplish this is to leave the structure alone and add methods in NamedGroup that pass through to the corresponding methods in NamedGroupFunctions. I made this change in the updated webrev. Let me know what you think. It looks like a problem to me to use this before it constructed. this.functions = new ECDHFunctions(this); and the use of new object for each named group is not effective. The current NamedGroupFunctions abstract class does not sound good enough to me, considering the numbers of the named groups. I have no idea so far, but I think you can improve it. Probably use static methods? 4. SSLKeyAgreementCredentials I did not see too much benefit of this new interface. It is not always true that a key agreement could have a public key. Although we can simplify the key agreement for public key based, but it also add additional layers. I know where this improvement comes from. Maybe, you can consolidate the named group functions, and encode/decode the credentials there. This interface is only used to validate public keys against algorithm constraints. In the latest webrev, I moved all of this functionality into NamedGroupFunctions and removed the SSLKeyAgreementCredentials interface. Thanks, I like it the new design. Xuelei
Re: RFR 8171279: Support X25519 and X448 in TLS 1.3
Updated webrev: http://cr.openjdk.java.net/~apetcher/8171279/webrev.01/ On 9/4/2018 3:25 PM, Xuelei Fan wrote: I have no finished the full code review. So far, I have a few question about the struct of the code. 1. XECParameters I can see the reason to dynamic parameters for something other than X25519/X448. But for JSSE, currently, only named curves are used. It would be nice to use the name for the static parameters. Sorry, I don't think I understand what you are suggesting here. As far as I know, nothing in sun.security.ssl uses XECParameters directly. It is used indirectly through ECUtil to encode and decode keys, and in this case the name is used to look up the parameters. Is there some place in the code where JSSE is doing something too complicated related to these parameters? 2. "TlsPremasterSecret" only XDH key agreement It would be nice the JCE implementation can support key agreement other than TLS protocols and providers other than SunJSSE provider. It would be nice if the JCE implementation does not bind to the SunJSSE provider private algorithm name. We used to use SunJSSE private name in the JCE crypto implementation. But there are some known problems with this dependence. Is there a problem to support generic key agreement? I simply continued the pattern that was used in DH and ECDH. We can use a different string here, but I worry that people will be surprised if DH and ECDH support "TlsPreMasterSecret" but XDH doesn't. I would rather continue to use "TlsPreMasterSecret" for now, and then add support for a standard, TLS-independent name (that means the same thing) in a separate ticket. But if you feel strongly that XDH should not support "TlsPreMasterSecret", then perhaps we can use a different string now. In this case, let me know if you have any suggestions for what string to use. 3. NamedGroupFunctions It might be more straightforward to define these functions in NamedGroup. If comparing nameGroup.getFunctions().getParameterSpec() and namedGroup.getParameterSpec(), I'd prefer the latter. A simple way to accomplish this is to leave the structure alone and add methods in NamedGroup that pass through to the corresponding methods in NamedGroupFunctions. I made this change in the updated webrev. Let me know what you think. 4. SSLKeyAgreementCredentials I did not see too much benefit of this new interface. It is not always true that a key agreement could have a public key. Although we can simplify the key agreement for public key based, but it also add additional layers. I know where this improvement comes from. Maybe, you can consolidate the named group functions, and encode/decode the credentials there. This interface is only used to validate public keys against algorithm constraints. In the latest webrev, I moved all of this functionality into NamedGroupFunctions and removed the SSLKeyAgreementCredentials interface.
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hi Matthias, I have reviewed your change, +1 I also reviewed the CSR. Best regards Christoph > -Original Message- > From: Baesken, Matthias > Sent: Mittwoch, 5. September 2018 10:07 > To: security-dev@openjdk.java.net; Weijun Wang > ; core-libs-...@openjdk.java.net > Cc: Langer, Christoph > Subject: Re: [RFR] 8205525 : Improve exception messages during manifest > parsing of jar archives > > Hi Max, thanks for adding yourself as a reviewer. > > I set the CSR ( https://bugs.openjdk.java.net/browse/JDK-8207768 ) to > proposed. > > Best regards, Matthias > > > > > Message: 2 > > Date: Tue, 4 Sep 2018 21:31:58 +0800 > > From: Weijun Wang > > To: "Baesken, Matthias" > > Cc: "security-dev@openjdk.java.net" , > > "core-libs-...@openjdk.java.net" > d...@openjdk.java.net> > > Subject: Re: [RFR] 8205525 : Improve exception messages during > > manifest parsing of jar archives > > Message-ID: <058bd7b5-4d3a-4b56-acb0-0dedddea2...@oracle.com> > > Content-Type: text/plain; charset=us-ascii > > > > I've added myself as a reviewer. You might want to set scope to "JDK". > > > > A CSR is approved by the CSR group after you finalize it. First you should > > propose it. If you think it's urgent or trivial, you can also fast-track it. > > > > Read https://wiki.openjdk.java.net/display/csr/Main for more details. > > > > Thanks > > Max > > > > > On Sep 4, 2018, at 7:33 PM, Baesken, Matthias > > wrote: > > > > > >> The change looks fine. We can enhance the name if we want to support > > .SF > > >> parsing later. > > >> > > >> Please revise your CSR and get it approved first. > > > > > > Hi Max, Thanks ! > > > > > > I adjusted the CSR , please approve : > > > > > > https://bugs.openjdk.java.net/browse/JDK-8207768 > > > > > > Best regards, Matthias
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hi Max, thanks for adding yourself as a reviewer. I set the CSR ( https://bugs.openjdk.java.net/browse/JDK-8207768 ) to proposed. Best regards, Matthias > > Message: 2 > Date: Tue, 4 Sep 2018 21:31:58 +0800 > From: Weijun Wang > To: "Baesken, Matthias" > Cc: "security-dev@openjdk.java.net" , > "core-libs-...@openjdk.java.net" d...@openjdk.java.net> > Subject: Re: [RFR] 8205525 : Improve exception messages during > manifest parsing of jar archives > Message-ID: <058bd7b5-4d3a-4b56-acb0-0dedddea2...@oracle.com> > Content-Type: text/plain; charset=us-ascii > > I've added myself as a reviewer. You might want to set scope to "JDK". > > A CSR is approved by the CSR group after you finalize it. First you should > propose it. If you think it's urgent or trivial, you can also fast-track it. > > Read https://wiki.openjdk.java.net/display/csr/Main for more details. > > Thanks > Max > > > On Sep 4, 2018, at 7:33 PM, Baesken, Matthias > wrote: > > > >> The change looks fine. We can enhance the name if we want to support > .SF > >> parsing later. > >> > >> Please revise your CSR and get it approved first. > > > > Hi Max, Thanks ! > > > > I adjusted the CSR , please approve : > > > > https://bugs.openjdk.java.net/browse/JDK-8207768 > > > > Best regards, Matthias
