Implement LoadTemporaryRoot for Windows

Add support for temporarily trusting a certificate for the duration of unit tests on Windows, rather than requiring the machine to be pre-configured out-of-band.

Given the lack of a Microsoft-provided high-level API to supply application-level trusts to the verification routines, this implements a workaround that intercepts attempts to open the trusted system root store and injects the test certificates directly. This allows the unit tests to work without requiring that the Test CA be added to the machine's Trusted Certificates store.

While doing so, clean up the interface to adding/removing trusted test certificates, so as to support more than one trusted certificate if necessary.

BUG=8470
TEST=To follow

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

[Ryan Sleevi, 2010-11-07 11:08:48]

@wtc: Here's the CL I mentioned earlier this week. Everything is green here on
my machine - I've removed the "Test CA" from my list of Trusted Roots and the
unit tests are still passing on Windows. Naturally it will pass on the Try Bots,
as they have the certificate added; if you know a way to (temporarily?) remove
it on one and/or find a special win-bot, please let me know.

@joth/bulach/phajdan.jr: FYI, since this CL has touched on previously discussed
areas.

http://codereview.chromium.org/4646001/diff/4001/5012
File net/net.gyp (right):

http://codereview.chromium.org/4646001/diff/4001/5012#newcode762
net/net.gyp:762: ],
@joth/bulach: I had to add this to get it to compile with use_openssl=1. Perhaps
this should be split into a separate CL that one of you can own (for timing
sake)

[Ryan Sleevi, 2010-11-07 11:18:21]

I should add, as a "Known Issue", that because the temporary roots singleton is
used in X509Certificate::Verify(), it's vulnerable to the same race conditions
described in http://crbug.com/61753 . I've not addressed it in this CL because
LeakySingletonTraits here would cause Very Bad Things(tm), in the same way that
EnsureNSSInit/EnsureOpenSSLInit cannot be safely changed to
LeakySingletonTraits.

[Paweł Hajdan Jr., 2010-11-08 10:42:31]

Drive-by with minor test comment. No need to wait for another review by me. By
the way, nice little cleanup of the test server code!

http://codereview.chromium.org/4646001/diff/10001/11013
File net/test/test_server.cc (right):

http://codereview.chromium.org/4646001/diff/10001/11013#newcode318
net/test/test_server.cc:318: if
(!root_certs->AddFromFile(GetRootCertificatePath()))
nit: Why not just return root_certs->AddFromFile?

[joth, 2010-11-08 15:22:25]

just a comment on this one question

http://codereview.chromium.org/4646001/diff/4001/5012
File net/net.gyp (right):

http://codereview.chromium.org/4646001/diff/4001/5012#newcode762
net/net.gyp:762: ],
On 2010/11/07 11:08:48, rsleevi wrote:
> @joth/bulach: I had to add this to get it to compile with use_openssl=1.
Perhaps
> this should be split into a separate CL that one of you can own (for timing
> sake)

Seems fine to include this in this patch. (Not sure why I don't get the same
build error though).

[bulach, 2010-11-08 18:16:39]

thanks ryan!
I'll look in more detail shortly, just a quick note about the gyp:

http://codereview.chromium.org/4646001/diff/10001/11012
File net/net.gyp (right):

http://codereview.chromium.org/4646001/diff/10001/11012#newcode739
net/net.gyp:739: 'socket/ssl_client_socket_nss_factory.h',
it's probably simpler to add 760-761 here

[bulach, 2010-11-09 16:21:09]

wtc surely needs to review the actual impl, I know nothing about mac :)

however, a few overall comments / suggestions, specially about the new file
names and moving some parts from x509 into the temporary root certs:

http://codereview.chromium.org/4646001/diff/19001/20003
File net/base/temporary_root_certs.cc (right):

http://codereview.chromium.org/4646001/diff/19001/20003#newcode18
net/base/temporary_root_certs.cc:18: LOG(ERROR) << "Can't load certificate " <<
filename.value().c_str();
s/c_str()//

http://codereview.chromium.org/4646001/diff/19001/20003#newcode23
net/base/temporary_root_certs.cc:23: raw_cert.c_str(), raw_cert.length(),
X509Certificate::FORMAT_AUTO);
s/c_str()/data()/

http://codereview.chromium.org/4646001/diff/19001/20003#newcode40
net/base/temporary_root_certs.cc:40: return false;
if there's an error with N-th, should we remove 0..N-1 certificates that were
added?

http://codereview.chromium.org/4646001/diff/19001/20004
File net/base/temporary_root_certs.h (right):

http://codereview.chromium.org/4646001/diff/19001/20004#newcode29
net/base/temporary_root_certs.h:29: class TemporaryRootCerts {
this is only ever going to be used for tests, right?
the previous version under "cert_test_util" made it slightly clearer.
perhaps we could name these files / classes as TestRootCerts,
TemporaryRootCertsForTests, or something along this line?

http://codereview.chromium.org/4646001/diff/19001/20005
File net/base/temporary_root_certs_mac.cc (right):

http://codereview.chromium.org/4646001/diff/19001/20005#newcode21
net/base/temporary_root_certs_mac.cc:21:
reinterpret_cast<SecCertificateRef>(const_cast<void*>(value2)));
indent+2 the 20-21

