RFR 8171279: Support X25519 and X448 in TLS 1.3

classic Classic list List threaded Threaded
24 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFR 8171279: Support X25519 and X448 in TLS 1.3

Adam Petcher
Webrev: http://cr.openjdk.java.net/~apetcher/8171279/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8171279

Please review the following change to add support for X25519 and X448
(XDH) to TLS 1.3. This change includes some refactoring to remove code
that was duplicated for DH and ECDH, and to avoid adding more for XDH.
In addition to running the included regression test, I tested by
connecting to an openssl server and confirmed that the connection was
established using TLS 1.3 and X25519/X448.

Here are some detailed notes:

*) The NamedGroupFunctions class was added to hold the functions that
are needed for key agreement with some named group. Most of the
group-specific code was moved into subclasses of NamedGroupFunctions.
This allowed me to remove a bunch of code like "if (type == ECDHE) {...}
else if (type == FFDHE) {...}".
*) There are a couple of files in the webrev with no changes due to a
webrev issue. Please ignore them.
*) I moved some code related to XDH parameters and encoding into
java.base. ECUtil now has code to encode/decode XDH public keys, and the
XECParameters file was moved into java.base/sun.security.util. This
organization is similar to how CurveDB and NamedCurve are in java.base,
and it allows the TLS implementation to encode/decode keys without using
the jdk.crypto.ec module.

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

Xuelei Fan-2
I have no finished the full code review.  So far, I have a few question
about the struct of the code.
1. XECParameters
I can see the reason to dynamic parameters for something other than
X25519/X448.  But for JSSE, currently, only named curves are used.  It
would be nice to use the name for the static parameters.

2. "TlsPremasterSecret" only XDH key agreement
It would be nice the JCE implementation can support key agreement other
than TLS protocols and providers other than SunJSSE provider.  It would
be nice if the JCE implementation does not bind to the SunJSSE provider
private algorithm name.

We used to use SunJSSE private name in the JCE crypto implementation.
But there are some known problems with this dependence.

Is there a problem to support generic key agreement?

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.

4. SSLKeyAgreementCredentials
I did not see too much benefit of this new interface.  It is not always
true that a key agreement could have a public key.  Although we can
simplify the key agreement for public key based, but it also add
additional layers.

I know where this improvement comes from.  Maybe, you can consolidate
the named group functions, and encode/decode the credentials there.

Xuelei


On 8/30/2018 8:58 AM, Adam Petcher wrote:

> Webrev: http://cr.openjdk.java.net/~apetcher/8171279/webrev.00/
> JBS: https://bugs.openjdk.java.net/browse/JDK-8171279
>
> Please review the following change to add support for X25519 and X448
> (XDH) to TLS 1.3. This change includes some refactoring to remove code
> that was duplicated for DH and ECDH, and to avoid adding more for XDH.
> In addition to running the included regression test, I tested by
> connecting to an openssl server and confirmed that the connection was
> established using TLS 1.3 and X25519/X448.
>
> Here are some detailed notes:
>
> *) The NamedGroupFunctions class was added to hold the functions that
> are needed for key agreement with some named group. Most of the
> group-specific code was moved into subclasses of NamedGroupFunctions.
> This allowed me to remove a bunch of code like "if (type == ECDHE) {...}
> else if (type == FFDHE) {...}".
> *) There are a couple of files in the webrev with no changes due to a
> webrev issue. Please ignore them.
> *) I moved some code related to XDH parameters and encoding into
> java.base. ECUtil now has code to encode/decode XDH public keys, and the
> XECParameters file was moved into java.base/sun.security.util. This
> organization is similar to how CurveDB and NamedCurve are in java.base,
> and it allows the TLS implementation to encode/decode keys without using
> the jdk.crypto.ec module.
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

Adam Petcher
Updated webrev: http://cr.openjdk.java.net/~apetcher/8171279/webrev.01/

On 9/4/2018 3:25 PM, Xuelei Fan wrote:

