[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