Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1903453003:
Fix CertVerifyProcTest.PaypalNullCertParsing on iOS SDK 9.3. (Closed)
|
Created:
4 years, 8 months ago by sdefresne Modified:
4 years, 8 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews, ramant (doing other things) Base URL:
https://chromium.googlesource.com/chromium/src.git@{interstitial} Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix CertVerifyProcTest.PaypalNullCertParsing on iOS SDK 9.3.
The error returned by Verify() changed on simulator running iOS
SDK 9.3 or later, so change the test expectation.
BUG=None
Patch Set 1 #
Messages
Total messages: 14 (5 generated)
sdefresne@chromium.org changed reviewers: + rtenneti@chromium.org
Please take a look.
Description was changed from ========== Fix CertVerifyProcTest.PaypalNullCertParsing on iOS SDK 9.3. The error returned by Verify() changed on simulator running iOS SDK 9.3 or later, so change the test expectation. BUG=None ========== to ========== Fix CertVerifyProcTest.PaypalNullCertParsing on iOS SDK 9.3. The error returned by Verify() changed on simulator running iOS SDK 9.3 or later, so change the test expectation. BUG=None ==========
rtenneti@google.com changed reviewers: + davidben@chromium.org - rtenneti@chromium.org
rtenneti@google.com changed reviewers: + rsleevi@chromium.org
rtenneti@google.com changed reviewers: + rtenneti@google.com
lgtm
I'm not comfortable with this fix; it papers over the root issue, and as a result, causes the test not to cover what it's meant to test. In this case, I'd be more comfortable if you disabled the test, and filed a bug - because then it's at least clear this test is missing coverage.
On 2016/04/20 22:45:03, Ryan Sleevi wrote: > I'm not comfortable with this fix; it papers over the root issue, and as a > result, causes the test not to cover what it's meant to test. > > In this case, I'd be more comfortable if you disabled the test, and filed a bug > - because then it's at least clear this test is missing coverage. Is there actually anything to file a bug about? It seems newer iOS SDKs behave correctly and while older ones didn't. (I'd buy skipping the test in the old SDK case though.)
On 2016/04/20 23:03:35, davidben wrote: > On 2016/04/20 22:45:03, Ryan Sleevi wrote: > > I'm not comfortable with this fix; it papers over the root issue, and as a > > result, causes the test not to cover what it's meant to test. > > > > In this case, I'd be more comfortable if you disabled the test, and filed a > bug > > - because then it's at least clear this test is missing coverage. > > Is there actually anything to file a bug about? It seems newer iOS SDKs behave > correctly and while older ones didn't. (I'd buy skipping the test in the old SDK > case though.) No, the newer SDKs behave incorrectly - they're treating it as a "CA trust issue", rather than an "invalid certificate issue". This suggests that the issuing CA is no longer trusted (perhaps a lack of intermediates to chain to a newer root), rather than as being a fundamental encoding issue. That's the concern - we're no longer sure we're actually testing the certificate as rejected, but instead that the CA is not trusted - which means we're not actually sure that an (otherwise trusted) embedded-null cert would be rejected. One approach to fix this would be to generate a local CA with a forcibly-embedded NUL, rather than relying on the current test certificate (which can cause issues like this when the underlying root store changes, as it did)
On 2016/04/20 23:13:44, Ryan Sleevi wrote: > On 2016/04/20 23:03:35, davidben wrote: > > On 2016/04/20 22:45:03, Ryan Sleevi wrote: > > > I'm not comfortable with this fix; it papers over the root issue, and as a > > > result, causes the test not to cover what it's meant to test. > > > > > > In this case, I'd be more comfortable if you disabled the test, and filed a > > bug > > > - because then it's at least clear this test is missing coverage. > > > > Is there actually anything to file a bug about? It seems newer iOS SDKs behave > > correctly and while older ones didn't. (I'd buy skipping the test in the old > SDK > > case though.) > > No, the newer SDKs behave incorrectly - they're treating it as a "CA trust > issue", rather than an "invalid certificate issue". This suggests that the > issuing CA is no longer trusted (perhaps a lack of intermediates to chain to a > newer root), rather than as being a fundamental encoding issue. That's the > concern - we're no longer sure we're actually testing the certificate as > rejected, but instead that the CA is not trusted - which means we're not > actually sure that an (otherwise trusted) embedded-null cert would be rejected. > > One approach to fix this would be to generate a local CA with a > forcibly-embedded NUL, rather than relying on the current test certificate > (which can cause issues like this when the underlying root store changes, as it > did) I see. Then we should have filed a bug ages ago, no? The non-simulator case (what we actually care about), Mac, and Windows already all use ERR_CERT_AUTHORITY_INVALID (line 241 on the RHS). It's just NSS that was doing anything different before this iOS codepath. The new SDK's simulator is correct in that it correctly matches the real result.
On 2016/04/20 23:19:07, davidben wrote: > I see. Then we should have filed a bug ages ago, no? Correct, we should have. > The non-simulator case > (what we actually care about), Mac, and Windows already all use > ERR_CERT_AUTHORITY_INVALID (line 241 on the RHS). It's just NSS that was doing > anything different before this iOS codepath. Right, it appears this test was inappropriately "fixed" (based on the real world) to no longer cover what it was supposed to be testing. > The new SDK's simulator is correct in that it correctly matches the real result.
On 2016/04/20 at 23:20:48, rsleevi wrote: > On 2016/04/20 23:19:07, davidben wrote: > > I see. Then we should have filed a bug ages ago, no? > > Correct, we should have. I've created https://bugs.chromium.org/p/chromium/issues/detail?id=605457 and will write a CL to disable the test. > > The non-simulator case > > (what we actually care about), Mac, and Windows already all use > > ERR_CERT_AUTHORITY_INVALID (line 241 on the RHS). It's just NSS that was doing > > anything different before this iOS codepath. > > Right, it appears this test was inappropriately "fixed" (based on the real world) to no longer cover what it was supposed to be testing. > > > The new SDK's simulator is correct in that it correctly matches the real result.
Message was sent while issue was closed.
Closing as this is not the correct fix. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
