Re: 8245686: Ed25519 and Ed448 present in handshake messages
Thanks for the comment.. I moved the code up toward the top. Tony On 6/9/20 4:04 PM, Xuelei Fan wrote: A simple fix like this looks good to me. I may check this first, before the EC available and signature checking. Xuelei On 6/9/2020 3:12 PM, Anthony Scarpino wrote: Hi, I need a code review of this very simple change for a situation that I'm not sure is a problem in the real world. The original TLS 1.3 putback added EdDSA to the TLS signature extensions enumeration before there was an EdDSA JCE implementation or JSSE support. Without an implementation, a signature checks would not include EdDSA for TLS extensions, signature_algorithms and signature_algorithm_cert. Now with JCE EdDSA support, the signature check adds EdDSA to the extension, despite JSSE not having support yet (JDK-8166596). This causes a signature scheme authentication failure, and JSSE moves onto the next certificate provided. The only time this is a problem is if EdDSA is the only cert provided. I'm not sure how realistic it is for one certificate to be provided. If someone knows multiple certificates are always available, I'm happy to not make this change. The fix is a simple check in the constructor to set the curves unavailable after the signature check. This code can be deleted when JDK-8166596 is fixed in jdk16. I had thought about commenting out the enums, but then the logging code would not know what the id's were when other clients and servers passed them to JSSE. https://cr.openjdk.java.net/~ascarpino/8245686/webrev/ Tony
Re: 8245686: Ed25519 and Ed448 present in handshake messages
Thanks for catching that. Tony On 6/9/20 5:23 PM, Bradford Wetmore wrote: Update the year, but otherwise looks good. Brad On 6/9/2020 4:04 PM, Xuelei Fan wrote: A simple fix like this looks good to me. I may check this first, before the EC available and signature checking. Xuelei On 6/9/2020 3:12 PM, Anthony Scarpino wrote: Hi, I need a code review of this very simple change for a situation that I'm not sure is a problem in the real world. The original TLS 1.3 putback added EdDSA to the TLS signature extensions enumeration before there was an EdDSA JCE implementation or JSSE support. Without an implementation, a signature checks would not include EdDSA for TLS extensions, signature_algorithms and signature_algorithm_cert. Now with JCE EdDSA support, the signature check adds EdDSA to the extension, despite JSSE not having support yet (JDK-8166596). This causes a signature scheme authentication failure, and JSSE moves onto the next certificate provided. The only time this is a problem is if EdDSA is the only cert provided. I'm not sure how realistic it is for one certificate to be provided. If someone knows multiple certificates are always available, I'm happy to not make this change. The fix is a simple check in the constructor to set the curves unavailable after the signature check. This code can be deleted when JDK-8166596 is fixed in jdk16. I had thought about commenting out the enums, but then the logging code would not know what the id's were when other clients and servers passed them to JSSE. https://cr.openjdk.java.net/~ascarpino/8245686/webrev/ Tony
Re: RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options
Good to see both -keystore and -trustcacerts mentioned. Some comments:
1. I think there is no need to say "from -file cert_file". Or, do you mean the
new function does not apply to those from -sslserver and -jarfile? If so, that
might be a problem.
2. While you said "attempts to construct a chain of trust", do you also want to
describe what happens if it succeeds or fails?
3. It will be nice if you can include the exact diff of the man page files
either inside the CSR itself or as a comment.
Thanks,
Max
> On Jun 9, 2020, at 10:51 PM, Hai-May Chao wrote:
>
>
>
>> On Jun 7, 2020, at 6:08 PM, Weijun Wang wrote:
>>
>> Looks fine to me.
>>
>> For CSR, since there is already a "Note" there for these 2 options, you can
>> add a few words about what -keystore and -trustcacerts can do.
>
> Updated CSR as suggested.
>
> Thanks,
> Hai-May
>
>
>>
>> Thanks,
>> Max
>>
>>> On Jun 8, 2020, at 4:01 AM, Hai-May Chao wrote:
>>>
>>> Updated webrev -
>>>
>>> https://cr.openjdk.java.net/~hchao/8244148/webrev.02/
>>>
>>> Thanks,
>>> Hai-May
>>>
>>>
On Jun 5, 2020, at 11:04 PM, Weijun Wang wrote:
I still think duplicated commands in TrustedCert.java are useless. Line
104 and line 133 are exactly the same, line 109 and line 138 are exactly
the same, and you haven't made any change to these 2 files in between.
Same for line 80 and line 96 of TrustedCRL.java.
Everything else is fine.
Thanks,
Max
> On Jun 6, 2020, at 2:25 AM, Hai-May Chao wrote:
>
> Updated webrev -
>
> https://cr.openjdk.java.net/~hchao/8244148/webrev.01/
>
> Added one command line -importcert in TrustCert.java.
> Added createCacerts() in test/lib SecurityTools.java.
>
> Thanks,
> Hai-May
>
>
>
>> On Jun 4, 2020, at 5:57 AM, Weijun Wang wrote:
>>
>>
>>
>>> On Jun 4, 2020, at 7:29 PM, Hai-May Chao
>>> wrote:
>>>
>>> Hi Max,
>>>
On Jun 3, 2020, at 12:59 AM, Weijun Wang
wrote:
The source change looks fine to me.
In TrustedCert.java:
- You can use FileOutputStream and Files.copy(Path,OutputStream) in
cat().
>>>
>>> This cat() is taken from WealAlg.java.
>>>
- There is no need to recreate root.jks and root.pem.
>>>
>>> The sequences of the commands used in this test scenario allows me to
>>> test -printcert for the -trustcacerts and -keytsore options. We had
>>> discussion offline about it. The test uses trusted certificates and
>>> checks no warnings on the weak algorithms to address the requirement
>>> described in the bug. I believe it does serve that purpose, and looks
>>> legitimate to me. There could be different ways of testing a
>>> functionality, and please let me know if there is a problem with the
>>> current approach.
>>
>> I just meant that the keytool commands generating root.jks and root.pem
>> are exactly the same and there is no need to recreate it.
>>
>>>
>>> Please also elaborate your comment about no need to recreate root.jks
>>> and root.pem.
>>>
- Why not use -trustcacerts below?
160 kt("-importcert -file server.pem -noprompt", "server.jks”);
>>>
>>>
>>> Because here is to import the server (end-entity) cert, and it will not
>>> make a difference for the test result whether to use the -trustcacerts
>>> or not. It’s the ca (intermediate) cert needs to have it in this test
>>> scenario. I intended to leave it out in #160 to distinguish between
>>> server and ca certs.
>>
>> OK.
>>
>> Then how about we add a new command before line 155?
>>
>> kt("-importcert -file ca.pem", "ca.jks").shouldNotHaveExitValue(0);
>>
>> This would prove the "-trustcacerts" on line 155 is really useful.
>>
>>>
- It's probably better to add a " " between cmd and options in
patchcmd(). Same in TrustedCRL.java.
>>>
>>> Ok, will change it.
>>>
In TrustedCRL.java:
- No need to recreate ks and ca.crl. Just call "-printcrl" with
different options.
>>>
>>> Same reply as above.
>>
>> Same question as above.
>>
>>>
- Why create using MD5withRSA? Do you meant to warn about the weak
algorithm?
>>>
>>> Yes, exactly, and it differentiates from the weak algorithm SHA1withRSA
>>> used in root CA where no warning will be emitted. There is another
>>> -gencrl in #119 without using MD5withRSA so I’d have two test cases.
>>>
Also I would suggest you create a dedicate method (maybe in
SecurityTools.java) to create your own cacerts. There is no need to
Re: 8245686: Ed25519 and Ed448 present in handshake messages
Update the year, but otherwise looks good. Brad On 6/9/2020 4:04 PM, Xuelei Fan wrote: A simple fix like this looks good to me. I may check this first, before the EC available and signature checking. Xuelei On 6/9/2020 3:12 PM, Anthony Scarpino wrote: Hi, I need a code review of this very simple change for a situation that I'm not sure is a problem in the real world. The original TLS 1.3 putback added EdDSA to the TLS signature extensions enumeration before there was an EdDSA JCE implementation or JSSE support. Without an implementation, a signature checks would not include EdDSA for TLS extensions, signature_algorithms and signature_algorithm_cert. Now with JCE EdDSA support, the signature check adds EdDSA to the extension, despite JSSE not having support yet (JDK-8166596). This causes a signature scheme authentication failure, and JSSE moves onto the next certificate provided. The only time this is a problem is if EdDSA is the only cert provided. I'm not sure how realistic it is for one certificate to be provided. If someone knows multiple certificates are always available, I'm happy to not make this change. The fix is a simple check in the constructor to set the curves unavailable after the signature check. This code can be deleted when JDK-8166596 is fixed in jdk16. I had thought about commenting out the enums, but then the logging code would not know what the id's were when other clients and servers passed them to JSSE. https://cr.openjdk.java.net/~ascarpino/8245686/webrev/ Tony
Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
Hi, Sean,
On 6/9/2020 12:21 PM, Sean Mullan wrote:
Looks good, just a couple of minor comments on the test:
* test/jdk/java/security/SecureRandom/DefaultAlgo.java
75 Objects.requireNonNull(p);
Not sure why you need this line, since the test never passes null.
True, the test never passes null, this line is just to make it clear
that the provider argument should not be null as the test is not
prepared to handle null provider. It's not essential, so I removed it
per your comment.
90 validate(new SecureRandom(), pName, algos[0]);
Is there a reason why you don't call removeService for each algorithm
when testing the non-legacy provider?
The Provider.removeService(...) call is protected. So, it's not a public
API for users of Provider objects.
Thanks,
Valerie
--Sean
On 6/9/20 12:52 PM, Valerie Peng wrote:
Thanks for review~
As for the isProviderInfo() name, since I reverted the code for its
impl to pre-7092821, I changed it back to the old name as you
noticed. Sean mentioned that he also wants to take a look at this
updated webrev, so I will wait for him to do that...
Valerie
On 6/8/2020 6:11 PM, Weijun Wang wrote:
Code change looks fine to me.
I re-look at every place where legacyStrings and prngAlgorithms are
used and they are all synchronized. Last time I thought some were
not. Sorry.
Only one comment: I like the isProviderInfo() name better, but I
notice it was the old name pre-7092821.
Thanks,
Max
On Jun 9, 2020, at 6:31 AM, Valerie Peng
wrote:
Webrev updated:
http://cr.openjdk.java.net/~valeriep/8246613/webrev.02/
Besides addressing Max's comments, I also made
updateSecureRandomEntries(...) method private and removed the
synchronized keyword as all of its accesses are synchronized.
Thanks,
Valerie
On 6/8/2020 2:33 PM, Valerie Peng wrote:
Hi Max,
Please find comments in line.
On 6/8/2020 2:34 AM, Weijun Wang wrote:
Looks like this should work, but still find it complicated.
1. Do we need to care about thread safety when managing
legacyStrings?
Right, it's more complicated than I like as well.
As for thread safety, the legacy relevant data are all
synchronized under the current provider object, i.e. this. Is
there a particular call path not doing this? This is the same as
the pre-7092821 code.
2. Does implReplaceAll() need to set legacyChanged = true?
Correct, the removal is by accident. Thanks for catching this.
3. How about using prngAlgorithms.iterator().next() below?
1416 return prngAlgorithms.toArray(new String[0])[0];
Sure, changed.
Valerie
--Max
On Jun 6, 2020, at 11:54 AM, Valerie Peng
wrote:
Thanks for reviewing and sharing the feedbacks on webrev.00.
In order to support all existing calls for legacy registration
for default secure random, I have to revert some of the
JDK-7092821 changes and re-introduce the 'legacyStrings'
LinkedHashMap. Updated the regression test with removal test for
provider using legacy registrations as well. Although removal is
supported, this is still not bullet proof as things may not work
as expected if a provider registered their impl in both ways,
i.e. legacy String pair and Service, and then remove/replace
some entries later. Please comment if you really need this
scenario to be supported. Although not explicitly documented, I
think the intention is to use one or the other, never both.
Webrev update:
http://cr.openjdk.java.net/~valeriep/8246613/webrev.01/
Thanks,
Valerie
On 6/5/2020 11:00 AM, Valerie Peng wrote:
Right, I try to keep the impl simple as I am not aware of any
property removal usage.
Oh-well, if we have to cater to the removal scenario, then we'd
have to add a List and keep track of ALL secure random algos
instead of only the FIRST one. Alright, I guess it costs for
covering all aspect. But one extra benefit of this is that it
should be easy to handle the future JDK property such as
"jdk.securerandom.disabledAlgorithms" if it were to be added.
Valerie
On 6/5/2020 7:54 AM, Weijun Wang wrote:
I don't know who in this world would want to do that, but
Prasad's concern is technically possible. I tried
'p.remove("SecureRandom.a")' in the new test, and new
SecureRandom() fails with
"java.security.NoSuchAlgorithmException: a SecureRandom not
available".
And people can also remove one entry and add it back in order
to move it to the end. One can even add new implementations
this way.
Unfortunately there is no ConcurrentLinkedHashMap.
--Max
On Jun 5, 2020, at 1:44 PM, Prasadrao Koppula
wrote:
Hi,
Looks good to me, one question
If first registered SecureRandom algo gets removed,
getDefaultSecureRandomAlgorithm return stale data, a refresh
required in remove?
Thanks,
Prasad.K
-Original Message-
From: Valerie Peng
Sent: Friday, June 5, 2020 2:52 AM
To: security-dev@openjdk.java.net
Subject: Re: [15] RFR JDK-8246613: Choose the default
SecureRandom algo
based on registration ordering
Hi, Sean,
Re: 8245686: Ed25519 and Ed448 present in handshake messages
A simple fix like this looks good to me. I may check this first, before the EC available and signature checking. Xuelei On 6/9/2020 3:12 PM, Anthony Scarpino wrote: Hi, I need a code review of this very simple change for a situation that I'm not sure is a problem in the real world. The original TLS 1.3 putback added EdDSA to the TLS signature extensions enumeration before there was an EdDSA JCE implementation or JSSE support. Without an implementation, a signature checks would not include EdDSA for TLS extensions, signature_algorithms and signature_algorithm_cert. Now with JCE EdDSA support, the signature check adds EdDSA to the extension, despite JSSE not having support yet (JDK-8166596). This causes a signature scheme authentication failure, and JSSE moves onto the next certificate provided. The only time this is a problem is if EdDSA is the only cert provided. I'm not sure how realistic it is for one certificate to be provided. If someone knows multiple certificates are always available, I'm happy to not make this change. The fix is a simple check in the constructor to set the curves unavailable after the signature check. This code can be deleted when JDK-8166596 is fixed in jdk16. I had thought about commenting out the enums, but then the logging code would not know what the id's were when other clients and servers passed them to JSSE. https://cr.openjdk.java.net/~ascarpino/8245686/webrev/ Tony
Re: [RFR] 8241680: KeyPairGen & Signature microbenchmarks need updating for disabled EC curves
+1 --Jamil On 6/8/2020 12:22 PM, Sergey Kuksenko wrote: Looks fine to me. On 6/8/20 11:15 AM, Anthony Scarpino wrote: Hi, I need a quick code review of updates to the microbenchmarks tests for EC. These tests used curves that are now disabled by default in 15. https://cr.openjdk.java.net/~ascarpino/8241680/webrev/ thanks Tony
Re: Thread leak by LdapLoginModule
Adding core-libs-dev ...
--Sean
On 6/9/20 5:15 PM, Mkrtchyan, Tigran wrote:
Hi all,
with Java-11 we have notice a thread leak with ldap module.
We use LDAP to authenticate users with username+pasword by
directly calling LdapLoginModule. This was ok with java 7 and
java 8. With java 11 we see threads getting accumulated. here is a
test case that demonstrates it:
```
private static final String USERNAME_KEY =
"javax.security.auth.login.name";
private static final String PASSWORD_KEY =
"javax.security.auth.login.password";
String ldapUrl = "ldap://;;
String peopleOU = "ou= ... o= ... c=...");
String user = ...;
String pass = ...;
@Test
public void threadLeakTest() throws AuthenticationException,
NoSuchPrincipalException, LoginException {
Map threadsBefore =
Thread.getAllStackTraces();
Map globalLoginOptions = Map.of(
"userProvider", ldapUrl + "/" + peopleOU,
"useSSL", "false",
"userFilter", "(uid={USERNAME})",
"useFirstPass", "true"
);
for (int i = 0; i < 10; i++) {
Map loginOptions = Map.of(
USERNAME_KEY, user,
PASSWORD_KEY, pass.toCharArray());
Subject subject = new Subject();
LdapLoginModule loginModule = new LdapLoginModule();
loginModule.initialize(subject, null, loginOptions,
globalLoginOptions);
loginModule.login();
loginModule.commit();
loginModule.logout();
}
Map threadsAfter =
Thread.getAllStackTraces();
assertEquals("Thread leak detected", threadsBefore.size() + 1,
threadsAfter.size());
}
```
The thread count difference is always equals to the number of iterations in the
loop, e.g. on each call a
thread is created and stays around. Eventually our server crashes with:
[19497.011s][warning][os,thread] Attempt to protect stack guard pages failed
(0x7fcc4c65c000-0x7fcc4c66).
OpenJDK 64-Bit Server VM warning: INFO: os::commit_memory(0x7fcc4c55b000,
16384, 0) failed; error='Not enough space' (errno=12)
The issue is not observed with java-14, thus I assume that the fix is related
to commit
http://hg.openjdk.java.net/jdk/jdk/rev/6717d7e59db4
As java-11 is LTS, what is the procedure to get it fix back-ported?
Regards,
Tigran.
Re: 8245686: Ed25519 and Ed448 present in handshake messages
Looks fine to me. --Jamil On 6/9/2020 3:12 PM, Anthony Scarpino wrote: Hi, I need a code review of this very simple change for a situation that I'm not sure is a problem in the real world. The original TLS 1.3 putback added EdDSA to the TLS signature extensions enumeration before there was an EdDSA JCE implementation or JSSE support. Without an implementation, a signature checks would not include EdDSA for TLS extensions, signature_algorithms and signature_algorithm_cert. Now with JCE EdDSA support, the signature check adds EdDSA to the extension, despite JSSE not having support yet (JDK-8166596). This causes a signature scheme authentication failure, and JSSE moves onto the next certificate provided. The only time this is a problem is if EdDSA is the only cert provided. I'm not sure how realistic it is for one certificate to be provided. If someone knows multiple certificates are always available, I'm happy to not make this change. The fix is a simple check in the constructor to set the curves unavailable after the signature check. This code can be deleted when JDK-8166596 is fixed in jdk16. I had thought about commenting out the enums, but then the logging code would not know what the id's were when other clients and servers passed them to JSSE. https://cr.openjdk.java.net/~ascarpino/8245686/webrev/ Tony
8245686: Ed25519 and Ed448 present in handshake messages
Hi, I need a code review of this very simple change for a situation that I'm not sure is a problem in the real world. The original TLS 1.3 putback added EdDSA to the TLS signature extensions enumeration before there was an EdDSA JCE implementation or JSSE support. Without an implementation, a signature checks would not include EdDSA for TLS extensions, signature_algorithms and signature_algorithm_cert. Now with JCE EdDSA support, the signature check adds EdDSA to the extension, despite JSSE not having support yet (JDK-8166596). This causes a signature scheme authentication failure, and JSSE moves onto the next certificate provided. The only time this is a problem is if EdDSA is the only cert provided. I'm not sure how realistic it is for one certificate to be provided. If someone knows multiple certificates are always available, I'm happy to not make this change. The fix is a simple check in the constructor to set the curves unavailable after the signature check. This code can be deleted when JDK-8166596 is fixed in jdk16. I had thought about commenting out the enums, but then the logging code would not know what the id's were when other clients and servers passed them to JSSE. https://cr.openjdk.java.net/~ascarpino/8245686/webrev/ Tony
Thread leak by LdapLoginModule
Hi all,
with Java-11 we have notice a thread leak with ldap module.
We use LDAP to authenticate users with username+pasword by
directly calling LdapLoginModule. This was ok with java 7 and
java 8. With java 11 we see threads getting accumulated. here is a
test case that demonstrates it:
```
private static final String USERNAME_KEY = "javax.security.auth.login.name";
private static final String PASSWORD_KEY =
"javax.security.auth.login.password";
String ldapUrl = "ldap://;;
String peopleOU = "ou= ... o= ... c=...");
String user = ...;
String pass = ...;
@Test
public void threadLeakTest() throws AuthenticationException,
NoSuchPrincipalException, LoginException {
Map threadsBefore =
Thread.getAllStackTraces();
Map globalLoginOptions = Map.of(
"userProvider", ldapUrl + "/" + peopleOU,
"useSSL", "false",
"userFilter", "(uid={USERNAME})",
"useFirstPass", "true"
);
for (int i = 0; i < 10; i++) {
Map loginOptions = Map.of(
USERNAME_KEY, user,
PASSWORD_KEY, pass.toCharArray());
Subject subject = new Subject();
LdapLoginModule loginModule = new LdapLoginModule();
loginModule.initialize(subject, null, loginOptions,
globalLoginOptions);
loginModule.login();
loginModule.commit();
loginModule.logout();
}
Map threadsAfter =
Thread.getAllStackTraces();
assertEquals("Thread leak detected", threadsBefore.size() + 1,
threadsAfter.size());
}
```
The thread count difference is always equals to the number of iterations in the
loop, e.g. on each call a
thread is created and stays around. Eventually our server crashes with:
[19497.011s][warning][os,thread] Attempt to protect stack guard pages failed
(0x7fcc4c65c000-0x7fcc4c66).
OpenJDK 64-Bit Server VM warning: INFO: os::commit_memory(0x7fcc4c55b000,
16384, 0) failed; error='Not enough space' (errno=12)
The issue is not observed with java-14, thus I assume that the fix is related
to commit
http://hg.openjdk.java.net/jdk/jdk/rev/6717d7e59db4
As java-11 is LTS, what is the procedure to get it fix back-ported?
Regards,
Tigran.
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
Hello Aleks,
Thank you very much for review.
I’ve fixed missed spaces and removed casting from LdapSasl.java
Failure of the SaslMutual test was caused by prop.remove() in the GssKrb5Client
This operation is not required any more. GssKrb5Client receives temporary copy
of the properties. Fixed
Also, I’ve added references to RFC-5929 and RFC-5056 into the TlsChannelBinding
class
Updated patch is located at:
http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v7/
Regards
Alexey
> On 9 Jun 2020, at 15:59, Aleks Efimov wrote:
>
> Hi Alexey,
>
> Thank you for incorporating LdapCtx and LdapSasl changes. I've reviewed both
> classes and they look good to me, with few minor comments in LdapSasl.java:
> missing spaces in the following lines: 78, 152
> With your last changes we can remove explicit cast of 'envProps' on
> line 176
>
> I've also run your changes through our CI and one test is failing due to the
> changes in GssKrb5Client:
> The failed test name: sun/security/krb5/auto/SaslMutual.java
>
> The observed failure:
> java.lang.UnsupportedOperationException
> at
> java.base/java.util.ImmutableCollections.uoe(ImmutableCollections.java:127)
> at
> java.base/java.util.ImmutableCollections$AbstractImmutableMap.remove(ImmutableCollections.java:910)
> at
> jdk.security.jgss/com.sun.security.sasl.gsskerb.GssKrb5Client.(GssKrb5Client.java:156)
> at
> jdk.security.jgss/com.sun.security.sasl.gsskerb.FactoryImpl.createSaslClient(FactoryImpl.java:63)
> at
> java.security.sasl/javax.security.sasl.Sasl.createSaslClient(Sasl.java:433)
> at SaslMutual.main(SaslMutual.java:50)
> at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
> Method)
> at
> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64)
> at
> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.base/java.lang.reflect.Method.invoke(Method.java:564)
> at
> com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
> at java.base/java.lang.Thread.run(Thread.java:832)
>
>
> For information about CSR process you can start from this wiki page:
> https://wiki.openjdk.java.net/display/csr
>
> Best Regards,
> Aleksei
>
> On 08/06/2020 22:33, Alexey Bakhtin wrote:
>> Hello Sean,
>>
>> Yes, I think we'll need CSR and release notes as soon as this patch adds new
>> property.
>> I do not know exact process for it, so I will be grateful if you could
>> explain me exact steps.
>>
>> This patch was developed to address compatibility issue with new LDAP
>> authentication over SSL/TLS announced by Microsoft [1]. It is not related to
>> RFC 5801. In my opinion “com.sun.jndi.ldap.tls.cbtype” name looks more
>> suitable for this property and should allow backport it to early JDK
>> versions.
>>
>> [1] -
>> https://support.microsoft.com/en-au/help/4034879/how-to-add-the-ldapenforcechannelbinding-registry-entry
>>
>> Regards
>> Alexey
>>
>>
>>> On 8 Jun 2020, at 22:03, Sean Mullan
>>> wrote:
>>>
>>> (resending to all lists on the review)
>>>
>>> I'm just catching up a bit on this review.
>>>
>>> Sorry if this has mentioned before, but are you planning to write a CSR and
>>> release note? I think this is needed for the com.sun.jndi.ldap.tls.cbtype
>>> property. I'm also wondering if this property should be documented in the
>>> javadocs, and why it is not a standard property (i.e.
>>> "java.naming.ldap.tls.cbtype").
>>>
>>> I was also wondering what relation this has to the "G2" standard SASL
>>> mechanisms defined in RFC 5801 [1], and whether that is something we should
>>> be using to negotiate this channel binding, and if not, why not. Or if this
>>> is something that is implementation-specific and will only work with
>>> Microsoft LDAP technology, in which case, we might want to make that more
>>> explicit, perhaps by including "microsoft" or something like that in the
>>> property name.
>>>
>>> Thanks,
>>> Sean
>>>
>>> [1]
>>> https://tools.ietf.org/html/rfc5801
>>>
>>>
>>> On 6/8/20 9:07 AM, Aleks Efimov wrote:
>>>
Hi Alexey,
I've looked through LdapCtx/LdapClient/SaslBind changes:
Do we need to check if CHANNEL_BINDING is set explicitly for all
connection types? Maybe we can move the check inside 'if (conn.sock
instanceof SSLSocket) {' block.
Also, instead of setting CHANNEL_BINDING in context environment and then
removing it in finally block, it would be better to clone the environment,
put calculated CHANNEL_BINDING into it, and pass the cloned one to
Sasl.createSaslClient.
Another suggestion about the code that verifies if both properties are set
before connection is started:
As you've already mentioned the new code in LdapCtx is only needed for
checking if timeout is set. Could we try to remove LdapCtx::cbType field
and all related
Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
Looks good, just a couple of minor comments on the test:
* test/jdk/java/security/SecureRandom/DefaultAlgo.java
75 Objects.requireNonNull(p);
Not sure why you need this line, since the test never passes null.
90 validate(new SecureRandom(), pName, algos[0]);
Is there a reason why you don't call removeService for each algorithm
when testing the non-legacy provider?
--Sean
On 6/9/20 12:52 PM, Valerie Peng wrote:
Thanks for review~
As for the isProviderInfo() name, since I reverted the code for its impl
to pre-7092821, I changed it back to the old name as you noticed. Sean
mentioned that he also wants to take a look at this updated webrev, so I
will wait for him to do that...
Valerie
On 6/8/2020 6:11 PM, Weijun Wang wrote:
Code change looks fine to me.
I re-look at every place where legacyStrings and prngAlgorithms are
used and they are all synchronized. Last time I thought some were not.
Sorry.
Only one comment: I like the isProviderInfo() name better, but I
notice it was the old name pre-7092821.
Thanks,
Max
On Jun 9, 2020, at 6:31 AM, Valerie Peng
wrote:
Webrev updated: http://cr.openjdk.java.net/~valeriep/8246613/webrev.02/
Besides addressing Max's comments, I also made
updateSecureRandomEntries(...) method private and removed the
synchronized keyword as all of its accesses are synchronized.
Thanks,
Valerie
On 6/8/2020 2:33 PM, Valerie Peng wrote:
Hi Max,
Please find comments in line.
On 6/8/2020 2:34 AM, Weijun Wang wrote:
Looks like this should work, but still find it complicated.
1. Do we need to care about thread safety when managing legacyStrings?
Right, it's more complicated than I like as well.
As for thread safety, the legacy relevant data are all synchronized
under the current provider object, i.e. this. Is there a particular
call path not doing this? This is the same as the pre-7092821 code.
2. Does implReplaceAll() need to set legacyChanged = true?
Correct, the removal is by accident. Thanks for catching this.
3. How about using prngAlgorithms.iterator().next() below?
1416 return prngAlgorithms.toArray(new String[0])[0];
Sure, changed.
Valerie
--Max
On Jun 6, 2020, at 11:54 AM, Valerie Peng
wrote:
Thanks for reviewing and sharing the feedbacks on webrev.00.
In order to support all existing calls for legacy registration for
default secure random, I have to revert some of the JDK-7092821
changes and re-introduce the 'legacyStrings' LinkedHashMap.
Updated the regression test with removal test for provider using
legacy registrations as well. Although removal is supported, this
is still not bullet proof as things may not work as expected if a
provider registered their impl in both ways, i.e. legacy String
pair and Service, and then remove/replace some entries later.
Please comment if you really need this scenario to be supported.
Although not explicitly documented, I think the intention is to
use one or the other, never both.
Webrev update:
http://cr.openjdk.java.net/~valeriep/8246613/webrev.01/
Thanks,
Valerie
On 6/5/2020 11:00 AM, Valerie Peng wrote:
Right, I try to keep the impl simple as I am not aware of any
property removal usage.
Oh-well, if we have to cater to the removal scenario, then we'd
have to add a List and keep track of ALL secure random algos
instead of only the FIRST one. Alright, I guess it costs for
covering all aspect. But one extra benefit of this is that it
should be easy to handle the future JDK property such as
"jdk.securerandom.disabledAlgorithms" if it were to be added.
Valerie
On 6/5/2020 7:54 AM, Weijun Wang wrote:
I don't know who in this world would want to do that, but
Prasad's concern is technically possible. I tried
'p.remove("SecureRandom.a")' in the new test, and new
SecureRandom() fails with
"java.security.NoSuchAlgorithmException: a SecureRandom not
available".
And people can also remove one entry and add it back in order to
move it to the end. One can even add new implementations this way.
Unfortunately there is no ConcurrentLinkedHashMap.
--Max
On Jun 5, 2020, at 1:44 PM, Prasadrao Koppula
wrote:
Hi,
Looks good to me, one question
If first registered SecureRandom algo gets removed,
getDefaultSecureRandomAlgorithm return stale data, a refresh
required in remove?
Thanks,
Prasad.K
-Original Message-
From: Valerie Peng
Sent: Friday, June 5, 2020 2:52 AM
To: security-dev@openjdk.java.net
Subject: Re: [15] RFR JDK-8246613: Choose the default
SecureRandom algo
based on registration ordering
Hi, Sean,
Thanks for the review and feedback. Webrev updated:
http://cr.openjdk.java.net/~valeriep/8246613/webrev.01/
getTypeAndAlgorithm(...) was not static due to an instance
variable used by
debugging output. I have removed it and made both method static.
I will wait for others' review comments also.
Thanks,
Valerie
On 6/4/2020 2:01 PM, Sean Mullan wrote:
On 6/4/20 3:34 PM, Valerie Peng wrote:
Hi,
Could someone
Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
Thanks for review~
As for the isProviderInfo() name, since I reverted the code for its impl
to pre-7092821, I changed it back to the old name as you noticed. Sean
mentioned that he also wants to take a look at this updated webrev, so I
will wait for him to do that...
Valerie
On 6/8/2020 6:11 PM, Weijun Wang wrote:
Code change looks fine to me.
I re-look at every place where legacyStrings and prngAlgorithms are used and
they are all synchronized. Last time I thought some were not. Sorry.
Only one comment: I like the isProviderInfo() name better, but I notice it was
the old name pre-7092821.
Thanks,
Max
On Jun 9, 2020, at 6:31 AM, Valerie Peng wrote:
Webrev updated: http://cr.openjdk.java.net/~valeriep/8246613/webrev.02/
Besides addressing Max's comments, I also made updateSecureRandomEntries(...)
method private and removed the synchronized keyword as all of its accesses are
synchronized.
Thanks,
Valerie
On 6/8/2020 2:33 PM, Valerie Peng wrote:
Hi Max,
Please find comments in line.
On 6/8/2020 2:34 AM, Weijun Wang wrote:
Looks like this should work, but still find it complicated.
1. Do we need to care about thread safety when managing legacyStrings?
Right, it's more complicated than I like as well.
As for thread safety, the legacy relevant data are all synchronized under the
current provider object, i.e. this. Is there a particular call path not doing
this? This is the same as the pre-7092821 code.
2. Does implReplaceAll() need to set legacyChanged = true?
Correct, the removal is by accident. Thanks for catching this.
3. How about using prngAlgorithms.iterator().next() below?
1416 return prngAlgorithms.toArray(new String[0])[0];
Sure, changed.
Valerie
--Max
On Jun 6, 2020, at 11:54 AM, Valerie Peng wrote:
Thanks for reviewing and sharing the feedbacks on webrev.00.
In order to support all existing calls for legacy registration for default
secure random, I have to revert some of the JDK-7092821 changes and
re-introduce the 'legacyStrings' LinkedHashMap. Updated the regression test
with removal test for provider using legacy registrations as well. Although
removal is supported, this is still not bullet proof as things may not work as
expected if a provider registered their impl in both ways, i.e. legacy String
pair and Service, and then remove/replace some entries later. Please comment if
you really need this scenario to be supported. Although not explicitly
documented, I think the intention is to use one or the other, never both.
Webrev update: http://cr.openjdk.java.net/~valeriep/8246613/webrev.01/
Thanks,
Valerie
On 6/5/2020 11:00 AM, Valerie Peng wrote:
Right, I try to keep the impl simple as I am not aware of any property removal
usage.
Oh-well, if we have to cater to the removal scenario, then we'd have to add a List and
keep track of ALL secure random algos instead of only the FIRST one. Alright, I guess it
costs for covering all aspect. But one extra benefit of this is that it should be easy to
handle the future JDK property such as "jdk.securerandom.disabledAlgorithms" if
it were to be added.
Valerie
On 6/5/2020 7:54 AM, Weijun Wang wrote:
I don't know who in this world would want to do that, but Prasad's concern is technically possible.
I tried 'p.remove("SecureRandom.a")' in the new test, and new SecureRandom() fails with
"java.security.NoSuchAlgorithmException: a SecureRandom not available".
And people can also remove one entry and add it back in order to move it to the
end. One can even add new implementations this way.
Unfortunately there is no ConcurrentLinkedHashMap.
--Max
On Jun 5, 2020, at 1:44 PM, Prasadrao Koppula
wrote:
Hi,
Looks good to me, one question
If first registered SecureRandom algo gets removed,
getDefaultSecureRandomAlgorithm return stale data, a refresh required in remove?
Thanks,
Prasad.K
-Original Message-
From: Valerie Peng
Sent: Friday, June 5, 2020 2:52 AM
To: security-dev@openjdk.java.net
Subject: Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo
based on registration ordering
Hi, Sean,
Thanks for the review and feedback. Webrev updated:
http://cr.openjdk.java.net/~valeriep/8246613/webrev.01/
getTypeAndAlgorithm(...) was not static due to an instance variable used by
debugging output. I have removed it and made both method static.
I will wait for others' review comments also.
Thanks,
Valerie
On 6/4/2020 2:01 PM, Sean Mullan wrote:
On 6/4/20 3:34 PM, Valerie Peng wrote:
Hi,
Could someone help reviewing this fix? This change keep tracks of the
first registered SecureRandom algorithm and returns it upon the
request of SecureRandom class.
This looks good to me. I would recommend that Max or someone else
review it as well.
Bug: https://bugs.openjdk.java.net/browse/JDK-8246613
Webrev: http://cr.openjdk.java.net/~valeriep/8246613/webrev.00/
A couple of minor comments, feel free to fix or ignore.
* SecureRandom.java
879 // For SUN provider,
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
On 6/9/20 12:40 PM, Xuelei Fan wrote:
About the prefix, it may follow RFC 5056 (See page 7, section 2.1).
o Specifications of channel bindings for any secure channels MUST
provide for a single, canonical octet string encoding of the
channel bindings. Under this framework, channel bindings MUST
start with the channel binding unique prefix followed by a colon
(ASCII 0x3A).
Thanks! Easy to miss.
I would recommend adding more comments in the code (ex, in
TLSChannelBinding) pointing to that RFC section, and other RFCs such
5929 for the tls cbtypes. Also, this RFC (and other specifications such
as RFC 5959) should be listed in the CSR so that we document precisely
what encodings and types the JDK implementation supports and is using.
--Sean
Xuelei
On 6/9/2020 8:52 AM, Alexey Bakhtin wrote:
Hello Sean,
Thank you for the link. I’ll follow it to create CSR
I could not find any clear document or specification for this Channel
Binding format.
The only document I found that describes this format is the following:
https://docs.microsoft.com/en-us/archive/blogs/openspecification/ntlm-and-channel-binding-hash-aka-extended-protection-for-authentication
So, it is hard to say - is it a standard or Microsoft implementation
specific
Regards
Alexey
On 9 Jun 2020, at 18:35, Sean Mullan wrote:
On 6/8/20 5:33 PM, Alexey Bakhtin wrote:
Hello Sean,
Yes, I think we'll need CSR and release notes as soon as this patch
adds new property.
I do not know exact process for it, so I will be grateful if you
could explain me exact steps.
The CSR process is documented at
https://wiki.openjdk.java.net/display/csr/Main. It should be fairly
self-explanatory but let me know if you have questions.
For the release note, we can tackle that later once the CSR is
approved now I have tagged the issue with the "release-note=yes"
label so we don't forget it.
This patch was developed to address compatibility issue with new
LDAP authentication over SSL/TLS announced by Microsoft [1]. It is
not related to RFC 5801. In my opinion
“com.sun.jndi.ldap.tls.cbtype” name looks more suitable for this
property and should allow backport it to early JDK versions.
Good point about backporting.
What RFC or specification defines the format you are using for the
channel binding in TlsChannelBinding.java, specifically where the
type prefix is encoded as "tls-server-end-point:" followed by the
binding data? I have looked through various RFCs but I can't find
exactly where this format is defined, so I am wondering if this is a
standard encoding or not.
Thanks,
Sean
[1] -
https://support.microsoft.com/en-au/help/4034879/how-to-add-the-ldapenforcechannelbinding-registry-entry
Regards
Alexey
On 8 Jun 2020, at 22:03, Sean Mullan wrote:
(resending to all lists on the review)
I'm just catching up a bit on this review.
Sorry if this has mentioned before, but are you planning to write a
CSR and release note? I think this is needed for the
com.sun.jndi.ldap.tls.cbtype property. I'm also wondering if this
property should be documented in the javadocs, and why it is not a
standard property (i.e. "java.naming.ldap.tls.cbtype").
I was also wondering what relation this has to the "G2" standard
SASL mechanisms defined in RFC 5801 [1], and whether that is
something we should be using to negotiate this channel binding, and
if not, why not. Or if this is something that is
implementation-specific and will only work with Microsoft LDAP
technology, in which case, we might want to make that more
explicit, perhaps by including "microsoft" or something like that
in the property name.
Thanks,
Sean
[1] https://tools.ietf.org/html/rfc5801
On 6/8/20 9:07 AM, Aleks Efimov wrote:
Hi Alexey,
I've looked through LdapCtx/LdapClient/SaslBind changes:
Do we need to check if CHANNEL_BINDING is set explicitly for all
connection types? Maybe we can move the check inside 'if
(conn.sock instanceof SSLSocket) {' block.
Also, instead of setting CHANNEL_BINDING in context environment
and then removing it in finally block, it would be better to clone
the environment, put calculated CHANNEL_BINDING into it, and pass
the cloned one to Sasl.createSaslClient.
Another suggestion about the code that verifies if both properties
are set before connection is started:
As you've already mentioned the new code in LdapCtx is only needed
for checking if timeout is set. Could we try to remove
LdapCtx::cbType field and all related methods from LdapCtx (this
class is already over-complicated and hard to read) and replace it
with some static method in LdapSasl? It will help to localize all
changes to LdapSasl except for one line in LdapCtx.
I mean something like this:
Replace
+
+ // verify LDAP channel binding property
+ if (cbType != null && connectTimeout == -1)
+ throw new
NamingException(TlsChannelBinding.CHANNEL_BINDING_TYPE +
+ "
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
About the prefix, it may follow RFC 5056 (See page 7, section 2.1).
o Specifications of channel bindings for any secure channels MUST
provide for a single, canonical octet string encoding of the
channel bindings. Under this framework, channel bindings MUST
start with the channel binding unique prefix followed by a colon
(ASCII 0x3A).
Xuelei
On 6/9/2020 8:52 AM, Alexey Bakhtin wrote:
Hello Sean,
Thank you for the link. I’ll follow it to create CSR
I could not find any clear document or specification for this Channel Binding
format.
The only document I found that describes this format is the following:
https://docs.microsoft.com/en-us/archive/blogs/openspecification/ntlm-and-channel-binding-hash-aka-extended-protection-for-authentication
So, it is hard to say - is it a standard or Microsoft implementation specific
Regards
Alexey
On 9 Jun 2020, at 18:35, Sean Mullan wrote:
On 6/8/20 5:33 PM, Alexey Bakhtin wrote:
Hello Sean,
Yes, I think we'll need CSR and release notes as soon as this patch adds new
property.
I do not know exact process for it, so I will be grateful if you could explain
me exact steps.
The CSR process is documented at
https://wiki.openjdk.java.net/display/csr/Main. It should be fairly
self-explanatory but let me know if you have questions.
For the release note, we can tackle that later once the CSR is approved now I have tagged
the issue with the "release-note=yes" label so we don't forget it.
This patch was developed to address compatibility issue with new LDAP
authentication over SSL/TLS announced by Microsoft [1]. It is not related to
RFC 5801. In my opinion “com.sun.jndi.ldap.tls.cbtype” name looks more suitable
for this property and should allow backport it to early JDK versions.
Good point about backporting.
What RFC or specification defines the format you are using for the channel binding in
TlsChannelBinding.java, specifically where the type prefix is encoded as
"tls-server-end-point:" followed by the binding data? I have looked through
various RFCs but I can't find exactly where this format is defined, so I am wondering if
this is a standard encoding or not.
Thanks,
Sean
[1] -
https://support.microsoft.com/en-au/help/4034879/how-to-add-the-ldapenforcechannelbinding-registry-entry
Regards
Alexey
On 8 Jun 2020, at 22:03, Sean Mullan wrote:
(resending to all lists on the review)
I'm just catching up a bit on this review.
Sorry if this has mentioned before, but are you planning to write a CSR and release note?
I think this is needed for the com.sun.jndi.ldap.tls.cbtype property. I'm also wondering
if this property should be documented in the javadocs, and why it is not a standard
property (i.e. "java.naming.ldap.tls.cbtype").
I was also wondering what relation this has to the "G2" standard SASL mechanisms defined
in RFC 5801 [1], and whether that is something we should be using to negotiate this channel
binding, and if not, why not. Or if this is something that is implementation-specific and will only
work with Microsoft LDAP technology, in which case, we might want to make that more explicit,
perhaps by including "microsoft" or something like that in the property name.
Thanks,
Sean
[1] https://tools.ietf.org/html/rfc5801
On 6/8/20 9:07 AM, Aleks Efimov wrote:
Hi Alexey,
I've looked through LdapCtx/LdapClient/SaslBind changes:
Do we need to check if CHANNEL_BINDING is set explicitly for all connection
types? Maybe we can move the check inside 'if (conn.sock instanceof SSLSocket)
{' block.
Also, instead of setting CHANNEL_BINDING in context environment and then
removing it in finally block, it would be better to clone the environment, put
calculated CHANNEL_BINDING into it, and pass the cloned one to
Sasl.createSaslClient.
Another suggestion about the code that verifies if both properties are set
before connection is started:
As you've already mentioned the new code in LdapCtx is only needed for checking
if timeout is set. Could we try to remove LdapCtx::cbType field and all related
methods from LdapCtx (this class is already over-complicated and hard to read)
and replace it with some static method in LdapSasl? It will help to localize
all changes to LdapSasl except for one line in LdapCtx.
I mean something like this:
Replace
+
+// verify LDAP channel binding property
+if (cbType != null && connectTimeout == -1)
+throw new
NamingException(TlsChannelBinding.CHANNEL_BINDING_TYPE +
+" property requires " +
+CONNECT_TIMEOUT +
+" property is set.");
With
+
LdapSasl.checkCbParameters((String)envprops.get(TlsChannelBinding.CHANNEL_BINDING_TYPE),
connectTimeout);
And add something like that to LdapSasl (or maybe pass the full env here):
+ public static void checkCbParameters(String cbTypePropertyValue, int
connectTimeout) throws NamingException {
+
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
Hello Sean,
Thank you for the link. I’ll follow it to create CSR
I could not find any clear document or specification for this Channel Binding
format.
The only document I found that describes this format is the following:
https://docs.microsoft.com/en-us/archive/blogs/openspecification/ntlm-and-channel-binding-hash-aka-extended-protection-for-authentication
So, it is hard to say - is it a standard or Microsoft implementation specific
Regards
Alexey
> On 9 Jun 2020, at 18:35, Sean Mullan wrote:
>
> On 6/8/20 5:33 PM, Alexey Bakhtin wrote:
>> Hello Sean,
>> Yes, I think we'll need CSR and release notes as soon as this patch adds new
>> property.
>> I do not know exact process for it, so I will be grateful if you could
>> explain me exact steps.
>
> The CSR process is documented at
> https://wiki.openjdk.java.net/display/csr/Main. It should be fairly
> self-explanatory but let me know if you have questions.
>
> For the release note, we can tackle that later once the CSR is approved now I
> have tagged the issue with the "release-note=yes" label so we don't forget it.
>
>> This patch was developed to address compatibility issue with new LDAP
>> authentication over SSL/TLS announced by Microsoft [1]. It is not related to
>> RFC 5801. In my opinion “com.sun.jndi.ldap.tls.cbtype” name looks more
>> suitable for this property and should allow backport it to early JDK
>> versions.
>
> Good point about backporting.
>
> What RFC or specification defines the format you are using for the channel
> binding in TlsChannelBinding.java, specifically where the type prefix is
> encoded as "tls-server-end-point:" followed by the binding data? I have
> looked through various RFCs but I can't find exactly where this format is
> defined, so I am wondering if this is a standard encoding or not.
>
> Thanks,
> Sean
>
>> [1] -
>> https://support.microsoft.com/en-au/help/4034879/how-to-add-the-ldapenforcechannelbinding-registry-entry
>> Regards
>> Alexey
>>> On 8 Jun 2020, at 22:03, Sean Mullan wrote:
>>>
>>> (resending to all lists on the review)
>>>
>>> I'm just catching up a bit on this review.
>>>
>>> Sorry if this has mentioned before, but are you planning to write a CSR and
>>> release note? I think this is needed for the com.sun.jndi.ldap.tls.cbtype
>>> property. I'm also wondering if this property should be documented in the
>>> javadocs, and why it is not a standard property (i.e.
>>> "java.naming.ldap.tls.cbtype").
>>>
>>> I was also wondering what relation this has to the "G2" standard SASL
>>> mechanisms defined in RFC 5801 [1], and whether that is something we should
>>> be using to negotiate this channel binding, and if not, why not. Or if this
>>> is something that is implementation-specific and will only work with
>>> Microsoft LDAP technology, in which case, we might want to make that more
>>> explicit, perhaps by including "microsoft" or something like that in the
>>> property name.
>>>
>>> Thanks,
>>> Sean
>>>
>>> [1] https://tools.ietf.org/html/rfc5801
>>>
>>> On 6/8/20 9:07 AM, Aleks Efimov wrote:
Hi Alexey,
I've looked through LdapCtx/LdapClient/SaslBind changes:
Do we need to check if CHANNEL_BINDING is set explicitly for all
connection types? Maybe we can move the check inside 'if (conn.sock
instanceof SSLSocket) {' block.
Also, instead of setting CHANNEL_BINDING in context environment and then
removing it in finally block, it would be better to clone the environment,
put calculated CHANNEL_BINDING into it, and pass the cloned one to
Sasl.createSaslClient.
Another suggestion about the code that verifies if both properties are set
before connection is started:
As you've already mentioned the new code in LdapCtx is only needed for
checking if timeout is set. Could we try to remove LdapCtx::cbType field
and all related methods from LdapCtx (this class is already
over-complicated and hard to read) and replace it with some static method
in LdapSasl? It will help to localize all changes to LdapSasl except for
one line in LdapCtx.
I mean something like this:
Replace
+
+// verify LDAP channel binding property
+if (cbType != null && connectTimeout == -1)
+throw new
NamingException(TlsChannelBinding.CHANNEL_BINDING_TYPE +
+" property requires " +
+CONNECT_TIMEOUT +
+" property is set.");
With
+
LdapSasl.checkCbParameters((String)envprops.get(TlsChannelBinding.CHANNEL_BINDING_TYPE),
connectTimeout);
And add something like that to LdapSasl (or maybe pass the full env here):
+ public static void checkCbParameters(String cbTypePropertyValue, int
connectTimeout) throws NamingException {
+ TlsChannelBindingType cbType =
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
On 6/8/20 5:33 PM, Alexey Bakhtin wrote:
Hello Sean,
Yes, I think we'll need CSR and release notes as soon as this patch adds new
property.
I do not know exact process for it, so I will be grateful if you could explain
me exact steps.
The CSR process is documented at
https://wiki.openjdk.java.net/display/csr/Main. It should be fairly
self-explanatory but let me know if you have questions.
For the release note, we can tackle that later once the CSR is approved
now I have tagged the issue with the "release-note=yes" label so we
don't forget it.
This patch was developed to address compatibility issue with new LDAP
authentication over SSL/TLS announced by Microsoft [1]. It is not related to
RFC 5801. In my opinion “com.sun.jndi.ldap.tls.cbtype” name looks more suitable
for this property and should allow backport it to early JDK versions.
Good point about backporting.
What RFC or specification defines the format you are using for the
channel binding in TlsChannelBinding.java, specifically where the type
prefix is encoded as "tls-server-end-point:" followed by the binding
data? I have looked through various RFCs but I can't find exactly where
this format is defined, so I am wondering if this is a standard encoding
or not.
Thanks,
Sean
[1] -
https://support.microsoft.com/en-au/help/4034879/how-to-add-the-ldapenforcechannelbinding-registry-entry
Regards
Alexey
On 8 Jun 2020, at 22:03, Sean Mullan wrote:
(resending to all lists on the review)
I'm just catching up a bit on this review.
Sorry if this has mentioned before, but are you planning to write a CSR and release note?
I think this is needed for the com.sun.jndi.ldap.tls.cbtype property. I'm also wondering
if this property should be documented in the javadocs, and why it is not a standard
property (i.e. "java.naming.ldap.tls.cbtype").
I was also wondering what relation this has to the "G2" standard SASL mechanisms defined
in RFC 5801 [1], and whether that is something we should be using to negotiate this channel
binding, and if not, why not. Or if this is something that is implementation-specific and will only
work with Microsoft LDAP technology, in which case, we might want to make that more explicit,
perhaps by including "microsoft" or something like that in the property name.
Thanks,
Sean
[1] https://tools.ietf.org/html/rfc5801
On 6/8/20 9:07 AM, Aleks Efimov wrote:
Hi Alexey,
I've looked through LdapCtx/LdapClient/SaslBind changes:
Do we need to check if CHANNEL_BINDING is set explicitly for all connection
types? Maybe we can move the check inside 'if (conn.sock instanceof SSLSocket)
{' block.
Also, instead of setting CHANNEL_BINDING in context environment and then
removing it in finally block, it would be better to clone the environment, put
calculated CHANNEL_BINDING into it, and pass the cloned one to
Sasl.createSaslClient.
Another suggestion about the code that verifies if both properties are set
before connection is started:
As you've already mentioned the new code in LdapCtx is only needed for checking
if timeout is set. Could we try to remove LdapCtx::cbType field and all related
methods from LdapCtx (this class is already over-complicated and hard to read)
and replace it with some static method in LdapSasl? It will help to localize
all changes to LdapSasl except for one line in LdapCtx.
I mean something like this:
Replace
+
+// verify LDAP channel binding property
+if (cbType != null && connectTimeout == -1)
+throw new
NamingException(TlsChannelBinding.CHANNEL_BINDING_TYPE +
+" property requires " +
+CONNECT_TIMEOUT +
+" property is set.");
With
+
LdapSasl.checkCbParameters((String)envprops.get(TlsChannelBinding.CHANNEL_BINDING_TYPE),
connectTimeout);
And add something like that to LdapSasl (or maybe pass the full env here):
+ public static void checkCbParameters(String cbTypePropertyValue, int
connectTimeout) throws NamingException {
+ TlsChannelBindingType cbType =
TlsChannelBinding.parseType(cbTypePropertyValue);
+ // verify LDAP channel binding property
+ if (cbType != null && connectTimeout == -1) {
+ throw new NamingException(TlsChannelBinding.CHANNEL_BINDING_TYPE +
+ " property requires com.sun.jndi.ldap.connect.timeout" +
+ " property is set.");
+ }
+ }
Other LdapCtx/LdapClient/SaslBind changes look fine to me.
With Kind Regards,
Aleksei
On 06/06/2020 20:45, Alexey Bakhtin wrote:
Hello Max, Daniel,
Thank you for review.
Please review new version of the patch :
http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v5/
In this version:
- TlsChannelBinding class is moved into the com.sun.jndi.ldap.sasl package
- SSL Ceritificate related code is moved from LdapClient into the
LdapSasl.saslBind method
- verification and removal of internal property is also moved to
LdapSasl.saslBind method
-
Re: RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options
> On Jun 7, 2020, at 6:08 PM, Weijun Wang wrote:
>
> Looks fine to me.
>
> For CSR, since there is already a "Note" there for these 2 options, you can
> add a few words about what -keystore and -trustcacerts can do.
Updated CSR as suggested.
Thanks,
Hai-May
>
> Thanks,
> Max
>
>> On Jun 8, 2020, at 4:01 AM, Hai-May Chao wrote:
>>
>> Updated webrev -
>>
>> https://cr.openjdk.java.net/~hchao/8244148/webrev.02/
>>
>> Thanks,
>> Hai-May
>>
>>
>>> On Jun 5, 2020, at 11:04 PM, Weijun Wang wrote:
>>>
>>> I still think duplicated commands in TrustedCert.java are useless. Line 104
>>> and line 133 are exactly the same, line 109 and line 138 are exactly the
>>> same, and you haven't made any change to these 2 files in between.
>>>
>>> Same for line 80 and line 96 of TrustedCRL.java.
>>>
>>> Everything else is fine.
>>>
>>> Thanks,
>>> Max
>>>
>>>
On Jun 6, 2020, at 2:25 AM, Hai-May Chao wrote:
Updated webrev -
https://cr.openjdk.java.net/~hchao/8244148/webrev.01/
Added one command line -importcert in TrustCert.java.
Added createCacerts() in test/lib SecurityTools.java.
Thanks,
Hai-May
> On Jun 4, 2020, at 5:57 AM, Weijun Wang wrote:
>
>
>
>> On Jun 4, 2020, at 7:29 PM, Hai-May Chao wrote:
>>
>> Hi Max,
>>
>>> On Jun 3, 2020, at 12:59 AM, Weijun Wang wrote:
>>>
>>> The source change looks fine to me.
>>>
>>> In TrustedCert.java:
>>>
>>> - You can use FileOutputStream and Files.copy(Path,OutputStream) in
>>> cat().
>>
>> This cat() is taken from WealAlg.java.
>>
>>>
>>> - There is no need to recreate root.jks and root.pem.
>>
>> The sequences of the commands used in this test scenario allows me to
>> test -printcert for the -trustcacerts and -keytsore options. We had
>> discussion offline about it. The test uses trusted certificates and
>> checks no warnings on the weak algorithms to address the requirement
>> described in the bug. I believe it does serve that purpose, and looks
>> legitimate to me. There could be different ways of testing a
>> functionality, and please let me know if there is a problem with the
>> current approach.
>
> I just meant that the keytool commands generating root.jks and root.pem
> are exactly the same and there is no need to recreate it.
>
>>
>> Please also elaborate your comment about no need to recreate root.jks
>> and root.pem.
>>
>>>
>>> - Why not use -trustcacerts below?
>>>
>>> 160 kt("-importcert -file server.pem -noprompt", "server.jks”);
>>
>>
>> Because here is to import the server (end-entity) cert, and it will not
>> make a difference for the test result whether to use the -trustcacerts
>> or not. It’s the ca (intermediate) cert needs to have it in this test
>> scenario. I intended to leave it out in #160 to distinguish between
>> server and ca certs.
>
> OK.
>
> Then how about we add a new command before line 155?
>
> kt("-importcert -file ca.pem", "ca.jks").shouldNotHaveExitValue(0);
>
> This would prove the "-trustcacerts" on line 155 is really useful.
>
>>
>>>
>>> - It's probably better to add a " " between cmd and options in
>>> patchcmd(). Same in TrustedCRL.java.
>>
>> Ok, will change it.
>>
>>>
>>> In TrustedCRL.java:
>>>
>>> - No need to recreate ks and ca.crl. Just call "-printcrl" with
>>> different options.
>>
>> Same reply as above.
>
> Same question as above.
>
>>
>>>
>>> - Why create using MD5withRSA? Do you meant to warn about the weak
>>> algorithm?
>>
>> Yes, exactly, and it differentiates from the weak algorithm SHA1withRSA
>> used in root CA where no warning will be emitted. There is another
>> -gencrl in #119 without using MD5withRSA so I’d have two test cases.
>>
>>>
>>> Also I would suggest you create a dedicate method (maybe in
>>> SecurityTools.java) to create your own cacerts. There is no need to
>>> copy over the system cacerts, just make sure the file is created with
>>> the JKS storetype. We are thinking of upgrading the storetype of
>>> cacerts and it's nice to do this at a single place so we can modify it
>>> easily later.
>>
>> I created a method in SecurityTools.java to create the own cacerts. With
>> this keystore, the subsequent importing a certificate reply would not
>> work. It turns out that its caks.size() is zero detected at
>> establishCertChain() in keytool/Main.java after root cert has been
>> imported to that cacerts. At this point I’d like to suggest a separate
>> bug be filed to cover the cacerts enhancement that you suggested.
>
> I meant creating the
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
Hi Alexey,
Thank you for incorporating LdapCtx and LdapSasl changes. I've reviewed
both classes and they look good to me, with few minor comments in
LdapSasl.java:
missing spaces in the following lines: 78, 152
With your last changes we can remove explicit cast of 'envProps'
on line 176
I've also run your changes through our CI and one test is failing due to
the changes in GssKrb5Client:
The failed test name: sun/security/krb5/auto/SaslMutual.java
The observed failure:
java.lang.UnsupportedOperationException
at
java.base/java.util.ImmutableCollections.uoe(ImmutableCollections.java:127)
at
java.base/java.util.ImmutableCollections$AbstractImmutableMap.remove(ImmutableCollections.java:910)
at
jdk.security.jgss/com.sun.security.sasl.gsskerb.GssKrb5Client.(GssKrb5Client.java:156)
at
jdk.security.jgss/com.sun.security.sasl.gsskerb.FactoryImpl.createSaslClient(FactoryImpl.java:63)
at
java.security.sasl/javax.security.sasl.Sasl.createSaslClient(Sasl.java:433)
at SaslMutual.main(SaslMutual.java:50)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
Method)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64)
at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:564)
at
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
at java.base/java.lang.Thread.run(Thread.java:832)
For information about CSR process you can start from this wiki page:
https://wiki.openjdk.java.net/display/csr
Best Regards,
Aleksei
On 08/06/2020 22:33, Alexey Bakhtin wrote:
Hello Sean,
Yes, I think we'll need CSR and release notes as soon as this patch adds new
property.
I do not know exact process for it, so I will be grateful if you could explain
me exact steps.
This patch was developed to address compatibility issue with new LDAP
authentication over SSL/TLS announced by Microsoft [1]. It is not related to
RFC 5801. In my opinion “com.sun.jndi.ldap.tls.cbtype” name looks more suitable
for this property and should allow backport it to early JDK versions.
[1] -
https://support.microsoft.com/en-au/help/4034879/how-to-add-the-ldapenforcechannelbinding-registry-entry
Regards
Alexey
On 8 Jun 2020, at 22:03, Sean Mullan wrote:
(resending to all lists on the review)
I'm just catching up a bit on this review.
Sorry if this has mentioned before, but are you planning to write a CSR and release note?
I think this is needed for the com.sun.jndi.ldap.tls.cbtype property. I'm also wondering
if this property should be documented in the javadocs, and why it is not a standard
property (i.e. "java.naming.ldap.tls.cbtype").
I was also wondering what relation this has to the "G2" standard SASL mechanisms defined
in RFC 5801 [1], and whether that is something we should be using to negotiate this channel
binding, and if not, why not. Or if this is something that is implementation-specific and will only
work with Microsoft LDAP technology, in which case, we might want to make that more explicit,
perhaps by including "microsoft" or something like that in the property name.
Thanks,
Sean
[1] https://tools.ietf.org/html/rfc5801
On 6/8/20 9:07 AM, Aleks Efimov wrote:
Hi Alexey,
I've looked through LdapCtx/LdapClient/SaslBind changes:
Do we need to check if CHANNEL_BINDING is set explicitly for all connection
types? Maybe we can move the check inside 'if (conn.sock instanceof SSLSocket)
{' block.
Also, instead of setting CHANNEL_BINDING in context environment and then
removing it in finally block, it would be better to clone the environment, put
calculated CHANNEL_BINDING into it, and pass the cloned one to
Sasl.createSaslClient.
Another suggestion about the code that verifies if both properties are set
before connection is started:
As you've already mentioned the new code in LdapCtx is only needed for checking
if timeout is set. Could we try to remove LdapCtx::cbType field and all related
methods from LdapCtx (this class is already over-complicated and hard to read)
and replace it with some static method in LdapSasl? It will help to localize
all changes to LdapSasl except for one line in LdapCtx.
I mean something like this:
Replace
+
+// verify LDAP channel binding property
+if (cbType != null && connectTimeout == -1)
+throw new
NamingException(TlsChannelBinding.CHANNEL_BINDING_TYPE +
+" property requires " +
+CONNECT_TIMEOUT +
+" property is set.");
With
+
LdapSasl.checkCbParameters((String)envprops.get(TlsChannelBinding.CHANNEL_BINDING_TYPE),
connectTimeout);
And add something like that to LdapSasl (or maybe pass the full env
