Hi, I just noticed commit efa555596b30134261920ea12d8c324f9bef34b4. This change is misleading:
- "Chain %d, element [%d,%d]: expected error %08x, got %08x\n", + "Chain %d, element [%d,%d]: expected error %08x, got %08x. %08x is " + "expected if no valid Verisign root certificate is available.\n", testIndex, chainIndex, elementIndex, expected->dwErrorStatus, - got->dwErrorStatus); + got->dwErrorStatus, CERT_TRUST_IS_UNTRUSTED_ROOT);
The only difference is to add CERT_TRUST_IS_UNTRUSTED_ROOT if any chain doesn't match, whatever the reason. On the other hand, only one chain out of 14 or so that are tested would ever unexpectedly have CERT_TRUST_IS_UNTRUSTED_ROOT set. The remainder of the chains are not rooted in any trusted CA, but are rooted in some untrusted cert. So I don't think this change is really all that beneficial.
Personally, I'd rather have the change reverted. Comments? --Juan
Hi Austin,
The error is very cryptic...currently only shows up on OpenSolaris and FreeBSD (if /usr/ports/security/ca_root_nss isn't installed). It apparently also shows up on some Linux distributions that don't ship OpenSSL with any certificates.
While the error may be cryptic, it shows a real bug: we don't support the correct root certificate location on OpenSolaris. As I said earlier, adding a warning that isn't applicable most of the time doesn't seem like an appropriate fix. --Juan
On Mon, May 4, 2009 at 11:18 AM, Juan Lang juan.lang@gmail.com wrote:
Hi Austin,
The error is very cryptic...currently only shows up on OpenSolaris and FreeBSD (if /usr/ports/security/ca_root_nss isn't installed). It apparently also shows up on some Linux distributions that don't ship OpenSSL with any certificates.
While the error may be cryptic, it shows a real bug: we don't support the correct root certificate location on OpenSolaris. As I said earlier, adding a warning that isn't applicable most of the time doesn't seem like an appropriate fix.
OpenSolaris doesn't have any Verisign root certificates installed by default, which is the problem there.
Would changing it to: "CERT_TRUST_IS_UNTRUSTED_ROOT is expected if no trusted root certificate is available." be better?
I'm working on installing Verisign root certificates on OpenSolaris, give me a couple days...
OpenSolaris doesn't have any Verisign root certificates installed by default, which is the problem there.
Yep, I know.
Would changing it to: "CERT_TRUST_IS_UNTRUSTED_ROOT is expected if no trusted root certificate is available." be better?
I don't think so. I think this test is a special case, because it depends on your system's configuration, whereas the other tests should not.
Part of me thinks the test could be removed, as it doesn't really exercise any code path that isn't already exercised with the existing tests. On the other hand, if the Verisign root CA isn't installed, iTunes will silently fail to start. I'm not sure which is worse, an arguably spurious test failure, or silent application failures.
I'm working on installing Verisign root certificates on OpenSolaris, give me a couple days...
Cool :) --Juan
On Mon, May 4, 2009 at 11:54 AM, Juan Lang juan.lang@gmail.com wrote:
Would changing it to: "CERT_TRUST_IS_UNTRUSTED_ROOT is expected if no trusted root certificate is available." be better?
I don't think so. I think this test is a special case, because it depends on your system's configuration, whereas the other tests should not.
We have a similar warning already: http://source.winehq.org/git/wine.git/?a=blob;f=dlls/ws2_32/tests/sock.c#l24...
Surely there's some warning we can agree on...currently, it doesn't give the user any hint of what's wrong. As far as they can tell, it is solely a Wine problem, not their system being misconfigured. If a user sees "no Verisign certificate", they'd be more likely to investigate why that is, and fix it (or, if they are installed, file a bug).
Part of me thinks the test could be removed, as it doesn't really exercise any code path that isn't already exercised with the existing tests. On the other hand, if the Verisign root CA isn't installed, iTunes will silently fail to start. I'm not sure which is worse, an arguably spurious test failure, or silent application failures.
Silent application failures are probably worse...
We have a similar warning already: http://source.winehq.org/git/wine.git/?a=blob;f=dlls/ws2_32/tests/sock.c#l24...
That warning is appropriate, because it's testing IPv6, so the warning applies in all cases. The reason I'm objecting in this case is that the warning is printed for any failure, no matter which chain is being checked, yet this condition can only occur on one of the 15 or so chains that are checked. Like I said, I think it's a special case. --Juan
On Mon, May 4, 2009 at 12:19 PM, Juan Lang juan.lang@gmail.com wrote:
We have a similar warning already: http://source.winehq.org/git/wine.git/?a=blob;f=dlls/ws2_32/tests/sock.c#l24...
That warning is appropriate, because it's testing IPv6, so the warning applies in all cases. The reason I'm objecting in this case is that the warning is printed for any failure, no matter which chain is being checked, yet this condition can only occur on one of the 15 or so chains that are checked. Like I said, I think it's a special case.
How about an if ( got->dwErrorStatus = CERT_TRUST_IS_UNTRUSTED_ROOT ) ?
On Mon, May 4, 2009 at 2:43 PM, Austin English austinenglish@gmail.com wrote:
On Mon, May 4, 2009 at 12:19 PM, Juan Lang juan.lang@gmail.com wrote:
We have a similar warning already: http://source.winehq.org/git/wine.git/?a=blob;f=dlls/ws2_32/tests/sock.c#l24...
That warning is appropriate, because it's testing IPv6, so the warning applies in all cases. The reason I'm objecting in this case is that the warning is printed for any failure, no matter which chain is being checked, yet this condition can only occur on one of the 15 or so chains that are checked. Like I said, I think it's a special case.
How about an if ( got->dwErrorStatus = CERT_TRUST_IS_UNTRUSTED_ROOT ) ?
-- -Austin
Alternatively, how about printing an error in crypt32 itself if no trusted root certificates are found? That way applications get the same benefit, not just the test suite? E.g., how we do for ntlm_auth in secur32?
Alternatively, how about printing an error in crypt32 itself if no trusted root certificates are found? That way applications get the same benefit, not just the test suite? E.g., how we do for ntlm_auth in secur32?
That sounds like a good approach. --Juan
On Mon, May 4, 2009 at 4:57 PM, Juan Lang juan.lang@gmail.com wrote:
Alternatively, how about printing an error in crypt32 itself if no trusted root certificates are found? That way applications get the same benefit, not just the test suite? E.g., how we do for ntlm_auth in secur32?
That sounds like a good approach. --Juan
I'm a bit busy with other things, so time's a bit short. Would you mind pointing me in the right direction for the best approach for this? Or perhaps send a patch? <g>
I'm a bit busy with other things, so time's a bit short. Would you mind pointing me in the right direction for the best approach for this? Or perhaps send a patch? <g>
Sure. How's the attached look to you? Should it be an ERR instead? What text would you find more helpful? --Juan
On Mon, May 4, 2009 at 8:51 PM, Juan Lang juan.lang@gmail.com wrote:
I'm a bit busy with other things, so time's a bit short. Would you mind pointing me in the right direction for the best approach for this? Or perhaps send a patch? <g>
Sure. How's the attached look to you? Should it be an ERR instead?
That was quick, thanks! Yes, please make it an error. It shoudn't happen very often, and it can be pretty fatal, for, e.g., iTunes.
What text would you find more helpful?
How about?
ERR("didn't find any trusted root certificates. Are they installed?\n");
How about?
ERR("didn't find any trusted root certificates. Are they installed?\n");
Done, and sent. Thanks for the suggestion and the help. --Juan
-
Austin English -
Juan Lang