[ios] Removed CertVerifierBlockAdapter.

net/cert/sec_trust_util.h

Index: net/cert/sec_trust_util.h
diff --git a/net/cert/sec_trust_util.h b/net/cert/sec_trust_util.h
new file mode 100644
index 0000000000000000000000000000000000000000..b8ef3ff21a8875cb4aaace500984626833d56438
--- /dev/null
+++ b/net/cert/sec_trust_util.h
@@ -0,0 +1,20 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef NET_CERT_SEC_TRUST_UTIL_H_
+#define NET_CERT_SEC_TRUST_UTIL_H_

[Ryan Sleevi, 2016/08/08 17:47:37]

Why are you introducing this file?

I'm particularly hostile towards _util files (because they grow and grow), and
am curious why this can't/doesn't say where it was before. That is, it's unclear
from the description & initial message, as well the preliminary review, the
design necessity of splitting this out, or the alternatives considered.

That's not to say it may not be warranted, but I'm trying to understand more the
decision here, because ideally, we'd avoid it if possible :)

[Eugene But (OOO till 7-30), 2016/08/08 20:22:23]

I don't think that sec_trust_util.h will grow, because its name sets certain
limitation to what should be added. 
Here are a few pros in extracting a separate file:
 - Clients will not need to include extra stuff (cert_verify_proc_ios.h has
CertVerifyProcIOS which is not useful for crw_cert_verification_controller)
 - GetCertFailureStatusFromTrust does not conceptually belong to
CertVerifyProcIOS class API

If you strongly believe that GetCertFailureStatusFromTrust should live in
cert_verify_proc_ios.h I will move it back there. Having it in a separate file
breaks compilation (cert_verify_proc_ios is not compiled on other platforms
probably because of ios suffix),  so getting rid of sec_trust_util will make
things simpler :)

[Ryan Sleevi, 2016/08/08 20:44:18]

It deals with SecTrust, that's a pretty big API :)
Well, they still need to include extra stuff (just sec_trust_util now)
That second one doesn't really feel like a 'pro'. Hidden behind it is the
statement that you don't believe it belongs to CertVerifyProcIOS, but what I was
trying to get at with the review feedback is that it's not clear to me why you
feel that way.
Put differently, there's still a piece of this question missing, and I'm having
to try to glean it from the code, rather than your explanation :)

I'm trying to understand why you need GetCertFailureStatusFromTrust.

In the old world, it was in CertVerifyProcIOS, which did the work.
crw_cert_verification_controller depended on CVPIOS to explain additional
information about the failure.

You're removing CVPIOS, and thus losing the ability to explain why the failure
happened. It seems like you're now introducing this abstraction so that
crw_c_v_c doesn't have to depend on CVPIOS (header), but it's unclear why you
need to do that, or what you considered.

The concern here, which I only briefly touched on:
- _util files tend to grow
- This doesn't have any indication it's iOS specific (SecTrust is also used on
OS X, but we *wouldn't* want this method used there)
- There's still an implicit tight coupling between
crw_cert_verification_controller and CertVerifyProciOS (we *want* them to
produce same output), but that coupling is now abstracted away through a util
file that may not yield the same results.


With respect to the crw_c_v_c changes, what is your goal? To simply map to a
CertStatus/net_error code? Why (for the error bypassing?). Should we be using
SecTrustSetExceptions/CopyExceptions instead, if we're going all-in for
SecTrust? Is it because the same SSL error handler will be used by stuff that
goes through crw_c_v_c and through CertVerifyProciOS (do we have such
situations? should we?)

There's a lot of design rationale hidden behind the design, and I'm just trying
to better understand what your constraints were. My gut is that we could simply
have a static method on CertVerifyProcIOS, which would at least express the
coupling much better (in that crw_c_v_c would have to call that static method),
but there's broader aspects to unwinding the code that I'm unclear of, and that
this subtle change highlights :)

+
+#include "net/cert/cert_status_flags.h"
+
+#include <Security/Security.h>
+
+namespace net {
+
+// Returns error CertStatus from the given |trust| object. Returns
+// CERT_STATUS_INVALID if the trust is null.
+CertStatus GetCertFailureStatusFromTrust(SecTrustRef trust);
+
+}  // namespace net
+
+#endif  // NET_CERT_SEC_TRUST_UTIL_H_