On 9/5/2018 12:49 PM, Adam Petcher wrote:
New webrev: http://cr.openjdk.java.net/~apetcher/8171279/webrev.02/
On 9/5/2018 1:35 PM, Xuelei Fan wrote:
On 9/5/2018 10:09 AM, Adam Petcher wrote:
Is there some place in the code where JSSE is doing something too
complicated related to these parameters?
Yes, the algorithm name is sufficient, I'm not sure the necessary to
use XECParameters in sun.security.util.
The algorithm name is not quite sufficient. See the new methods that
were added to ECUtil that encode/decode public keys. We also need to
know the key length (which is in XECParameters) in order to
encode/decode public keys.
I did not get your point. The public key sizes for x25519 and x448 are
fixed, right?
I simply continued the pattern that was used in DH and ECDH. We can
use a different string here, but I worry that people will be
surprised if DH and ECDH support "TlsPreMasterSecret" but XDH doesn't.
It could support "TlsPreMasterSecret", right? My concern is about not
limited to this one only.
Yes, it supports "TlsPreMasterSecret" in the webrev now. Perhaps I'm not
sure what you are suggesting here.
OKay, let me say it in another way.
Thank you for making it works with the SunJSSE provider as you use a
SunJSSE provider private algorithm name "TlsPreMasterSecret", and
implement the algorithm in the cyrpto provider. That's good.
The "TlsPreMasterSecret" is not a public one, and it is not expected to
be used in other JSSE provider. If I want to implement a JSSE provider
by myself, and I use the name "x25519", the crypto provider will deny
it. So I have to use the "TlsPreMasterSecret" for my provider.
However, the name is not supported, and the name can be changed at
anytime in the future. How should I do to use the crypto provider for
my JSSE provider? Looks like no way to use the JDK cyrpto provider
unless I use the SunJSSE private name. Should I implement my own crypto
provider? I don't want to do that unless it is really necessary.
For the KeyAgreement.generateSecret(String algorithm) method, it seems
that the supported algorithms of each provider are missed in the
documentation. As may make the method hard to use.
This issue is not specific to XDH. I'm fine if you don't want to
address it in this update.
3. NamedGroupFunctions
It might be more straightforward to define these functions in
NamedGroup. If comparing
nameGroup.getFunctions().getParameterSpec() and
namedGroup.getParameterSpec(), I'd prefer the latter.
A simple way to accomplish this is to leave the structure alone and
add methods in NamedGroup that pass through to the corresponding
methods in NamedGroupFunctions. I made this change in the updated
webrev. Let me know what you think.
It looks like a problem to me to use this before it constructed.
this.functions = new ECDHFunctions(this);
and the use of new object for each named group is not effective. The
current NamedGroupFunctions abstract class does not sound good enough
to me, considering the numbers of the named groups.
I have no idea so far, but I think you can improve it. Probably use
static methods?
In the latest webrev, I changed it so there is a single static
NamedGroupFunctions of each type, and the NamedGroup is passed in as the
first argument to each method that requires it (rather than being a
member of NamedGroupFunctions).
After the re-org, I guess you can put the inner classes in NamedGroup
enum and declare them as private? The getFunctions() method may be
unnecessary then.
Thanks,
Xuelei
