Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation

[Xuelei Fan]

I don't think the underlying provider is ready to support named curves. 
Additional RFEs may be required to standardize the names and improve the 
underlying provider.


Xuelei

On 11/7/2018 7:05 PM, Weijun Wang wrote:

In CurveDB.java, we have

add("secp256r1 [NIST P-256, X9.62 prime256v1]", "1.2.840.10045.3.1.7", PD,
 "0001",
 "0001FFFC",
 "5AC635D8AA3A93E7B3EBBD55769886BC651D06B0CC53B0F63BCE3C3E27D2604B",
 "6B17D1F2E12C4247F8BCE6E563A440F277037D812DEB33A0F4A13945D898C296",
 "4FE342E2FE1A7F9B8EE7EB4A7C0F9E162BCE33576B315ECECBB6406837BF51F5",
 "BCE6FAADA7179E84F3B9CAC2FC632551",
 1, nameSplitPattern);

So the aliases of secp256r1 are now "NIST P-256" and "X9.62 prime256v1". Do we 
really want to keep the organization name prefix after JDK-8208156? The alias can be used in 
ECGenParameterSpec and the proposed keytool -groupname option.

The following shows this behavior.


jshell> KeyPairGenerator.getInstance("EC")
$3 ==> java.security.KeyPairGenerator$Delegate@64bfbc86

jshell> $3.initialize(new ECGenParameterSpec("secp256r1"))

jshell> $3.initialize(new ECGenParameterSpec("prime256v1"))
|  Exception java.security.InvalidAlgorithmParameterException: Unknown curve 
name: prime256v1
|at ECKeyPairGenerator.initialize (ECKeyPairGenerator.java:103)
|at KeyPairGenerator$Delegate.initialize (KeyPairGenerator.java:699)
|at KeyPairGenerator.initialize (KeyPairGenerator.java:436)
|at (#6:1)

jshell> $3.initialize(new ECGenParameterSpec("X9.62 prime256v1"))


Thanks
Max


On Nov 7, 2018, at 11:48 PM, Weijun Wang  wrote:

CSR updated. With such a generalized option, I won't recommend -groupname over 
-keysize now, although I still intend to print some warning for EC.

Please take a review.

Thanks
Max

Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation

[Weijun Wang]

In CurveDB.java, we have

add("secp256r1 [NIST P-256, X9.62 prime256v1]", "1.2.840.10045.3.1.7", PD,
"0001",
"0001FFFC",
"5AC635D8AA3A93E7B3EBBD55769886BC651D06B0CC53B0F63BCE3C3E27D2604B",
"6B17D1F2E12C4247F8BCE6E563A440F277037D812DEB33A0F4A13945D898C296",
"4FE342E2FE1A7F9B8EE7EB4A7C0F9E162BCE33576B315ECECBB6406837BF51F5",
"BCE6FAADA7179E84F3B9CAC2FC632551",
1, nameSplitPattern);

So the aliases of secp256r1 are now "NIST P-256" and "X9.62 prime256v1". Do we 
really want to keep the organization name prefix after JDK-8208156? The alias 
can be used in ECGenParameterSpec and the proposed keytool -groupname option.

The following shows this behavior.

> jshell> KeyPairGenerator.getInstance("EC")
> $3 ==> java.security.KeyPairGenerator$Delegate@64bfbc86
> 
> jshell> $3.initialize(new ECGenParameterSpec("secp256r1"))
> 
> jshell> $3.initialize(new ECGenParameterSpec("prime256v1"))
> |  Exception java.security.InvalidAlgorithmParameterException: Unknown curve 
> name: prime256v1
> |at ECKeyPairGenerator.initialize (ECKeyPairGenerator.java:103)
> |at KeyPairGenerator$Delegate.initialize (KeyPairGenerator.java:699)
> |at KeyPairGenerator.initialize (KeyPairGenerator.java:436)
> |at (#6:1)
> 
> jshell> $3.initialize(new ECGenParameterSpec("X9.62 prime256v1"))

Thanks
Max

> On Nov 7, 2018, at 11:48 PM, Weijun Wang  wrote:
> 
> CSR updated. With such a generalized option, I won't recommend -groupname 
> over -keysize now, although I still intend to print some warning for EC.
> 
> Please take a review.
> 
> Thanks
> Max
> 

Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation

[Weijun Wang]

Oh, I didn't know that.

To make sure -keyalg matches KeyPairGenerator.getInstance(), I'd like to 
support it. If I read the impl correctly, you don't need to initialize it 
anymore and if you really want to initialize it the params must be the same. 
Currently keytool always calls initialize(). In this case, there will be no 
default -keysize, and initializa() will be not be called if user has not 
specified one. If user provides -groupname or -keysize just use it and keytool 
fails if API call fails.

Thanks
Max

> On Nov 8, 2018, at 8:01 AM, Xuelei Fan  wrote:
> 
> On 11/7/2018 3:38 PM, Weijun Wang wrote:
>> This sounds a little misleading to me. Alg name and alg params are 2 
>> different things. This is like asking user to call 
>> KeyPairGenerator.getInstance("secp256r1").
> Well, KeyPairGenerator.getInstance("x25519") is a case that JDK 11 has 
> supported now.
> 
> Otherwise, there is a need to check the conflict of alg name and group name.
> 
> Xuelei
> 
>> --Max
>>> On Nov 8, 2018, at 1:47 AM, Xuelei Fan  wrote:
>>> 
>>> Maybe, the -groupname/-curvename option can be replaced by extending the 
>>> existing -keyalg option:
>>>  -keyalg secp256r1
>>> 
>>> Then there is no conflict between the curve/group name and the key alg.
>>> 
>>> Xuelei
>>> 
>>> On 11/7/2018 7:48 AM, Weijun Wang wrote:
 CSR updated. With such a generalized option, I won't recommend -groupname 
 over -keysize now, although I still intend to print some warning for EC.
 Please take a review.
 Thanks
 Max
> On Nov 7, 2018, at 10:36 PM, Adam Petcher  wrote:
> 
> One issue that just came to me: How will this work for EdDSA? I think the 
> CSR could be generalized a bit:
> 
> 1) Make the first item in the "Solution" more general. Instead of 
> limiting it to "EC" allow any valid algorithm/curve combination.
> 2) (Optional) Use -groupname instead of -curvename and change "curve" to 
> "group" everywhere in the CSR. Then this mechanism can also be used for 
> DSA (with named groups) and other algorithms that use groups that aren't 
> curves.
> Also, see below for a comment about curve ambiguity.
> On 11/6/2018 7:59 PM, Weijun Wang wrote:
>>> Otherwise, there are may be more curve categories. As it is not the 
>>> recommended option, I may just remove this and the following one 
>>> sentence.
>>> 
>> I'll just leave it there as a FYI since it's not part of the spec.
> 
> I agree with Xuelei that this part should be removed. Unless you are 
> planning on implementing this curve selection logic in keytool, then we 
> can't control which curve is selected, and it wholly depends on the 
> behavior of the providers. We can't even guarantee that there is any 
> relationship between "key size" and the field size of the curve. Also, we 
> shouldn't use the word "random" here unless we plan to actually randomize 
> the selection of the curve at runtime (similar to random iteration order 
> for maps/sets). I suggest something more general and vague like:
> 
> If only -keysize is specified, an arbitrary curve of the specified size 
> is used
> 

