Re: RFR 8026953: Add support for MS Cryptography next generation (CNG) (step 1)

[Weijun Wang]

> On Oct 30, 2018, at 2:22 AM, Michael StJohns  wrote:
> 
> On 10/25/2018 11:09 PM, Weijun Wang wrote:
>> Hi Mike
>> 
>> Thanks for the feedback.
>> 
>> I understand the current SunMSCAPI implementation recognizes RSA keys only 
>> and it's certainly incorrect to put something like getModulus() and 
>> getPublicExponent() in a general CKey class. They will be fixed later. When 
>> I have more sub class names, I'll definitely use them. You can see I've 
>> moved some CSignature methods into CSignature$RSA. I just haven't done it 
>> everywhere.
> OK.
>> 
>> We'll still need a base CKey for a CNG key, no matter what the underlying 
>> algorithm is. Since CNG uses the same NCRYPT_KEY_HANDLE type for different 
>> types of keys, we will do something similar on the Java side. Since the 
>> current CPublicKey and CPrivateKey are very light, it will be easy to move 
>> the content into algorithm-specific classes.
> This is where I think you need to fix the structure:
> 
> abstract class CKey
> 
> public class CRSAPublicKey extends CKey implements RSAKey, RSAPublicKey, 
> PublicKey
> public class CRSAExtractablePrivateKey extends CKey implements RSAKey, 
> RSAPrivateKey, PrivateKey[,RSAMultiPrimePrivateCrt | RSAPrivateCrtKey]
> public class CRSAPrivateKey extends CKey implements RSAKey, PrivateKey
> 
> public class CECPublicKey extends CKey implements ECKey, ECPublicKey, 
> PublicKey
> public class CECExtractablePrivateKey extends CKey implements ECKey, 
> PrivateKey
> public class CECPrivateKey extends CKey implements ECKey, ECPrivateKey, 
> ECPrivateKey

Very likely.

I'll get the signature signing/verification working first, and then will have 
time to expose the key info with these classes.

> 
> Note the two different versions of the private keys to match up with the key 
> handling bits as well as some additional interfaces that may be needed to be 
> added to support the underlying provider's requirements for the RSA keys.
> 
> Also, I'm looking ahead a little bit and thinking about how the JCA would use 
> the windows PCP (Platform Crypto Provider) which uses the TPM to enforce 
> hardware security.  It would be useful if you didn't have to re-write 
> everything just because of a different underlying Windows provider. (There's 
> a PCP development kit that's got some useful sample code that might help a 
> little bit with refactoring the JCA MSCAPI provider even for the existing 
> code).   E.g. eventually supporting an MSCAPI-PCP provider shouldn't require 
> all new code.

Great. I really need more sample code to study.

