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