Re: A new proposal to add methods to HttpsURLConnection to access SSLSession

[Xuelei Fan]

On 11/7/2018 1:30 PM, Sean Mullan wrote:

   https://bugs.openjdk.java.net/browse/JDK-8213161
   http://cr.openjdk.java.net/~xuelei/8212261/webrev.03/


I didn't see a test for SecureCacheResponse - is it possible?

JDK does not have the reference implementation of SecureCacheResponse.


You could also add a test case for IllegalStateExc.


Added in the same test file.

lines 108-113 of HttpsSession.java should be indented 4 more spaces. 
Otherwise looks ok to me.



Updated.

New webrev:
http://cr.openjdk.java.net/~xuelei/8212261/webrev.04/

Thanks,
Xuelei

Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation

[Xuelei Fan]

On 11/7/2018 3:38 PM, Weijun Wang wrote:

This sounds a little misleading to me. Alg name and alg params are 2 different things. 
This is like asking user to call KeyPairGenerator.getInstance("secp256r1").
Well, KeyPairGenerator.getInstance("x25519") is a case that JDK 11 has 
supported now.


Otherwise, there is a need to check the conflict of alg name and group name.

Xuelei



--Max


On Nov 8, 2018, at 1:47 AM, Xuelei Fan  wrote:

Maybe, the -groupname/-curvename option can be replaced by extending the 
existing -keyalg option:
  -keyalg secp256r1

Then there is no conflict between the curve/group name and the key alg.

Xuelei

On 11/7/2018 7:48 AM, Weijun Wang wrote:

CSR updated. With such a generalized option, I won't recommend -groupname over 
-keysize now, although I still intend to print some warning for EC.
Please take a review.
Thanks
Max

On Nov 7, 2018, at 10:36 PM, Adam Petcher  wrote:

One issue that just came to me: How will this work for EdDSA? I think the CSR 
could be generalized a bit:

1) Make the first item in the "Solution" more general. Instead of limiting it to 
"EC" allow any valid algorithm/curve combination.
2) (Optional) Use -groupname instead of -curvename and change "curve" to 
"group" everywhere in the CSR. Then this mechanism can also be used for DSA (with named 
groups) and other algorithms that use groups that aren't curves.
Also, see below for a comment about curve ambiguity.
On 11/6/2018 7:59 PM, Weijun Wang wrote:

Otherwise, there are may be more curve categories. As it is not the recommended 
option, I may just remove this and the following one sentence.


I'll just leave it there as a FYI since it's not part of the spec.