> I have no finished the full code review.  So far, I have a few
> question about the struct of the code.
> 1. XECParameters
> I can see the reason to dynamic parameters for something other than
> X25519/X448.  But for JSSE, currently, only named curves are used.  It
> would be nice to use the name for the static parameters.

Sorry, I don't think I understand what you are suggesting here. As far
as I know, nothing in sun.security.ssl uses XECParameters directly. It
is used indirectly through ECUtil to encode and decode keys, and in this
case the name is used to look up the parameters. Is there some place in
the code where JSSE is doing something too complicated related to these
parameters?

>
> 2. "TlsPremasterSecret" only XDH key agreement
> It would be nice the JCE implementation can support key agreement
> other than TLS protocols and providers other than SunJSSE provider. 
> It would be nice if the JCE implementation does not bind to the
> SunJSSE provider private algorithm name.
>
> We used to use SunJSSE private name in the JCE crypto implementation.
> But there are some known problems with this dependence.
>
> Is there a problem to support generic key agreement?

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. I would rather
continue to use "TlsPreMasterSecret" for now, and then add support for a
standard, TLS-independent name (that means the same thing) in a separate
ticket. But if you feel strongly that XDH should not support
"TlsPreMasterSecret", then perhaps we can use a different string now. In
this case, let me know if you have any suggestions for what string to use.

>
> 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.

>
> 4. SSLKeyAgreementCredentials
> I did not see too much benefit of this new interface.  It is not
> always true that a key agreement could have a public key. Although we
> can simplify the key agreement for public key based, but it also add
> additional layers.
>
> I know where this improvement comes from.  Maybe, you can consolidate
> the named group functions, and encode/decode the credentials there.

This interface is only used to validate public keys against algorithm
constraints. In the latest webrev, I moved all of this functionality
into NamedGroupFunctions and removed the SSLKeyAgreementCredentials
interface.

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

Xuelei Fan-2
On 9/5/2018 10:09 AM, Adam Petcher wrote:

> Updated webrev: http://cr.openjdk.java.net/~apetcher/8171279/webrev.01/
>
> On 9/4/2018 3:25 PM, Xuelei Fan wrote:
>
>> I have no finished the full code review.  So far, I have a few
>> question about the struct of the code.
>> 1. XECParameters
>> I can see the reason to dynamic parameters for something other than
>> X25519/X448.  But for JSSE, currently, only named curves are used.  It
>> would be nice to use the name for the static parameters.
>
> Sorry, I don't think I understand what you are suggesting here. As far
> as I know, nothing in sun.security.ssl uses XECParameters directly.
Okay, it is used indirectly.

> It
> is used indirectly through ECUtil to encode and decode keys, and in this
> case the name is used to look up the parameters.
Can the public NamedParameterSpec be used, instead of the private
XECParameters?  I think XECParameters may be better in the EC provider,
please try to mitigate the dependence between modules.  If one day the
sun.security.util.XECParameters get updated, the EC implementation get
impacted a lot.  I prefer to use public APIs if possible.

> 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.

>>
>> 2. "TlsPremasterSecret" only XDH key agreement
>> It would be nice the JCE implementation can support key agreement
>> other than TLS protocols and providers other than SunJSSE provider. It
>> would be nice if the JCE implementation does not bind to the SunJSSE
>> provider private algorithm name.
>>
>> We used to use SunJSSE private name in the JCE crypto implementation.
>> But there are some known problems with this dependence.
>>
>> Is there a problem to support generic key agreement?
>
> 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.

> I would rather
> continue to use "TlsPreMasterSecret" for now, and then add support for a
> standard, TLS-independent name (that means the same thing) in a separate
> ticket. But if you feel strongly that XDH should not support
> "TlsPreMasterSecret", then perhaps we can use a different string now. In
> this case, let me know if you have any suggestions for what string to use.
>
See above.

>>
>> 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?

