Re: [1/2] dlls/crypt32: implement PFXImportCertStore()

[Philippe Casgrain]

[Finally have a little bit of downtime, can turn my attention back to this...]

On 2010-03-25, at 2:34 PM, Juan Lang wrote:

 I don't have any password-protected certificates to test with, so I can't 
 add such a test (it was not required for our implementation).
 
 You can if you create one (on Windows.)

I do not know how to do that. The motivation behind this patch is to get 
something of value inside WineHQ so that the implementation of this function 
can be started. It is possible that we may implement password-protection and 
private key extraction in the future, or someone else may do it. This would be 
great for the community. But it's not something that I can do right now.

 So what you want is to import two new functions (sk_X509_new_null() and 
 sk_X509_free()) and use them to create a STACK_OF(X509) whose sole purpose 
 is to detect if there are more certificates, and then ERR or TRACE if there 
 are, and then dispose?
 
 No, I suppose not.  A test marked todo_wine would be better.

I don't disagree that this implementation is incomplete. It is, however, much 
less incomplete than what was there before in the sense of 
 
 Ah. In that case, it does not really matter what the string is, right? I can 
 remove it if you don't want it.
 
 Please do ;)

Unnecessary strings will be removed in the next submission.

The motivation behind this patch is/was to get *something* into WineHQ so that 
the next time we or someone else needs to work on this, there is a basic 
skeleton in place. As a bonus, this skeleton actually does something useful 
(get the certificate using OpenSSL). We know it works and is useful because a 
now-shipping game is using it.

If the WineHQ community feels that an incomplete implementation is not good 
enough for a patch submission, that is entirely their prerogative. I would 
obviously prefer that my patch be (eventually) incorporated, but I understand 
the reasoning.

Philippe Casgrain
[Again, apologies for the delay, things are just crazy-busy around here...]

Re: [1/2] dlls/crypt32: implement PFXImportCertStore()

[Philippe Casgrain]

Hi Juan,

Thanks for reviewing my patches. Here are my comments:

 this attempt looks pretty incomplete.  First off:
 
 +ret = pPKCS12_verify_mac(pkcs12, password, len);
 +if (ret == 0)
 +ERR_(crypt)(failed to verify pkcs12 {%p} with password
 \%s\ using func {%p}\n, pkcs12, password, pPKCS12_verify_mac);
 +else
 +TRACE_(crypt)(verified pkcs12 {%p} with password \%s\
 using func {%p}\n, pkcs12, password, pPKCS12_verify_mac);
 
 You accept the PKCS12 file even if the password is incorrect.  This is
 clearly wrong.

It is not accepted. If the verification fails, ERR is spewed out and the next 
step (parse, below) will fail as well.

 +/* extract private key and certificate */
 +ret = pPKCS12_parse(pkcs12, password, pkey, cert, NULL);
 
 You don't support more than a single certificate in the PKCS12 file.
 This may be fine for the majority of uses, but at least a warning
 indicating more certificates are present would be helpful.

Hmmm. How do you suggest I do that? From 
http://www.openssl.org/docs/crypto/PKCS12_parse.html I get this:

BUGS

Only a single private key and corresponding certificate is returned by this 
function. More complex PKCS#12 
files with multiple private keys will only return the first match.

 Also, a
 PKCS12 file can contain more than just certificates, and the tests
 ought at least to check this.  For example, what about a PKCS12 file
 with a CRL in it?

I have not seen, nor needed to implement this, so I'm not sure how to test for 
it. Maybe add a comment to the test? Or a wine_todo test so we don't lose this 
information?

 
 OpenSSL also supports at least a couple of attributes associated with
 the certificates.  From the man page for PKCS12_parse:
   The friendlyName and localKeyID attributes (if present) on each
   certificate will be stored in the alias and keyid attributes of the
   X509 structure.
 The Crypto API also supports setting such attributes, and if you
 aren't going to support these, at least the tests should cover them
 (and marked todo_wine) so we know they're still not done.