I agree with Xuelei that this part should be removed. Unless you are planning on implementing this 
curve selection logic in keytool, then we can't control which curve is selected, and it wholly 
depends on the behavior of the providers. We can't even guarantee that there is any relationship 
between "key size" and the field size of the curve. Also, we shouldn't use the word 
"random" here unless we plan to actually randomize the selection of the curve at runtime 
(similar to random iteration order for maps/sets). I suggest something more general and vague like:

If only -keysize is specified, an arbitrary curve of the specified size is used

Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation

[Weijun Wang]

This sounds a little misleading to me. Alg name and alg params are 2 different 
things. This is like asking user to call 
KeyPairGenerator.getInstance("secp256r1").

--Max

> On Nov 8, 2018, at 1:47 AM, Xuelei Fan  wrote:
> 
> Maybe, the -groupname/-curvename option can be replaced by extending the 
> existing -keyalg option:
>  -keyalg secp256r1
> 
> Then there is no conflict between the curve/group name and the key alg.
> 
> Xuelei
> 
> On 11/7/2018 7:48 AM, Weijun Wang wrote:
>> CSR updated. With such a generalized option, I won't recommend -groupname 
>> over -keysize now, although I still intend to print some warning for EC.
>> Please take a review.
>> Thanks
>> Max
>>> On Nov 7, 2018, at 10:36 PM, Adam Petcher  wrote:
>>> 
>>> One issue that just came to me: How will this work for EdDSA? I think the 
>>> CSR could be generalized a bit:
>>> 
>>> 1) Make the first item in the "Solution" more general. Instead of limiting 
>>> it to "EC" allow any valid algorithm/curve combination.
>>> 2) (Optional) Use -groupname instead of -curvename and change "curve" to 
>>> "group" everywhere in the CSR. Then this mechanism can also be used for DSA 
>>> (with named groups) and other algorithms that use groups that aren't curves.
>>> Also, see below for a comment about curve ambiguity.
>>> On 11/6/2018 7:59 PM, Weijun Wang wrote:
> Otherwise, there are may be more curve categories. As it is not the 
> recommended option, I may just remove this and the following one sentence.
> 
 I'll just leave it there as a FYI since it's not part of the spec.
>>> 
>>> I agree with Xuelei that this part should be removed. Unless you are 
>>> planning on implementing this curve selection logic in keytool, then we 
>>> can't control which curve is selected, and it wholly depends on the 
>>> behavior of the providers. We can't even guarantee that there is any 
>>> relationship between "key size" and the field size of the curve. Also, we 
>>> shouldn't use the word "random" here unless we plan to actually randomize 
>>> the selection of the curve at runtime (similar to random iteration order 
>>> for maps/sets). I suggest something more general and vague like:
>>> 
>>> If only -keysize is specified, an arbitrary curve of the specified size is 
>>> used
>>> 

Re: A new proposal to add methods to HttpsURLConnection to access SSLSession

[Sean Mullan]

On 11/5/18 1:52 PM, Xuelei Fan wrote:

Hi Chris and Sean,

There are a few feedback for the CSR approval.  I updated to use 
Optional for the returned value.  For more details, please 
refer to:

   https://bugs.openjdk.java.net/browse/JDK-8213161
   http://cr.openjdk.java.net/~xuelei/8212261/webrev.03/


I didn't see a test for SecureCacheResponse - is it possible? You could 
also add a test case for IllegalStateExc.


lines 108-113 of HttpsSession.java should be indented 4 more spaces. 
Otherwise looks ok to me.


--Sean




Please let me know if you are OK with this change.

Thanks,
Xuelei

On 11/2/2018 11:42 AM, Xuelei Fan wrote:

Thanks for the review and suggestions, Chris and Sean.

I just finalized the CSR.

Thanks,
Xuelei

On 11/2/2018 5:58 AM, Chris Hegarty wrote:

Thanks for the updates Xuelei, some minor comments inline..

On 1 Nov 2018, at 23:42, Xuelei Fan > wrote:


On 11/1/2018 11:24 AM, Sean Mullan wrote:

On 10/31/18 11:52 AM, Chris Hegarty wrote:

Xuelei,

On 30/10/18 20:55, Xuelei Fan wrote:

Hi,

For the current HttpsURLConnection, there is not much security 
parameters exposed in the public APIs.  An application may need 
richer information for the underlying TLS connections, for 
example the negotiated TLS protocol version.


Please let me know if you have concerns to add a new method 
HttpsURLConnection.getSSLSession() and deprecate the duplicated 
methods, by the end of Nov. 2, 2018.


Here is the proposal:
https://bugs.openjdk.java.net/browse/JDK-8213161
Are there any security issues associated with returning the 
SSLSession, since it is mutable?
It should be fine.  The update APIs of the session (invalidating, 
bind values) does not impact the connection.


Alternatively, as is done in the new HTTP Client, an immutable
SSLSession instance can be returned.

+ *   SHOULD override this method with appropriate 
implementation.

