Looks fine to me.
--Sean
On 5/30/18 5:39 PM, Jamil Nimeh wrote:
Hopefully the final set of updates before committing this feature:
* Updated the ChaChaEngine implementations in ChaCha20Cipher.java to
properly conform to Cipher's specification when the output buffer
used in doUpdate
Hopefully the final set of updates before committing this feature:
* Updated the ChaChaEngine implementations in ChaCha20Cipher.java to
properly conform to Cipher's specification when the output buffer
used in doUpdate is too small. Instead of throwing
IndexOutOfBoundsException like
The updated webrev looks good. I have no further comments.
--Sean
On 5/22/18 4:43 PM, Jamil Nimeh wrote:
A couple updates:
* Touched up the comments surrounding init/wrap/unwrap methods. None
of these affect public javadocs.
* Added an implementation for engineGetParameters. This was
A couple updates:
* Touched up the comments surrounding init/wrap/unwrap methods. None
of these affect public javadocs.
* Added an implementation for engineGetParameters. This was something
that just got forgotten from the initial development of the cipher
when there were no
Wow, good catch. This is something that got left by the wayside,
probably from one of the earliest revisions of the code when we didn't
have an AlgorithmParameters implementation. I'll get this fixed up
today. Shouldn't take too long to get this working. I think it should
return null if it
Question on engineGetParameters() in ChaCha20Cipher.java:
231 protected AlgorithmParameters engineGetParameters() {
232 return null;
233 }
Shouldn't this return the params that are passed into engineInit()?
Also, when null is passed in and the params are generated using
Hi John, comments in-line
On 5/21/2018 8:30 PM, sha.ji...@oracle.com wrote:
Hi Jamil,
-- ChaCha20Cipher.java
497 /**
498 * Perform additional initialization actions based on the key
and operation
499 * type.
500 *
501 * @param opmode the type of operation to do.
Hi Jamil,
-- ChaCha20Cipher.java
497 /**
498 * Perform additional initialization actions based on the key
and operation
499 * type.
500 *
501 * @param opmode the type of operation to do. This value must
be either
502 * {@code Cipher.ENCRYPT_MODE} or
And the fun just keeps going! Round 7.
This revision removes wrap/unwrap support for ChaCha20 and
ChaCha20-Poly1305 until there is a standardized key wrapping approach
for these ciphers (similar to how AES has its own key wrapping scheme in
RFC 3394).
Initializing the cipher with
Hi Jamil,
On 19/05/2018 22:40, Jamil Nimeh wrote:
Hi John, comments below:
On 5/19/2018 1:31 AM, sha.ji...@oracle.com wrote:
Hi Jamil,
On 19/05/2018 07:27, Jamil Nimeh wrote:
Hi John,
Yes, the second call must throw IllegalStateException. See the
class description in
Hi John, comments below:
On 5/19/2018 1:31 AM, sha.ji...@oracle.com wrote:
Hi Jamil,
On 19/05/2018 07:27, Jamil Nimeh wrote:
Hi John,
Yes, the second call must throw IllegalStateException. See the class
description in javax.crypto.Cipher where it talks about AEAD modes
and AAD
Hi Jamil,
On 19/05/2018 07:27, Jamil Nimeh wrote:
Hi John,
Yes, the second call must throw IllegalStateException. See the class
description in javax.crypto.Cipher where it talks about AEAD modes and
AAD processing. It states that all AAD has to be supplied before
update and/or doFinal
Hi John,
Yes, the second call must throw IllegalStateException. See the class
description in javax.crypto.Cipher where it talks about AEAD modes and
AAD processing. It states that all AAD has to be supplied before update
and/or doFinal methods are invoked. This constraint is also talked
Hi Jamil,
-- ChaCha20Cipher.java
430 } else if (aadDone) {
431 // No AAD updates allowed after the PT/CT update
method is called
432 throw new IllegalStateException("Attempted to update
AAD on " +
433 "Cipher after plaintext/ciphertext
Round 6.
This brings ChaCha20/ChaCha20-Poly1305 into conformance with Cipher's
specification when forms of init that take AlgorithmParameters or
AlgorithmParameterSpec are used. Previously, a non-null AP or APS object
was required. In order to better conform to the specification, if a
null
Okay, let me fix the code and the CSR and make them match. I'll put the
CSR back in draft and make a note in the comments about what is
changing. This is an innocuous change, but if Sean feels that code-wise
the final class should not have final methods, then making the CSR match
the fixed
To clarify, both the impl and the CSR mark the ChaCha20ParameterSpec
class as final, but the CSR also (accidentally) marked the methods of
the class as final. If that qualifies as an API change, then we should
fix the CSR, but I don't know for sure. It's an odd case.
--Sean
On 5/8/18 2:03
Hi,
"final" is a important modifier of the method signature and if the CSR
and the implementation are
different it might raise a question about which is the correct signature
when the JCK folks write tests.
It is pretty lightweight process to return the CSR to draft and update
it and
On 5/8/18 1:52 PM, Jamil Nimeh wrote:
I'll make those fixes. One question with respect to the final methods:
the CSR had those methods in the description and they were marked as
final. That CSR is now complete. Will removing the final qualifier in
the methods be an issue? In terms of
I'll make those fixes. One question with respect to the final methods:
the CSR had those methods in the description and they were marked as
final. That CSR is now complete. Will removing the final qualifier in
the methods be an issue? In terms of effect on the code it doesn't
matter. It's
- ChaCha20ParameterSpec.java
The methods don't need to be final now that the class is final.
- ChaCha20Poly1305Parameters.java
122 if (decodingMethod.equalsIgnoreCase(DEFAULT_FMT)) {
123 engineInit(encoded);
The spec for engineInit() says if decodingMethod is null, the
Round 5.
This adds Sean's comments. Sean, I was never able to execute a case on
init where a half-baked object would fail. In most cases it would fail
in checks in javax.crypto.Cipher before it ever made it down to my
code. I'm pretty confident the init sequence is OK. I did move the
On 5/2/18 11:23 AM, Jamil Nimeh wrote:
Ok. I think as long as these fields are never dependent on previous
values if you call engineInit again, then it is ok. In other words,
they should be the same even if the previous call to engineInit throws
an Exception. It might be safer as the first
On 05/02/2018 07:35 AM, Sean Mullan wrote:
On 5/1/18 6:55 PM, Jamil Nimeh wrote:
262 counter = 1;
Here you set the counter field to 1, but this method can still throw
exceptions after that. Is there any risk of leaving the object in an
inconsistent state by setting counter=1 if
On 5/1/18 6:55 PM, Jamil Nimeh wrote:
262 counter = 1;
Here you set the counter field to 1, but this method can still throw
exceptions after that. Is there any risk of leaving the object in an
inconsistent state by setting counter=1 if this method fails to
complete successfully? Same
On 5/1/18 5:45 PM, Jamil Nimeh wrote:
134 throw new IllegalArgumentException(
135 "Unsupported parameter format: " +
decodingMethod);
Although this seems like a reasonable exception to throw,
AlgorithmParametersSpi.engineInit is not spec'ed to throw this, so
Update: forget my comment on this finding, Sean. I'm already wrapping
IOE in IAPE when IOE gets thrown so it's better to go the route you
suggested. I didn't read the method carefully enough.
375 if (dv.tag == DerValue.tag_OctetString) {
376
Hi Sean, many thanks for taking a detailed look at the code. My
comments are in-line
On 5/1/2018 1:53 PM, Sean Mullan wrote:
On 5/1/18 1:20 PM, Sean Mullan wrote:
On 4/27/18 5:21 PM, Jamil Nimeh wrote:
Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff
mostly:
* Added
Comments in-line:
On 5/1/2018 10:20 AM, Sean Mullan wrote:
On 4/27/18 5:21 PM, Jamil Nimeh wrote:
Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff
mostly:
* Added words in the description of javax.crypto.Cipher recommending
callers reinitialize the Cipher to use
On 5/1/18 1:20 PM, Sean Mullan wrote:
On 4/27/18 5:21 PM, Jamil Nimeh wrote:
Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff
mostly:
* Added words in the description of javax.crypto.Cipher recommending
callers reinitialize the Cipher to use different nonces after each
On 4/27/18 5:21 PM, Jamil Nimeh wrote:
Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff mostly:
* Added words in the description of javax.crypto.Cipher recommending
callers reinitialize the Cipher to use different nonces after each
complete encryption or decryption
Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff mostly:
* Added words in the description of javax.crypto.Cipher recommending
callers reinitialize the Cipher to use different nonces after each
complete encryption or decryption (similar language to what exists
already
Agreed. I will make that change.
--Jamil
On 04/26/2018 11:20 AM, Sean Mullan wrote:
On 4/26/18 1:36 PM, Jamil Nimeh wrote:
Will do. I made some wording changes there to move away from
"96-bit" to "12-byte" also since that probably is more useful to
developers.
One other comment is that
On 4/26/18 1:36 PM, Jamil Nimeh wrote:
Will do. I made some wording changes there to move away from "96-bit"
to "12-byte" also since that probably is more useful to developers.
One other comment is that I think the get methods of
ChaCha20ParameterSpec should be final. There should be no
On 4/26/18 11:57 AM, Sean Mullan wrote:
The ChaCha20ParameterSpec.java file should have an @since 11 annotation
on it.
Also:
65 if (nonce.length == 12) {
66 this.nonce = nonce.clone();
67 } else {
68 throw new IllegalArgumentException(
69
gt;, OpenJDK Dev list <security-dev@openjdk.java.net>
Subject: Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations
On 4/26/18 11:57 AM, Sean Mullan wrote:
> The ChaCha20ParameterSpec.java file should have an @since 11 annotation
> on it.
Also:
65 if (non
o: Jamil Nimeh <jamil.j.ni...@oracle.com>,
OpenJDK Dev list <security-dev@openjdk.java.net> Subject: Re: RFR: ChaCha20 and
ChaCha20/Poly1305 Cipher implementations
The ChaCha20ParameterSpec.java file should have an @since 11 annotation
on it.
--Sean
On 3/26/18 3:08 PM, Jamil Nimeh wrot
The ChaCha20ParameterSpec.java file should have an @since 11 annotation
on it.
--Sean
On 3/26/18 3:08 PM, Jamil Nimeh wrote:
Hello all,
This is a request for review for the ChaCha20 and ChaCha20-Poly1305
cipher implementations. Links to the webrev and the JEP which outlines
the
Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:
* Removed the key field in ChaCha20 and Poly1305 implementations and
only retain the key bytes as an object field (thanks Thomas for catching
this)
* Added additional protections against key/nonce reuse. This is a
behavioral change to
Hi,
ok that is an good point that if we have more values in the structure
than we use this can make some confusion.
I was only looking from the coding point of view and saw that if i can
use the same structure for booth cases this
can reduce the coding overhead. But i can fully understand
Hi,
same with key/keyBytes is with the class Poly1305 also here the key is
not used.
Gruß Thomas
On 4/11/2018 5:54 PM, Jamil Nimeh wrote:
Yes, that does appear to be the case, good catch! I'll make that change.
--Jamil
On 4/11/2018 7:18 AM, Thomas Lußnig wrote:
Hi,
i found another
Hi,
i found another point. The "key" field can be removed from ChaCha20Cipher.
1) This field is only set once and later checked if it was initialized.
But we do not want to knew is the key exists but if key bytes exists.
2) So if two lines are changed from key to keyBytes we can remove this
Okay, I will check that out and fix as appropriate. Thanks for the
comments!
--Jamil
On 4/11/2018 10:56 AM, Thomas Lußnig wrote:
Hi,
same with key/keyBytes is with the class Poly1305 also here the key is
not used.
Gruß Thomas
On 4/11/2018 5:54 PM, Jamil Nimeh wrote:
Yes, that does
Yes, that does appear to be the case, good catch! I'll make that change.
--Jamil
On 4/11/2018 7:18 AM, Thomas Lußnig wrote:
Hi,
i found another point. The "key" field can be removed from
ChaCha20Cipher.
1) This field is only set once and later checked if it was initialized.
But we do
Hello everyone,
This is a quick update to the previous webrev:
* When using the form of engineInit that does only takes op, key and
random, the nonce will always be random even if the random parameter is
null. A default instance of SecureRandom will be used to create the
nonce in this case,
Hello Thomas, et al.,
On 3/26/2018 1:49 PM, Jamil Nimeh wrote:
Hi Thomas, thanks for the feedback
1. Were there guidelines? Not really, though I looked at other
parameter definitions in com.sun.crypto.provider and tried to
follow along the same lines that they do. One thing that
I think I could pull in Convert.hexStringToByteArray. I might try
pulling in that test class locally just to make sure things still run
before you commit the field arithmetic stuff. I also noticed I left in
some debug routines in ChaCha20Cipher that need to get the axe, so I'll
cut those out
On 3/28/2018 2:48 AM, sha.ji...@oracle.com wrote:
Would you like to move this method to a test lib class, like
test/lib/jdk/test/lib/Utils.java? In fact, this class has a method,
named toHexString, for converting bin to hex.
This method appears to be the same as Convert.hexStringToByteArray
Hi Jamil,
I have a minor point on your tests.
-- com/sun/crypto/provider/Cipher/ChaCha20/ChaCha20KAT.java
505 private static byte[] hex2bin(String hex) {
506 int i;
507 int len = hex.length();
508 byte[] data = new byte [len / 2];
509 for (i = 0; i < len;
Hi,
this choice is even better than the current version. Because than the
default system wide
secure random provider is used.
Gruß Thomas
On 3/27/2018 12:23 AM, Jamil Nimeh wrote:
Another thought on #2: Another way we could go with this is to create
a new SecureRandom() or use
Hi Jamil,
1) where there any guidelines about how the engineToString should be
formatted ?
I ask because i wondering why we need two new lines with access to the
System property.
If it is represented as single line json no need to line break would be
needed.
Gruß Thomas
/** * Creates a
Another thought on #2: Another way we could go with this is to create a
new SecureRandom() or use JceSecurity.RANDOM when the random parameter
is null. That would make init(op, key, random) and init(op, key) behave
the same when random is null. You would always get a random nonce in
these
Hi Thomas, thanks for the feedback
1. Were there guidelines? Not really, though I looked at other
parameter definitions in com.sun.crypto.provider and tried to follow
along the same lines that they do. One thing that should be changed
is the LINE_SEP assignment shouldn't be an
Hello all,
This is a request for review for the ChaCha20 and ChaCha20-Poly1305
cipher implementations. Links to the webrev and the JEP which outlines
the characteristics and behavior of the ciphers are listed below.
http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
54 matches
Mail list logo