Same answer. I guess I can update the test set with more wine_todo().

 More questions:
 +const WCHAR storeName[] = {'S', 't', 'o', 'r', 'e', ' ', 'N',
 'a', 'm', 'e', 0};
 +store = CertOpenStore(CERT_STORE_PROV_MEMORY, 0,
 (HCRYPTPROV)NULL, CERT_STORE_CREATE_NEW_FLAG, storeName);
 
 What's the purpose of the store name?  Does the native implementation
 do this?  If not, it can be omitted.  If so, the tests should check
 it.

If you create a store with no name, you run the risk of it not being created 
(if there is another store with no name). 

I don't see a way from native to get the store name as it was created (only the 
Localized Name, which may or may not be what we want).

Philippe

Re: [1/2] dlls/crypt32: implement PFXImportCertStore()

[Juan Lang]

Hi Philippe,

 You accept the PKCS12 file even if the password is incorrect.  This is
 clearly wrong.

 It is not accepted. If the verification fails, ERR is spewed out and the next 
 step (parse, below) will fail as well.

Is this how Windows fails?  That is, with a parse error?  Please add a
test to cover this case.

 You don't support more than a single certificate in the PKCS12 file.
 This may be fine for the majority of uses, but at least a warning
 indicating more certificates are present would be helpful.

 Hmmm. How do you suggest I do that? From 
 http://www.openssl.org/docs/crypto/PKCS12_parse.html I get this:

    BUGS

    Only a single private key and corresponding certificate is returned by 
 this function. More complex PKCS#12
    files with multiple private keys will only return the first match.

Look at the 5th parameter of PKCS12_parse.  It's true that OpenSSL
will only return a single certificate with a private key, but not
every certificate in a PKCS12 file need contain a private key.

 Also, a
 PKCS12 file can contain more than just certificates, and the tests
 ought at least to check this.  For example, what about a PKCS12 file
 with a CRL in it?

 I have not seen, nor needed to implement this, so I'm not sure how to test 
 for it. Maybe add a comment to the test? Or a wine_todo test so we don't lose 
 this information?

Test for it the way you should any Wine test:  on Windows.  Create a
store with a CRL in it, export it to a PKCS12 file, and use that as
your test case.

 The Crypto API also supports setting such attributes, and if you
 aren't going to support these, at least the tests should cover them
 (and marked todo_wine) so we know they're still not done.

 Same answer. I guess I can update the test set with more wine_todo().

Yes, I'd appreciate that.

 If you create a store with no name, you run the risk of it not being created 
 (if there is another store with no name).

Not for a memory store, it's just a linked list.
--Juan

Re: [1/2] dlls/crypt32: implement PFXImportCertStore()

[Philippe Casgrain]

On 2010-03-25, at 12:14 PM, Juan Lang wrote:

 Hi Philippe,
 
 You accept the PKCS12 file even if the password is incorrect.  This is
 clearly wrong.
 
 It is not accepted. If the verification fails, ERR is spewed out and the 
 next step (parse, below) will fail as well.
 
 Is this how Windows fails?  That is, with a parse error?  Please add a
 test to cover this case.

I don't have any password-protected certificates to test with, so I can't add 
such a test (it was not required for our implementation).

 
 You don't support more than a single certificate in the PKCS12 file.
 This may be fine for the majority of uses, but at least a warning
 indicating more certificates are present would be helpful.
 
 Hmmm. How do you suggest I do that? From 
 http://www.openssl.org/docs/crypto/PKCS12_parse.html I get this:
 
BUGS
 
Only a single private key and corresponding certificate is returned by 
 this function. More complex PKCS#12
files with multiple private keys will only return the first match.
 
 Look at the 5th parameter of PKCS12_parse.  It's true that OpenSSL
 will only return a single certificate with a private key, but not
 every certificate in a PKCS12 file need contain a private key.

So what you want is to import two new functions (sk_X509_new_null() and 
sk_X509_free()) and use them to create a STACK_OF(X509) whose sole purpose is 
to detect if there are more certificates, and then ERR or TRACE if there are, 
and then dispose?