s/appropriate/an appropriate/
I would probably not capitalize "SHOULD" and just say "should". 
"SHOULD" is more common in RFCs. I don't see that much in javadocs.
+ * @implNote The JDK Reference Implementation supports this 
operation.
+ *   As an application may have to use this operation 
for more
+ *   security parameters, it is recommended to support 
this

+ *   operation in all implementations.
I think it should be obvious that the JDK implementation would 
override this method so not sure that first sentence is necessary. 
The other sentence seems like it could be combined with the 
previous sentence, ex:
"Subclasses should override this method with an appropriate 
implementation since an application may need to access additional 
parameters associated with the SSL session."

Updated accordingly, in the CSR and webrev:
https://bugs.openjdk.java.net/browse/JDK-8213161


The CSR looks good. I made a few minor edits to the verbiage
and added myself as reviewer.

The title will need to be updated to reflect the addition of the
new method in SecureCacheResponse. Maybe:

"Add SSLSession accessors to HttpsURLConnection and
SecureCacheResponse"

-Chris.

Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation

[Xuelei Fan]

Maybe, the -groupname/-curvename option can be replaced by extending the 
existing -keyalg option:

  -keyalg secp256r1

Then there is no conflict between the curve/group name and the key alg.

Xuelei

On 11/7/2018 7:48 AM, Weijun Wang wrote:

CSR updated. With such a generalized option, I won't recommend -groupname over 
-keysize now, although I still intend to print some warning for EC.

Please take a review.

Thanks
Max



On Nov 7, 2018, at 10:36 PM, Adam Petcher  wrote:

One issue that just came to me: How will this work for EdDSA? I think the CSR 
could be generalized a bit:

1) Make the first item in the "Solution" more general. Instead of limiting it to 
"EC" allow any valid algorithm/curve combination.
2) (Optional) Use -groupname instead of -curvename and change "curve" to 
"group" everywhere in the CSR. Then this mechanism can also be used for DSA (with named 
groups) and other algorithms that use groups that aren't curves.
Also, see below for a comment about curve ambiguity.
On 11/6/2018 7:59 PM, Weijun Wang wrote:

Otherwise, there are may be more curve categories. As it is not the recommended 
option, I may just remove this and the following one sentence.


I'll just leave it there as a FYI since it's not part of the spec.


I agree with Xuelei that this part should be removed. Unless you are planning on implementing this 
curve selection logic in keytool, then we can't control which curve is selected, and it wholly 
depends on the behavior of the providers. We can't even guarantee that there is any relationship 
between "key size" and the field size of the curve. Also, we shouldn't use the word 
"random" here unless we plan to actually randomize the selection of the curve at runtime 
(similar to random iteration order for maps/sets). I suggest something more general and vague like:

If only -keysize is specified, an arbitrary curve of the specified size is used

NamedCurve.getName()

[Weijun Wang]

This method returns "secp256r1 [NIST P-256, X9.62 prime256v1]" because that's 
how we added it in CurveDB:

add("secp256r1 [NIST P-256, X9.62 prime256v1]", "1.2.840.10045.3.1.7", PD,
"0001",
"0001FFFC",
"5AC635D8AA3A93E7B3EBBD55769886BC651D06B0CC53B0F63BCE3C3E27D2604B",
"6B17D1F2E12C4247F8BCE6E563A440F277037D812DEB33A0F4A13945D898C296",
"4FE342E2FE1A7F9B8EE7EB4A7C0F9E162BCE33576B315ECECBB6406837BF51F5",
"BCE6FAADA7179E84F3B9CAC2FC632551",
1, nameSplitPattern);

Maybe it should be getNames()?

Thanks
Max

Re: RFR 8212003: Obsoleting the default keytool -keyalg option

[Weijun Wang]

Oops, I take this back. The CSR needs more update.

Sorry if you have already start reading it.

Thanks
Max


> On Nov 7, 2018, at 9:27 AM, Weijun Wang  wrote:
> 
> After some discussion, we decided to cover -keysize and -sigalg in this 
> deprecation process too.
> 
> Please review the updated CSR at
> 
>https://bugs.openjdk.java.net/browse/JDK-8212111
> 
> No webrev available yet.
> 
> Thanks
> Max
> 
> 
>> On Oct 18, 2018, at 10:34 AM, Weijun Wang  wrote:
>> 
>> Please review the code change and CSR for 
>> 
>>  JBS: https://bugs.openjdk.java.net/browse/JDK-8212003
>> 
>> at
>> 
>>  webrev: http://cr.openjdk.java.net/~weijun/8212003/webrev.00/
>>  CSR: https://bugs.openjdk.java.net/browse/JDK-8212111
>> 
>> When -keyalg is not provided for -genkeypair or -genseckey, keytool will 
>> print out a warning. We plan to make this an error in a future release.
>> 
>> A new regression test ObsoleteKeyalg.java added. "-keyalg DSA" or "-keyalg 
>> DES" added to other tests.
>> 
>> A Mach5 job on tier1 and tier2 running now.
>> 
>> Thanks
>> Max
>> 
> 

Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation

[Weijun Wang]

CSR updated. With such a generalized option, I won't recommend -groupname over 
-keysize now, although I still intend to print some warning for EC.

Please take a review.

Thanks
Max


> On Nov 7, 2018, at 10:36 PM, Adam Petcher  wrote:
> 
> One issue that just came to me: How will this work for EdDSA? I think the CSR 
> could be generalized a bit:
> 
> 1) Make the first item in the "Solution" more general. Instead of limiting it 
> to "EC" allow any valid algorithm/curve combination.
> 2) (Optional) Use -groupname instead of -curvename and change "curve" to 
> "group" everywhere in the CSR. Then this mechanism can also be used for DSA 
> (with named groups) and other algorithms that use groups that aren't curves.
> Also, see below for a comment about curve ambiguity.
> On 11/6/2018 7:59 PM, Weijun Wang wrote:
>>> Otherwise, there are may be more curve categories. As it is not the 
>>> recommended option, I may just remove this and the following one sentence.
>>> 
>> I'll just leave it there as a FYI since it's not part of the spec.
> 
> I agree with Xuelei that this part should be removed. Unless you are planning 
> on implementing this curve selection logic in keytool, then we can't control 
> which curve is selected, and it wholly depends on the behavior of the 
> providers. We can't even guarantee that there is any relationship between 
> "key size" and the field size of the curve. Also, we shouldn't use the word 
> "random" here unless we plan to actually randomize the selection of the curve 
> at runtime (similar to random iteration order for maps/sets). I suggest 
> something more general and vague like:
> 
> If only -keysize is specified, an arbitrary curve of the specified size is 
> used
> 

Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation

[Weijun Wang]

I don't think there is any current AlgorithmParameterSpec that allow this for a 
KeyPairGenerator. When a curve name is used, keysize is calculated from the 
field size.

--Max

> On Nov 7, 2018, at 4:05 PM, Michael StJohns  wrote:
> 
> Inline below.
> 
> On 11/6/2018 2:18 AM, Weijun Wang wrote:
>> 
>>> On Nov 6, 2018, at 1:06 PM, Xuelei Fan  wrote:
>>> 
>>> On 11/5/2018 8:37 PM, Weijun Wang wrote:
> On Nov 6, 2018, at 12:12 PM, Xuelei Fan  wrote:
> 
> On 11/5/2018 7:13 PM, Weijun Wang wrote:
>> Please take a review at the CSR at
>>   https://bugs.openjdk.java.net/browse/JDK-8213401
>> As for implementation, I intend to report an error when -keyalg is not 
>> EC but -curvename is provided. If both -curvename and -keysize are 
>> provided, I intend to ignore -keysize no matter if they match or not.
> Why not use a strict mode: fail if not match.  It might be misleading if 
> ignoring unmatched options.
 We can do that, but misleading to what? That we treat -curvename and 
 -keysize the same important?
