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
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
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
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
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
[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...]
-
Juan Lang -
Philippe Casgrain