Re: RFR 8244565: Accept PKCS #8 with version number 1
Sure, I have no more comments.
Thanks,
Valerie
On 6/3/2020 7:48 PM, Weijun Wang wrote:
RSAPrivateKeyImpl and RSAPrivateCrtKeyImpl
- throws InvalidKeyException when RSAUtil.createAlgorithmId(type, keyParams)
fails. I'll keep it.
EdDSAPrivateKeyImpl, XDHPrivateKeyImpl and ECPrivateKeyImpl
- check the input ECParameterSpec params and might throw an
InvalidKeyException. I'll keep it.
Some of these constructors now throws ProviderException or InvalidKeyException
when DER encoding goes wrong. This should never happen, and I'll throw
AssertionError for it.
In short, the only possible exception is now InvalidKeyException. I also throw
AssertionError but it's not possible and definitely means a programming error
somewhere (Ex: DerValue::putInteger goes wrong).
So here is the updated webrev
http://cr.openjdk.java.net/~weijun/8244565/webrev.04/
Thanks,
Max
On Jun 4, 2020, at 7:37 AM, Valerie Peng wrote:
Hi, Max,
Overall looks very good. Just one more thing:
Different key classes seems to differ in their handling of IOException in their
constructor which produces the DER bytes.
DSAPrivateKey changed to use AssertionError, XDHPrivateKeyImpl and
EdDSAPrivateKeyImpl throws ProviderException. It seems that all other
PrivateKey classes use InvalidKeyException. Perhaps it'd be nice to use
InvalidKeyException for consistency sake?
Thanks,
Valerie
On 6/1/2020 12:35 AM, Weijun Wang wrote:
Updated webrev at
https://cr.openjdk.java.net/~weijun/8244565/webrev.03.
I've inlined more methods that is only called once and inside the same method.
Thanks,
Max
On May 28, 2020, at 9:16 AM, Weijun Wang wrote:
On May 28, 2020, at 8:46 AM, Valerie Peng wrote:
Hi Max,
I like this new structure better. Much easier to understand. Most of the
changes are technical debt that's accumulated inside PKCS8Key class.
A few notable differences which I am not so sure about are
- the encodedKey is returned by getEncoded() directly w/o cloning, and
Too bad. I'll fix.
- the protected parseKeyBits() method being made private. I wonder if the
parseKeyBits() method should be made abstract so it's more obvious that the
subclasses needs to parse the key bytes into algorithm-specific components.
Or how about I just inline parseKeyBits in those child classes into their
constructors? I don't think the child class will forget it. Otherwise, why
write a child class?
Thanks,
Max
Thanks,
Valerie
On 5/26/2020 5:54 PM, Weijun Wang wrote:
Can you please take a look (not a review) at an updated webrev at
http://cr.openjdk.java.net/~weijun/8244565/webrev.02/? It contains the code to inspect
the extra fields. I haven't deal with the "..." here yet.
However, when I tried to consolidate the DER parsing into one place, I've made
more and more changes everywhere. The major change now is a base constructor
PKCS8Key(byte[]) and subclass constructors that call it and no more protected
parseKeyBits(). Although I don't think there is any technical error here but at
the end of the day I'm asking myself if this is too much code change for such a
simple bug.
Do you like the overall structure? If yes, I'll continue this way. Otherwise, I
can restrict the change only inside PKCS8Key.
Thanks,
Max
On May 27, 2020, at 7:14 AM, Valerie Peng wrote:
I suppose we can allow trailing data to conform to "..." then.
But the "[[2: publicKey [1] PublicKey OPTIONAL ]]," line mean that the
publicKey should not be present for V1? This should be checked?
Valerie
On 5/25/2020 9:02 PM, Weijun Wang wrote:
The new definition at https://tools.ietf.org/html/rfc5958 shows,
OneAsymmetricKey ::= SEQUENCE {
version Version,
privateKeyAlgorithm PrivateKeyAlgorithmIdentifier,
privateKeyPrivateKey,
attributes[0] Attributes OPTIONAL,
...,
[[2: publicKey[1] PublicKey OPTIONAL ]],
...
}
According to
https://www.ibm.com/support/pages/what-does-string-three-elipses-mean-asn1:
The string "..." in ASN.1 is called an extension marker. The extension
marker,
ellipsis, is an indication that extension additions are expected. It makes
no
statement as to how such additions should be handled. However they shall
not be
treated as an error during the decoding process.
So there might be more fields in the future. Do you still insist we need more
validation?
Thanks,
Max
On May 20, 2020, at 1:37 PM, Valerie Peng wrote:
True, the current handling of the extra bytes in parseKey() and decode() are
kind of opposite, one does not allow any extra bytes but the other ignores all.
The trailing bytes may look harmless but simply ignores it may open up
something unexpected.
Given that this is key related class, I think it's important to do reasonable
validation even though we currently do not use these trailing bytes. It'd also
be good if the handling can be consistent regardless of the call path.
Re: RFR 8244565: Accept PKCS #8 with version number 1
RSAPrivateKeyImpl and RSAPrivateCrtKeyImpl
- throws InvalidKeyException when RSAUtil.createAlgorithmId(type, keyParams)
fails. I'll keep it.
EdDSAPrivateKeyImpl, XDHPrivateKeyImpl and ECPrivateKeyImpl
- check the input ECParameterSpec params and might throw an
InvalidKeyException. I'll keep it.
Some of these constructors now throws ProviderException or InvalidKeyException
when DER encoding goes wrong. This should never happen, and I'll throw
AssertionError for it.
In short, the only possible exception is now InvalidKeyException. I also throw
AssertionError but it's not possible and definitely means a programming error
somewhere (Ex: DerValue::putInteger goes wrong).
So here is the updated webrev
http://cr.openjdk.java.net/~weijun/8244565/webrev.04/
Thanks,
Max
> On Jun 4, 2020, at 7:37 AM, Valerie Peng wrote:
>
> Hi, Max,
>
> Overall looks very good. Just one more thing:
>
> Different key classes seems to differ in their handling of IOException in
> their constructor which produces the DER bytes.
>
> DSAPrivateKey changed to use AssertionError, XDHPrivateKeyImpl and
> EdDSAPrivateKeyImpl throws ProviderException. It seems that all other
> PrivateKey classes use InvalidKeyException. Perhaps it'd be nice to use
> InvalidKeyException for consistency sake?
>
> Thanks,
> Valerie
> On 6/1/2020 12:35 AM, Weijun Wang wrote:
>> Updated webrev at
>>
>>https://cr.openjdk.java.net/~weijun/8244565/webrev.03.
>>
>> I've inlined more methods that is only called once and inside the same
>> method.
>>
>> Thanks,
>> Max
>>
>>
>>> On May 28, 2020, at 9:16 AM, Weijun Wang wrote:
>>>
>>>
>>>
On May 28, 2020, at 8:46 AM, Valerie Peng wrote:
Hi Max,
I like this new structure better. Much easier to understand. Most of the
changes are technical debt that's accumulated inside PKCS8Key class.
A few notable differences which I am not so sure about are
- the encodedKey is returned by getEncoded() directly w/o cloning, and
>>> Too bad. I'll fix.
>>>
- the protected parseKeyBits() method being made private. I wonder if the
parseKeyBits() method should be made abstract so it's more obvious that
the subclasses needs to parse the key bytes into algorithm-specific
components.
>>> Or how about I just inline parseKeyBits in those child classes into their
>>> constructors? I don't think the child class will forget it. Otherwise, why
>>> write a child class?
>>>
>>> Thanks,
>>> Max
>>>
Thanks,
Valerie
On 5/26/2020 5:54 PM, Weijun Wang wrote:
> Can you please take a look (not a review) at an updated webrev at
> http://cr.openjdk.java.net/~weijun/8244565/webrev.02/? It contains the
> code to inspect the extra fields. I haven't deal with the "..." here yet.
>
> However, when I tried to consolidate the DER parsing into one place, I've
> made more and more changes everywhere. The major change now is a base
> constructor PKCS8Key(byte[]) and subclass constructors that call it and
> no more protected parseKeyBits(). Although I don't think there is any
> technical error here but at the end of the day I'm asking myself if this
> is too much code change for such a simple bug.
>
> Do you like the overall structure? If yes, I'll continue this way.
> Otherwise, I can restrict the change only inside PKCS8Key.
>
> Thanks,
> Max
>
>
>> On May 27, 2020, at 7:14 AM, Valerie Peng
>> wrote:
>>
>> I suppose we can allow trailing data to conform to "..." then.
>>
>> But the "[[2: publicKey [1] PublicKey OPTIONAL ]]," line mean that the
>> publicKey should not be present for V1? This should be checked?
>>
>> Valerie
>>
>> On 5/25/2020 9:02 PM, Weijun Wang wrote:
>>> The new definition at https://tools.ietf.org/html/rfc5958 shows,
>>>
>>> OneAsymmetricKey ::= SEQUENCE {
>>> version Version,
>>> privateKeyAlgorithm PrivateKeyAlgorithmIdentifier,
>>> privateKeyPrivateKey,
>>> attributes[0] Attributes OPTIONAL,
>>> ...,
>>> [[2: publicKey[1] PublicKey OPTIONAL ]],
>>> ...
>>> }
>>>
>>> According to
>>> https://www.ibm.com/support/pages/what-does-string-three-elipses-mean-asn1:
>>>
>>>The string "..." in ASN.1 is called an extension marker. The
>>> extension marker,
>>>ellipsis, is an indication that extension additions are expected. It
>>> makes no
>>>statement as to how such additions should be handled. However they
>>> shall not be
>>>treated as an error during the decoding process.
>>>
>>> So there might be more fields in the future. Do you still insist we
>>> need more validation?
>>>
>>> Thanks,
>>> Max
>>>
>>>
On May 20, 2020,
Re: RFR 8244565: Accept PKCS #8 with version number 1
Hi, Max,
Overall looks very good. Just one more thing:
Different key classes seems to differ in their handling of IOException
in their constructor which produces the DER bytes.
DSAPrivateKey changed to use AssertionError, XDHPrivateKeyImpl and
EdDSAPrivateKeyImpl throws ProviderException. It seems that all other
PrivateKey classes use InvalidKeyException. Perhaps it'd be nice to use
InvalidKeyException for consistency sake?
Thanks,
Valerie
On 6/1/2020 12:35 AM, Weijun Wang wrote:
Updated webrev at
https://cr.openjdk.java.net/~weijun/8244565/webrev.03.
I've inlined more methods that is only called once and inside the same method.
Thanks,
Max
On May 28, 2020, at 9:16 AM, Weijun Wang wrote:
On May 28, 2020, at 8:46 AM, Valerie Peng wrote:
Hi Max,
I like this new structure better. Much easier to understand. Most of the
changes are technical debt that's accumulated inside PKCS8Key class.
A few notable differences which I am not so sure about are
- the encodedKey is returned by getEncoded() directly w/o cloning, and
Too bad. I'll fix.
- the protected parseKeyBits() method being made private. I wonder if the
parseKeyBits() method should be made abstract so it's more obvious that the
subclasses needs to parse the key bytes into algorithm-specific components.
Or how about I just inline parseKeyBits in those child classes into their
constructors? I don't think the child class will forget it. Otherwise, why
write a child class?
Thanks,
Max
Thanks,
Valerie
On 5/26/2020 5:54 PM, Weijun Wang wrote:
Can you please take a look (not a review) at an updated webrev at
http://cr.openjdk.java.net/~weijun/8244565/webrev.02/? It contains the code to inspect
the extra fields. I haven't deal with the "..." here yet.
However, when I tried to consolidate the DER parsing into one place, I've made
more and more changes everywhere. The major change now is a base constructor
PKCS8Key(byte[]) and subclass constructors that call it and no more protected
parseKeyBits(). Although I don't think there is any technical error here but at
the end of the day I'm asking myself if this is too much code change for such a
simple bug.
Do you like the overall structure? If yes, I'll continue this way. Otherwise, I
can restrict the change only inside PKCS8Key.
Thanks,
Max
On May 27, 2020, at 7:14 AM, Valerie Peng wrote:
I suppose we can allow trailing data to conform to "..." then.
But the "[[2: publicKey [1] PublicKey OPTIONAL ]]," line mean that the
publicKey should not be present for V1? This should be checked?
Valerie
On 5/25/2020 9:02 PM, Weijun Wang wrote:
The new definition at https://tools.ietf.org/html/rfc5958 shows,
OneAsymmetricKey ::= SEQUENCE {
version Version,
privateKeyAlgorithm PrivateKeyAlgorithmIdentifier,
privateKeyPrivateKey,
attributes[0] Attributes OPTIONAL,
...,
[[2: publicKey[1] PublicKey OPTIONAL ]],
...
}
According to
https://www.ibm.com/support/pages/what-does-string-three-elipses-mean-asn1:
The string "..." in ASN.1 is called an extension marker. The extension
marker,
ellipsis, is an indication that extension additions are expected. It makes
no
statement as to how such additions should be handled. However they shall
not be
treated as an error during the decoding process.
So there might be more fields in the future. Do you still insist we need more
validation?
Thanks,
Max
On May 20, 2020, at 1:37 PM, Valerie Peng wrote:
True, the current handling of the extra bytes in parseKey() and decode() are
kind of opposite, one does not allow any extra bytes but the other ignores all.
The trailing bytes may look harmless but simply ignores it may open up
something unexpected.
Given that this is key related class, I think it's important to do reasonable
validation even though we currently do not use these trailing bytes. It'd also
be good if the handling can be consistent regardless of the call path.
Thanks,
Valerie
On 5/18/2020 9:36 PM, Weijun Wang wrote:
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 =
Re: RFR 8244565: Accept PKCS #8 with version number 1
Updated webrev at
https://cr.openjdk.java.net/~weijun/8244565/webrev.03.
I've inlined more methods that is only called once and inside the same method.
Thanks,
Max
> On May 28, 2020, at 9:16 AM, Weijun Wang wrote:
>
>
>
>> On May 28, 2020, at 8:46 AM, Valerie Peng wrote:
>>
>> Hi Max,
>>
>> I like this new structure better. Much easier to understand. Most of the
>> changes are technical debt that's accumulated inside PKCS8Key class.
>>
>> A few notable differences which I am not so sure about are
>>
>> - the encodedKey is returned by getEncoded() directly w/o cloning, and
>
> Too bad. I'll fix.
>
>>
>> - the protected parseKeyBits() method being made private. I wonder if the
>> parseKeyBits() method should be made abstract so it's more obvious that the
>> subclasses needs to parse the key bytes into algorithm-specific components.
>
> Or how about I just inline parseKeyBits in those child classes into their
> constructors? I don't think the child class will forget it. Otherwise, why
> write a child class?
>
> Thanks,
> Max
>
>>
>> Thanks,
>> Valerie
>> On 5/26/2020 5:54 PM, Weijun Wang wrote:
>>> Can you please take a look (not a review) at an updated webrev at
>>> http://cr.openjdk.java.net/~weijun/8244565/webrev.02/? It contains the code
>>> to inspect the extra fields. I haven't deal with the "..." here yet.
>>>
>>> However, when I tried to consolidate the DER parsing into one place, I've
>>> made more and more changes everywhere. The major change now is a base
>>> constructor PKCS8Key(byte[]) and subclass constructors that call it and no
>>> more protected parseKeyBits(). Although I don't think there is any
>>> technical error here but at the end of the day I'm asking myself if this is
>>> too much code change for such a simple bug.
>>>
>>> Do you like the overall structure? If yes, I'll continue this way.
>>> Otherwise, I can restrict the change only inside PKCS8Key.
>>>
>>> Thanks,
>>> Max
>>>
>>>
On May 27, 2020, at 7:14 AM, Valerie Peng wrote:
I suppose we can allow trailing data to conform to "..." then.
But the "[[2: publicKey [1] PublicKey OPTIONAL ]]," line mean that the
publicKey should not be present for V1? This should be checked?
Valerie
On 5/25/2020 9:02 PM, Weijun Wang wrote:
> The new definition at https://tools.ietf.org/html/rfc5958 shows,
>
> OneAsymmetricKey ::= SEQUENCE {
> version Version,
> privateKeyAlgorithm PrivateKeyAlgorithmIdentifier,
> privateKeyPrivateKey,
> attributes[0] Attributes OPTIONAL,
> ...,
> [[2: publicKey[1] PublicKey OPTIONAL ]],
> ...
> }
>
> According to
> https://www.ibm.com/support/pages/what-does-string-three-elipses-mean-asn1:
>
>The string "..." in ASN.1 is called an extension marker. The extension
> marker,
>ellipsis, is an indication that extension additions are expected. It
> makes no
>statement as to how such additions should be handled. However they
> shall not be
>treated as an error during the decoding process.
>
> So there might be more fields in the future. Do you still insist we need
> more validation?
>
> Thanks,
> Max
>
>
>> On May 20, 2020, at 1:37 PM, Valerie Peng
>> wrote:
>>
>>
>> True, the current handling of the extra bytes in parseKey() and decode()
>> are kind of opposite, one does not allow any extra bytes but the other
>> ignores all. The trailing bytes may look harmless but simply ignores it
>> may open up something unexpected.
>>
>> Given that this is key related class, I think it's important to do
>> reasonable validation even though we currently do not use these trailing
>> bytes. It'd also be good if the handling can be consistent regardless of
>> the call path.
>>
>> Thanks,
>> Valerie
>> On 5/18/2020 9:36 PM, Weijun Wang wrote:
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;
Re: RFR 8244565: Accept PKCS #8 with version number 1
> On May 28, 2020, at 8:46 AM, Valerie Peng wrote:
>
> Hi Max,
>
> I like this new structure better. Much easier to understand. Most of the
> changes are technical debt that's accumulated inside PKCS8Key class.
>
> A few notable differences which I am not so sure about are
>
> - the encodedKey is returned by getEncoded() directly w/o cloning, and
Too bad. I'll fix.
>
> - the protected parseKeyBits() method being made private. I wonder if the
> parseKeyBits() method should be made abstract so it's more obvious that the
> subclasses needs to parse the key bytes into algorithm-specific components.
Or how about I just inline parseKeyBits in those child classes into their
constructors? I don't think the child class will forget it. Otherwise, why
write a child class?
Thanks,
Max
>
> Thanks,
> Valerie
> On 5/26/2020 5:54 PM, Weijun Wang wrote:
>> Can you please take a look (not a review) at an updated webrev at
>> http://cr.openjdk.java.net/~weijun/8244565/webrev.02/? It contains the code
>> to inspect the extra fields. I haven't deal with the "..." here yet.
>>
>> However, when I tried to consolidate the DER parsing into one place, I've
>> made more and more changes everywhere. The major change now is a base
>> constructor PKCS8Key(byte[]) and subclass constructors that call it and no
>> more protected parseKeyBits(). Although I don't think there is any technical
>> error here but at the end of the day I'm asking myself if this is too much
>> code change for such a simple bug.
>>
>> Do you like the overall structure? If yes, I'll continue this way.
>> Otherwise, I can restrict the change only inside PKCS8Key.
>>
>> Thanks,
>> Max
>>
>>
>>> On May 27, 2020, at 7:14 AM, Valerie Peng wrote:
>>>
>>> I suppose we can allow trailing data to conform to "..." then.
>>>
>>> But the "[[2: publicKey [1] PublicKey OPTIONAL ]]," line mean that the
>>> publicKey should not be present for V1? This should be checked?
>>>
>>> Valerie
>>>
>>> On 5/25/2020 9:02 PM, Weijun Wang wrote:
The new definition at https://tools.ietf.org/html/rfc5958 shows,
OneAsymmetricKey ::= SEQUENCE {
version Version,
privateKeyAlgorithm PrivateKeyAlgorithmIdentifier,
privateKeyPrivateKey,
attributes[0] Attributes OPTIONAL,
...,
[[2: publicKey[1] PublicKey OPTIONAL ]],
...
}
According to
https://www.ibm.com/support/pages/what-does-string-three-elipses-mean-asn1:
The string "..." in ASN.1 is called an extension marker. The extension
marker,
ellipsis, is an indication that extension additions are expected. It
makes no
statement as to how such additions should be handled. However they
shall not be
treated as an error during the decoding process.
So there might be more fields in the future. Do you still insist we need
more validation?
Thanks,
Max
> On May 20, 2020, at 1:37 PM, Valerie Peng wrote:
>
>
> True, the current handling of the extra bytes in parseKey() and decode()
> are kind of opposite, one does not allow any extra bytes but the other
> ignores all. The trailing bytes may look harmless but simply ignores it
> may open up something unexpected.
>
> Given that this is key related class, I think it's important to do
> reasonable validation even though we currently do not use these trailing
> bytes. It'd also be good if the handling can be consistent regardless of
> the call path.
>
> Thanks,
> Valerie
> On 5/18/2020 9:36 PM, Weijun Wang wrote:
>>> 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
>>>
Re: RFR 8244565: Accept PKCS #8 with version number 1
Hi Max,
I like this new structure better. Much easier to understand. Most of the
changes are technical debt that's accumulated inside PKCS8Key class.
A few notable differences which I am not so sure about are
- the encodedKey is returned by getEncoded() directly w/o cloning, and
- the protected parseKeyBits() method being made private. I wonder if
the parseKeyBits() method should be made abstract so it's more obvious
that the subclasses needs to parse the key bytes into algorithm-specific
components.
Thanks,
Valerie
On 5/26/2020 5:54 PM, Weijun Wang wrote:
Can you please take a look (not a review) at an updated webrev at
http://cr.openjdk.java.net/~weijun/8244565/webrev.02/? It contains the code to inspect
the extra fields. I haven't deal with the "..." here yet.
However, when I tried to consolidate the DER parsing into one place, I've made
more and more changes everywhere. The major change now is a base constructor
PKCS8Key(byte[]) and subclass constructors that call it and no more protected
parseKeyBits(). Although I don't think there is any technical error here but at
the end of the day I'm asking myself if this is too much code change for such a
simple bug.
Do you like the overall structure? If yes, I'll continue this way. Otherwise, I
can restrict the change only inside PKCS8Key.
Thanks,
Max
On May 27, 2020, at 7:14 AM, Valerie Peng wrote:
I suppose we can allow trailing data to conform to "..." then.
But the "[[2: publicKey [1] PublicKey OPTIONAL ]]," line mean that the
publicKey should not be present for V1? This should be checked?
Valerie
On 5/25/2020 9:02 PM, Weijun Wang wrote:
The new definition at https://tools.ietf.org/html/rfc5958 shows,
OneAsymmetricKey ::= SEQUENCE {
version Version,
privateKeyAlgorithm PrivateKeyAlgorithmIdentifier,
privateKeyPrivateKey,
attributes[0] Attributes OPTIONAL,
...,
[[2: publicKey[1] PublicKey OPTIONAL ]],
...
}
According to
https://www.ibm.com/support/pages/what-does-string-three-elipses-mean-asn1:
The string "..." in ASN.1 is called an extension marker. The extension
marker,
ellipsis, is an indication that extension additions are expected. It makes
no
statement as to how such additions should be handled. However they shall
not be
treated as an error during the decoding process.
So there might be more fields in the future. Do you still insist we need more
validation?
Thanks,
Max
On May 20, 2020, at 1:37 PM, Valerie Peng wrote:
True, the current handling of the extra bytes in parseKey() and decode() are
kind of opposite, one does not allow any extra bytes but the other ignores all.
The trailing bytes may look harmless but simply ignores it may open up
something unexpected.
Given that this is key related class, I think it's important to do reasonable
validation even though we currently do not use these trailing bytes. It'd also
be good if the handling can be consistent regardless of the call path.
Thanks,
Valerie
On 5/18/2020 9:36 PM, Weijun Wang wrote:
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
Re: RFR 8244565: Accept PKCS #8 with version number 1
Can you please take a look (not a review) at an updated webrev at
http://cr.openjdk.java.net/~weijun/8244565/webrev.02/? It contains the code to
inspect the extra fields. I haven't deal with the "..." here yet.
However, when I tried to consolidate the DER parsing into one place, I've made
more and more changes everywhere. The major change now is a base constructor
PKCS8Key(byte[]) and subclass constructors that call it and no more protected
parseKeyBits(). Although I don't think there is any technical error here but at
the end of the day I'm asking myself if this is too much code change for such a
simple bug.
Do you like the overall structure? If yes, I'll continue this way. Otherwise, I
can restrict the change only inside PKCS8Key.
Thanks,
Max
> On May 27, 2020, at 7:14 AM, Valerie Peng wrote:
>
> I suppose we can allow trailing data to conform to "..." then.
>
> But the "[[2: publicKey [1] PublicKey OPTIONAL ]]," line mean that the
> publicKey should not be present for V1? This should be checked?
>
> Valerie
>
> On 5/25/2020 9:02 PM, Weijun Wang wrote:
>> The new definition at https://tools.ietf.org/html/rfc5958 shows,
>>
>> OneAsymmetricKey ::= SEQUENCE {
>>version Version,
>>privateKeyAlgorithm PrivateKeyAlgorithmIdentifier,
>>privateKeyPrivateKey,
>>attributes[0] Attributes OPTIONAL,
>>...,
>>[[2: publicKey[1] PublicKey OPTIONAL ]],
>>...
>> }
>>
>> According to
>> https://www.ibm.com/support/pages/what-does-string-three-elipses-mean-asn1:
>>
>> The string "..." in ASN.1 is called an extension marker. The extension
>> marker,
>> ellipsis, is an indication that extension additions are expected. It
>> makes no
>> statement as to how such additions should be handled. However they shall
>> not be
>> treated as an error during the decoding process.
>>
>> So there might be more fields in the future. Do you still insist we need
>> more validation?
>>
>> Thanks,
>> Max
>>
>>
>>> On May 20, 2020, at 1:37 PM, Valerie Peng wrote:
>>>
>>>
>>> True, the current handling of the extra bytes in parseKey() and decode()
>>> are kind of opposite, one does not allow any extra bytes but the other
>>> ignores all. The trailing bytes may look harmless but simply ignores it may
>>> open up something unexpected.
>>>
>>> Given that this is key related class, I think it's important to do
>>> reasonable validation even though we currently do not use these trailing
>>> bytes. It'd also be good if the handling can be consistent regardless of
>>> the call path.
>>>
>>> Thanks,
>>> Valerie
>>> On 5/18/2020 9:36 PM, Weijun Wang wrote:
> 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,
Re: RFR 8244565: Accept PKCS #8 with version number 1
I suppose we can allow trailing data to conform to "..." then.
But the "[[2: publicKey [1] PublicKey OPTIONAL ]]," line mean that the
publicKey should not be present for V1? This should be checked?
Valerie
On 5/25/2020 9:02 PM, Weijun Wang wrote:
The new definition at https://tools.ietf.org/html/rfc5958 shows,
OneAsymmetricKey ::= SEQUENCE {
version Version,
privateKeyAlgorithm PrivateKeyAlgorithmIdentifier,
privateKeyPrivateKey,
attributes[0] Attributes OPTIONAL,
...,
[[2: publicKey[1] PublicKey OPTIONAL ]],
...
}
According to
https://www.ibm.com/support/pages/what-does-string-three-elipses-mean-asn1:
The string "..." in ASN.1 is called an extension marker. The extension
marker,
ellipsis, is an indication that extension additions are expected. It makes
no
statement as to how such additions should be handled. However they shall
not be
treated as an error during the decoding process.
So there might be more fields in the future. Do you still insist we need more
validation?
Thanks,
Max
On May 20, 2020, at 1:37 PM, Valerie Peng wrote:
True, the current handling of the extra bytes in parseKey() and decode() are
kind of opposite, one does not allow any extra bytes but the other ignores all.
The trailing bytes may look harmless but simply ignores it may open up
something unexpected.
Given that this is key related class, I think it's important to do reasonable
validation even though we currently do not use these trailing bytes. It'd also
be good if the handling can be consistent regardless of the call path.
Thanks,
Valerie
On 5/18/2020 9:36 PM, Weijun Wang wrote:
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/
Re: RFR 8244565: Accept PKCS #8 with version number 1
The new definition at https://tools.ietf.org/html/rfc5958 shows,
OneAsymmetricKey ::= SEQUENCE {
version Version,
privateKeyAlgorithm PrivateKeyAlgorithmIdentifier,
privateKeyPrivateKey,
attributes[0] Attributes OPTIONAL,
...,
[[2: publicKey[1] PublicKey OPTIONAL ]],
...
}
According to
https://www.ibm.com/support/pages/what-does-string-three-elipses-mean-asn1:
The string "..." in ASN.1 is called an extension marker. The extension
marker,
ellipsis, is an indication that extension additions are expected. It makes
no
statement as to how such additions should be handled. However they shall
not be
treated as an error during the decoding process.
So there might be more fields in the future. Do you still insist we need more
validation?
Thanks,
Max
> On May 20, 2020, at 1:37 PM, Valerie Peng wrote:
>
>
> True, the current handling of the extra bytes in parseKey() and decode() are
> kind of opposite, one does not allow any extra bytes but the other ignores
> all. The trailing bytes may look harmless but simply ignores it may open up
> something unexpected.
>
> Given that this is key related class, I think it's important to do reasonable
> validation even though we currently do not use these trailing bytes. It'd
> also be good if the handling can be consistent regardless of the call path.
>
> Thanks,
> Valerie
> On 5/18/2020 9:36 PM, Weijun Wang wrote:
>>> 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:
>>
Re: RFR 8244565: Accept PKCS #8 with version number 1
I find the duplication of parsing code annoyed. I'll see if I can consolidate
them into one.
--Max
> On May 20, 2020, at 1:37 PM, Valerie Peng wrote:
>
>
> True, the current handling of the extra bytes in parseKey() and decode() are
> kind of opposite, one does not allow any extra bytes but the other ignores
> all. The trailing bytes may look harmless but simply ignores it may open up
> something unexpected.
>
> Given that this is key related class, I think it's important to do reasonable
> validation even though we currently do not use these trailing bytes. It'd
> also be good if the handling can be consistent regardless of the call path.
>
> Thanks,
> Valerie
> On 5/18/2020 9:36 PM, Weijun Wang wrote:
>>> 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 8244565: Accept PKCS #8 with version number 1
True, the current handling of the extra bytes in parseKey() and decode()
are kind of opposite, one does not allow any extra bytes but the other
ignores all. The trailing bytes may look harmless but simply ignores it
may open up something unexpected.
Given that this is key related class, I think it's important to do
reasonable validation even though we currently do not use these trailing
bytes. It'd also be good if the handling can be consistent regardless of
the call path.
Thanks,
Valerie
On 5/18/2020 9:36 PM, Weijun Wang wrote:
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 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 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
Re: RFR 8244565: Accept PKCS #8 with version number 1
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 8244565: Accept PKCS #8 with version number 1
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 >
RFR 8244565: Accept PKCS #8 with version number 1
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
