Re: RFR 8206915: XDH TCK issues
On 7/11/2018 9:12 AM, Adam Petcher wrote: On 7/11/2018 12:02 PM, Xuelei Fan wrote: Does it make sense if secret is not temporarily stored as a class filed? I agree that it's a bit strange, but it is organized this way because of the zero result check described in the RFC. If the result of the key agreement is zero, then that means that the public key is invalid. So we compute the shared secret early in engineDoPhase so we can throw an InvalidKeyException at the correct time. Then the computed secret is kept around so it can be returned by engineGenerateSecret. I see. Thanks, Xuelei
Re: RFR 8206915: XDH TCK issues
On 7/11/18 11:01 AM, Adam Petcher wrote: On 7/11/2018 10:41 AM, Sean Mullan wrote: XDHKeyAgreement.java 176 byte[] result = secret; Shouldn't this be: 176 byte[] result = secret.clone(); since engineGenerateSecret() says it is returned in a new buffer. I don't think cloning is necessary. The new array is created in engineDoPhase, and it is always set to null in engineGenerateSecret after it is returned or copied to the output buffer. In essence, this overload of engineDoPhase transfers ownership of the array, and the other one destroys it. So this engineDoPhase effectively returns a new array, and I don't think it is possible for two clients (in the same thread) to get the same array from these methods. Though I would appreciate it if you could double-check this and make sure you agree. Ok, I see. I think it should be ok. I checked and the KeyAgreement/Spi APIs do not specifically say anything about concurrent access by multiple threads. However, typically I think it's normal to assume that they don't support concurrent access (i.e. unexpected behavior may result if so) unless it specifically says so. --Sean
Re: RFR 8206915: XDH TCK issues
On 7/11/2018 12:02 PM, Xuelei Fan wrote: Does it make sense if secret is not temporarily stored as a class filed? I agree that it's a bit strange, but it is organized this way because of the zero result check described in the RFC. If the result of the key agreement is zero, then that means that the public key is invalid. So we compute the shared secret early in engineDoPhase so we can throw an InvalidKeyException at the correct time. Then the computed secret is kept around so it can be returned by engineGenerateSecret. Xuelei
Re: RFR 8206915: XDH TCK issues
Does it make sense if secret is not temporarily stored as a class filed? Xuelei On 7/11/2018 8:01 AM, Adam Petcher wrote: On 7/11/2018 10:41 AM, Sean Mullan wrote: XDHKeyAgreement.java 176 byte[] result = secret; Shouldn't this be: 176 byte[] result = secret.clone(); since engineGenerateSecret() says it is returned in a new buffer. I don't think cloning is necessary. The new array is created in engineDoPhase, and it is always set to null in engineGenerateSecret after it is returned or copied to the output buffer. In essence, this overload of engineDoPhase transfers ownership of the array, and the other one destroys it. So this engineDoPhase effectively returns a new array, and I don't think it is possible for two clients (in the same thread) to get the same array from these methods. Though I would appreciate it if you could double-check this and make sure you agree.
Re: RFR 8206915: XDH TCK issues
On 7/11/2018 10:41 AM, Sean Mullan wrote: XDHKeyAgreement.java 176 byte[] result = secret; Shouldn't this be: 176 byte[] result = secret.clone(); since engineGenerateSecret() says it is returned in a new buffer. I don't think cloning is necessary. The new array is created in engineDoPhase, and it is always set to null in engineGenerateSecret after it is returned or copied to the output buffer. In essence, this overload of engineDoPhase transfers ownership of the array, and the other one destroys it. So this engineDoPhase effectively returns a new array, and I don't think it is possible for two clients (in the same thread) to get the same array from these methods. Though I would appreciate it if you could double-check this and make sure you agree.
Re: RFR 8206915: XDH TCK issues
XDHKeyAgreement.java 176 byte[] result = secret; Shouldn't this be: 176 byte[] result = secret.clone(); since engineGenerateSecret() says it is returned in a new buffer. Looks fine otherwise. --Sean On 7/9/18 11:29 AM, Adam Petcher wrote: A couple of conformance issues were found during TCK development for XDH. Both required very small modifications to fix, so I put them under the same ticket/review in order to save time. The actual information about the issues can be found in the sub-tasks of the ticket. This fix needs to go into JDK 11, so I'm hoping a Reviewer can take a look at this soon. JBS: https://bugs.openjdk.java.net/browse/JDK-8206915 Webrev: http://cr.openjdk.java.net/~apetcher/8206915/webrev.00/
