[refactor] Extract the CertVerifyResult assignment of has_md2, has_md4,

net/cert/cert_verify_proc.h

Index: net/cert/cert_verify_proc.h
diff --git a/net/cert/cert_verify_proc.h b/net/cert/cert_verify_proc.h
index 352610f22e54d073a735b790eb568be1fc839bff..80d1f685566b4248e119c2e467288f0a54e0a000 100644
--- a/net/cert/cert_verify_proc.h
+++ b/net/cert/cert_verify_proc.h
@@ -14,6 +14,7 @@
 #include "base/memory/ref_counted.h"
 #include "net/base/net_export.h"
 #include "net/cert/x509_cert_types.h"
+#include "net/cert/x509_certificate.h"
 
 namespace net {
 
@@ -134,6 +135,21 @@ class NET_EXPORT CertVerifyProc
   DISALLOW_COPY_AND_ASSIGN(CertVerifyProc);
 };
 
+// Sets the weak signature hash fields of |verify_result| to true if
+// applicable for |cert|, otherwise does not modify them.
+//
+// The fields in question are: |has_md2|, |has_md4|, |has_md5|,|has_sha1| and
+// |has_sha1_leaf|.
+//
+// Returns true if any of the fields were set to true (meaning the
+// certificate uses a weak signature).
+//
+// This function is intended to be used as a helper by platform-specific
+// CertVerifyProc implementations.
+bool FillCertVerifyResultWeakSignature(X509Certificate::OSCertHandle cert,

[Ryan Sleevi, 2017/01/05 22:48:24]

DESIGN: From the header, it's unclear why the bool return value would be
meaningful/significant at all (versus making use of the |verify_result| flags
themselves). It feels like this becomes two things - a computation and a policy
decision.

[Ryan Sleevi, 2017/01/05 22:48:24]

DESIGN: Does this need to be a free function in the public header? Why not make
it a static (protected) method and friend the test?

[Ryan Sleevi, 2017/01/05 22:48:24]

NAMING: The "FillCertVerifyResult" feels redundant with the method signature
(being that it takes a CertVerifyResult)?

static bool CheckWeakSignature(...) ?

[eroman, 2017/01/05 23:21:53]

RE: Static method vs free-floating function
The consumers of this function are all in free-floating helper functions, so
they cannot access a protected method. I could make it a public static method,
but that mostly just makes for a longer name.

RE: bool return value
This is used only by the mac verifier, which fills a boolean if a weak signature
is used. If I use the same function signature but without the return value the
consumer needs to check each of the boolean parameters to see if any of them
were set to true, which is a bit tricky/gross.

How about instead of a bool I return the hash algorithm enum? The mac
implementation can then do a switch on that and decide whether to set the "weak
signature" bool, and the other implementations can disregard it.

[eroman, 2017/01/05 23:36:30]

No longer returns a bool -- now returns the algorithm enum.

[eroman, 2017/01/05 23:36:30]

I haven't changed the name yet.

I am not sure about CheckWeakSignature() given that it no longer returns a bool

[Ryan Sleevi, 2017/01/05 23:46:30]

I'm wondering whether we could/should consider uplifting the call to this (to
populate the CertVerifyResult) into CertVerifyProc::Verify (that is, moving it
out of the respective VerifyInternal implementations)

That would/should only affect the macOS implementation, which is checking to see
if the chain is weak (in which case, it triggers the extra building logic), but
that code doesn't care about has_md* so much as knowing that the enum was
something !other, and whether that was because of the leaf or the chain.

That's a larger refactoring than perhaps you intended, but might also  solve the
bool return value thing.

Concretely, I'm thinking of something like:
SignatureHashAlgorithm X509Certificate::GetSignatureHashAlgorithm(...)

void CertVerifyProc::FillWeakKeyInfo(X509Certificate* cert, CertVerifyResult*
verify_result) {
  SomeHelperMethod(cert->os_cert_handle(), verify_result);
  verify_result->has_sha1_leaf = verify_result->has_sha1;
  for (const auto& chain_cert : cert->GetIntermediateCertificates()) {
    SomeHelperMethod(chain_cert, verify_result);
  }
}

Where SomeHelperMethod just does enum -> bool flag flips


And then we'd call that somewhere around
https://cs.chromium.org/chromium/src/net/cert/cert_verify_proc.cc?rcl=0&l=469 or
https://cs.chromium.org/chromium/src/net/cert/cert_verify_proc.cc?rcl=0&l=456

Now, for the macOS code, which needs to know whether the chain is weak, we'd
update the logic in
https://cs.chromium.org/chromium/src/net/cert/cert_verify_proc_mac.cc?rcl=0&l...
to do something like:

bool weak_chain = false;
for (CFIndex i = 1 /* not 0 */, size = CFArrayGetCount(temp_chain); i < size;
++i) {
  weak_chain |=
(X509Certificate::X509Certificate::GetSignatureHashAlgorithm(CFArrayGetValueAtIndex(temp_chain,
i)) != X509Certificate::kSignatureHashAlgorithmOther);
}


That would/should allow us to fully 'hide' a lot of these details. WDYT?

[eroman, 2017/01/06 00:47:23]

That sounds plausible, however TBH I am trying to avoid too much scope creep, as
this refactor is already 2 tasks removed from what I actually want to be getting
done :)

What you described sounds like it could be an iterative change that builds on
top of this code extraction.

What if I proceeded with the change at this layer, then the accompanying
unit-test simplification, (and then the parameterization change I have in
progress), and then can circle back to improvements that internalize this into
the shared cert verify proc code? Or do you feel strongly this needs to be
tackled in this iteration?

+                                       bool is_leaf,
+                                       CertVerifyResult* verify_result);
+
 }  // namespace net
 
 #endif  // NET_CERT_CERT_VERIFY_PROC_H_