On 9/10/2018 8:14 AM, Adam Petcher wrote:
Xuelei,
Here is the latest webrev:
http://cr.openjdk.java.net/~apetcher/8171279/webrev.04/
I modified the TLS implementation so that it uses X509EncodedKeySpec
when interacting with the provider. I did a small amount of refactoring
in X509Key to expose the functionality I needed to do this.
I don't think it is not straightforward choice to us X509EncodedKeySpec.
We have to use the right OID, and make sure the key bytes for X.509
protocols are exactly the same as TLS. Not mention to the cost to add
the OID and remove the OID.
Maybe, using EncodedKeySpec is more simple.
package sun.security.ssl;
private class XDHEncodeKeySpec extension EncodedKeySpec {
// algorithm: x25519 or x448
XDHEncodeKeySpec(byte[] encodedKey, String algorithm) {
super(encodedKey, algorithm);
}
@Override
String getFormat() {
return "raw";
}
...
}
package sun.security.ec;
public class XDHKeyFactory extends KeyFactorySpi {
private PublicKey generatePublicImpl(KeySpec keySpec)
throws InvalidKeyException, InvalidKeySpecException {
...
} else if (keySpec instanceof EncodedKeySpec &&
encodedKeySpec.getAlgorithm() is "x25519" &&
encodedKeySpec.getFormat is "raw") {
// raw public key
byte[] radPublicKey = encodedKeySpec.getEncoded();
} else {
}
}
}
This change
removed the dependency on XECParameters, and I moved it back into
jdk.crypto.ec. I also moved some more functions into NamedGroup (e.g.
isAvailableGroup).
This webrev also has the change to NamedGroup that makes
NamedGroupFunctions private. I put the NamedGroup enum into its own
file, which I think is reasonable because it is used in several places
other than SupportedGroupsExtension. It also makes sense to separate all
of the known named groups and their properties (NamedGroups.java) from
the supported_groups extension and the logic related to which groups are
supported for key exchange (SupportedGroupsExtension.java). This change
required changes to the import statements of several files.
They do not sound like good reasons to me. You may not want to do that
again if you understand the following questions you have.
I'm not totally sure I understood your concern related to the
AlgorithmParameters map. The map is always created, but we only create
and cache parameters when they are first needed.
In the current implementation, there is only one map and the parameters
are not create unless they are supported.
In your implementation, there are three maps, which are created always,
even if FFDHE/XDH is not supported. This is a minor issue to me.
If I did not miss something, in your implementation, the parameters can
be created and push to the map even it is not supported? Am I right?
We used to try avoid to generate parameters for unsupported groups.
Of course, the map is
not a proper cache, and the objects stay in memory forever once they are
created. Is this your concern, or is it something else?
I've made several structural changes since the first webrev, and I
haven't been paying too close attention to style, so you probably
shouldn't either. Once the approach and structure look good to you, I
can clean up the code and submit another webrev so you can check it for
style, formatting, etc. I also haven't merged in changes from the
default branch in a while, so I'll need to do that, too.
Yes, let's work out the big picture at first.
Help it helps.
Xuelei
On 9/7/2018 12:12 PM, Xuelei Fan wrote:
On 9/7/2018 9:03 AM, Adam Petcher wrote:
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?
I did not search the spec definitions. At least we can use the
EncodedKeySpec class. It's nice if you can find a better one, or
define a new one for XDH.
Your following comments make sense to me.
Thanks,
Xuelei
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.