http://codereview.chromium.org/4646001/diff/19001/20005#newcode24
net/base/temporary_root_certs_mac.cc:24: const void* DummyRetain(CFAllocatorRef
unused, const void* value) {
maybe RetainWrapper?

http://codereview.chromium.org/4646001/diff/19001/20005#newcode28
net/base/temporary_root_certs_mac.cc:28: void DummyRelease(CFAllocatorRef
unused, const void* value) {
maybe ReleaseWrapper?

http://codereview.chromium.org/4646001/diff/19001/20005#newcode35
net/base/temporary_root_certs_mac.cc:35: static CFArrayCallBacks
kCertArrayCallbacks = {
const? also, since it's in a local namespace, could remove static.

http://codereview.chromium.org/4646001/diff/19001/20006
File net/base/temporary_root_certs_nss.cc (right):

http://codereview.chromium.org/4646001/diff/19001/20006#newcode71
net/base/temporary_root_certs_nss.cc:71: TrustEntry
entry(certificate->os_cert_handle(), nss_trust);
could move this further down to 88

http://codereview.chromium.org/4646001/diff/19001/20007
File net/base/temporary_root_certs_openssl.cc (right):

http://codereview.chromium.org/4646001/diff/19001/20007#newcode40
net/base/temporary_root_certs_openssl.cc:40: // Either way seems messy for
something intended as a temporary test utility.
maybe:
LOG(WARNING) << "OpenSSL doesn't support removing temporary certs";

it may be helpful if we ever really rely on this on new tests in the future.

http://codereview.chromium.org/4646001/diff/19001/20008
File net/base/temporary_root_certs_win.cc (right):

http://codereview.chromium.org/4646001/diff/19001/20008#newcode38
net/base/temporary_root_certs_win.cc:38: : temporary_roots_(NULL),
cert_count_(0) {
nit: I think it's either all in one line (including the ctor decl) or one
initalization per line..

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

http://codereview.chromium.org/4646001/diff/19001/20009
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/4646001/diff/19001/20009#newcode535
net/base/x509_certificate_mac.cc:535: }
could we move 494-534 to something like 

status TemporaryRootCerts::SetAnchorCertificates(..) ?

it looks like this whole lot (only used for tests, right?) would fit there
nicely.

http://codereview.chromium.org/4646001/diff/19001/20011
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/4646001/diff/19001/20011#newcode301
net/base/x509_certificate_win.cc:301: void FixTestRootTrust(PCERT_CHAIN_CONTEXT
chain_context) {
same as above, I think this should be exposed somehow in the
temporary_root_certs rather than here.

http://codereview.chromium.org/4646001/diff/19001/20013
File net/test/test_server.cc (right):

http://codereview.chromium.org/4646001/diff/19001/20013#newcode316
net/test/test_server.cc:316: // TODO(dkegel): figure out how to get this to only
happen once?
afaict, this is no longer necessary as TemporaryRootCerts already takes care of
being called multiple times?

[wtc, 2010-11-16 20:26:29]

Somehow I missed this CL.  Sorry!  I will review it today.

[wtc, 2010-11-16 23:24:01]

rsleevi: thanks a lot for writing this CL.  Please see my
comments below.

Overall comments:

I'm disappointed that we can't trust a root certificate
temporarily on Windows.  The hack we have to use means our
unit tests will exercise different code paths, which will
diminish the value of the unit tests.  We should add a TODO
comment to implement the proper method for Windows 7 in the
future.

I suggested some changes to the API: AddFromFile should
add only one cert, and use a Clear/RemoveAll method instead
of Remove (of an individual root cert).

Specific comments follow.

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
File net/base/temporary_root_certs.cc (right):

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
net/base/temporary_root_certs.cc:14: namespace {
Nit: add a blank line after the beginning and before the end
of a namespace.

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
File net/base/temporary_root_certs.h (right):

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
net/base/temporary_root_certs.h:9: #include "base/singleton.h"
Nit: list "base/singleton.h" after all the other headers,
to conform to the Style Guide's recommendation as much as
we can.

Nit: include <windows.h> before <wincrypt.h>.

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
net/base/temporary_root_certs.h:29: class TemporaryRootCerts {
I agree with bulach's suggestion of naming this class
TestRootCerts to make it clear this class is used for tests
only.

Please add a concise block comment to describe what this class
does.

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
net/base/temporary_root_certs.h:31: // Obtain the Singleton instance to the
trusted certificates.
When documenting methods in a header file, please use
descriptive ("Obtains") rather than imperative ("Obtain").

See the Style Guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
net/base/temporary_root_certs.h:40: bool AddFromFile(const FilePath& file);
I commented on this issue before: it is better to only support
one certificate per file.  This comment also applies to
RemoveFromFile below, and the LoadCertificates method in
temporary_root_certs.cc.

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
net/base/temporary_root_certs.h:45: void Remove(X509Certificate* certificate);
Perhaps all we need is a Clear/RemoveAll method that removes
all test root certs?

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
net/base/temporary_root_certs.h:51: bool IsEmpty() const;
It is strange that IsEmpty() is defined for only some platforms.

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
net/base/temporary_root_certs.h:77: CertTrustMap cert_trust_map_;
You should document what you store in cert_trust_map_ is
the original trust.  This was not obvious until I read
temporary_root_certs_nss.cc.

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
File net/base/temporary_root_certs_mac.cc (right):

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
net/base/temporary_root_certs_mac.cc:36: 0, /* version */
Nit: use C++ style comment:
  0,  // version

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
File net/base/temporary_root_certs_nss.cc (right):

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
net/base/temporary_root_certs_nss.cc:40:
TemporaryRootCerts::TrustEntry::TrustEntry(const TrustEntry& entry)
It seems that this is exactly what the compiler can define
for us, so we don't need to define the copy constructor.

If it's necessary to define the copy constructor, remove
    certificate_(NULL)
from the initializer list.

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
net/base/temporary_root_certs_nss.cc:74: // TODO(port): remove this const_cast
after NSS 3.12.3 is released.
Nit: you can remove this TODO comment and the const_cast
because we require NSS 3.12.3 now.

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
net/base/temporary_root_certs_nss.cc:89:
cert_trust_map_.insert(std::make_pair(certificate->fingerprint(), entry));
Isn't this equivalent to
  cert_trust_map_[certificate->fingerprint()] = entry;
?

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
File net/base/temporary_root_certs_win.cc (right):

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
net/base/temporary_root_certs_win.cc:17: if (GetLastError() == CRYPT_E_EXISTS)
Nit: you can just say
  if (!ok) {
    // If certificate is already added, return successfully.
    return GetLastError() == CRYPT_E_EXISTS;
  }

http://codereview.chromium.org/4646001/diff/19001/net/base/x509_certificate_w...
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/4646001/diff/19001/net/base/x509_certificate_w...
net/base/x509_certificate_win.cc:705: scoped_collection_store ?
scoped_collection_store :
Nit: use a local variable for this, so that we don't need
to repeat this expression on line 723 below.

http://codereview.chromium.org/4646001/diff/19001/net/base/x509_certificate_w...
net/base/x509_certificate_win.cc:736:
FixTestRootTrust(const_cast<PCERT_CHAIN_CONTEXT>(chain_context));
BUG: Move this up, so that we call FixTestRootTrust before
using chain_context.  In particular, we must call
FixTestRootTrust before GetCertChainInfo.  Did I miss
something?

http://codereview.chromium.org/4646001/diff/19001/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/4646001/diff/19001/net/net.gyp#newcode734
net/net.gyp:734: 'ocsp/nss_ocsp.cc',
Is this a merge error?  This change doesn't belong in this
changelist.

[Ryan Sleevi, 2010-11-17 09:37:43]

Thanks wtc and bulach, I've uploaded a new patchset that should have all the
changes you requested integrated. I apologize, but it appears that Reitveld
cannot track renames of added files across patchsets, so there is no
side-by-side diff. If that poses a problem, please let me know.

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
File net/base/temporary_root_certs_nss.cc (right):

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
net/base/temporary_root_certs_nss.cc:40:
TemporaryRootCerts::TrustEntry::TrustEntry(const TrustEntry& entry)
My understanding is that the implicitly generated copy constructor calls the
copy constructor of member types, meaning it would simply copy the pointer,
rather than invoking our assignment operator (which increments the reference
count)

The NULL initializer is because the assignment constructor first calls
CERT_DestroyCertificate(certificate_), so that the current certificate is
released. Without the NULL, certificate_ may be uninitialized and access invalid
memory when freeing.

It's unnecessary now and is removed, as described below.

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
net/base/temporary_root_certs_nss.cc:71: TrustEntry
entry(certificate->os_cert_handle(), nss_trust);
On 2010/11/09 16:21:09, bulach wrote:
> could move this further down to 88

No, this copies |nss_trust|, which is then modified to the new value on line 75.
However, since there was confusion, I changed to use two CERTCertTrusts to
reduce it.

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
net/base/temporary_root_certs_nss.cc:89:
cert_trust_map_.insert(std::make_pair(certificate->fingerprint(), entry));
Depends :) Using operator[] for a map<Key, T> forces T to be
DefaultConstructible. If operator[] is not used, T is only required to be
Assignable and CopyConstructible.

Either way, I reverted to a simple std::list<> for NSS. With Remove() replaced
with a Clear(), it's no longer necessary to be able to quickly find a given
certificate.

http://codereview.chromium.org/4646001/diff/19001/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/4646001/diff/19001/net/net.gyp#newcode734
net/net.gyp:734: 'ocsp/nss_ocsp.cc',
On 2010/11/16 23:24:01, wtc wrote:
> Is this a merge error?  This change doesn't belong in this
> changelist.

See message #1. It was a necessary change to get this patchset to compile with
use_openssl=1. I just used this CL to flag the initial issue, since they had
expressed comments/concerns related to some of the other stuff. joth landed this
change (along with several other openssl compile fixes), so it's no longer part
of this patchset.

[joth, 2010-11-17 10:37:01]

Quick heads up now that I landed the openssl_utils refactor.
Cheers!

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
File net/base/temporary_root_certs_openssl.cc (right):

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
net/base/temporary_root_certs_openssl.cc:19: if
(!X509_STORE_add_cert(openssl_init->x509_store(),
note this will now need to be
X509_STORE_add_cert(X509Certificate::cert_store(),

http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_cer...
net/base/temporary_root_certs_openssl.cc:40: // Either way seems messy for
something intended as a temporary test utility.
On 2010/11/09 16:21:09, bulach wrote:
> maybe:
> LOG(WARNING) << "OpenSSL doesn't support removing temporary certs";
> 
> it may be helpful if we ever really rely on this on new tests in the future.

Would it help if we added something of a 'test mode' into
X509Certificate::Verify that allows you to inject a STACK_OF(X509) which is then
used rather than a X509_STORE i.e. using X509_STORE_CTX_trusted_stack?
A STACK_OF(X509) looks simpler to manipulate.

This would mean in test code (once 1 or more test certs are added) we will only
verify against those certs, and we exercise an ever so slightly different path
to in production, which is unfortunate but at least means the test cert store is
fully under the test code's control.

[joth, 2010-11-17 16:29:20]

correction to my previous comment.

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_op...
File net/base/test_root_certs_openssl.cc (right):

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_op...
net/base/test_root_certs_openssl.cc:26: } while ((error_code = ERR_get_error())
!= 0);
minor suggestion: you can now just call ClearOpenSSLERRStack()

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_op...
net/base/test_root_certs_openssl.cc:37: openssl_init->ReinitializeStore();
ah sorry I competely missed this when I sent the last comment.

So you'll need to poke this method through on X509Certificate now, rather than
OpenSSLInitSingleton. The logic should be pretty much the same though.

Sorry for the patch churn.

As an aside: can we / should we guard this call under "if (!empty_)"?

[bulach, 2010-11-17 17:17:30]

thanks, ryan! a fresh look at it gave me new insight, sorry I should've thought
about them earlier: basically, I suggested a few things to further separate real
code from test code, let me know if it makes sense.

plus a few nits below:

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs.cc
File net/base/test_root_certs.cc (right):

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs.cc...
net/base/test_root_certs.cc:17: using ::operator<<;
is this needed?

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_ma...
File net/base/test_root_certs_mac.cc (right):

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_ma...
net/base/test_root_certs_mac.cc:53: OurSecCertificateEqual
append ,

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_ns...
File net/base/test_root_certs_nss.cc (right):

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_ns...
net/base/test_root_certs_nss.cc:41: CERTCertTrust trust)
unindent

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_ns...
net/base/test_root_certs_nss.cc:43: trust_(trust) {}
I think } goes to the next line

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_op...
File net/base/test_root_certs_openssl.cc (right):

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_op...
net/base/test_root_certs_openssl.cc:46: : empty_(true) {}
nit: \n}

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_op...
net/base/test_root_certs_openssl.cc:48: TestRootCerts::~TestRootCerts() {}
nit: \n}

http://codereview.chromium.org/4646001/diff/56001/net/base/x509_certificate_m...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/4646001/diff/56001/net/base/x509_certificate_m...
net/base/x509_certificate_mac.cc:491: status =
root_certs->SetAnchorCertificates(trust_ref);
I know SetAnchorCertificates already checks, but I think at this point it'd be
clearer to comment that this is only for test code.

TestRootCerts* root_certs = TestRootCerts::GetInstance();
if (!root_certs->IsEmpty()) {
  // We're running tests, set the anchor certificates.
  status = root_certs->SetAnchorCertificates(trust_ref);
  if (status)
    return NetErrorFromOSStatus(status);
}

it may be even clearer to have this under a 
if (TestRootCerts::HasInstance()) {
  ...
}

and require that tests that needs this would initialize the singleton
beforehand. thoughts?

http://codereview.chromium.org/4646001/diff/56001/net/base/x509_certificate_w...
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/4646001/diff/56001/net/base/x509_certificate_w...
net/base/x509_certificate_win.cc:619: // search both.
similarly here.
my (ignorable) suggestion both here and mac is to try to separate test code from
production code. right now, it's not really straightforward.
maybe even split this into:
HCERTSTORE CreateStoreForTestsIfNeeded(X509Certificate::OSCertHandle handle) {
...
}

so then it become:

ScopedHCERTSTORE scoped_test_store(CreateStoreForTestsIfNeeded(cert_handle_));

HCERTSTORE intermediates_store = scoped_test_store.get() ?
  scoped_test_store.get() : cert_handle_->hCertStore;

[wtc, 2010-11-17 19:44:39]

rsleevi: I haven't reviewed Patch Set 7 yet.  Let me first
send you some comments that I thought of last night.

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_wi...
File net/base/test_root_certs_win.cc (right):

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_wi...
net/base/test_root_certs_win.cc:57: // manual, brute-force method is used for
unit tests. Look through every
Could you please add a short version of your description of
the algorithm in
http://code.google.com/p/chromium/issues/detail?id=8470#c3 ?
In particular, that our solution is inspired by the way
Outlook and EFS uses the Trusted People” store as described in
http://technet.microsoft.com/en-us/library/bb457027.aspx.

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_wi...
net/base/test_root_certs_win.cc:86: CERT_TRUST_REVOCATION_STATUS_UNKNOWN |
Two comments about the revocation error flags.

1. I think we should not clear CERT_TRUST_REVOCATION_STATUS_UNKNOWN
and CERT_TRUST_IS_OFFLINE_REVOCATION.  I believe
CERT_TRUST_REVOCATION_STATUS_UNKNOWN is set not because
the test root cert is untrusted, but because the test
server cert doesn't have a CRL distribution point and
AIA extension with OCSP responder URL.
(CERT_TRUST_IS_OFFLINE_REVOCATION is always set if
CERT_TRUST_REVOCATION_STATUS_UNKNOWN is set, in my experience.)

2. I believe our hack of using an untrusted test root cert
and ignoring CERT_TRUST_IS_UNTRUSTED_ROOT will work for
most unit tests, but won't work for a unit test on
revocation, because CryptoAPI won't be able to verify a
CRL or OCSP response signed by the test root CA.

Do you agree?

[wtc, 2010-11-18 02:12:47]

LGTM.

http://codereview.chromium.org/4646001/diff/56001/net/base/openssl_util.h
File net/base/openssl_util.h (right):

http://codereview.chromium.org/4646001/diff/56001/net/base/openssl_util.h#new...
net/base/openssl_util.h:51: // any modifications that TestRootCerts may have
done, by initializing
Nit: remove the parentheses in "(Re-)initializes".  Alternatively,
name this method "InitializeStore" and just say
"Initializes |store_| to the default state."

Nit: remove ", by initializing |store_| to the default state",
which repeats the first sentence.

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs.h
File net/base/test_root_certs.h (right):

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs.h#...
net/base/test_root_certs.h:11: #include "base/singleton.h"
Nit: I believe brettw's recommendation is to simply include
"build/build_config.h" in alphabetical order.  Here we would
do:
  #include "base/singleton.h"
  #include "build/build_config.h"

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs.h#...
net/base/test_root_certs.h:67: void UpdateChainContext(PCERT_CHAIN_CONTEXT
chain_context) const;
Nit: rename this method FixupChainContext.  "Update" is
not clear.

http://codereview.chromium.org/4646001/diff/56001/net/base/x509_certificate_m...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/4646001/diff/56001/net/base/x509_certificate_m...
net/base/x509_certificate_mac.cc:491: status =
root_certs->SetAnchorCertificates(trust_ref);
Nit: I find this harder to understand because this looks
like an operation on root_certs, but this actually acts
on trust_ref.  The original code shows that more clearly.

Two possible solutions:
1. Rename the method:
  status = root_certs->SetAnchorCertificatesOnSecTrust(trust_ref);
2. Go back to the style of the original code:
  CFArrayRef anchor_array = root_certs->CopyRootCertArray();
  status = SecTrustSetAnchorCertificates(trust_ref, anchor_array);

[Ryan Sleevi, 2010-11-18 05:31:57]

wtc, bulach: Thanks for the feedback. I've integrated what you asked for in
patchset 8, except where mentioned below.

joth: Thanks for the patch sent via e-mail. I've rebased to trunk and worked in
your changes for patchset 9, although with some slight modifications. Could you
please check over to make sure you're ok with them. I expect to land this on the
weekend, so hopefully that should not be too long for the patchset you
mentioned.

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs.cc
File net/base/test_root_certs.cc (right):

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs.cc...
net/base/test_root_certs.cc:17: using ::operator<<;
On 2010/11/17 17:17:30, bulach wrote:
> is this needed?

Until http://codereview.chromium.org/5162001/ lands, unfortunately.

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_ns...
File net/base/test_root_certs_nss.cc (right):

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_ns...
net/base/test_root_certs_nss.cc:43: trust_(trust) {}
On 2010/11/17 17:17:30, bulach wrote:
> I think } goes to the next line

Thanks. wtc had commented in the past about empty methods, and it looks like I
overapplied this to rule multi-line ctors. I think you're right, judging by the
style erg used for the FBTF cleanup, http://crrev.com/61100

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_op...
File net/base/test_root_certs_openssl.cc (right):

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_op...
net/base/test_root_certs_openssl.cc:46: : empty_(true) {}
On 2010/11/17 17:17:30, bulach wrote:
> nit: \n}

Looking at
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...
, it looks like sticking this all on one line is fine, which is what I went
with.

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_op...
net/base/test_root_certs_openssl.cc:48: TestRootCerts::~TestRootCerts() {}
On 2010/11/17 17:17:30, bulach wrote:
> nit: \n}