>>> If the option "-keysize 256 -curvename sect163k1" work, I may think that 
>>> the key size if 256 bits. I want to create a 256 bits sect163k1 EC key, and 
>>> the tool allows this behavior, so I should get a 256 bits sect163k1 EC key. 
>>>  Sure, that's incorrect, but I don't know it is incorrect as the tool 
>>> ignore the key size.  What's the problem of the command, I don't know 
>>> either unless I clearly understand sect163k1 is not 256 bits.  The next 
>>> question to me, what's the key size actually is? 256 bits or 163 bits?  
>>> which option are used?  It adds more confusing to me.
>> Well explained. I've updated the CSR and this will be an error.
> 
> Sorry to drop in late.
> 
> Basically, for EC private keys - either binary or prime curves, you will 
> reduce whatever initial random value you generate mod n of the curve to get 
> the final private key.  The generation logic should take care of this.You 
> could use key size as a way of controlling how many extra bits are 
> generated(see FIPS 186-4, section B.4.1) and error only if key size was less 
> than the size of the curve's n.
> 
> So 1) generate a random value of keysize length or if not specified the 
> length of the N of the curve plus 64, 2) reduce mod N.
> 
> Mime.
> 
>> 
>>> We can ignore the -keysize option, but it is complicated to me to use the 
>>> tool.
>>> 
>> Another question: in sun.security.util.CurveDB, we have
>>// Return EC parameters for the specified field size. If there are 
>> known
>>// NIST recommended parameters for the given length, they are 
>> returned.
>>// Otherwise, if there are multiple matches for the given size, an
>>// arbitrary one is returns.
>>// If no parameters are known, the method returns null.
>>// NOTE that this method returns both prime and binary curves.
>>static NamedCurve lookup(int length) {
>>return lengthMap.get(length);
>>}
>> FIPS 186-4 has 2 recommendations (K- and B-) for a binary curve field 
>> size. Do we have a choice?
>> In fact, CurveDB.java seems to have a bug when adding the curves:
>>add("sect163k1 [NIST K-163]", "1.3.132.0.1", BD,...
>>add("sect163r2 [NIST B-163]", "1.3.132.0.15", BD,... // Another 
>> default?
>>add("sect233k1 [NIST K-233]", "1.3.132.0.26", BD,...
>>add("sect233r1 [NIST B-233]", "1.3.132.0.27", B,...
>> and now 163 is sect163r2 and 233 is sect233k1.
>> I assume we should always prefer the K- one?
> TLS 1.3 uses secp256r1/secp384r1/secp521r1, no K- curves.
 There is no ambiguity for prime curves.
> Do you mean if no -curvename option, there is a need to choose a named 
> curve?
 ECKeyPairGenerator::initialize(int) will choose one and keytool will use 
 it. I just meant if we have a bug here and if we should be more public on 
 what curve is chosen.
>>> I see your concerns.
>>> 
>>> It might be a potential issue if we use a named curve if no curvename 
>>> specified.  If the compatibility is not serious, I may suggest supported 
>>> named curves only, or use arbitrary curves but with a warning.
>> If people only want prime curves then -keysize still works. A warning is 
>> enough since in the CSR I've also said "we recommend".
>> 
>> Thanks
>> Max
>> 
>>> Xuelei

Re: Additional debug log message in KeyTab

[Seán Coffey]

On 07/11/18 13:12, Lars Francke wrote:

Seán,

thank you!

I've already sent the OCA and am in the process of getting it filed.
According to the docs[0] the next step would be to provide a patch to 
this mailing list.


I have to admit that I'm a bit overwhelmed still (having never built 
OpenJDK myself) so it'll take a while.


Would this patch also require a test?
you might get away without a test and add the noreg-trivial to the 
eventual bug report. However, it might be no harm to examine the current 
test code to see if the debug flag is exercised. If it is, it might be 
the case of a simple edit to an existing test.


regards,
Sean.


Assuming it's just of the form
if (debug) {
  System.out.println(...);;
}

Cheers,
Lars



On Wed, Nov 7, 2018 at 1:44 PM Seán Coffey > wrote:


Looks like a reasonable minor enhancement to me.

To contribute, you'll need to sign the OCA first. More information
at :

https://openjdk.java.net/contribute/

Regards,
Sean.

On 07/11/18 11:40, Lars Francke wrote:
> Hi,
>
> I have to preface this by saying that this would be my first
> contribution to OpenJDK and I'm still learning the ways. Not
sure for
> example if this is the correct mailing list or if a more generic
JDK
> one would be appropriate.
>
> While working on Hadoop based systems I frequently need to debug
> Kerberos related issues. And while there are
sun.security.krb5.debug
> and sun.security.spnego.debug the error messages could be more
helpful
> at times.
>
> One of these instance
> is sun.security.krb5.internal.ktab.Keytab#readServiceKeys.
>
> It logs that it's looking for keys but (at least for me) it
would be
> very helpful if it also logged how many keys it found after the
for-loop.
>
> If it finds keys it also logs them but if it can't find any then
> there's no message so it's easy to miss.
>
> What do you think?
>
> Cheers,
> Lars

Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation

[Adam Petcher]

One issue that just came to me: How will this work for EdDSA? I think 
the CSR could be generalized a bit:


1) Make the first item in the "Solution" more general. Instead of 
limiting it to "EC" allow any valid algorithm/curve combination.
2) (Optional) Use -groupname instead of -curvename and change "curve" to 
"group" everywhere in the CSR. Then this mechanism can also be used for 
DSA (with named groups) and other algorithms that use groups that aren't 
curves.


Also, see below for a comment about curve ambiguity.

On 11/6/2018 7:59 PM, Weijun Wang wrote:


Otherwise, there are may be more curve categories. As it is not the recommended 
option, I may just remove this and the following one sentence.

I'll just leave it there as a FYI since it's not part of the spec.


I agree with Xuelei that this part should be removed. Unless you are 
planning on implementing this curve selection logic in keytool, then we 
can't control which curve is selected, and it wholly depends on the 
behavior of the providers. We can't even guarantee that there is any 
relationship between "key size" and the field size of the curve. Also, 
we shouldn't use the word "random" here unless we plan to actually 
randomize the selection of the curve at runtime (similar to random 
iteration order for maps/sets). I suggest something more general and 
vague like:


If only |-keysize| is specified, an arbitrary curve of the specified 
size is used

Re: Additional debug log message in KeyTab

[Lars Francke]

Seán,

thank you!

I've already sent the OCA and am in the process of getting it filed.
According to the docs[0] the next step would be to provide a patch to this
mailing list.

I have to admit that I'm a bit overwhelmed still (having never built
OpenJDK myself) so it'll take a while.

Would this patch also require a test?

Assuming it's just of the form
if (debug) {
  System.out.println(...);;
}

Cheers,
Lars



On Wed, Nov 7, 2018 at 1:44 PM Seán Coffey  wrote:

> Looks like a reasonable minor enhancement to me.
>
> To contribute, you'll need to sign the OCA first. More information at :
>
> https://openjdk.java.net/contribute/
>
> Regards,
> Sean.
>
> On 07/11/18 11:40, Lars Francke wrote:
> > Hi,
> >
> > I have to preface this by saying that this would be my first
> > contribution to OpenJDK and I'm still learning the ways. Not sure for
> > example if this is the correct mailing list or if a more generic JDK
> > one would be appropriate.
> >
> > While working on Hadoop based systems I frequently need to debug
> > Kerberos related issues. And while there are sun.security.krb5.debug
> > and sun.security.spnego.debug the error messages could be more helpful
> > at times.
> >
> > One of these instance
> > is sun.security.krb5.internal.ktab.Keytab#readServiceKeys.
> >
> > It logs that it's looking for keys but (at least for me) it would be
> > very helpful if it also logged how many keys it found after the for-loop.
> >
> > If it finds keys it also logs them but if it can't find any then
> > there's no message so it's easy to miss.
> >
> > What do you think?
> >
> > Cheers,
> > Lars
>
>

Re: Additional debug log message in KeyTab

[Seán Coffey]

Looks like a reasonable minor enhancement to me.

To contribute, you'll need to sign the OCA first. More information at :

https://openjdk.java.net/contribute/

Regards,
Sean.

On 07/11/18 11:40, Lars Francke wrote:

Hi,

I have to preface this by saying that this would be my first 
contribution to OpenJDK and I'm still learning the ways. Not sure for 
example if this is the correct mailing list or if a more generic JDK 
one would be appropriate.


While working on Hadoop based systems I frequently need to debug 
Kerberos related issues. And while there are sun.security.krb5.debug 
and sun.security.spnego.debug the error messages could be more helpful 
at times.


One of these instance 
is sun.security.krb5.internal.ktab.Keytab#readServiceKeys.


It logs that it's looking for keys but (at least for me) it would be 
very helpful if it also logged how many keys it found after the for-loop.


If it finds keys it also logs them but if it can't find any then 
there's no message so it's easy to miss.


What do you think?

Cheers,
Lars

Additional debug log message in KeyTab

[Lars Francke]

Hi,

I have to preface this by saying that this would be my first contribution
to OpenJDK and I'm still learning the ways. Not sure for example if this is
the correct mailing list or if a more generic JDK one would be appropriate.

While working on Hadoop based systems I frequently need to debug Kerberos
related issues. And while there are sun.security.krb5.debug and
sun.security.spnego.debug the error messages could be more helpful at times.

One of these instance
is sun.security.krb5.internal.ktab.Keytab#readServiceKeys.

It logs that it's looking for keys but (at least for me) it would be very
helpful if it also logged how many keys it found after the for-loop.

If it finds keys it also logs them but if it can't find any then there's no
message so it's easy to miss.

What do you think?

Cheers,
Lars

RE: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath - enhance some IOExceptions with path causing the issue

[Baesken, Matthias]

> Sorry, I haven't had time to look at this in more detail yet. But, let's
> take a step back first. Can you or Matthias explain in more detail why
> this fix is necessary? What are the use cases and motivation?

Hello, 
adding paths  (or maybe more details)  to exception messages just makes 
analyzing errors easier.
You do not get just "Bad path",  but also the real bad path which gives you a 
hint where to look and analyze further .

That's why we introduced it in our JVM ages ago.
I have to agree that additionally  printing cwd / user.dir is a bit special,  
so I omit that from this revision of the patch.
This makes the patch more simple.
New revision :

http://cr.openjdk.java.net/~mbaesken/webrevs/8211752.1/


Unfortunately the usage of sun.security.util.SecurityProperties  (which was 
considered)  in the  static { ... } 
class initializers (e.g. UnixFileSystem.java) just does not work.
It fails with already in the build (!) with :

Error occurred during initialization of boot layer
java.lang.ExceptionInInitializerError
Caused by: java.lang.NullPointerException

(seems it is too early in the game for SecurityProperties).
(btw. this is another  NOT very helpful exception error message)


So I unfortunately had to go back to using system properties.


