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