>>
>> 4. SSLKeyAgreementCredentials
>> I did not see too much benefit of this new interface.  It is not
>> always true that a key agreement could have a public key. Although we
>> can simplify the key agreement for public key based, but it also add
>> additional layers.
>>
>> I know where this improvement comes from.  Maybe, you can consolidate
>> the named group functions, and encode/decode the credentials there.
>
> This interface is only used to validate public keys against algorithm
> constraints. In the latest webrev, I moved all of this functionality
> into NamedGroupFunctions and removed the SSLKeyAgreementCredentials
> interface.
>
Thanks, I like it the new design.

Xuelei
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

Adam Petcher
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 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.

>
>>>
>>> 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).

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

Xuelei Fan-2
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

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

Adam Petcher
On 9/6/2018 12:10 PM, Xuelei Fan wrote:

>> 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?

Yes, the key sizes are fixed. All we need in ECUtil is a mapping from
curve name to this (fixed) size. Are you suggesting some other solution,
other than using the XECParameters to map curve names to key sizes?

>
>>
>> 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.

I agree that this is an issue, but I think it is out of scope for this
ticket. There are several tickets[1,2] related to the issues associated
with generateSecret, we just need to find the time to work on them.
Continuing work on the KeyDerivation JEP[3] may also help with all of this.

[1] https://bugs.openjdk.java.net/browse/JDK-8189441
[2] https://bugs.openjdk.java.net/browse/JDK-6791936
[3] https://bugs.openjdk.java.net/browse/JDK-8189808

>
>>
>> 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.

I don't know if that works, exactly, due to the fact that I can't
reference static enum members in the body of an enum constructor. How
about this alternative? I can move the NamedGroup enum and all the
NamedGroupFunction classes into a separate class (in a separate file)
called NamedGroups. Then all the NamedGroupFunction classes can be
private in this class, but the NamedGroup enum can still have package
access. I would prefer to leave the getFunctions() method of NamedGroup
(and keep it private) because the functions object may be missing and
the Optional return type of getFunctions() forces me to deal with this
when I call it from within NamedGroup.


Reply | Threaded
Open this post in threaded view
|

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

Adam Petcher
Minor correction below.


On 9/6/2018 1:04 PM, Adam Petcher wrote:

>
>>
>>>
>>> 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.
>
> I don't know if that works, exactly, due to the fact that I can't
> reference static enum members in the body of an enum constructor. How
> about this alternative? I can move the NamedGroup enum and all the
> NamedGroupFunction classes into a separate class (in a separate file)
> called NamedGroups. Then all the NamedGroupFunction classes can be
> private in this class, but the NamedGroup enum can still have package
> access. I would prefer to leave the getFunctions() method of
> NamedGroup (and keep it private) because the functions object may be
> missing and the Optional return type of getFunctions() forces me to
> deal with this when I call it from within NamedGroup.
>
>
Actually, it does work. I just have to move the static members of each
NamedGroupFunctions subclass into its subclass (e.g. make them
singletons). Still, I like my proposed alternative better, because it
allows us to simplify SupportedGroupsExtension. Let me know if you have
a preference.
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

Xuelei Fan-2
In reply to this post by Adam Petcher


On 9/6/2018 10:04 AM, Adam Petcher wrote:

> On 9/6/2018 12:10 PM, Xuelei Fan wrote:
>
>>> 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?
>
> Yes, the key sizes are fixed. All we need in ECUtil is a mapping from
> curve name to this (fixed) size. Are you suggesting some other solution,
> other than using the XECParameters to map curve names to key sizes?
Using name only (NamedParameterSpec?) and have the JCE provider handle
it, then you don't need to move XECParameters into java.base module.

Xuelei
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

Xuelei Fan-2
In reply to this post by Adam Petcher

On 9/6/2018 10:17 AM, Adam Petcher wrote:

>>>>
>>>> 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.
>>
>> I don't know if that works, exactly, due to the fact that I can't
>> reference static enum members in the body of an enum constructor. How
>> about this alternative? I can move the NamedGroup enum and all the
>> NamedGroupFunction classes into a separate class (in a separate file)
>> called NamedGroups. Then all the NamedGroupFunction classes can be
>> private in this class, but the NamedGroup enum can still have package
>> access. I would prefer to leave the getFunctions() method of
>> NamedGroup (and keep it private) because the functions object may be
>> missing and the Optional return type of getFunctions() forces me to
>> deal with this when I call it from within NamedGroup.
>>
I think it should be able to use the "functions" field directly.
    Optional ngf = getFunctions()
    if (ngf.isEmpty() {
       ...
    }

    V.S.

    if (functions != null) {
       ...
    }

I did not see the benefits of the getFunctions() method.

>>
> Actually, it does work. I just have to move the static members of each
> NamedGroupFunctions subclass into its subclass (e.g. make them
> singletons). Still, I like my proposed alternative better, because it
> allows us to simplify SupportedGroupsExtension. Let me know if you have
> a preference.
My concerns are mainly about:
1. the NamedGroupFunctions should be private and should not be used
other than the NamedGroup enum impl.
2. the ffdh/ecdh/xdhFunctions static fields should be private of the
NamedGroup enum as well, and better be lazy instantiated as you are
using Map objects in the NamedGroupFunctions implementation (for
performance).
3. NamedGroupFunctions.namedGroupParams is fine in general, but in this
context, it means the map will always be generated.  We used to use a
SupportedGroups to wrap and cache the parameters, and don't care about
the unsupported groups.  But in the new re-org, looks like the
unsupported groups may also have a chance to cache/use the parameters.

#3 is a new find when I'm trying to understand your proposal.  It would
be nice if you could think about the SupportedGroups impact.

For the question about how it works, there are a few approaches. You can
use singletons as you said or inner enum (See CipherSuite.java).

Xuelei
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

Adam Petcher
In reply to this post by Xuelei Fan-2
On 9/6/2018 1:55 PM, Xuelei Fan wrote:

>> Yes, the key sizes are fixed. All we need in ECUtil is a mapping from
>> curve name to this (fixed) size. Are you suggesting some other
>> solution, other than using the XECParameters to map curve names to
>> key sizes?
> Using name only (NamedParameterSpec?) and have the JCE provider handle
> it, then you don't need to move XECParameters into java.base module.
>
Do you have a specific suggestion on how I can do that? I don't think
there is anything in the JCE API for XDH that allows a lookup from name
to key length. Are you proposing that I enhance the public API to avoid
using XECParameters here?

Also, why do you object to having XECParameters in java.base? Most of
the crypto code is in java.base, including similar classes like
ECParameters and CurveDB. I admit that it is unfortunate that
XECParameters is used directly here, instead of going over JCE---is that
what you object to?

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

Xuelei Fan-2
I think I suggested to use NamedParameterSpec, which is a public API.

    NamedParameterSpec -> name "x25519"
        -> key size is the key size of x25519.

Please let me know if the logic is wrong.

 > Also, why do you object to having XECParameters in java.base?
Please read my previous comments.

 > ... similar classes like ECParameters and CurveDB.
Previously, there is no NamedParameterSpec, so we have to workaround to
use ECParameters.  Now, we don't have to do the ugly thing any more if I
did not miss something.

Thanks,
Xuelei

On 9/6/2018 12:13 PM, Adam Petcher wrote:

> On 9/6/2018 1:55 PM, Xuelei Fan wrote:
>
>>> Yes, the key sizes are fixed. All we need in ECUtil is a mapping from
>>> curve name to this (fixed) size. Are you suggesting some other
>>> solution, other than using the XECParameters to map curve names to
>>> key sizes?
>> Using name only (NamedParameterSpec?) and have the JCE provider handle
>> it, then you don't need to move XECParameters into java.base module.
>>
> Do you have a specific suggestion on how I can do that? I don't think
> there is anything in the JCE API for XDH that allows a lookup from name
> to key length. Are you proposing that I enhance the public API to avoid
> using XECParameters here?
>
> Also, why do you object to having XECParameters in java.base? Most of
> the crypto code is in java.base, including similar classes like
> ECParameters and CurveDB. I admit that it is unfortunate that
> XECParameters is used directly here, instead of going over JCE---is that
> what you object to?
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

Adam Petcher
On 9/6/2018 3:19 PM, Xuelei Fan wrote:

> I think I suggested to use NamedParameterSpec, which is a public API.
>
>    NamedParameterSpec -> name "x25519"
>        -> key size is the key size of x25519.
>
> Please let me know if the logic is wrong.

It's that last arrow that I still don't get. How does this code figure
out that "X25519" -> 255 and "X448" -> 448? Perhaps you can reply with
some code to illustrate how you think this should work.



Reply | Threaded
Open this post in threaded view
|

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

Xuelei Fan-2
I asked the question in a previous email.  The key size for x25529 is
fixed, right?

If it is not right, stop here and tell me that it is not right.  Keep
reading if it is right.

OK, as the key size for x25519 is fixed, when you know the algorithm is
x25519, you know the key size.  Does it sound right to you?

If it is not right, stop here and tell me that it is not right.
Otherwise, keep reading.

 From the name you know the key size, when you create a
NamedParameterSpec object for "x25519", you know the name and key size
from the object, right?

Let's look at the x25519 case at first.  If we figure it out, we then
can look into the x488.

Thanks,
Xuelei

On 9/6/2018 1:43 PM, Adam Petcher wrote:

> On 9/6/2018 3:19 PM, Xuelei Fan wrote:
>
>> I think I suggested to use NamedParameterSpec, which is a public API.
>>
>>    NamedParameterSpec -> name "x25519"
>>        -> key size is the key size of x25519.
>>
>> Please let me know if the logic is wrong.
>
> It's that last arrow that I still don't get. How does this code figure
> out that "X25519" -> 255 and "X448" -> 448? Perhaps you can reply with
> some code to illustrate how you think this should work.
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

Adam Petcher
On 9/6/2018 4:49 PM, Xuelei Fan wrote:

> I asked the question in a previous email.  The key size for x25529 is
> fixed, right?

Right.

>
> If it is not right, stop here and tell me that it is not right. Keep
> reading if it is right.
>
> OK, as the key size for x25519 is fixed, when you know the algorithm
> is x25519, you know the key size.  Does it sound right to you?

Possibly right---it depends on what you mean by "know". If all you have
is the name, then you need use a static mapping to look up the key length.

>
> If it is not right, stop here and tell me that it is not right.
> Otherwise, keep reading.
>
> From the name you know the key size, when you create a
> NamedParameterSpec object for "x25519", you know the name and key size
> from the object, right?

The NamedParameterSpec object holds the name only, and not the key size.
We create the NamedParameterSpec from the algorithm name in the
NamedGroup enum, which also doesn't have the key size. Are you
suggesting that I add the key size to this enum as well? Like this:

         // x25519 and x448
         X25519      (0x001D, "x25519", true, "x25519", 255,
                             ProtocolVersion.PROTOCOLS_TO_13),
         X448        (0x001E, "x448", true, "x448", 448,
                             ProtocolVersion.PROTOCOLS_TO_13),

The constructor will take this length and store it. Then we can get this
value out of the NamedGroup in XDHKeyExchange and pass it in to the
methods of ECUtil so we don't need to get it from XECParameters. Is this
what you had in mind?
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

Xuelei Fan-2
 > The NamedParameterSpec object holds the name only, and not the key size.
The name is not a meaningless string, it refer to a specific thing.  For
more examples, please refer to the standard names documentation, every
name has its specific meaning and the background.  If the name is just a
meaningless string, there is nothing we can do for it and we may not
want to define a meaningless API.

The parse of the NamedParameterSpec name is really about the
implementation details.  For example, for KeyFactory:

    public PublicKey engineGeneratePublic(KeySpec keySpec) {
       if (the algorithm is 'x25519') {
           // use the X25519 parameters, including key size
       } else if ('x448') {
           // use the X25519 parameters, including key size
       }
    }

There are a few alternatice ways.  You can define a enum in the XDH
provider, or just use switch, or use Map, or something else you like.
Which one is a better one, it may depends on the implementation details.

Please don't define the x25519 parameters in JSSE.  JSSE should use the
'x25519' name (via NamedParameterSpec object) only.  The underlying JCE
provider should take the responsibility to support the
NamedParameterSpec and defines the internal/private parameters for the
specific name.

Thanks,
Xuelei

On 9/7/2018 5:49 AM, Adam Petcher wrote:

> On 9/6/2018 4:49 PM, Xuelei Fan wrote:
>
>> I asked the question in a previous email.  The key size for x25529 is
>> fixed, right?
>
> Right.
>
>>
>> If it is not right, stop here and tell me that it is not right. Keep
>> reading if it is right.
>>
>> OK, as the key size for x25519 is fixed, when you know the algorithm
>> is x25519, you know the key size.  Does it sound right to you?
>
> Possibly right---it depends on what you mean by "know". If all you have
> is the name, then you need use a static mapping to look up the key length.
>
>>
>> If it is not right, stop here and tell me that it is not right.
>> Otherwise, keep reading.
>>
>> From the name you know the key size, when you create a
>> NamedParameterSpec object for "x25519", you know the name and key size
>> from the object, right?
>
> The NamedParameterSpec object holds the name only, and not the key size.
> We create the NamedParameterSpec from the algorithm name in the
> NamedGroup enum, which also doesn't have the key size. Are you
> suggesting that I add the key size to this enum as well? Like this:
>
>          // x25519 and x448
>          X25519      (0x001D, "x25519", true, "x25519", 255,
>                              ProtocolVersion.PROTOCOLS_TO_13),
>          X448        (0x001E, "x448", true, "x448", 448,
>                              ProtocolVersion.PROTOCOLS_TO_13),
>
> The constructor will take this length and store it. Then we can get this
> value out of the NamedGroup in XDHKeyExchange and pass it in to the
> methods of ECUtil so we don't need to get it from XECParameters. Is this
> what you had in mind?
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

Xuelei Fan-2


On 9/7/2018 6:34 AM, Xuelei Fan wrote:

>  > The NamedParameterSpec object holds the name only, and not the key size.
> The name is not a meaningless string, it refer to a specific thing.  For
> more examples, please refer to the standard names documentation, every
> name has its specific meaning and the background.  If the name is just a
> meaningless string, there is nothing we can do for it and we may not
> want to define a meaningless API.
>
> The parse of the NamedParameterSpec name is really about the
> implementation details.  For example, for KeyFactory:
>
>     public PublicKey engineGeneratePublic(KeySpec keySpec) {
>        if (the algorithm is 'x25519') {
>            // use the X25519 parameters, including key size
>        } else if ('x448') {
>            // use the X25519 parameters, including key size
stupid copy and past: X25519 -> X448

>        }
>     }
>
> There are a few alternatice ways.  You can define a enum in the XDH
> provider, or just use switch, or use Map, or something else you like.
> Which one is a better one, it may depends on the implementation details.
>
> Please don't define the x25519 parameters in JSSE.  JSSE should use the
> 'x25519' name (via NamedParameterSpec object) only.  The underlying JCE
> provider should take the responsibility to support the
> NamedParameterSpec and defines the internal/private parameters for the
> specific name.
>
> Thanks,
> Xuelei
>
> On 9/7/2018 5:49 AM, Adam Petcher wrote:
>> On 9/6/2018 4:49 PM, Xuelei Fan wrote:
>>
>>> I asked the question in a previous email.  The key size for x25529 is
>>> fixed, right?
>>
>> Right.
>>
>>>
>>> If it is not right, stop here and tell me that it is not right. Keep
>>> reading if it is right.
>>>
>>> OK, as the key size for x25519 is fixed, when you know the algorithm
>>> is x25519, you know the key size.  Does it sound right to you?
>>
>> Possibly right---it depends on what you mean by "know". If all you
>> have is the name, then you need use a static mapping to look up the
>> key length.
>>
>>>
>>> If it is not right, stop here and tell me that it is not right.
>>> Otherwise, keep reading.
>>>
>>> From the name you know the key size, when you create a
>>> NamedParameterSpec object for "x25519", you know the name and key
>>> size from the object, right?
>>
>> The NamedParameterSpec object holds the name only, and not the key
>> size. We create the NamedParameterSpec from the algorithm name in the
>> NamedGroup enum, which also doesn't have the key size. Are you
>> suggesting that I add the key size to this enum as well? Like this:
>>
>>          // x25519 and x448
>>          X25519      (0x001D, "x25519", true, "x25519", 255,
>>                              ProtocolVersion.PROTOCOLS_TO_13),
>>          X448        (0x001E, "x448", true, "x448", 448,
>>                              ProtocolVersion.PROTOCOLS_TO_13),
>>
>> The constructor will take this length and store it. Then we can get
>> this value out of the NamedGroup in XDHKeyExchange and pass it in to
>> the methods of ECUtil so we don't need to get it from XECParameters.
>> Is this what you had in mind?
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

Adam Petcher
In reply to this post by Xuelei Fan-2
On 9/7/2018 9:34 AM, Xuelei Fan wrote:

> JSSE should use the 'x25519' name (via NamedParameterSpec object) only.

This is the part that I don't know how to do. In JSSE, we convert
between the array containing the encoded public key and the BigInteger
representation of the public key used in XECPublicKeySpec. To do this,
you need to know the length of the key, in bits. That means that JSSE
needs to know the length of the key, in addition to the name, in order
to do this conversion. I understand that there are lots of ways to get
this information in JSSE, but I don't know which ways you would find
acceptable.

We keep going back and forth, saying the exact same things, and we don't
seem to be making any progress. Can you please provide some code to
illustrate what you want me to do? All I need is an acceptable
implementation of the following method, that is used by JSSE to decode
public keys:

public static XECPublicKeySpec decodeXecPublicKey(byte[] key,
                                         NamedParameterSpec spec)
         throws InvalidParameterSpecException {

     XECParameters params = XECParameters.get(
         InvalidParameterSpecException::new, spec);
     BigInteger u = decodeXecPublicKey(key, params.getBits());
     return new XECPublicKeySpec(spec, u);
}

For reference, here is the implementation of the helper method that does
the actual decoding, using the length.

public static BigInteger decodeXecPublicKey(byte[] key,
                                             int bits) {

     ArrayUtil.reverse(key);
     // clear the extra bits
     int bitsMod8 = bits % 8;
     if (bitsMod8 != 0) {
         int mask = (1 << bitsMod8) - 1;
         key[0] &= mask;
     }
     return new BigInteger(1, key);
}


Reply | Threaded
Open this post in threaded view
|

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

Xuelei Fan-2
Please let me know if you understand the following logic.  Otherwise, I
will see if I could do something for you, maybe a prototype, maybe a
more detailed description.  However, I may need more time for a
prototype/detailed description.

Per RFC 8446/7748, the X25519 key size is 32 bytes.  Here we know the
key size. [1]

Per RFC 8446, the X25519 public key is encoded as byte string inputs and
outputs, as defined in RFC 7748.  Her we know the encoding of the key. [2]

Suppose x25519 is used [3], then we know that the key sized per [1] and
the encoded key per [2].  Next step, let convert the encoded key bytes
to PublicKey.

EncodedKeySpec keySpec = ... // find a way to construct the keySpec
                              // at least, we can use:
                              //    new EncodedKeySpec(byte[]);
                              // But please check if there's a better one


KeyFactory kFac = KeyFactory.getInstance("x25519");
                              // we know the name in [3]


PublicKey pubKey = kFac.generatePublic​(keySpec);

Here you got the public key.


We may also need to generate the key pair.

KeyPairGenerator kpg = KeyPairGenerator.getInstance("x25519");
                              // we know the name in [3]

// may be optional, we know the name in [3].
NamedParameterSpec nps = new NamedParameterSpec("x25519");
kpg.initialize(nps);

KeyPair kp = kpg.generateKeyPair();

How you get the private key.

That's it.  I know you may need to adjust the crypto implementation if
the provider does not support the above scenarios yet.

Xuelei

On 9/7/2018 7:30 AM, Adam Petcher wrote:

> On 9/7/2018 9:34 AM, Xuelei Fan wrote:
>
>> JSSE should use the 'x25519' name (via NamedParameterSpec object) only.
>
> This is the part that I don't know how to do. In JSSE, we convert
> between the array containing the encoded public key and the BigInteger
> representation of the public key used in XECPublicKeySpec. To do this,
> you need to know the length of the key, in bits. That means that JSSE
> needs to know the length of the key, in addition to the name, in order
> to do this conversion. I understand that there are lots of ways to get
> this information in JSSE, but I don't know which ways you would find
> acceptable.
>
> We keep going back and forth, saying the exact same things, and we don't
> seem to be making any progress. Can you please provide some code to
> illustrate what you want me to do? All I need is an acceptable
> implementation of the following method, that is used by JSSE to decode
> public keys:
>
> public static XECPublicKeySpec decodeXecPublicKey(byte[] key,
>                                          NamedParameterSpec spec)
>          throws InvalidParameterSpecException {
>
>      XECParameters params = XECParameters.get(
>          InvalidParameterSpecException::new, spec);
>      BigInteger u = decodeXecPublicKey(key, params.getBits());
>      return new XECPublicKeySpec(spec, u);
> }
>
> For reference, here is the implementation of the helper method that does
> the actual decoding, using the length.
>
> public static BigInteger decodeXecPublicKey(byte[] key,
>                                              int bits) {
>
>      ArrayUtil.reverse(key);
>      // clear the extra bits
>      int bitsMod8 = bits % 8;
>      if (bitsMod8 != 0) {
>          int mask = (1 << bitsMod8) - 1;
>          key[0] &= mask;
>      }
>      return new BigInteger(1, key);
> }
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

Adam Petcher
This is more clear, thanks. See below.


On 9/7/2018 11:34 AM, Xuelei Fan wrote:
> EncodedKeySpec keySpec = ... // find a way to construct the keySpec
>                              // at least, we can use:
>                              //    new EncodedKeySpec(byte[]);
>                              // But please check if there's a better one

Do you mean X509EncodedKeySpec, or some class that is specific to XDH?
An X509EncodedKeySpec would probably work---we would just need to put
the key into a SubjectPublicKeyInfo, which means I need the OID. Is it
okay for me to put the OID in JSSE? I could put it in the NamedGroup
enum, like this:

     X25519      (0x001D, "x25519", true, "x25519", "1.3.101.110",
         ProtocolVersion.PROTOCOLS_TO_13),
     X448        (0x001E, "x448", true, "x448", "1.3.101.111",
         ProtocolVersion.PROTOCOLS_TO_13),

I'm not sure if the second x25519/x448 strings (for algorithm and
NamedParameterSpec names) would still be needed, since I can use the OID
in some of these places. If it's not needed, then perhaps I can remove
it and only use the OID to interact with the JCA provider.

We don't have a type for XDH encoded public keys. It would be nice to be
able to do something simple like:

byte[] keyBytes = ...
NamedParameterSpec paramSpec = new NamedParameterSpec("X25519");
XECEncodedPublicKeySpec keySpec = new XECEncodedKeySpec(paramSpec,
keyBytes);

and then give keySpec to the "XDH" key factory. But this
XECEncodedKeySpec type does not exist, so this would require an
enhancement to the API.
12