X.509-related cleanup

Clean-up style issues in net:: related to X.509 data-types, eliminating unnecessary includes and marking platform-specific implementations as such. Also clearly document that CertPrincipal::Matches() is not suitable for security-relevant name checks.

In addition, because x509_cert_types.h no longer includes base/singleton.h, fix all the classes that broke because they weren't including what they used.

BUG=none
TEST=existing

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66941

[Ryan Sleevi, 2010-11-17 09:19:03]

bulach: Would you mind taking a look? This is (almost) purely a style clean-up
patch, with no functional changes except for two: CertPrincipal::Matches was
moved to be Mac only and the operator<< overload for CertPrincipal was removed.

Matches() needs to go away: It suggests one thing (oh, an X509Certificate has a
CertPrincipal subject() and issuer()? I can just use Matches() to make security
decisions), when in fact, it's something very, very basic and very much
insecure. It was intended only for Mac, to do with platform limitations in
selecting client certificates, where a false positive isn't generally a
fatal/security error. Longer term, Matches() will disappear once the
SSLClientSocket*'s are harmonized, as Mac will use NSS to do the actual parsing
and (secure) comparison for client certificate chain building.

operator<< was removed because it's not used. It also has the side benefit of
solving an annoying C++ issue where net::operator<<(const std::ostream&, const
CertPrincipal&) was hiding the global operator<<const std::ostream&, const
wchar_t*) that base/logging.h defines.

Everything else should just be shuffling and shaking out some dependencies that
turned up.

wtc: Just FYI, an even easier solution than the using ::operator<< was just to
remove it, and once I got started, I couldn't stop :)

[bulach, 2010-11-17 12:16:50]

LGTM

I love the smell of red patches :)
(but not so much when it also paints the trybots.. :)