Judging by http://crrev.com/61100 , I see the original style as frequently as
what you propose. I've left it as is, considering it looks to be prevalent in
net::.

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_wi...
File net/base/test_root_certs_win.cc (right):

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_wi...
net/base/test_root_certs_win.cc:57: // manual, brute-force method is used for
unit tests. Look through every
On 2010/11/17 19:44:39, wtc wrote:
> Could you please add a short version of your description of
> the algorithm in
> http://code.google.com/p/chromium/issues/detail?id=8470#c3 ?
> In particular, that our solution is inspired by the way
> Outlook and EFS uses the Trusted People” store as described in
> http://technet.microsoft.com/en-us/library/bb457027.aspx.
> 

Sure. There are still differences in how we're doing it, judging by how the
Windows Communication Framework implements
System.IdentityModel.Selectors.X509CertificateValidator.PeerOrChainTrustValidator.

Namely, *no* chain building happens (EFS, Outlook, WCF) when checking for peer
trust - it's simply checked that the cert fingerprint appears in "TrustedPeople"
and not in "Disallowed" - CAPI traces and Reflector confirm.

http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_wi...
net/base/test_root_certs_win.cc:86: CERT_TRUST_REVOCATION_STATUS_UNKNOWN |
On 2010/11/17 19:44:39, wtc wrote:
> Two comments about the revocation error flags.
> 
> 1. I think we should not clear CERT_TRUST_REVOCATION_STATUS_UNKNOWN
> and CERT_TRUST_IS_OFFLINE_REVOCATION.  I believe
> CERT_TRUST_REVOCATION_STATUS_UNKNOWN is set not because
> the test root cert is untrusted, but because the test
> server cert doesn't have a CRL distribution point and
> AIA extension with OCSP responder URL.
> (CERT_TRUST_IS_OFFLINE_REVOCATION is always set if
> CERT_TRUST_REVOCATION_STATUS_UNKNOWN is set, in my experience.)