Btw. another question regarding path output in exceptions  :
you seem to consider it a bad thing  to (unconditionally) print paths in the 
exception messages,
but then on the other hand we have  it  already in OpenJDK UnixFileSystem_md.c 
: 

 269 JNIEXPORT jboolean JNICALL
 270 Java_java_io_UnixFileSystem_createFileExclusively(JNIEnv *env, jclass cls,
 271   jstring pathname)
 272 {
 ...
 277 /* The root directory always exists */
 278 if (strcmp (path, "/")) {
 279 fd = handleOpen(path, O_RDWR | O_CREAT | O_EXCL, 0666);
 280 if (fd < 0) {
 281 if (errno != EEXIST)
 282 JNU_ThrowIOExceptionWithLastError(env, path);
 283 } else {
 284 if (close(fd) == -1)
 285 JNU_ThrowIOExceptionWithLastError(env, path);


Why is it fine here for a long time , but considered harmful at the other 
places ?
If we want to be consistent, we should then  write  "Bad path" here (or  allow 
the path output at the other places too ).


Thanks, Matthias



> -Original Message-
> From: Sean Mullan 
> Sent: Freitag, 12. Oktober 2018 17:19
> To: Langer, Christoph ; Baesken, Matthias
> ; Alan Bateman ;
> security-dev@openjdk.java.net; core-libs-...@openjdk.java.net
> Subject: Re: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath -
> enhance some IOExceptions with path causing the issue
> 
> On 10/12/18 10:33 AM, Langer, Christoph wrote:
> > Sean, what is your take on this?
> 
> Sorry, I haven't had time to look at this in more detail yet. But, let's
> take a step back first. Can you or Matthias explain in more detail why
> this fix is necessary? What are the use cases and motivation? The bug
> report doesn't go into any detail about that and there isn't anything
> in the initial RFR email that explains why this change is useful or
> necessary. As a general guideline or advice, RFEs should include this
> type of information so that Reviewers understand more of the context and
> motivation behind the change.
> 
> Thanks,
> Sean

Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation

[Michael StJohns]

Inline below.

On 11/6/2018 2:18 AM, Weijun Wang wrote:



On Nov 6, 2018, at 1:06 PM, Xuelei Fan  wrote:

On 11/5/2018 8:37 PM, Weijun Wang wrote:

On Nov 6, 2018, at 12:12 PM, Xuelei Fan  wrote:

On 11/5/2018 7:13 PM, Weijun Wang wrote:

Please take a review at the CSR at
https://bugs.openjdk.java.net/browse/JDK-8213401
As for implementation, I intend to report an error when -keyalg is not EC but 
-curvename is provided. If both -curvename and -keysize are provided, I intend 
to ignore -keysize no matter if they match or not.

Why not use a strict mode: fail if not match.  It might be misleading if 
ignoring unmatched options.

We can do that, but misleading to what? That we treat -curvename and -keysize 
the same important?

If the option "-keysize 256 -curvename sect163k1" work, I may think that the 
key size if 256 bits.  I want to create a 256 bits sect163k1 EC key, and the tool allows 
this behavior, so I should get a 256 bits sect163k1 EC key.  Sure, that's incorrect, but 
I don't know it is incorrect as the tool ignore the key size.  What's the problem of the 
command, I don't know either unless I clearly understand sect163k1 is not 256 bits.  The 
next question to me, what's the key size actually is?  256 bits or 163 bits?  which 
option are used?  It adds more confusing to me.

Well explained. I've updated the CSR and this will be an error.


Sorry to drop in late.

Basically, for EC private keys - either binary or prime curves, you will 
reduce whatever initial random value you generate mod n of the curve to 
get the final private key.  The generation logic should take care of 
this.    You could use key size as a way of controlling how many extra 
bits are generated(see FIPS 186-4, section B.4.1) and error only if key 
size was less than the size of the curve's n.


So 1) generate a random value of keysize length or if not specified the 
length of the N of the curve plus 64, 2) reduce mod N.


Mime.




We can ignore the -keysize option, but it is complicated to me to use the tool.


Another question: in sun.security.util.CurveDB, we have
 // Return EC parameters for the specified field size. If there are known
 // NIST recommended parameters for the given length, they are returned.
 // Otherwise, if there are multiple matches for the given size, an
 // arbitrary one is returns.
 // If no parameters are known, the method returns null.
 // NOTE that this method returns both prime and binary curves.
 static NamedCurve lookup(int length) {
 return lengthMap.get(length);
 }
FIPS 186-4 has 2 recommendations (K- and B-) for a binary curve field size. Do 
we have a choice?
In fact, CurveDB.java seems to have a bug when adding the curves:
 add("sect163k1 [NIST K-163]", "1.3.132.0.1", BD,...
 add("sect163r2 [NIST B-163]", "1.3.132.0.15", BD,... // Another default?
 add("sect233k1 [NIST K-233]", "1.3.132.0.26", BD,...
 add("sect233r1 [NIST B-233]", "1.3.132.0.27", B,...
and now 163 is sect163r2 and 233 is sect233k1.
I assume we should always prefer the K- one?

TLS 1.3 uses secp256r1/secp384r1/secp521r1, no K- curves.

There is no ambiguity for prime curves.

Do you mean if no -curvename option, there is a need to choose a named curve?

ECKeyPairGenerator::initialize(int) will choose one and keytool will use it. I 
just meant if we have a bug here and if we should be more public on what curve 
is chosen.

I see your concerns.

It might be a potential issue if we use a named curve if no curvename 
specified.  If the compatibility is not serious, I may suggest supported named 
curves only, or use arbitrary curves but with a warning.

If people only want prime curves then -keysize still works. A warning is enough since in 
the CSR I've also said "we recommend".

Thanks
Max


Xuelei