great cleanup, and whilst here, a few nits and suggestions (not on your code,
but since we're moving it anyway, seems a good time for tidying it up..). also,
please look at the bot logs.

http://codereview.chromium.org/5162001/diff/13001/net/base/x509_cert_types_ma...
File net/base/x509_cert_types_mac.cc (right):

http://codereview.chromium.org/5162001/diff/13001/net/base/x509_cert_types_ma...
net/base/x509_cert_types_mac.cc:125: for (size_t i = 0; i < length; i++)
nit: ++i

http://codereview.chromium.org/5162001/diff/13001/net/base/x509_cert_types_ma...
net/base/x509_cert_types_mac.cc:149: void SetSingle(const
std::vector<std::string> &values,
nit: s/> &values/>& values/

http://codereview.chromium.org/5162001/diff/13001/net/base/x509_cert_types_ma...
net/base/x509_cert_types_mac.cc:150: std::string* single_value) {
nit:unindent

http://codereview.chromium.org/5162001/diff/13001/net/base/x509_cert_types_ma...
net/base/x509_cert_types_mac.cc:157: bool match(const std::string &str, const
std::string &against) {
nit: s/string &/string& / for both params

http://codereview.chromium.org/5162001/diff/13001/net/base/x509_cert_types_ma...
net/base/x509_cert_types_mac.cc:164: const std::vector<std::string> &rdn2) {
nit: s/> &rd/>& rd/ for the two above

http://codereview.chromium.org/5162001/diff/13001/net/base/x509_cert_types_ma...
net/base/x509_cert_types_mac.cc:302: match(common_name, against.common_name) &&
301 and 302 are redundant

http://codereview.chromium.org/5162001/diff/13001/net/base/x509_cert_types_un...
File net/base/x509_cert_types_unittest.cc (right):

http://codereview.chromium.org/5162001/diff/13001/net/base/x509_cert_types_un...
net/base/x509_cert_types_unittest.cc:216: #if defined(OS_MACOSX)  // These
should only be implemented on Mac.
maybe we should rename this file to x509_cert_types_mac_unittest.cc ? it seems
there are only mac tests here..

[wtc, 2010-11-17 21:31:53]

LGTM.  I'm glad you can simply remove operator<<.

http://codereview.chromium.org/5162001/diff/13001/chrome/browser/dom_ui/const...
File chrome/browser/dom_ui/constrained_html_ui.cc (right):

http://codereview.chromium.org/5162001/diff/13001/chrome/browser/dom_ui/const...
chrome/browser/dom_ui/constrained_html_ui.cc:7: #include "base/singleton.h"
Nit: why is this change part of this CL?  Just to save the
trouble of creating a separate CL?  (I'm just curious...)

This comment also applies to several other  files in this CL.

http://codereview.chromium.org/5162001/diff/13001/net/base/x509_cert_types.h
File net/base/x509_cert_types.h (right):

http://codereview.chromium.org/5162001/diff/13001/net/base/x509_cert_types.h#...
net/base/x509_cert_types.h:9: #include "build/build_config.h"
There is a new Chromium-specific recommendation on how to
include "build/build_config.h" and include platform-specific
headers.  Please consult

http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

and

http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code

http://codereview.chromium.org/5162001/diff/13001/net/base/x509_cert_types_ma...
File net/base/x509_cert_types_mac.cc (left):

http://codereview.chromium.org/5162001/diff/13001/net/base/x509_cert_types_ma...
net/base/x509_cert_types_mac.cc:55: void CertPrincipal::Parse(const
CSSM_X509_NAME* name) {
PERSONAL OPINION: when a file becomes stable, it may not be
a good idea to move large blocks of code around to conform
to the Style Guide's recommendation of same
declaration/definition order, because it generates noise
in the diffs.

I am willing to live with imperfect matching of
declaration and definition orders, in exchange for diffs
that show me only what actually was changed.

Since you've already made the change and bulach has
reviewed it, no need to change it back.

http://codereview.chromium.org/5162001/diff/13001/net/base/x509_cert_types_un...
File net/base/x509_cert_types_unittest.cc (right):

http://codereview.chromium.org/5162001/diff/13001/net/base/x509_cert_types_un...
net/base/x509_cert_types_unittest.cc:341: #endif
Nit: This ifdef spans a lot of code, so it would be nice to
add a comment to #endif to note the macro that is tested.

http://codereview.chromium.org/5162001/diff/13001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/5162001/diff/13001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:58: // identifiers for cipher suites
(Security.framework uses an enum, NSS uses a
I believe the only conflicting identifier is
SSL_NULL_WITH_NULL_NULL.  Can you confirm that?
That would be useful info to note here.

[Ryan Sleevi, 2010-11-18 08:21:31]

Thanks. Comments and feedback addressed in patchset 3. Depending on the trybots,
there may be another patchset, but the extent should only be adding singleton
for more dependents.

http://codereview.chromium.org/5162001/diff/13001/chrome/browser/dom_ui/const...
File chrome/browser/dom_ui/constrained_html_ui.cc (right):

http://codereview.chromium.org/5162001/diff/13001/chrome/browser/dom_ui/const...
chrome/browser/dom_ui/constrained_html_ui.cc:7: #include "base/singleton.h"
On 2010/11/17 21:31:53, wtc wrote:
> Nit: why is this change part of this CL?  Just to save the
> trouble of creating a separate CL?  (I'm just curious...)
> 
> This comment also applies to several other  files in this CL.

In removing it from x509_cert_types.h, all the files affected began breaking,
because they failed to explicitly include it. I could fix them separately, but
since they're only broken by this change, it seemed correct to do it here.

http://codereview.chromium.org/5162001/diff/13001/net/base/x509_cert_types_ma...
File net/base/x509_cert_types_mac.cc (left):

http://codereview.chromium.org/5162001/diff/13001/net/base/x509_cert_types_ma...
net/base/x509_cert_types_mac.cc:55: void CertPrincipal::Parse(const
CSSM_X509_NAME* name) {
On 2010/11/17 21:31:53, wtc wrote:
> PERSONAL OPINION: when a file becomes stable, it may not be
> a good idea to move large blocks of code around to conform
> to the Style Guide's recommendation of same
> declaration/definition order, because it generates noise
> in the diffs.
> 
> I am willing to live with imperfect matching of
> declaration and definition orders, in exchange for diffs
> that show me only what actually was changed.
> 
> Since you've already made the change and bulach has
> reviewed it, no need to change it back.

It was more about pushing this code into the anonymous namespace than the
ordering. Because the CertPrincipal function was smack in the middle, it had to
be shifted out, and unfortunately the diff didn't take that well.

So I can better understand, is the concern in general against moving code as
part of cleanup? Or even more broadly, cleanup tasks? I know the Google Style
Guide lists it as acceptable to disregard the standard when working existing
non-conformant code, what I wasn't sure is where it fits for things like this
situation: where several files in net/base/ diverge for no discernible reason
from the pattern of the rest of the files next to it and from the Style Guide in
general (the Mac cleanup, the unittests, and this). Are these changes generally
not desirable?

http://codereview.chromium.org/5162001/diff/13001/net/base/x509_cert_types_ma...
File net/base/x509_cert_types_mac.cc (right):

http://codereview.chromium.org/5162001/diff/13001/net/base/x509_cert_types_ma...
net/base/x509_cert_types_mac.cc:302: match(common_name, against.common_name) &&
On 2010/11/17 12:16:50, bulach wrote:
> 301 and 302 are redundant

Thanks! Good catch!

http://codereview.chromium.org/5162001/diff/13001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/5162001/diff/13001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:58: // identifiers for cipher suites
(Security.framework uses an enum, NSS uses a
On 2010/11/17 21:31:53, wtc wrote:
> I believe the only conflicting identifier is
> SSL_NULL_WITH_NULL_NULL.  Can you confirm that?
> That would be useful info to note here.

No, it's all the cipher suites that follow the standard names. sslproto.h of NSS
defines them for NSS, <Security/CipherSuite.h> (included by
<Security/Security.h>) defines them for SecureTransport. The compiler is just
reporting the first error.

I went with explicitly specifying the headers to include, since it seems that
pattern is accepted/preferred elsewhere in the codebase with things like Cocoa
or Foundation, so it would seem acceptable here for Security.framework to avoid
pulling in all of Security.h

[wtc, 2010-11-18 23:12:41]

LGTM.

Keep the #include "base/singleton.h" changes in this CL.
It wasn't obvious to me they're necessitated by a change
in this CL; I thought they're unrelated changes.

Re: cleanup tasks: certain cleanup tasks generate diffs so
large that most code reviewers would only have the patience
of doing a "rubber stamp" code review.  So those cleanup
tasks carry an additional risk.  If the benefit of the
cleanup task is low, such as merely to bring the declaration
and definition orders to match, the risk may not be worth
taking.  But this is just my personal opinion, and you can
always cite the Style Guide to overrule this opinion.

http://codereview.chromium.org/5162001/diff/28001/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/5162001/diff/28001/net/net.gyp#newcode869
net/net.gyp:869: 'base/x509_cert_types_mac_unittest.cc',
Does renaming the file this way automatically make it a
Mac-only file?

[Ryan Sleevi, 2010-11-18 23:17:14]

Thanks. I'll look to land sometime this weekend.

http://codereview.chromium.org/5162001/diff/28001/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/5162001/diff/28001/net/net.gyp#newcode869
net/net.gyp:869: 'base/x509_cert_types_mac_unittest.cc',
On 2010/11/18 23:12:46, wtc wrote:
> Does renaming the file this way automatically make it a
> Mac-only file?

Yes, build\common.gypi has an exclude glob setup on lines 622-626. Relevant is
the first exclude rule:

          ['OS!="mac"', {
            'sources/': [ ['exclude', '_(cocoa|mac)(_unittest)?\\.(h|cc)$'],
                          ['exclude', '(^|/)(cocoa|mac)/'],
                          ['exclude', '\\.mm?$' ] ],
          }],