No, I believe the issue is directly related to the fact that the certificate
doesn't appear in the list. The revocation checking process is described at
http://technet.microsoft.com/en-us/library/bb457027.aspx#EKAA . Under item #3,
the relevant section I've marked up with ***

When a root certificate—a certificate that contains the same DN for both the
Subject and Issuer attributes—is encountered, a revocation check may or may not
occur. If the certificate chaining engine behavior will depend on whether the
application enables the CERT_CHAIN_REVOCATION_CHECK_CHAIN_EXCLUDE_ROOT flag,
which is the default, the root CA’s certificate is not checked for revocation.
***If the flag is not enabled, the root CA certificate is checked for revocation
if the root CA certificate includes the CDP extension. If the CDP extension is
not included, no revocation check is performed.***

However, this only applies when it appears in the "Trusted Root", as described
below. Because it does not, no revocation checking is even considered, hence the
STATUS_UNKNOWN. AFAICT, the _OFFLINE_REVOCATION is set for the same reason: Not
enough information was available (by means of appearing in the "Trusted Roots"),
so the absence of the CDP is non-authoritative and thus didn't happen.

> 
> 2. I believe our hack of using an untrusted test root cert
> and ignoring CERT_TRUST_IS_UNTRUSTED_ROOT will work for
> most unit tests, but won't work for a unit test on
> revocation, because CryptoAPI won't be able to verify a
> CRL or OCSP response signed by the test root CA.
> 
> Do you agree?

Yes, I believe you're right. The general process of revocation checking under
Vista+ is described at
http://technet.microsoft.com/en-us/library/ee619754(WS.10).aspx#BKMK_BC

Only Evaluate Chains that Terminate in a Trusted Root Certificate
After CryptoAPI builds a certificate chain up to a trusted root authority
certificate, it will check for certificate revocation. The validation will
continue from the root CA certificate to the entity certificate presented to the
application. If there are multiple certificate chains that terminate in a
trusted root authority certificate, then CryptoAPI will validate each chain
until it finds a chain that is not revoked.

Under the section "Building the OCSP Signing Certificate Chain", the following
is stated: "If a certificate chain from the OCSP signing certificate to a
trusted root CA cannot be validated, then CryptoAPI 2.0 will return an error
message that the revocation server was offline."

http://codereview.chromium.org/4646001/diff/56001/net/base/x509_certificate_m...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/4646001/diff/56001/net/base/x509_certificate_m...
net/base/x509_certificate_mac.cc:491: status =
root_certs->SetAnchorCertificates(trust_ref);
On 2010/11/17 17:17:30, bulach wrote:
> I know SetAnchorCertificates already checks, but I think at this point it'd be
> clearer to comment that this is only for test code.
> 
> TestRootCerts* root_certs = TestRootCerts::GetInstance();
> if (!root_certs->IsEmpty()) {
>   // We're running tests, set the anchor certificates.
>   status = root_certs->SetAnchorCertificates(trust_ref);
>   if (status)
>     return NetErrorFromOSStatus(status);
> }
> 
> it may be even clearer to have this under a 
> if (TestRootCerts::HasInstance()) {
>   ...
> }
> 
> and require that tests that needs this would initialize the singleton
> beforehand. thoughts?

