Re: RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options

[sha . jiang]

Hi Hai-May,

On 2020/6/13 06:34, Hai-May Chao wrote:

Hi John,

Updated Webrev -
https://cr.openjdk.java.net/~hchao/8244148/webrev.03/

Thanks for this updated webrev!
I have no more comment.

Best regards,
John Jiang



On Jun 11, 2020, at 1:45 AM, sha.ji...@oracle.com 
 wrote:


Hi Hai-May,

On 2020/6/8 04:01, Hai-May Chao wrote:

Updated webrev -

https://cr.openjdk.java.net/~hchao/8244148/webrev.02/

-- src/java.base/share/classes/sun/security/util/FilePaths.java
Would it be better to use String.join() or even java.nio.file.Path to
build the file path?



The current code is based on the original code such as in 
KeyStoreUtil.java and is consistent with others.



-- src/java.base/share/classes/sun/security/util/AnchorCertificates.java
55 File f = new File(FilePaths.cacerts());
...
59 try (FileInputStream fis = new 
FileInputStream(f)) {

f is used only once, so it may be unnecessary.


This is the existing code struct which is working fine.



56 KeyStore cacerts;
Though it's not in your change, would you mind to declare this variable
in the try block? I just want to narrow the variable scope.


Done.



-- test/lib/jdk/test/lib/SecurityTools.java
Could move method createCacerts() to 
test/lib/jdk/test/lib/security/KeyStoreUtils.java?

This class contains a set of methods for creating trust/key stores.
And getCertFromFile() in test/lib/jdk/test/lib/security/CertUtils.java
can load certificate from a file.


Moved the method createCacerts() to the suggested location under test/.
Tried to use getCertFromFile() and it did not work as it read from 
test.src. So we need to read the cert directly.




289 int pos = 0;
...
294 for (String crt : crts) {
If not using for-each style, pos can be declared in the for statement.


Ok, changed to not using for-each style.



--
test/jdk/sun/security/tools/keytool/fakecacerts/java.base/sun/security/util/FilePaths.java
This patch can be used by other tests, so could you please move it to a
common path? Like 
test/jdk/sun/security/util/module_patch/java.base/sun/security/util/FilePaths.java


Moved the patch to the suggested location under test/.



32 public static String cacerts() {
33 return "mycacerts";
34 }
Could it be flexible for returning an alternative path?
For example, accept a system property, say test.cacerts, for specifying
an alternative path. mycacerts is the default value of this property.


Done.

Thanks,
Hai-May




Best regards,
John Jiang



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 

Re: RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options

[Hai-May Chao]

> On Jun 12, 2020, at 5:37 AM, Weijun Wang  wrote:
> 
> I re-read the CSR.
> 
> The precise meaning of the 2 options for -printcert is: "If the cert is a 
> trusted certificate in either keystore or cacerts, we will not warn if it 
> uses a weak signature algorithm". This is so subtle and I wonder it's worth 
> describing it. Or we just say "This command does not check for the weakness 
> of a certificate's signature algorithm if it's a trusted certificate in the 
> user keystore (specified by -keystore) or the `cacerts` keystore (if 
> -trustcacerts is specified)”.

Updated CSR with the second mentioned.

> 
> For -printcrl, the "issuer CA" is a little confusing. I would say "This 
> commands attempts to verify the CRL using a certificate from the user 
> keystore (specified by -keystore) or the `cacerts` keystore (if -trustcacerts 
> is specified), and print out a warning if it cannot be verified”.

CRL issuer is usually the CA, and the implementation of -printcrl tries to get 
its issuer from the user or cacerts keystore. Updated CSR as suggested as if 
there may be confusing.

Thanks,
Hai-May

> 
> Thanks,
> Max
> 
> p.s. A new kind of check comes to my mind. Suppose you are printing a 
> certificate whose own key is 2048-bit RSA, signature is using SHA256withRSA, 
> but the signer key is only 512 bits (we can notice this from the size of the 
> signature). Shall we print out a warning? 

Your thought?

> 
>> On Jun 11, 2020, at 11:21 AM, Weijun Wang  wrote:
>> 
>> "to a self-signed certificate in the keystore". This is not correct. What we 
>> look for is a TrustedCertificateEntry.
>> 
>> "It emits warning when a certificate is not trusted and uses weak 
>> algorithms". Precisely, it's "uses a weak signature algorithm".
>> 
>> --Max
>> 
>> 
>>> On Jun 10, 2020, at 5:31 PM, Hai-May Chao  wrote:
>>> 
>>> 
>>> 
 On Jun 9, 2020, at 5:58 PM, Weijun Wang  wrote:
 
 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.
>>> 
>>> You’re right. Removed it.
>>> 
 
 2. While you said "attempts to construct a chain of trust", do you also 
 want to describe what happens if it succeeds or fails?
>>> 
>>> Updated manpage.
>>> 
 
 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.
 
>>> 
>>> Included the diff of the manpage in the CSR.
>>> 
>>> Thanks,
>>> Hai-May
>>> 
>>> 
 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 

Re: RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options

[Hai-May Chao]

Hi John,

Updated Webrev -
https://cr.openjdk.java.net/~hchao/8244148/webrev.03/

> On Jun 11, 2020, at 1:45 AM, sha.ji...@oracle.com wrote:
> 
> Hi Hai-May,
> 
> On 2020/6/8 04:01, Hai-May Chao wrote:
>> Updated webrev -
>> 
>> https://cr.openjdk.java.net/~hchao/8244148/webrev.02/ 
>> -- 
>> src/java.base/share/classes/sun/security/util/FilePaths.java
> Would it be better to use String.join() or even java.nio.file.Path to
> build the file path?
> 

The current code is based on the original code such as in KeyStoreUtil.java and 
is consistent with others.

> -- src/java.base/share/classes/sun/security/util/AnchorCertificates.java
> 55 File f = new File(FilePaths.cacerts());
> ...
> 59 try (FileInputStream fis = new FileInputStream(f)) {
> f is used only once, so it may be unnecessary.

This is the existing code struct which is working fine.

> 
> 56 KeyStore cacerts;
> Though it's not in your change, would you mind to declare this variable
> in the try block? I just want to narrow the variable scope.

Done.

> 
> -- test/lib/jdk/test/lib/SecurityTools.java
> Could move method createCacerts() to 
> test/lib/jdk/test/lib/security/KeyStoreUtils.java?
> This class contains a set of methods for creating trust/key stores.
> And getCertFromFile() in test/lib/jdk/test/lib/security/CertUtils.java
> can load certificate from a file.

Moved the method createCacerts() to the suggested location under test/.
Tried to use getCertFromFile() and it did not work as it read from test.src. So 
we need to read the cert directly.

> 
> 289 int pos = 0;
> ...
> 294 for (String crt : crts) {
> If not using for-each style, pos can be declared in the for statement.

Ok, changed to not using for-each style.

> 
> --  
> test/jdk/sun/security/tools/keytool/fakecacerts/java.base/sun/security/util/FilePaths.java
> This patch can be used by other tests, so could you please move it to a
> common path? Like 
> test/jdk/sun/security/util/module_patch/java.base/sun/security/util/FilePaths.java

Moved the patch to the suggested location under test/.

> 
> 32 public static String cacerts() {
> 33 return "mycacerts";
> 34 }
> Could it be flexible for returning an alternative path?
> For example, accept a system property, say test.cacerts, for specifying
> an alternative path. mycacerts is the default value of this property.

Done.

Thanks,
Hai-May


> 
> Best regards,
> John Jiang
> 
>> 
>> 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?
>>> 

Re: Request for review, typo in exception message

[Bradford Wetmore]

Approved.  :)

Or if you want, I could review this much more carefully...

Brad


On 6/12/2020 2:43 PM, Xuelei Fan wrote:

Hi,

May I have the following typo correction reviewed in test file?

$ hg diff test/jdk/javax/net/ssl/SSLEngine/IllegalHandshakeMessage.java
@@ -70,7 +70,7 @@
  cliToSrv.put(7, (byte)0x80);    // use illegal message length
  } else {
  // unlikely
-    throw new Exception("No handshage message generated.");
+    throw new Exception("No handshake message generated.");
  }


Thanks,
Xuelei

Request for review, typo in exception message

[Xuelei Fan]

Hi,

May I have the following typo correction reviewed in test file?

$ hg diff test/jdk/javax/net/ssl/SSLEngine/IllegalHandshakeMessage.java
@@ -70,7 +70,7 @@
 cliToSrv.put(7, (byte)0x80);// use illegal message length
 } else {
 // unlikely
-throw new Exception("No handshage message generated.");
+throw new Exception("No handshake message generated.");
 }


Thanks,
Xuelei

Re: RFR 8245679: KeyStore cannot probe PKCS12 keystore if BouncyCastle is the top security provider

[Sean Mullan]

Looks good to me.

--Sean

On 6/8/20 8:25 AM, Weijun Wang wrote:

Please take a look at

https://cr.openjdk.java.net/~weijun/8245679/webrev.00/

If two providers support the same keystore type, we only try engineProbe() on 
the 1st one, and fail if it hasn't implemented it. The correct way is to try 
KeyStoreSpi from all providers.

I also fixed an error that engineProbe() of both JKS and CaseExactJKS would 
return true on a JKS keystore, since the method is implemented in their common 
base class. This means there's a chance that a JKS keystore would be recognized 
as a CaseExactJKS keystore. This is seen after my code change in KeyStore.

Thanks,
Max

Re: RFR: 8218021: jarsigner strips the execute permission when signing a .zip file

[Lance Andersen]

Hi Sean,

I think your changes look fine so all good FMPOV.

Best
Lance

> On Jun 12, 2020, at 6:21 AM, Seán Coffey  wrote:
> 
> Hi,
> 
> I'd like to reboot this jarsigner enhancement request[1]. I've removed the 
> problem references to zip file name extensions. Instead, there's a new JDK 
> implementation specific jarsigner option: -keepposixperms
> 
> https://bugs.openjdk.java.net/browse/JDK-8218021
> https://cr.openjdk.java.net/~coffeys/webrev.8218021.v2/webrev/
> 
> regards,
> Sean.
> 
> [1] 
> http://mail.openjdk.java.net/pipermail/security-dev/2020-January/021141.html
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options

[Weijun Wang]

I re-read the CSR.

The precise meaning of the 2 options for -printcert is: "If the cert is a 
trusted certificate in either keystore or cacerts, we will not warn if it uses 
a weak signature algorithm". This is so subtle and I wonder it's worth 
describing it. Or we just say "This command does not check for the weakness of 
a certificate's signature algorithm if it's a trusted certificate in the user 
keystore (specified by -keystore) or the `cacerts` keystore (if -trustcacerts 
is specified)".

For -printcrl, the "issuer CA" is a little confusing. I would say "This 
commands attempts to verify the CRL using a certificate from the user keystore 
(specified by -keystore) or the `cacerts` keystore (if -trustcacerts is 
specified), and print out a warning if it cannot be verified".

Thanks,
Max

p.s. A new kind of check comes to my mind. Suppose you are printing a 
certificate whose own key is 2048-bit RSA, signature is using SHA256withRSA, 
but the signer key is only 512 bits (we can notice this from the size of the 
signature). Shall we print out a warning? 

> On Jun 11, 2020, at 11:21 AM, Weijun Wang  wrote:
> 
> "to a self-signed certificate in the keystore". This is not correct. What we 
> look for is a TrustedCertificateEntry.
> 
> "It emits warning when a certificate is not trusted and uses weak 
> algorithms". Precisely, it's "uses a weak signature algorithm".
> 
> --Max
> 
> 
>> On Jun 10, 2020, at 5:31 PM, Hai-May Chao  wrote:
>> 
>> 
>> 
>>> On Jun 9, 2020, at 5:58 PM, Weijun Wang  wrote:
>>> 
>>> 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.
>> 
>> You’re right. Removed it.
>> 
>>> 
>>> 2. While you said "attempts to construct a chain of trust", do you also 
>>> want to describe what happens if it succeeds or fails?
>> 
>> Updated manpage.
>> 
>>> 
>>> 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.
>>> 
>> 
>> Included the diff of the manpage in the CSR.
>> 
>> Thanks,
>> Hai-May
>> 
>> 
>>> 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 

RFR: 8218021: jarsigner strips the execute permission when signing a .zip file

[Seán Coffey]

Hi,

I'd like to reboot this jarsigner enhancement request[1]. I've removed 
the problem references to zip file name extensions. Instead, there's a 
new JDK implementation specific jarsigner option: -keepposixperms


https://bugs.openjdk.java.net/browse/JDK-8218021
https://cr.openjdk.java.net/~coffeys/webrev.8218021.v2/webrev/

regards,
Sean.

[1] 
http://mail.openjdk.java.net/pipermail/security-dev/2020-January/021141.html

Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos

[Daniel Fuchs]

Hi Alexey,

This is starting to look good.
Thank you for persisting!

I have a couple of comments - on the LDAP/JNDI side.

There are several places where your new code is supposed to
trigger the throwing of a NamingException:

  LdapSasl.java: lines 76, 85, 140, 168

Please write a test to verify that it does so. Since the
exceptions are all supposed to be thrown before the actual
binding happens, there's no need to have an actual LDAP server
that supports any kind of authentication to test that.

The simple dummy servers that we have in the test base should
be enough to write such a test.

In addition, there are a couple of places where stray exceptions
could theoretically be thrown, and where they should be wrapped in 
NamingException (but are not). Maybe it's OK, because this has

been checked before - but it would be better if we had a test that
proves that it is so:

For instance: LdapSasl.java
  82 connectTimeout = Integer.parseInt((String)propVal);
What if the value of the propVal is "Foo" - or not a String?
What unexpected exception might be relayed to the calling code?

Still in that same file:
  84 if (connectTimeout == -1)

should probably be if (connectTimeout <= 0) since
Connection.java checks for connectTimeout > 0 to determine
whether to start the initial handshake.

Which makes me wonder if we should find a better way to
instruct Connection to start the initial handshake...

TlsChannelBinding.java:

It would be much better if static factories methods were used instead of
public constructors. That would allow you to check all arguments before
actually constructing your object:

public static TlsChannelBinding create((byte[] finishedMessage) throws 
SaslException  {
throw new UnsupportedOperationException("tls-unique channel binding 
is not supported");

}

public statuic TlsChannelBinding create(X509Certificate 
serverCertificate) throws SaslException {

   var cbType = TlsChannelBindingType.TLS_SERVER_END_POINT;
   byte[] cbData;
   try {
  // compute cbdata
   ...

   return new TlsChannelBinding(cbType, cbData);
}

private TlsChannelBinding(TlsChannelBindingType cbType, byte[] cbData) {
   this.cbType = cbType;
   this.cbData = cbData;
}

That's all I have for now.

best regards,

-- daniel


On 09/06/2020 22:03, Alexey Bakhtin wrote:

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