I added a FIXME comment.

 Also, a
 PKCS12 file can contain more than just certificates, and the tests
 ought at least to check this.  For example, what about a PKCS12 file
 with a CRL in it?
 
 I have not seen, nor needed to implement this, so I'm not sure how to test 
 for it. Maybe add a comment to the test? Or a wine_todo test so we don't 
 lose this information?
 
 Test for it the way you should any Wine test:  on Windows.  Create a
 store with a CRL in it, export it to a PKCS12 file, and use that as
 your test case.

I added the information as comments inside the test case. Mailing lists are 
harder to search than source code.

 The Crypto API also supports setting such attributes, and if you
 aren't going to support these, at least the tests should cover them
 (and marked todo_wine) so we know they're still not done.
 
 Same answer. I guess I can update the test set with more wine_todo().
 
 Yes, I'd appreciate that.

I did not realize that you wanted the actual set of test cases, disabled with 
todo_wine in front. I have added this information as comments inside the test 
case.

 If you create a store with no name, you run the risk of it not being created 
 (if there is another store with no name).
 
 Not for a memory store, it's just a linked list.

Ah. In that case, it does not really matter what the string is, right? I can 
remove it if you don't want it.

Philippe

Re: [1/2] dlls/crypt32: implement PFXImportCertStore()

[Juan Lang]

Whoops, forgot to cc wine-devel.

 I don't have any password-protected certificates to test with, so I can't add 
 such a test (it was not required for our implementation).

You can if you create one (on Windows.)

 So what you want is to import two new functions (sk_X509_new_null() and 
 sk_X509_free()) and use them to create a STACK_OF(X509) whose sole purpose is 
 to detect if there are more certificates, and then ERR or TRACE if there are, 
 and then dispose?

No, I suppose not.  A test marked todo_wine would be better.

 Ah. In that case, it does not really matter what the string is, right? I can 
 remove it if you don't want it.

Please do ;)
--Juan

Re: [1/2] dlls/crypt32: implement PFXImportCertStore()

[Juan Lang]

Hi Phillippe,

this attempt looks pretty incomplete.  First off:

+ret = pPKCS12_verify_mac(pkcs12, password, len);
+if (ret == 0)
+ERR_(crypt)(failed to verify pkcs12 {%p} with password
\%s\ using func {%p}\n, pkcs12, password, pPKCS12_verify_mac);
+else
+TRACE_(crypt)(verified pkcs12 {%p} with password \%s\
using func {%p}\n, pkcs12, password, pPKCS12_verify_mac);

You accept the PKCS12 file even if the password is incorrect.  This is
clearly wrong.

+/* extract private key and certificate */
+ret = pPKCS12_parse(pkcs12, password, pkey, cert, NULL);

You don't support more than a single certificate in the PKCS12 file.
This may be fine for the majority of uses, but at least a warning
indicating more certificates are present would be helpful.  Also, a
PKCS12 file can contain more than just certificates, and the tests
ought at least to check this.  For example, what about a PKCS12 file
with a CRL in it?

OpenSSL also supports at least a couple of attributes associated with
the certificates.  From the man page for PKCS12_parse:
   The friendlyName and localKeyID attributes (if present) on each
   certificate will be stored in the alias and keyid attributes of the
   X509 structure.
The Crypto API also supports setting such attributes, and if you
aren't going to support these, at least the tests should cover them
(and marked todo_wine) so we know they're still not done.

More questions:
+const WCHAR storeName[] = {'S', 't', 'o', 'r', 'e', ' ', 'N',
'a', 'm', 'e', 0};
+store = CertOpenStore(CERT_STORE_PROV_MEMORY, 0,
(HCRYPTPROV)NULL, CERT_STORE_CREATE_NEW_FLAG, storeName);

What's the purpose of the store name?  Does the native implementation
do this?  If not, it can be omitted.  If so, the tests should check
it.

Thanks,
--Juan