re: IsEmpty(), it seems unnecessary to put the check in two places. I've updated
the documentation of SetAnchorCertificates to better describe the behaviour,
especially with respect to IsEmpty(). Does that work for you?

re: HasInstance: How do you see something like that working? I don't see
anything that can check-without-create on Singleton<> or LazyInstance<>.

http://codereview.chromium.org/4646001/diff/56001/net/base/x509_certificate_m...
net/base/x509_certificate_mac.cc:491: status =
root_certs->SetAnchorCertificates(trust_ref);
On 2010/11/18 02:12:49, wtc wrote:
> Nit: I find this harder to understand because this looks
> like an operation on root_certs, but this actually acts
> on trust_ref.  The original code shows that more clearly.
> 
> Two possible solutions:
> 1. Rename the method:
>   status = root_certs->SetAnchorCertificatesOnSecTrust(trust_ref);
> 2. Go back to the style of the original code:
>   CFArrayRef anchor_array = root_certs->CopyRootCertArray();
>   status = SecTrustSetAnchorCertificates(trust_ref, anchor_array);

I went with 3, which renamed it to FixupSecTrustRef to match FixupChainContext
on Windows. Does that sufficiently disambiguate?

[joth, 2010-11-18 10:27:19]

You patch on my patch LGTM, thanks.

[bulach, 2010-11-18 12:42:11]

thanks ryan! the code looks much simpler now, but I still have one suggestion to
avoid creating the test object in production code, let me know if it makes
sense:

http://codereview.chromium.org/4646001/diff/56001/net/base/x509_certificate_m...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/4646001/diff/56001/net/base/x509_certificate_m...
net/base/x509_certificate_mac.cc:491: status =
root_certs->SetAnchorCertificates(trust_ref);
On 2010/11/18 05:31:58, rsleevi wrote:
> On 2010/11/18 02:12:49, wtc wrote:
> > Nit: I find this harder to understand because this looks
> > like an operation on root_certs, but this actually acts
> > on trust_ref.  The original code shows that more clearly.
> > 
> > Two possible solutions:
> > 1. Rename the method:
> >   status = root_certs->SetAnchorCertificatesOnSecTrust(trust_ref);
> > 2. Go back to the style of the original code:
> >   CFArrayRef anchor_array = root_certs->CopyRootCertArray();
> >   status = SecTrustSetAnchorCertificates(trust_ref, anchor_array);
> 
> I went with 3, which renamed it to FixupSecTrustRef to match FixupChainContext
> on Windows. Does that sufficiently disambiguate?

nit: I'd rename root_certs to test_root_certs to make it even more explicit this
is only used for tests.

this works for me, but just to clarify re:singleton:
since the tests would have to initialize the singleton on startup before the
actual test hits this, a I think a simple static bool on TestRootCerts
initialized to false and toggled on the singleton ctor would be sufficient..

your current code is clear to me now, but perhaps we'd like to avoid
instantiating the TestRootCerts object in production code and only initialize it
in test code?

http://codereview.chromium.org/4646001/diff/95001/net/base/x509_certificate_w...
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/4646001/diff/95001/net/base/x509_certificate_w...
net/base/x509_certificate_win.cc:698: TestRootCerts* root_certs =
TestRootCerts::GetInstance();
nit: test_root_certs, and similar comment to the mac side about instantiating
this only in test code.

[wtc, 2010-11-19 01:29:44]

rsleevi: I don't have time to review new Patch Sets.
So I'll be counting on you and bulach to review them.

FixupSecTrustRef is a good name because it indicates
the action is on SecTrustRef as opposed to 'this'.

I agree with the test_root_certs variable name that
bulach suggested.

I'm very worried about the inability to do revocation
checking on our test certs, so we should plan on
implementing the Windows 7 only solution soon.

[Ryan Sleevi, 2010-11-21 22:15:30]

Bulach: I've added HasInstance() and renamed as requested where appropriate.

Wan-Teh: Based on your concerns regarding revocation, and our conversation via
e-mail, I've uploaded a fundamentally different approach for the Windows trust.
Instead of manipulating the validation results, the CryptoAPI functionality for
opening the Root store is hooked and shimmed.

This approach has the benefit of successfully allowing CRLs to validate (this
I've tested), and I believe will allow OCSP to validate (this I've not tested).
This approach should be API-valid for as far back as the first CryptoAPI release
(IE 3.0/9x), although I've only tested on Win7 and the try bots.

Because this approach is so different, I added a few simple unit tests to the
TestRootCerts, to make sure that the trust settings were properly being updated
and reset. As it turns out, they were not for NSS, so this CL also fixes it.

Because this approach successfully allows the trust settings to be injected and
removed, and propagates to revocation checks, I also regenerated the Test Root
CA and have updated the appropriate files and documentation, and included their
keys. This was also done to ensure that on the try bots and on the production
builders, the trust settings are truly being manipulated by the class, and it's
not because of the pre-configured settings of the bots.

I realize this it is probably not preferable to change like this, this late in
the review cycle and especially after the LGTMs. However, because this approach
fundamentally replaces the existing approach, and addressed concerns you
expressed with this approach, it seemed wrong to commit something that would
likely be replaced shortly thereafter.

If this is the wrong way to do it, please let me know, and I'll back out Patch
Set 10 to commit, and create a separate CL to re-address this.

[joth, 2010-11-22 13:05:40]

It LGTM but needs wtc's input on the win32 api shimming design.

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs.cc
File net/base/test_root_certs.cc (right):

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs.cc...
net/base/test_root_certs.cc:41: return g_has_instance;
this might give some valgrind warnings as g_has_instance could be written to
repeatedly from multiple threads whilst others are reading it.
Setting g_has_instance from TestRootCerts c'tor should ix this (so it just
happens once) however that pushes it into each of the platform files.

Perhaps as well to make it an explicit getter/setter pair:

bool g_test_mode_enabled = false;

void EnableTestMode() {  // Called once e.g. in test Init()
  g_test_mode_enabled = true;
}

bool TestModeEnabled() {
  return g_test_mode_enabled;
}


TestRootCerts::GetInstance() {
  CHECK(TestModeEnabled()); 
  ...
}

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs_wi...
File net/base/test_root_certs_win.cc (right):

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs_wi...
net/base/test_root_certs_win.cc:19: BOOL WINAPI InterceptedOpenStoreW(LPCSTR
store_provider,
Maybe comment
// Provides a custom implementation that will be called in place of
CertOpenStore

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs_wi...
net/base/test_root_certs_win.cc:36: PFN_CERT_DLL_OPEN_STORE_PROV_FUNC
original_function;
make this private (with trailing _ ) and provide a getter:
PFN_CERT_DLL_OPEN_STORE_PROV_FUNC original_function() { return
original_function_; }

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs_wi...
net/base/test_root_certs_win.cc:108: // Only inject certificates for the Root
store. If
Not quite a complete sentence. Maybe:
Conditionally inject certificates for the Root store.

[bulach, 2010-11-22 14:36:50]

LGTM

it'd be nice to double check the win implementation, and some suggestions to
further separate tests for prod based on joth's idea:

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs.cc
File net/base/test_root_certs.cc (right):

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs.cc...
net/base/test_root_certs.cc:41: return g_has_instance;
On 2010/11/22 13:05:40, joth wrote:
> this might give some valgrind warnings as g_has_instance could be written to
> repeatedly from multiple threads whilst others are reading it.
> Setting g_has_instance from TestRootCerts c'tor should ix this (so it just
> happens once) however that pushes it into each of the platform files.
> 
> Perhaps as well to make it an explicit getter/setter pair:
> 
> bool g_test_mode_enabled = false;
> 
> void EnableTestMode() {  // Called once e.g. in test Init()
>   g_test_mode_enabled = true;
> }
> 
> bool TestModeEnabled() {
>   return g_test_mode_enabled;
> }
> 
> 
> TestRootCerts::GetInstance() {
>   CHECK(TestModeEnabled()); 
>   ...
> }

