> On Dec 24, 2019, at 12:11 AM, Xuelei Fan <xuelei....@oracle.com> wrote:
>
> Will the update for getEncodedParams() has other compatibility impact? As it
> is a public method, I'm not sure if the update is necessary and will impact
> ECDSA encoding in other places. Please double check it.
With the current code, ECDSA will always fail when trying to parse a parameters
byte array containing a hash algorithm. Therefore this fix will not bring more
error.
The only parameter for a ECDSA algorithm we can handle now is ECParameters
(named curve). For ecdsa-with-SHA2, the parameter already contains the hash
algorithm and I have no idea where to put the curve name. My fix just throws
away the whole parameter. This should not be a problem since an EC key already
knows its curve, and the best ECDSA can do is to reject the signature if the
curve there does not match the key's curve.
The final copy of https://tools.ietf.org/html/rfc5758 didn't mention
ecdsa-with-SHA2 at all. It was removed in an earlier draft [1] and hopefully no
one should be using it.
Thanks,
Max
[1]
https://tools.ietf.org/rfcdiff?difftype=--hwdiff&url2=draft-ietf-pkix-sha2-dsa-ecdsa-05.txt
>
> Otherwise, looks good to me.
>
> Xuelei
>
> On 12/21/2019 5:33 AM, Weijun Wang wrote:
>> I uploaded a less disruptive webrev at
>> https://cr.openjdk.java.net/~weijun/8236470/webrev.01.
>> --Max
>>> On Dec 21, 2019, at 7:47 PM, Weijun Wang <weijun.w...@oracle.com> wrote:
>>>
>>> Oh, I take back the "mutates the internal" words, it didn't. algName is
>>> only a local variable.
>>>
>>> So the major problem of the current code is that while getName() is
>>> modified to return the "full" signature algorithm name, getEncodedParams()
>>> still returns it.
>>>
>>> I realized my fix does have the problem that
>>> AlgorithmId.parse(input).encode() will be different from input, but making
>>> change at the root is still better than recognizing specifiedWithECDSA_oid
>>> and ignoring parameters everywhere.
>>>
>>> Thanks,
>>> Max
>>>
>>>
>>>> On Dec 21, 2019, at 7:23 PM, Weijun Wang <weijun.w...@oracle.com> wrote:
>>>>
>>>> Please take a review at
>>>>
>>>> http://cr.openjdk.java.net/~weijun/8236470/webrev.00/
>>>>
>>>> The current implementation is not good at dealing with ECDSA specified by
>>>> ecdsa-with-SHA2 plus a hash algorithm. While the AlgorithmId::getName is
>>>> able to return the "full" signature algorithm name, it mutates the
>>>> internal, cannot be guaranteed to be called, and leaves the parameters
>>>> unchanged. This fix move the logic to AlgorithmId::parse and the class
>>>> becomes practically immutable.
>>>>
>>>> For Oracle internal reviewers: An update is made to the test also. The
>>>> closed path has been wrong since the repo consolidation. We do have such
>>>> certs in the closed area.
>>>>
>>>> Thanks,
>>>> Max
>>>>
>>>
