Add ability to create self signed certs to mac.

Add ability to create self signed certs to mac.

BUG=67929
TEST=BUILD

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

[dmac, 2011-02-04 21:44:12]

Add rsleevi to the review

[Ryan Sleevi, 2011-02-05 00:23:37]

More nits than bugs, which is always a good thing :-) A few leaks, but one bug I
am concerned with - the string -> X.500 name conversion routine.

I'd like to take a second look to see how that's resolved (assuming I didn't
fail at reading)

http://codereview.chromium.org/6312157/diff/1/base/crypto/cssm_init.cc
File base/crypto/cssm_init.cc (right):

http://codereview.chromium.org/6312157/diff/1/base/crypto/cssm_init.cc#newcode24
base/crypto/cssm_init.cc:24: void* CSSMMalloc(CSSM_SIZE size, void *alloc_ref) {
nit: void *alloc_ref -> void* alloc_ref

http://codereview.chromium.org/6312157/diff/1/base/crypto/cssm_init.cc#newcode48
base/crypto/cssm_init.cc:48: CSSM_CL_HANDLE cl_handle() const  { return
cl_handle_; }
nit: only once space between const and {

http://codereview.chromium.org/6312157/diff/1/base/crypto/cssm_init.cc#newcod...
base/crypto/cssm_init.cc:203: void LogCSSMError(const char *fn_name, CSSM_RETURN
err) {
nit: const char *fn_name -> const char* fn_name

http://codereview.chromium.org/6312157/diff/1/base/crypto/cssm_init.h
File base/crypto/cssm_init.h (right):

http://codereview.chromium.org/6312157/diff/1/base/crypto/cssm_init.h#newcode66
base/crypto/cssm_init.h:66: // Destructor frees the memory properly.
nit: Can you specify that this should only be used when using the CL/TP/CSP
handles from above, since that's the only time we're guaranteed (or supposed to
be guaranteed) that our memory management functions will be used.

The point being is that when using Sec* APIs, they memory is managed by the Sec*
API, and typically lives as long as the native handle (SecKey, SecCertificate,
SecTrust, etc), so it shouldn't be used for those.

This also applies to ScopedCSSMData

http://codereview.chromium.org/6312157/diff/1/net/base/x509_certificate_mac.cc
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/6312157/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:386: bool CERTNameToCSSMOIDMap(CERTName* name,
CSSMOIDMap* out_values) {
BUG: I could be reading wrong, but I don't think this is converting between the
CERTName and the CSSM OIDs correctly.

The concern is that by using a CSSMOIDMap, the ordering that originally appears
in the CERTName is lost, instead replaced with the ordering of the CSSM_OIDs in
memory. Since the ordering is significant, this will probably cause trouble.

I would think using a vector of pair<const CSSM_OID*, std::string> when
converting here, and then converting that into a vector<CSSM_APPLE_TP_NAME_OID>
below at line 513, should preserve the relative sequence order.

It won't, however, be guaranteed return the same name that NSS sees, in that
multi-AVA RDNs (which NSS and CryptoAPI support) are NOT preserved by using
CSSM_APPLE_TP_NAME_OID. Instead, each multi-AVA RDN is flattened into a sequence
of single-AVA RDNs. If this is problematic, then its be necessary to use
CSSM_CL_CertCreateTemplate, which takes a CSSM_X509_NAME. This is (under the
hood) what is Apple's TP is doing (see
security_apple_x509_tp-33583\lib\tpCredRequest.cpp makeCertTemplate)

http://codereview.chromium.org/6312157/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:487: DCHECK(subject.length() > 0);
nit: DCHECK(!subject.empty()) or DCHECK_GT(subject.length(), 0u)

http://codereview.chromium.org/6312157/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:541: // This is the Apple defaults.
world's smallest nit: defaults -> default or this is -> these are

http://codereview.chromium.org/6312157/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:545: certReq.notAfter =
valid_duration.InSeconds();
nit: InSeconds() returns an Int64, but notAfter expects a uint32. Admitted
extreme edge case, but should there be anything special here to handle it?

http://codereview.chromium.org/6312157/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:582: if (confirmRequired) {
BUG/nit? If confirmRequired is true, it's still possible that the is
memory/responses in resultSet needs to be freed.

For what it's worth, Apple's TP doesn't set confirmRequired, so it's up to you
if you want to use that knowledge here.

http://codereview.chromium.org/6312157/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:586: 
BUG: Check that resultSet->NumberOfResults == 1 before using resultSet->Results
/ encCert

http://codereview.chromium.org/6312157/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:591: base::AutoLock
sec_lock(base::GetMacSecurityServicesLock());
You shouldn't need to lock here. It's only needed for routines that modify the
keychain.

http://codereview.chromium.org/6312157/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:600: }
BUG: You also need to free
resultSet->Results
and
encCert->CertBlob.Data (see apple_x509_tp-33583\lib\tpCredRequest.cpp
AppleTPSession::RetrieveCredResult)

[dmac, 2011-02-08 01:23:45]

Ryan, I think I got everything you pointed out. Can you take another look-see?

http://codereview.chromium.org/6312157/diff/1/base/crypto/cssm_init.cc
File base/crypto/cssm_init.cc (right):

http://codereview.chromium.org/6312157/diff/1/base/crypto/cssm_init.cc#newcode24
base/crypto/cssm_init.cc:24: void* CSSMMalloc(CSSM_SIZE size, void *alloc_ref) {
On 2011/02/05 00:23:37, Ryan Sleevi wrote:
> nit: void *alloc_ref -> void* alloc_ref

Done.

http://codereview.chromium.org/6312157/diff/1/base/crypto/cssm_init.cc#newcode48
base/crypto/cssm_init.cc:48: CSSM_CL_HANDLE cl_handle() const  { return
cl_handle_; }
On 2011/02/05 00:23:37, Ryan Sleevi wrote:
> nit: only once space between const and {

Done.

http://codereview.chromium.org/6312157/diff/1/base/crypto/cssm_init.cc#newcod...
base/crypto/cssm_init.cc:203: void LogCSSMError(const char *fn_name, CSSM_RETURN
err) {
On 2011/02/05 00:23:37, Ryan Sleevi wrote:
> nit: const char *fn_name -> const char* fn_name

Done.

http://codereview.chromium.org/6312157/diff/1/base/crypto/cssm_init.h
File base/crypto/cssm_init.h (right):

http://codereview.chromium.org/6312157/diff/1/base/crypto/cssm_init.h#newcode66
base/crypto/cssm_init.h:66: // Destructor frees the memory properly.
On 2011/02/05 00:23:37, Ryan Sleevi wrote:
> nit: Can you specify that this should only be used when using the CL/TP/CSP
> handles from above, since that's the only time we're guaranteed (or supposed
to
> be guaranteed) that our memory management functions will be used.
> 
> The point being is that when using Sec* APIs, they memory is managed by the
Sec*
> API, and typically lives as long as the native handle (SecKey, SecCertificate,
> SecTrust, etc), so it shouldn't be used for those.
> 
> This also applies to ScopedCSSMData

Done.

http://codereview.chromium.org/6312157/diff/1/net/base/x509_certificate_mac.cc
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/6312157/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:386: bool CERTNameToCSSMOIDMap(CERTName* name,
CSSMOIDMap* out_values) {
On 2011/02/05 00:23:37, Ryan Sleevi wrote:
> BUG: I could be reading wrong, but I don't think this is converting between
the
> CERTName and the CSSM OIDs correctly.
> 
> The concern is that by using a CSSMOIDMap, the ordering that originally
appears
> in the CERTName is lost, instead replaced with the ordering of the CSSM_OIDs
in
> memory. Since the ordering is significant, this will probably cause trouble.
> 
> I would think using a vector of pair<const CSSM_OID*, std::string> when
> converting here, and then converting that into a
vector<CSSM_APPLE_TP_NAME_OID>
> below at line 513, should preserve the relative sequence order.
> 
> It won't, however, be guaranteed return the same name that NSS sees, in that
> multi-AVA RDNs (which NSS and CryptoAPI support) are NOT preserved by using
> CSSM_APPLE_TP_NAME_OID. Instead, each multi-AVA RDN is flattened into a
sequence
> of single-AVA RDNs. If this is problematic, then its be necessary to use
> CSSM_CL_CertCreateTemplate, which takes a CSSM_X509_NAME. This is (under the
> hood) what is Apple's TP is doing (see
> security_apple_x509_tp-33583\lib\tpCredRequest.cpp makeCertTemplate)

Done.

http://codereview.chromium.org/6312157/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:541: // This is the Apple defaults.
On 2011/02/05 00:23:37, Ryan Sleevi wrote:
> world's smallest nit: defaults -> default or this is -> these are

Done.

http://codereview.chromium.org/6312157/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:545: certReq.notAfter =
valid_duration.InSeconds();
On 2011/02/05 00:23:37, Ryan Sleevi wrote:
> nit: InSeconds() returns an Int64, but notAfter expects a uint32. Admitted
> extreme edge case, but should there be anything special here to handle it?

Done.

http://codereview.chromium.org/6312157/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:582: if (confirmRequired) {
On 2011/02/05 00:23:37, Ryan Sleevi wrote:
> BUG/nit? If confirmRequired is true, it's still possible that the is
> memory/responses in resultSet needs to be freed.
> 
> For what it's worth, Apple's TP doesn't set confirmRequired, so it's up to you
> if you want to use that knowledge here.

Done.

http://codereview.chromium.org/6312157/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:586: 
On 2011/02/05 00:23:37, Ryan Sleevi wrote:
> BUG: Check that resultSet->NumberOfResults == 1 before using
resultSet->Results
> / encCert

Done.

http://codereview.chromium.org/6312157/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:591: base::AutoLock
sec_lock(base::GetMacSecurityServicesLock());
On 2011/02/05 00:23:37, Ryan Sleevi wrote:
> You shouldn't need to lock here. It's only needed for routines that modify the
> keychain.

Done.

http://codereview.chromium.org/6312157/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:600: }
On 2011/02/05 00:23:37, Ryan Sleevi wrote:
> BUG: You also need to free
> resultSet->Results
> and
> encCert->CertBlob.Data (see apple_x509_tp-33583\lib\tpCredRequest.cpp
> AppleTPSession::RetrieveCredResult)

Done.

[Ryan Sleevi, 2011-02-08 02:27:55]

LGTM. Few super-small style nits, nothing requiring re-review.

http://codereview.chromium.org/6312157/diff/8002/base/crypto/rsa_private_key.cc
File base/crypto/rsa_private_key.cc (right):

http://codereview.chromium.org/6312157/diff/8002/base/crypto/rsa_private_key....
base/crypto/rsa_private_key.cc:104: // Add the sequence as the contents of a bit
string.
Lines 93-103 can be replaced with a call to ExportPublicKey now, to reduce
duplication.

http://codereview.chromium.org/6312157/diff/8002/net/base/x509_certificate_ma...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/6312157/diff/8002/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:419: }
nit: no need for braces around 1-line if.

http://codereview.chromium.org/6312157/diff/8002/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:433: }
nit: no need for braces around 1-line if.

http://codereview.chromium.org/6312157/diff/8002/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:459:
reinterpret_cast<CSSM_ENCODED_CERT*>(results_->Results);
nit: move lines 458-459 outside of the loop?

http://codereview.chromium.org/6312157/diff/8002/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:522: }
nit: no braces for one-line if's

http://codereview.chromium.org/6312157/diff/8002/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:558: // See comment about serial numbers above.
nit: remove extra spaces

http://codereview.chromium.org/6312157/diff/8002/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:595: &estTime, &refId);
nit: extra space

http://codereview.chromium.org/6312157/diff/8002/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:614: // true,
world's smallest nit pt 2: , -> .

http://codereview.chromium.org/6312157/diff/8002/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:621: << resultSet->NumberOfResults;
nit: missing space

http://codereview.chromium.org/6312157/diff/8002/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:630: encCert->CertType, encCert->CertEncoding,
&certificate_ref);
Is this style OK? I thought it was either align them all under the paren,
multiple per line, or align them all at 4 spaces? See also lines 593-595 and
compare with line 637-639/603-604. I'd defer to wtc/hawk though, as I still
haven't quite mastered the style guide.

[wtc, 2011-02-09 01:03:31]

I suggest some changes below.

My main complaint is that we only went halfway in avoiding
using NSS outside the SSL code.  Perhaps I underestimated the
difficulty and this is the best we can do.  Still, I hope we
can avoid using the NSS CERT_xxx functions by changing the
'subject' input parameter of X509Certificate::CreateSelfSigned
so that no parsing is needed.

http://codereview.chromium.org/6312157/diff/8002/base/crypto/cssm_init.cc
File base/crypto/cssm_init.cc (right):

http://codereview.chromium.org/6312157/diff/8002/base/crypto/cssm_init.cc#new...
base/crypto/cssm_init.cc:92: const CSSM_API_MEMORY_FUNCS cssmMemoryFunctions = {
cssmMemoryFunctions should be renamed either
kCssmMemoryFunctions or cssm_memory_functions to conform to
the Style Guide.

http://codereview.chromium.org/6312157/diff/8002/base/crypto/cssm_init.cc#new...
base/crypto/cssm_init.cc:200: return ::CSSMMalloc(size, NULL);
We should avoid overloading the CSSMMalloc and CSSMFree
functions like this.

http://codereview.chromium.org/6312157/diff/8002/base/crypto/cssm_init.cc#new...
base/crypto/cssm_init.cc:227: data_.Data = NULL;
Nit: if you reset data_.Data, you should also reset
data_.Length.

http://codereview.chromium.org/6312157/diff/8002/base/crypto/cssm_init.h
File base/crypto/cssm_init.h (right):

http://codereview.chromium.org/6312157/diff/8002/base/crypto/cssm_init.h#newc...
base/crypto/cssm_init.h:32: extern const CSSM_API_MEMORY_FUNCS
kCssmMemoryFunctions;
Delete the kCssmMemoryFunctions declaration.

http://codereview.chromium.org/6312157/diff/8002/net/base/x509_certificate_ma...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/6312157/diff/8002/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:26: #include
"third_party/nss/mozilla/security/nss/lib/certdb/cert.h"
The reason we went through the pain of writing several
hundred lines of CSSM code is to avoid using NSS outside
the SSL code.

Using NSS (CERT_xxx functions) here means we didn't achieve
that goal.  It seems that you just need to parse a simple
"CN=chromoting" string.  Perhaps we should change the API
so that parsing is not needed.

[Ryan Sleevi, 2011-02-09 01:54:59]

On 2011/02/09 01:03:31, wtc wrote:
> I suggest some changes below.
> 
> My main complaint is that we only went halfway in avoiding
> using NSS outside the SSL code.  Perhaps I underestimated the
> difficulty and this is the best we can do.  Still, I hope we
> can avoid using the NSS CERT_xxx functions by changing the
> 'subject' input parameter of X509Certificate::CreateSelfSigned
> so that no parsing is needed.
> 

wtc: For what it's worth, the Apple name routines are so buggy/broken that one
of my CLs explicitly uses NSS for the name parsing (it's towards the end of the
queue ;p). The purpose in that CL to find acceptable client certificates. Right
now, we do a poor-man's parse and hope for the best - but it's equally likely to
exclude valid certs as include invalid ones. Last I looked, the CERTName
functions were linked in due to libssl using them, so I don't know if it's
adding particular code overhead. That's why I think, on Mac at least, this isn't
too bad, because there really is nothing comparable or even decent offered by
the system.