I like this idea, just some minor suggestions on top of it:

1. check that EnableTestMode() is only defined for UNIT_TEST (this macro is
defined when depending on gtest, so I think it should be available for tests,
but not for production code):
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/testing/gtest...

2. add a message on the CHECK for GetInstance():

#if defined(UNIT_TEST)
void EnableTestMode() {  // Called once e.g. in test Init()
  g_test_mode_enabled = true;
  GetInstance();
}
#endif  // UNIT_TEST

TestRootCerts::GetInstance() {
  CHECK(g_test_mode_enabled) << "Can't access TestRootCerts without enabling it
for tests";
  return Singleton<TestRootCerts>::get();
}

[wtc, 2010-11-23 00:30:11]

LGTM.  I'm glad you discovered a good solution for Windows.

We should not check in the previous broken solution for
Windows because we now know we can do better.

However, it is hard to review a changelist of this size
repeatedly, so please consider making some of my suggested
changes (such as generating the test certificates
differently) below in a follow-up changelist.

Also, we will create the M9 branch next Monday (11/29).
You should also consider checking in this CL after the M9
branch is created.  If necessary, I can check in this CL
for you.

http://codereview.chromium.org/4646001/diff/82002/base/openssl_util.h
File base/openssl_util.h (right):

http://codereview.chromium.org/4646001/diff/82002/base/openssl_util.h#newcode21
base/openssl_util.h:21: reset(NULL);
Nit: since you let the argument default to NULL, you can
just write
    reset();
here.

http://codereview.chromium.org/4646001/diff/82002/net/base/cert_test_util.h
File net/base/cert_test_util.h (right):

