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