Re: RFR 8244565: Accept PKCS #8 with version number 1
> On May 19, 2020, at 1:41 AM, Valerie Peng wrote:
>
> Hi Max,
>
> I saw in the bug description that handling and parsing of the public key will
> be resolved in a separate enhancement which is fine with me.
>
> In addition to relaxing the PKCS8 version check to accept 1, the current
> webrev does not check for the trailing bytes and its validity. Perhaps we
> should consider adding necessary checks to ensure spec conformance? Perhaps
> some utility method like: (This includes basic checks plus checks for
> multiple attributes and version 1 w/ public key. )
>
> private static void checkTrailingBytes(DerInputStream derIn, int version)
> throws IOException {
> boolean hasAttributes = false;
> boolean hasPublicKey = false;
> while (derIn.available () != 0) {
> // check for OPTIONAL attributes and/or publicKey
> DerValue tmp = derIn.getDerValue();
> if (tmp.isContextSpecific((byte)0) && !hasAttributes) {
> // OPTIONAL attributes not supported yet
> // discard for now and move on
> hasAttributes = true;
> } else if (version == V2 && tmp.isContextSpecific((byte)1) &&
> !hasPublicKey) {
> // OPTIONAL v2 public key not supported yet
> // discard for now and move on
> hasPublicKey = true;
> } else {
> throw new IOException ("illegal encoding in private key");
> }
> }
> }
I wonder if this is necessary. The extra bytes are quite harmless, and we
didn't check it in decode().
>
> Besides the encoding parsing check above, maybe V1, V2 can be made private
> static?
OK.
> I noticed that the PKCS8Key class has a block of code under the comments "Try
> again using JDK1.1-style...", however the provider property
> "PrivateKey.PKCS#8." seems long gone? Without these property, this block
> of code seems useless and probably should be cleaned up as well.
Yes.
>
> As for the test, the existing comments for the EXPECTED bytes are off and
> plain misleading. Could you please fix them or at least remove them. For
> example, the comment of "integer 3" should be associated with "0x02, 0x01,
> 0x03," bytes instead of "0x01, 0x02, 0x02,". In the updated test,
> NEW_ENCODED_KEY_INTS is PKCS8 v1 key and NEW_ENCODED_KEY_INTS_2 is PKCS8 v2
> key w/ public key. Since the version value is always at index 4, we can
> either clone the existing bytes or directly override the version value in
> NEW_ENCODED_KEY_INTS or NEW_ENCODED_KEY_INTS_2 to add additional negative
> tests, e.g. encoding w/ the version value 2 (anything besides the allowed 0
> and 1), version value 0 w/ trailing public key. It'd be nice to test version
> 1 w/ trailing bytes w/ invalid tag value as well.
If you still want checkTrailingBytes, then yes.
Thanks,
Max
>
> Thanks,
>
> Valerie
>
>
> On 5/13/2020 5:16 PM, Valerie Peng wrote:
>> I will take a look.
>>
>> Valerie
>>
>> On 5/8/2020 3:39 AM, Weijun Wang wrote:
>>> Found an existing test I can update. Webrev updated at
>>>
>>> http://cr.openjdk.java.net/~weijun/8244565/webrev.01/
>>>
>>> Thanks,
>>> Max
>>>
On May 8, 2020, at 5:41 PM, Weijun Wang wrote:
Please take a review at
http://cr.openjdk.java.net/~weijun/8244565/webrev.00/
Now we accepts a PKCS8 file with version being 0 or 1. The publicKey and
attributes are still not parsed.
I also take this chance to make some format change.
There is no regression test and I'll add one. I can generate a PKCS8 key
and then modify the version from 0 to 1 and try to read it. Or I can find
a PKCS8 generated by some other tool and embed it the test to read it.
Please advise which is better.
Thanks,
Max
Re: RFR[15] 8245151: jarsigner should not raise duplicate warnings on verification
Looks fine to me. Thanks, Max > On May 19, 2020, at 4:05 AM, Hai-May Chao wrote: > > Hi, > > I’d like to request a review for - > > JBS: https://bugs.openjdk.java.net/browse/JDK-8245151 > Webrev: https://cr.openjdk.java.net/~hchao/8245151/webrev.00/ > > The change is to provide a distinct warning for jarsigner -verify command > when it detects weak timestamp digest algorithms are used (by -tsadigestalg > when signing). > > Thanks, > Hai-May >
Re: [15] RFR JDK-8242151 Improve OID mapping and reuse among JDK security providers for aliases registration
Still looks fine to me. Thanks, Max > On May 19, 2020, at 5:36 AM, Valerie Peng wrote: > > Updated again due to the merge with Tony's EdDSA change: > > http://cr.openjdk.java.net/~valeriep/8242151/webrev.06 > > Added Ed25519 and Ed448 to KnownOIDs, and rest are just adjustments > accordingly. > > Touched: > > src/jdk.crypto.ec/share/classes/sun/security/ec/ed/EdDSAParameters.java > src/jdk.crypto.ec/share/classes/sun/security/ec/SunEC.java > src/java.base/share/classes/sun/security/x509/AlgorithmId.java > src/java.base/share/classes/sun/security/util/KnownOIDs.java > > Pre-integration Mach5 job is running. > > Thanks, > > Valerie > > On 5/18/2020 11:44 AM, Valerie Peng wrote: >> Great, thanks much for the thorough review~ >> >> Valerie >> >> On 5/15/2020 8:57 PM, Weijun Wang wrote: >>> Well done. Everything looks fine to me. >>> >>> --Max >>> On May 16, 2020, at 5:47 AM, Valerie Peng wrote: Hi Max, I have updated the webrev (http://cr.openjdk.java.net/~valeriep/8242151/webrev.05/) to address your suggestion below. Touched classes are NamedCurve, CurveDB, ConstraintsParameters, and SunEC. The result of using the single method looks pretty good - clean and shorter code. :) > CurveDB.getNamesByOID is only used in > ConstraintsParameters.getNamedCurveFromKey(), but we already have a > NamedCurve there and you can directly use it without converting to > nc.getObjectId(). > > In fact, it looks like nc.getAliases() and nc.getName() are always used > together. Can we just remove these 2 and add a new method > nc.getNameAndAliases()? Then there will be no compatibility impact for > getName() at all! > Thanks, Valerie
Re: [15] RFR JDK-8242151 Improve OID mapping and reuse among JDK security providers for aliases registration
Updated again due to the merge with Tony's EdDSA change: http://cr.openjdk.java.net/~valeriep/8242151/webrev.06 Added Ed25519 and Ed448 to KnownOIDs, and rest are just adjustments accordingly. Touched: src/jdk.crypto.ec/share/classes/sun/security/ec/ed/EdDSAParameters.java src/jdk.crypto.ec/share/classes/sun/security/ec/SunEC.java src/java.base/share/classes/sun/security/x509/AlgorithmId.java src/java.base/share/classes/sun/security/util/KnownOIDs.java Pre-integration Mach5 job is running. Thanks, Valerie On 5/18/2020 11:44 AM, Valerie Peng wrote: Great, thanks much for the thorough review~ Valerie On 5/15/2020 8:57 PM, Weijun Wang wrote: Well done. Everything looks fine to me. --Max On May 16, 2020, at 5:47 AM, Valerie Peng wrote: Hi Max, I have updated the webrev (http://cr.openjdk.java.net/~valeriep/8242151/webrev.05/) to address your suggestion below. Touched classes are NamedCurve, CurveDB, ConstraintsParameters, and SunEC. The result of using the single method looks pretty good - clean and shorter code. :) CurveDB.getNamesByOID is only used in ConstraintsParameters.getNamedCurveFromKey(), but we already have a NamedCurve there and you can directly use it without converting to nc.getObjectId(). In fact, it looks like nc.getAliases() and nc.getName() are always used together. Can we just remove these 2 and add a new method nc.getNameAndAliases()? Then there will be no compatibility impact for getName() at all! Thanks, Valerie
RFR[15] 8245151: jarsigner should not raise duplicate warnings on verification
Hi, I’d like to request a review for - JBS: https://bugs.openjdk.java.net/browse/JDK-8245151 Webrev: https://cr.openjdk.java.net/~hchao/8245151/webrev.00/ The change is to provide a distinct warning for jarsigner -verify command when it detects weak timestamp digest algorithms are used (by -tsadigestalg when signing). Thanks, Hai-May
Re: [15] RFR JDK-8242151 Improve OID mapping and reuse among JDK security providers for aliases registration
Great, thanks much for the thorough review~ Valerie On 5/15/2020 8:57 PM, Weijun Wang wrote: Well done. Everything looks fine to me. --Max On May 16, 2020, at 5:47 AM, Valerie Peng wrote: Hi Max, I have updated the webrev (http://cr.openjdk.java.net/~valeriep/8242151/webrev.05/) to address your suggestion below. Touched classes are NamedCurve, CurveDB, ConstraintsParameters, and SunEC. The result of using the single method looks pretty good - clean and shorter code. :) CurveDB.getNamesByOID is only used in ConstraintsParameters.getNamedCurveFromKey(), but we already have a NamedCurve there and you can directly use it without converting to nc.getObjectId(). In fact, it looks like nc.getAliases() and nc.getName() are always used together. Can we just remove these 2 and add a new method nc.getNameAndAliases()? Then there will be no compatibility impact for getName() at all! Thanks, Valerie
Re: RFR 8244565: Accept PKCS #8 with version number 1
Hi Max,
I saw in the bug description that handling and parsing of the public key
will be resolved in a separate enhancement which is fine with me.
In addition to relaxing the PKCS8 version check to accept 1, the current
webrev does not check for the trailing bytes and its validity. Perhaps
we should consider adding necessary checks to ensure spec conformance?
Perhaps some utility method like: (This includes basic checks plus
checks for multiple attributes and version 1 w/ public key. )
private static void checkTrailingBytes(DerInputStream derIn, int
version)
throws IOException {
boolean hasAttributes = false;
boolean hasPublicKey = false;
while (derIn.available () != 0) {
// check for OPTIONAL attributes and/or publicKey
DerValue tmp = derIn.getDerValue();
if (tmp.isContextSpecific((byte)0) && !hasAttributes) {
// OPTIONAL attributes not supported yet
// discard for now and move on
hasAttributes = true;
} else if (version == V2 && tmp.isContextSpecific((byte)1) &&
!hasPublicKey) {
// OPTIONAL v2 public key not supported yet
// discard for now and move on
hasPublicKey = true;
} else {
throw new IOException ("illegal encoding in private key");
}
}
}
Besides the encoding parsing check above, maybe V1, V2 can be made
private static? I noticed that the PKCS8Key class has a block of code
under the comments "Try again using JDK1.1-style...", however the
provider property "PrivateKey.PKCS#8." seems long gone? Without
these property, this block of code seems useless and probably should be
cleaned up as well.
As for the test, the existing comments for the EXPECTED bytes are off
and plain misleading. Could you please fix them or at least remove them.
For example, the comment of "integer 3" should be associated with "0x02,
0x01, 0x03," bytes instead of "0x01, 0x02, 0x02,". In the updated test,
NEW_ENCODED_KEY_INTS is PKCS8 v1 key and NEW_ENCODED_KEY_INTS_2 is PKCS8
v2 key w/ public key. Since the version value is always at index 4, we
can either clone the existing bytes or directly override the version
value in NEW_ENCODED_KEY_INTS or NEW_ENCODED_KEY_INTS_2 to add
additional negative tests, e.g. encoding w/ the version value 2
(anything besides the allowed 0 and 1), version value 0 w/ trailing
public key. It'd be nice to test version 1 w/ trailing bytes w/ invalid
tag value as well.
Thanks,
Valerie
On 5/13/2020 5:16 PM, Valerie Peng wrote:
I will take a look.
Valerie
On 5/8/2020 3:39 AM, Weijun Wang wrote:
Found an existing test I can update. Webrev updated at
http://cr.openjdk.java.net/~weijun/8244565/webrev.01/
Thanks,
Max
On May 8, 2020, at 5:41 PM, Weijun Wang wrote:
Please take a review at
http://cr.openjdk.java.net/~weijun/8244565/webrev.00/
Now we accepts a PKCS8 file with version being 0 or 1. The publicKey
and attributes are still not parsed.
I also take this chance to make some format change.
There is no regression test and I'll add one. I can generate a PKCS8
key and then modify the version from 0 to 1 and try to read it. Or I
can find a PKCS8 generated by some other tool and embed it the test
to read it. Please advise which is better.
Thanks,
Max