http://codereview.chromium.org/4646001/diff/82002/net/base/cert_test_util.h#n...
net/base/cert_test_util.h:27: scoped_refptr<X509Certificate>
ImportCertFromFile(const FilePath& certs_dir,
Is this file deleted?

This function should return X509Certificate*.  (Remember to
remove #include "base/ref_counted.h", too.)

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs.h
File net/base/test_root_certs.h (right):

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs.h#...
net/base/test_root_certs.h:30: // artificially mark a certificate as trusted,
independent of the local
Nit: a certificate => a root CA certificate

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs.h#...
net/base/test_root_certs.h:38: static bool HasInstance();
BUG: your implementation of HasInstance() is not thread-safe.

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs_ns...
File net/base/test_root_certs_nss.cc (right):

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs_ns...
net/base/test_root_certs_nss.cc:57: rv = CERT_DecodeTrustString(&original_trust,
"c,c,c");
Could you add a comment to explain what you're doing here?

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs_un...
File net/base/test_root_certs_unittest.cc (right):

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs_un...
net/base/test_root_certs_unittest.cc:102: &restored_verify_result);
Nit: this should be aligned with the opening parenthesis '('
of the previous line.

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs_wi...
File net/base/test_root_certs_win.cc (right):

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs_wi...
net/base/test_root_certs_win.cc:19: BOOL WINAPI InterceptedOpenStoreW(LPCSTR
store_provider,
Please add a comment to note that this is a
CertDllOpenStoreProv callback function.  Optionally, link
to http://msdn.microsoft.com/en-us/library/aa376043%28v=VS.85%29.aspx

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs_wi...
net/base/test_root_certs_win.cc:102: return FALSE;
Do we need to call SetLastError() to set an error code?

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs_wi...
net/base/test_root_certs_win.cc:125: // root store is inserted to the front of
the collection, which allows the
COMMENT BUG: you pass 0 as the dwPriority argument to
CertAddStoreToCollection, and MSDN says 0 means
TestRootCerts::GetInstance()->temporary_roots() is appended
as the last store in the collection.  This contradicts what
your comment says here.

http://codereview.chromium.org/4646001/diff/82002/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/4646001/diff/82002/net/base/x509_certificate.h...
net/base/x509_certificate.h:309: static void ResetStore();
Nit: consider naming this method ResetCertStore.

http://codereview.chromium.org/4646001/diff/82002/net/data/ssl/certificates/e...
File net/data/ssl/certificates/expired_cert.pem (right):

http://codereview.chromium.org/4646001/diff/82002/net/data/ssl/certificates/e...
net/data/ssl/certificates/expired_cert.pem:10: Subject: C=US, ST=California,
L=Mountain View, O=Test CA, O=Expired, CN=127.0.0.1
Could you add a subjectAltName extension with iPAddress=127.0.0.1?

http://codereview.chromium.org/4646001/diff/82002/net/data/ssl/certificates/o...
File net/data/ssl/certificates/ok_cert.pem (right):

http://codereview.chromium.org/4646001/diff/82002/net/data/ssl/certificates/o...
net/data/ssl/certificates/ok_cert.pem:9: Not After : Nov 18 12:37:14 2020 GMT
The validity period should be at least 50 years.

http://codereview.chromium.org/4646001/diff/82002/net/data/ssl/certificates/o...
File net/data/ssl/certificates/openssl_ca.cnf (right):

http://codereview.chromium.org/4646001/diff/82002/net/data/ssl/certificates/o...
net/data/ssl/certificates/openssl_ca.cnf:23: authorityKeyIdentifier =
keyid,issuer:always
We probably should not include 'issuer'.  Having 'keyid'
should be enough.

This comment also applies to lines 29 and 33 below in the
ca_cert and crl_extensions sections.

I suggest looking at certificates and CRLs issued by a real
CA such as VeriSign as examples.

http://codereview.chromium.org/4646001/diff/82002/net/data/ssl/certificates/r...
File net/data/ssl/certificates/root_ca_cert.crt (right):

http://codereview.chromium.org/4646001/diff/82002/net/data/ssl/certificates/r...
net/data/ssl/certificates/root_ca_cert.crt:17: -----BEGIN PRIVATE KEY-----
Should this file be renamed to indicate that it also contains
the private key now?

[Ryan Sleevi, 2010-12-03 03:28:06]

wtc: Just to make sure I understand your feedback - you're ok with the current
test certs going in, and in a subsequent CL, re-generating them and changing the
parameters, correct? Or was your comment meant that the unit tests & test certs
should be pulled out for a separate CL (meaning commit w/o tests)? Just wanting
to make sure I understood.

joth/bulach: Some questions below regarding your feedback.

http://codereview.chromium.org/4646001/diff/82002/net/base/cert_test_util.h
File net/base/cert_test_util.h (right):

http://codereview.chromium.org/4646001/diff/82002/net/base/cert_test_util.h#n...
net/base/cert_test_util.h:27: scoped_refptr<X509Certificate>
ImportCertFromFile(const FilePath& certs_dir,
On 2010/11/23 00:30:11, wtc wrote:
> Is this file deleted?
> 
> This function should return X509Certificate*.  (Remember to
> remove #include "base/ref_counted.h", too.)

The reason it's returning scoped_refptr<> is because it gets a scoped_refptr<>
from X509Certificate::CreateCertificateListFromBytes (which handles PEM, PKCS#7,
and DER equally). To return an X509Certificate* would mean either AddRef()'ing
the X509Certificate* or releasing the scoped_refptr<>, both of which would
require the caller to call Release on the returned handle. 

It seemed better (perhaps I'm mistaken) to churn refs rather than impose the
uncommon requirement on the caller.

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs.cc
File net/base/test_root_certs.cc (right):

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs.cc...
net/base/test_root_certs.cc:41: return g_has_instance;
On 2010/11/22 13:05:40, joth wrote:
> this might give some valgrind warnings as g_has_instance could be written to
> repeatedly from multiple threads whilst others are reading it.
> Setting g_has_instance from TestRootCerts c'tor should ix this (so it just
> happens once) however that pushes it into each of the platform files.
> 
> Perhaps as well to make it an explicit getter/setter pair:
> 
> bool g_test_mode_enabled = false;
> 
> void EnableTestMode() {  // Called once e.g. in test Init()
>   g_test_mode_enabled = true;
> }
> 
> bool TestModeEnabled() {
>   return g_test_mode_enabled;
> }
> 
> 
> TestRootCerts::GetInstance() {
>   CHECK(TestModeEnabled()); 
>   ...
> }

joth: I pushed the platform-specific implementation into an explicit Init()
method, while the platform-agnostic details, specifically g_has_instance, is now
handled in the ctor in test_root_certs.cc

I'm not sure I understand the desire to provide the explicit getter/setter pair,
perhaps you could elaborate?

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs.cc...
net/base/test_root_certs.cc:41: return g_has_instance;
On 2010/11/22 14:36:50, bulach wrote:
> On 2010/11/22 13:05:40, joth wrote:
> > this might give some valgrind warnings as g_has_instance could be written to
> > repeatedly from multiple threads whilst others are reading it.
> > Setting g_has_instance from TestRootCerts c'tor should ix this (so it just
> > happens once) however that pushes it into each of the platform files.
> > 
> > Perhaps as well to make it an explicit getter/setter pair:
> > 
> > bool g_test_mode_enabled = false;
> > 
> > void EnableTestMode() {  // Called once e.g. in test Init()
> >   g_test_mode_enabled = true;
> > }
> > 
> > bool TestModeEnabled() {
> >   return g_test_mode_enabled;
> > }
> > 
> > 
> > TestRootCerts::GetInstance() {
> >   CHECK(TestModeEnabled()); 
> >   ...
> > }
> 
> I like this idea, just some minor suggestions on top of it:
> 
> 1. check that EnableTestMode() is only defined for UNIT_TEST (this macro is
> defined when depending on gtest, so I think it should be available for tests,
> but not for production code):
>
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/testing/gtest...
> 
> 2. add a message on the CHECK for GetInstance():
> 
> #if defined(UNIT_TEST)
> void EnableTestMode() {  // Called once e.g. in test Init()
>   g_test_mode_enabled = true;
>   GetInstance();
> }
> #endif  // UNIT_TEST
> 
> TestRootCerts::GetInstance() {
>   CHECK(g_test_mode_enabled) << "Can't access TestRootCerts without enabling
it
> for tests";
>   return Singleton<TestRootCerts>::get();
> }
> 
> 

bulach: Could you explain the reasoning behind this, as I'm not sure I really
understand why the desire for the UNIT_TEST check. 

My understanding of why you were requesting HasInstance() was because the
constructors were doing non-trivial work (CertOpenStore in the case of Windows,
CFMutableArrayCreate() in the case of Mac).

The reason why I went with HasInstance() as such was because it may be able to
be optimized out by an aggressive enough optimizer (g_has_instance will always
be false in "normal" binaries, so all of the TestRootCerts code can be
eliminated and the branch in X509Certificate removed)

As far as thread-safety, the underlying TestRootCerts isn't thread-safe (and
hasn't been, historically). There's definitely a race between HasInstance() and
GetInstance(), even still, but there's also implicit races between GetInstance()
and the verify code, so I'm not sure the benefit of addressing one without the
other.

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs.h
File net/base/test_root_certs.h (right):

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs.h#...
net/base/test_root_certs.h:30: // artificially mark a certificate as trusted,
independent of the local
On 2010/11/23 00:30:11, wtc wrote:
> Nit: a certificate => a root CA certificate

In the case of NSS and Windows, it's actually "a certificate" (RFC 3280 "Trust
Anchor"). OpenSSL's default handler doesn't support "trust anchors" fully yet
(limited support in 1.1, IIRC), and Mac only checks the root, IIRC.

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs_wi...
File net/base/test_root_certs_win.cc (right):

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs_wi...
net/base/test_root_certs_win.cc:36: PFN_CERT_DLL_OPEN_STORE_PROV_FUNC
original_function;
On 2010/11/22 13:05:40, joth wrote:
> make this private (with trailing _ ) and provide a getter:
> PFN_CERT_DLL_OPEN_STORE_PROV_FUNC original_function() { return
> original_function_; }

Since this class is entirely internal to this implementation, I'm not sure I
understand the reason for the visibility guard. I've made this class a struct,
which seemed to be the preferred/acceptable form in the Style Guide, and exposed
both members as public.

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs_wi...
net/base/test_root_certs_win.cc:102: return FALSE;
On 2010/11/23 00:30:11, wtc wrote:
> Do we need to call SetLastError() to set an error code?

Nothing I've seen in the documentation (MSDN, Header, SDK samples) has stated
it's required. CryptoAPI will propagate the error back to the caller of
CertOpenStore, but as far as I can tell, the error code is not used internally
by CryptoAPI.

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs_wi...
net/base/test_root_certs_win.cc:125: // root store is inserted to the front of
the collection, which allows the
On 2010/11/23 00:30:11, wtc wrote:
> COMMENT BUG: you pass 0 as the dwPriority argument to
> CertAddStoreToCollection, and MSDN says 0 means
> TestRootCerts::GetInstance()->temporary_roots() is appended
> as the last store in the collection.  This contradicts what
> your comment says here.

Thanks for spotting this. You're right that it was only a comment bug - the
exact position of temporary_roots() does not matter for this method to work. It
would only matter if we wanted to add certificates/CRLs to the root store
without the dialog, in which case the memory store would need to be the highest
priority and CERT_PHYSICAL_STORE_ADD_ENABLE_FLAG would have had to be passed as
the third parameter (instead of zero). I've updated the comment to better
describe the behaviour of "how/why it works", without the buggy portion.

[joth, 2010-12-03 10:40:59]

Thanks for the detailed follow up. My comments are now niceties, please don't
let another round of review from me hold you up!
Answers to your specific questions below
Cheers
Joth

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs.cc
File net/base/test_root_certs.cc (right):

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs.cc...
net/base/test_root_certs.cc:41: return g_has_instance;
On 2010/12/03 03:28:06, Ryan Sleevi wrote:
> On 2010/11/22 13:05:40, joth wrote:
> > this might give some valgrind warnings as g_has_instance could be written to
> > repeatedly from multiple threads whilst others are reading it.
> > Setting g_has_instance from TestRootCerts c'tor should ix this (so it just
> > happens once) however that pushes it into each of the platform files.
> > 
> > Perhaps as well to make it an explicit getter/setter pair:
> > 
> > bool g_test_mode_enabled = false;
> > 
> > void EnableTestMode() {  // Called once e.g. in test Init()
> >   g_test_mode_enabled = true;
> > }
> > 
> > bool TestModeEnabled() {
> >   return g_test_mode_enabled;
> > }
> > 
> > 
> > TestRootCerts::GetInstance() {
> >   CHECK(TestModeEnabled()); 
> >   ...
> > }
> 
> joth: I pushed the platform-specific implementation into an explicit Init()
> method, while the platform-agnostic details, specifically g_has_instance, is
now
> handled in the ctor in test_root_certs.cc
> 
> I'm not sure I understand the desire to provide the explicit getter/setter
pair,
> perhaps you could elaborate?

My thinking was to put the "g_has_instance = true;" into it's own method
("TestRootCerts::EnableTestMode") which is called explicitly by the test code,
so it's impossible for the production code to accidentally enable test mode as a
side effect of an inappropriate call to GetInstance. (as per bulach's idea, if
TestRootCerts::EnableTestMode is only visible to the test code under a UNIT_TEST
guard, it gets really very impossible for production code to accidentally enable
it).

I'm also OK with it as you have it now though, as it will fix the main concern
around valgrind problems. (assuming the first GetInstance call does not race
with HasInstance, which it shouldn't as it will occur in test-setup phase
anyway)

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs_wi...
File net/base/test_root_certs_win.cc (right):

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs_wi...
net/base/test_root_certs_win.cc:36: PFN_CERT_DLL_OPEN_STORE_PROV_FUNC
original_function;
On 2010/12/03 03:28:06, Ryan Sleevi wrote:
> On 2010/11/22 13:05:40, joth wrote:
> > make this private (with trailing _ ) and provide a getter:
> > PFN_CERT_DLL_OPEN_STORE_PROV_FUNC original_function() { return
> > original_function_; }
> 
> Since this class is entirely internal to this implementation, I'm not sure I
> understand the reason for the visibility guard. I've made this class a struct,
> which seemed to be the preferred/acceptable form in the Style Guide, and
exposed
> both members as public.

I was mostly just quoting the rule that on classes, all data members must be
private -- regardless of the class definition only being visible to this .cc.
Fully public structs are fine for passive data-carrying objects (I tend to think
of this in terms of "value" vs "identity" classes). What muddies the water here
is the boiler plating to make it lazy instance, as it enforces identity rather
than value semantics. (that is, no default/copy c'tor and singleton-like
properties imposed).
Also the name xxx "Injector" doesn't really suggest it is a passive object.

In line with the view that it's private to the .cc and access control isn't
important, I'd suggest go the full way and remove the private label and friend
declarations too, leaving it a full public struct.
The alternative is to extract the constructor logic from here into a custom
LeakyLazyInstanceTraits::New overload, which really would leave this struct as
pure passive data-carrier, but does seem to be getting excessive just to avoid
defining one public getter!

[bulach, 2010-12-03 10:58:26]

thanks ryan! clarified my idea below, but I'll leave your good judgement to
decide whether or not it's a good one.. :) from my point of view, feel free to
go ahead.

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs.cc
File net/base/test_root_certs.cc (right):

http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs.cc...
net/base/test_root_certs.cc:41: return g_has_instance;
On 2010/12/03 03:28:06, Ryan Sleevi wrote:
> On 2010/11/22 14:36:50, bulach wrote:
> > On 2010/11/22 13:05:40, joth wrote:
> > > this might give some valgrind warnings as g_has_instance could be written
to
> > > repeatedly from multiple threads whilst others are reading it.
> > > Setting g_has_instance from TestRootCerts c'tor should ix this (so it just
> > > happens once) however that pushes it into each of the platform files.
> > > 
> > > Perhaps as well to make it an explicit getter/setter pair:
> > > 
> > > bool g_test_mode_enabled = false;
> > > 
> > > void EnableTestMode() {  // Called once e.g. in test Init()
> > >   g_test_mode_enabled = true;
> > > }
> > > 
> > > bool TestModeEnabled() {
> > >   return g_test_mode_enabled;
> > > }
> > > 
> > > 
> > > TestRootCerts::GetInstance() {
> > >   CHECK(TestModeEnabled()); 
> > >   ...
> > > }
> > 
> > I like this idea, just some minor suggestions on top of it:
> > 
> > 1. check that EnableTestMode() is only defined for UNIT_TEST (this macro is
> > defined when depending on gtest, so I think it should be available for
tests,
> > but not for production code):
> >
>
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/testing/gtest...
> > 
> > 2. add a message on the CHECK for GetInstance():
> > 
> > #if defined(UNIT_TEST)
> > void EnableTestMode() {  // Called once e.g. in test Init()
> >   g_test_mode_enabled = true;
> >   GetInstance();
> > }
> > #endif  // UNIT_TEST
> > 
> > TestRootCerts::GetInstance() {
> >   CHECK(g_test_mode_enabled) << "Can't access TestRootCerts without enabling
> it
> > for tests";
> >   return Singleton<TestRootCerts>::get();
> > }
> > 
> > 
> 
> bulach: Could you explain the reasoning behind this, as I'm not sure I really
> understand why the desire for the UNIT_TEST check. 
> 
> My understanding of why you were requesting HasInstance() was because the
> constructors were doing non-trivial work (CertOpenStore in the case of
Windows,
> CFMutableArrayCreate() in the case of Mac).
> 
> The reason why I went with HasInstance() as such was because it may be able to
> be optimized out by an aggressive enough optimizer (g_has_instance will always
> be false in "normal" binaries, so all of the TestRootCerts code can be
> eliminated and the branch in X509Certificate removed)
> 
> As far as thread-safety, the underlying TestRootCerts isn't thread-safe (and
> hasn't been, historically). There's definitely a race between HasInstance()
and
> GetInstance(), even still, but there's also implicit races between
GetInstance()
> and the verify code, so I'm not sure the benefit of addressing one without the
> other.

sorry, I wasn't clear :) let me try to explain my thoughts.. 

the reason I suggested "HasInstance()" was to separate the test path from the
real production code, i.e., we'd only go to a different path in production code
"if (TestRootCerts::HasInstance())".


however, that doesn't entirely prevent production code from (accidentally)
instantiating TestRootCerts.

I liked joth's idea of "EnableTestMode()", and then to ensure this would only
ever be called by test code, I suggested guarding it with the UNIT_TEST check..

I guess my suggestions are about:
1. on production code mark it clear that we're following a different path for
tests.

2. make sure only test code can inject this. 

it would also clarify the HasInstance() / GetInstance() / EnableTest()
threading, i.e., HasInstance() can be called any time, GetInstance() only ever
after HasInstance(), and EnableTest() must be called before doing anything else.

I'm probably being overzealous though :) I'm happy with the current solution
(but would be happier would the EnableTestMode()).

[wtc, 2010-12-03 19:20:57]

On 2010/12/03 03:28:06, Ryan Sleevi wrote:
> wtc: Just to make sure I understand your feedback - you're ok with the current
> test certs going in, and in a subsequent CL, re-generating them and changing
the
> parameters, correct? Or was your comment meant that the unit tests & test
certs
> should be pulled out for a separate CL (meaning commit w/o tests)? Just
wanting
> to make sure I understood.

I have a slight preference over not changing the test certs twice.
But either way is fine by me, as long as it's easy for me to review
just the new test certs and the parameters.

[wtc, 2010-12-03 19:22:16]

rsleevi: I had a typo in my previous comment.  I meant:

I have a slight preference for not changing the test certs twice.

[Ryan Sleevi, 2010-12-06 08:29:22]

Thanks all for the reviews, and thanks joth&bulach for the clarification. I'll
look to address those concerns in a separate CL, to keep this one sane.

For those interested, I've split off the unit tests and new certificates into a
new CL, http://codereview.chromium.org/5535006/ , to make it easier to review.
I'm holding off committing this CL until the other is LGTMed, just to ensure
that both changes land close enough together to tickle out any (hidden) bugs in
this implementation, even though both are passing the try bots fine.