> 
>> 
>> The main reason I want to take this first step is that after some study on 
>> CNG I make some progress and also see some blockers. For example, I am able 
>> to expose a EC CNG key stored in Windows-MY now and use it to sign/verify. 
>> However, I still don't know how to import external keys inside there 
>> (certmgr.exe can so it's possible). Until now, the most requested function 
>> is to use existing keys in signatures and I want to start working on it. The 
>> first thing I noticed then is that the current class names are unsuitable 
>> and I think a refactoring will make them look better.
> AFAICT, you're not going to be able to use any external key without importing 
> it or running it through a key factory.  The main class you're going to be 
> using is NCryptImportKey (or alternately BCryptImportKeyPair).

I tried NCryptCreatePersistedKey and NCryptImportKey but there are always some 
flags I cannot get right.

> 
>> 
>> Once I start working on the next step, I'll need to have different sub 
>> classes in CKey and CSignature. I even have an alternative plan to ditch 
>> CPublicKey and use the PublicKey classes in SunEC and SunRsaSign. This was 
>> actually already used in the RSASSA-PSS signature handling in SunMSCAPI we 
>> added into JDK 11 in the last minute.
> 
> So you just use software classes in another provider for 
> encrypting/verifying?  To be honest this sounds messy and may come back to 
> bite you down the road.

In JDK 11 that was a workaround because I don't have time to get the native 
verifying part correctly. I don't mean that's a better solution.

> 
>> 
>> As for KeyFactory, we do not have an urgent requirement to use external keys 
>> in a CNG Signature object or store them into Windows-MY. Also, we can use 
>> the one in SunRsaSign.
> Hmm... one of the more common things is to move around .p12 files with your 
> certs and keys.  They can be imported by the Windows tools - it would be 
> *really* nice if you can do the same thing with the Java provider.

That's how I am testing now. Creating p12 files in Java and import them using 
certmgr.exe and then see if I can signing from inside Windows.

In Java, either there will be a factory class or the CKeyStore::setKey method 
will need to understand soft keys.

Thanks
Max

> 
>> 
>> Thanks again.
>> 
>> --Max
>> 
>>> On Oct 26, 2018, at 1:25 AM, Michael StJohns  wrote:
>>> 
>>> Hi Max -
>>> 
>>> 

Re: RFR 8026953: Add support for MS Cryptography next generation (CNG) (step 1)

[Michael StJohns]

On 10/25/2018 11:09 PM, Weijun Wang wrote:

Hi Mike

Thanks for the feedback.

I understand the current SunMSCAPI implementation recognizes RSA keys only and 
it's certainly incorrect to put something like getModulus() and 
getPublicExponent() in a general CKey class. They will be fixed later. When I 
have more sub class names, I'll definitely use them. You can see I've moved 
some CSignature methods into CSignature$RSA. I just haven't done it everywhere.

OK.


We'll still need a base CKey for a CNG key, no matter what the underlying 
algorithm is. Since CNG uses the same NCRYPT_KEY_HANDLE type for different 
types of keys, we will do something similar on the Java side. Since the current 
CPublicKey and CPrivateKey are very light, it will be easy to move the content 
into algorithm-specific classes.

This is where I think you need to fix the structure:

abstract class CKey

public class CRSAPublicKey extends CKey implements RSAKey, RSAPublicKey, 
PublicKey
public class CRSAExtractablePrivateKey extends CKey implements RSAKey, 
RSAPrivateKey, PrivateKey[,RSAMultiPrimePrivateCrt | RSAPrivateCrtKey]

public class CRSAPrivateKey extends CKey implements RSAKey, PrivateKey

public class CECPublicKey extends CKey implements ECKey, ECPublicKey, 
PublicKey
public class CECExtractablePrivateKey extends CKey implements ECKey, 
PrivateKey
public class CECPrivateKey extends CKey implements ECKey, ECPrivateKey, 
ECPrivateKey


Note the two different versions of the private keys to match up with the 
key handling bits as well as some additional interfaces that may be 
needed to be added to support the underlying provider's requirements for 
the RSA keys.


Also, I'm looking ahead a little bit and thinking about how the JCA 
would use the windows PCP (Platform Crypto Provider) which uses the TPM 
to enforce hardware security.  It would be useful if you didn't have to 
re-write everything just because of a different underlying Windows 
provider. (There's a PCP development kit that's got some useful sample 
code that might help a little bit with refactoring the JCA MSCAPI 
provider even for the existing code).   E.g. eventually supporting an 
MSCAPI-PCP provider shouldn't require all new code.




The main reason I want to take this first step is that after some study on CNG 
I make some progress and also see some blockers. For example, I am able to 
expose a EC CNG key stored in Windows-MY now and use it to sign/verify. 
However, I still don't know how to import external keys inside there 
(certmgr.exe can so it's possible). Until now, the most requested function is 
to use existing keys in signatures and I want to start working on it. The first 
thing I noticed then is that the current class names are unsuitable and I think 
a refactoring will make them look better.
AFAICT, you're not going to be able to use any external key without 
importing it or running it through a key factory.  The main class you're 
going to be using is NCryptImportKey (or alternately BCryptImportKeyPair).




Once I start working on the next step, I'll need to have different sub classes 
in CKey and CSignature. I even have an alternative plan to ditch CPublicKey and 
use the PublicKey classes in SunEC and SunRsaSign. This was actually already 
used in the RSASSA-PSS signature handling in SunMSCAPI we added into JDK 11 in 
the last minute.


So you just use software classes in another provider for 
encrypting/verifying?  To be honest this sounds messy and may come back 
to bite you down the road.




As for KeyFactory, we do not have an urgent requirement to use external keys in 
a CNG Signature object or store them into Windows-MY. Also, we can use the one 
in SunRsaSign.
Hmm... one of the more common things is to move around .p12 files with 
your certs and keys.  They can be imported by the Windows tools - it 
would be *really* nice if you can do the same thing with the Java provider.




Thanks again.

--Max


On Oct 26, 2018, at 1:25 AM, Michael StJohns  wrote:

Hi Max -

For the same reason I was pushing back on Adam's EC provider I think I need to 
push back here.  You're recasting an RSAPublicKey to just a PublicKey and 
making it difficult to move key material in and out of the MSCAPI proivider.   
Same thing with the private key.

For example, in the CPublicKey class you still have "getModulus()" and 
"getPublicExponent()", but to get at them you'd have to use CPublicKey rather than 
PublicKey.

And looking forward, I'm not sure how you actually implement the EC classes 
here using this model.

I'd suggest not making the change this way and overloading the existing 
classes, but instead adding the appropriate provider classes for new key types 
as you implement support for them.  E.g. Keep CRSAKey, CRSAPublicKey and 
CRSAPrivateKey as distinct classes, add CECKey, CECPublicKey and CECPrivateKey 
when you get there.

Are you missing a KeyFactory class as well?

Lastly, you may want to change the subclass/methods for CSignature (and 

Re: RFR 8026953: Add support for MS Cryptography next generation (CNG) (step 1)

[Weijun Wang]

Hi Mike

Thanks for the feedback.

I understand the current SunMSCAPI implementation recognizes RSA keys only and 
it's certainly incorrect to put something like getModulus() and 
getPublicExponent() in a general CKey class. They will be fixed later. When I 
have more sub class names, I'll definitely use them. You can see I've moved 
some CSignature methods into CSignature$RSA. I just haven't done it everywhere.

We'll still need a base CKey for a CNG key, no matter what the underlying 
algorithm is. Since CNG uses the same NCRYPT_KEY_HANDLE type for different 
types of keys, we will do something similar on the Java side. Since the current 
CPublicKey and CPrivateKey are very light, it will be easy to move the content 
into algorithm-specific classes.

The main reason I want to take this first step is that after some study on CNG 
I make some progress and also see some blockers. For example, I am able to 
expose a EC CNG key stored in Windows-MY now and use it to sign/verify. 
However, I still don't know how to import external keys inside there 
(certmgr.exe can so it's possible). Until now, the most requested function is 
to use existing keys in signatures and I want to start working on it. The first 
thing I noticed then is that the current class names are unsuitable and I think 
a refactoring will make them look better.

Once I start working on the next step, I'll need to have different sub classes 
in CKey and CSignature. I even have an alternative plan to ditch CPublicKey and 
use the PublicKey classes in SunEC and SunRsaSign. This was actually already 
used in the RSASSA-PSS signature handling in SunMSCAPI we added into JDK 11 in 
the last minute.

As for KeyFactory, we do not have an urgent requirement to use external keys in 
a CNG Signature object or store them into Windows-MY. Also, we can use the one 
in SunRsaSign.

Thanks again.

--Max

> On Oct 26, 2018, at 1:25 AM, Michael StJohns  wrote:
> 
> Hi Max - 
> 
> For the same reason I was pushing back on Adam's EC provider I think I need 
> to push back here.  You're recasting an RSAPublicKey to just a PublicKey and 
> making it difficult to move key material in and out of the MSCAPI proivider.  
>  Same thing with the private key.   
> 
> For example, in the CPublicKey class you still have "getModulus()" and 
> "getPublicExponent()", but to get at them you'd have to use CPublicKey rather 
> than PublicKey.  
> 
> And looking forward, I'm not sure how you actually implement the EC classes 
> here using this model.
> 
> I'd suggest not making the change this way and overloading the existing 
> classes, but instead adding the appropriate provider classes for new key 
> types as you implement support for them.  E.g. Keep CRSAKey, CRSAPublicKey 
> and CRSAPrivateKey as distinct classes, add CECKey, CECPublicKey and 
> CECPrivateKey when you get there.
> 
> Are you missing a KeyFactory class as well?
> 
> Lastly, you may want to change the subclass/methods for CSignature (and 
> probably other classes) to reflect the type of Signature you're returning:
> 
>>   if (algo.equals("NONEwithRSA")) {
>> 
>> -return new RSASignature.Raw();
>> +return new CSignature.Raw();
> 
> Instead:   "return new CSignature.RSARaw()"
> 
> And this definitely isn't going to work if you have even one other Cipher 
> you'll be returning:
>>  if (algo.equals("RSA") ||
>>  algo.equals("RSA/ECB/PKCS1Padding")) {
>> 
>> -return new RSACipher();
>> +return new CCipher();
>> 
>>  }
>> 
> 
> 
> Later, Mike
> 
> 
> 
> 
> 
> On 10/25/2018 4:38 AM, Weijun Wang wrote:
>> Please review the change at
>> 
>>
>> https://cr.openjdk.java.net/~weijun/8026953/webrev.00/
>> 
>> 
>> (I will use a sub-task id for this change but currently JBS is down).
>> 
>> The major change is renaming classes. Since we are going to support 
>> algorithms other than RSA, I've renamed the classes like RSAPrivateKey -> 
>> CPrivateKey. Classes that have the same name as JCA classes (like Key, 
>> KeyStore) are also renamed (to CKey, CKeyStore) so it's easy to tell them 
>> apart.
>> 
>> Others are not about renaming but they are also related to supporting other 
>> algorithms, and there is no behavior change. They include:
>> 
>> - CKey (plus its child classes CPublicKey and CPrivateKey) has a new field 
>> "algorithm". This field is used by 
>> CKeyStore::generateRSAKeyAndCertificateChain and its value is obtained from 
>> the public key algorithm in a cert [1].
>> 
>> - Child class named "RSA" of CKeyPairGenerator.
>> 
>> - Child class named "RSA" of CSignature. I also moved some RSA-related 
>> methods into this child class as overridden methods.
>> 
>> - CKeyStore::setPrivateKey's key parameter has a new type Key, but it still 
>> only accepts RSAPrivateCrtKey now.
>> 
>> Noreg-cleanup.
>> 
>> Thanks
>> Max
>> 
>> [1] 
>> 

Re: RFR 8026953: Add support for MS Cryptography next generation (CNG) (step 1)

[Michael StJohns]

Hi Max -

For the same reason I was pushing back on Adam's EC provider I think I 
need to push back here.  You're recasting an RSAPublicKey to just a 
PublicKey and making it difficult to move key material in and out of the 
MSCAPI proivider.   Same thing with the private key.


For example, in the CPublicKey class you still have "getModulus()" and 
"getPublicExponent()", but to get at them you'd have to use CPublicKey 
rather than PublicKey.


And looking forward, I'm not sure how you actually implement the EC 
classes here using this model.


I'd suggest not making the change this way and overloading the existing 
classes, but instead adding the appropriate provider classes for new key 
types as you implement support for them.  E.g. Keep CRSAKey, 
CRSAPublicKey and CRSAPrivateKey as distinct classes, add CECKey, 
CECPublicKey and CECPrivateKey when you get there.


Are you missing a KeyFactory class as well?

Lastly, you may want to change the subclass/methods for CSignature (and 
probably other classes) to reflect the type of Signature you're returning:



   if (algo.equals("NONEwithRSA")) {
- return new RSASignature.Raw();
+ return new CSignature.Raw();


Instead:   "return new CSignature.RSARaw()"

And this definitely isn't going to work if you have even one other 
Cipher you'll be returning:

  if (algo.equals("RSA") ||
  algo.equals("RSA/ECB/PKCS1Padding")) {
- return new RSACipher();
+ return new CCipher();
  }



Later, Mike





On 10/25/2018 4:38 AM, Weijun Wang wrote:

Please review the change at

https://cr.openjdk.java.net/~weijun/8026953/webrev.00/

(I will use a sub-task id for this change but currently JBS is down).

The major change is renaming classes. Since we are going to support algorithms 
other than RSA, I've renamed the classes like RSAPrivateKey -> CPrivateKey. 
Classes that have the same name as JCA classes (like Key, KeyStore) are also 
renamed (to CKey, CKeyStore) so it's easy to tell them apart.

Others are not about renaming but they are also related to supporting other 
algorithms, and there is no behavior change. They include:

- CKey (plus its child classes CPublicKey and CPrivateKey) has a new field 
"algorithm". This field is used by CKeyStore::generateRSAKeyAndCertificateChain 
and its value is obtained from the public key algorithm in a cert [1].

- Child class named "RSA" of CKeyPairGenerator.

- Child class named "RSA" of CSignature. I also moved some RSA-related methods 
into this child class as overridden methods.

- CKeyStore::setPrivateKey's key parameter has a new type Key, but it still 
only accepts RSAPrivateCrtKey now.

Noreg-cleanup.

Thanks
Max

[1] 
https://docs.microsoft.com/en-gb/windows/desktop/api/wincrypt/ns-wincrypt-_crypt_algorithm_identifier

RFR 8026953: Add support for MS Cryptography next generation (CNG) (step 1)

[Weijun Wang]

Please review the change at

   https://cr.openjdk.java.net/~weijun/8026953/webrev.00/

(I will use a sub-task id for this change but currently JBS is down).

The major change is renaming classes. Since we are going to support algorithms 
other than RSA, I've renamed the classes like RSAPrivateKey -> CPrivateKey. 
Classes that have the same name as JCA classes (like Key, KeyStore) are also 
renamed (to CKey, CKeyStore) so it's easy to tell them apart.

Others are not about renaming but they are also related to supporting other 
algorithms, and there is no behavior change. They include:

- CKey (plus its child classes CPublicKey and CPrivateKey) has a new field 
"algorithm". This field is used by CKeyStore::generateRSAKeyAndCertificateChain 
and its value is obtained from the public key algorithm in a cert [1].

- Child class named "RSA" of CKeyPairGenerator.

- Child class named "RSA" of CSignature. I also moved some RSA-related methods 
into this child class as overridden methods.

- CKeyStore::setPrivateKey's key parameter has a new type Key, but it still 
only accepts RSAPrivateCrtKey now.

Noreg-cleanup.

Thanks
Max

[1] 
https://docs.microsoft.com/en-gb/windows/desktop/api/wincrypt/ns-wincrypt-_crypt_algorithm_identifier