> On Oct 30, 2018, at 2:22 AM, Michael StJohns <mstjo...@comcast.net> 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 <mstjo...@comcast.net> 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]
>>>> https://docs.microsoft.com/en-gb/windows/desktop/api/wincrypt/ns-wincrypt-_crypt_algorithm_identifier
> 
> 

Reply via email to