----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14570/#review26869 -----------------------------------------------------------
This looks basically fine, but some nits. (there are whitespace errors too) http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/SSL <https://reviews.apache.org/r/14570/#comment52251> I think you need to specify that these are environment variables being set (or config file entries if you can figure out where to put the config file) http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/client/windows/SslConnector.cpp <https://reviews.apache.org/r/14570/#comment52255> How about encapsulating these lines into a single routine which does the messing around. I think it would be more readable. http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/client/windows/SslConnector.cpp <https://reviews.apache.org/r/14570/#comment52256> Why not allocate a vector the size you just discovered instead of erroring out? http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/client/windows/SslConnector.cpp <https://reviews.apache.org/r/14570/#comment52262> It makes more sense to me convert the passwd here as it is only PFXImport...() that requires the utf16 (or UCS-2 or whatever...) encoded password. http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/client/windows/SslConnector.cpp <https://reviews.apache.org/r/14570/#comment52257> Don't ever use wstring! well not in new code anyway (wstring is not adequately defined as to size) I think you may mean vector<uint16_t>. If this is really a windows type use that type directly. http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/client/windows/SslConnector.cpp <https://reviews.apache.org/r/14570/#comment52260> Why not just return the UTF8 in a string. And do the conversion for whatever API requires that conversion. - Andrew Stitcher On Oct. 10, 2013, 6:37 a.m., Cliff Jansen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14570/ > ----------------------------------------------------------- > > (Updated Oct. 10, 2013, 6:37 a.m.) > > > Review request for qpid and Andrew Stitcher. > > > Bugs: QPID-3914 > https://issues.apache.org/jira/browse/QPID-3914 > > > Repository: qpid > > > Description > ------- > > This patch borrows much from the last submitted patch but takes it in a > different direction. Wherever possible, similarities between Windows > certificate handling and NSS certificate handling are emphasized and > differences de-emphasized. > > For example, certificates are now specified by their "Friendly Name" which > most closely resembles the NSS nickname. The password is now provided in a > file rather than from the command line or environment variable (and using the > same specifier). > > However, some differences cannot be glossed over. Microsoft's SChannel > implementation severely restricts where the final trust for a server > certificate may reside (either the self-signed certificate itself or the > trusted root CA of a certificate chain). It cannot be specified from an > arbitrary Windows store or file based store (unlike a client certificate). > > To make things worse, the tools available to install/manage certificates vary > between Windows versions or the presence of optional packages not regularly > installed on all target machines. So I am certainly sympathetic to the > desire to provide a mechanism to simplify the installation of the server > certificate. Nevertheless, I removed that capability for several reasons: > > Andrew's original objection is still valid regarding qpid intruding on normal > system administration of certificates; the previous patch is too specific > (handles self signed certs but not certificate chains); the solution is at > the mercy of the user who may click in the wrong spot no matter how well he > may be coaxed. > > As best I can tell, this is nuisance for anyone needing SSL validation of > their product. Some combination of certutil.exe, certmgr.msc, or powershell > wizardry is used but with a fallback description of how to import the > certificate by browsing to the certificate in MMC or Windows Explorer. > > > This patch also provides a revised store opening mechanism that doesn't > create stray stores if the user species the name incorrectly. > > > Diffs > ----- > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/SSL 1530859 > http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/CMakeLists.txt > 1530859 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/client/windows/SaslFactory.cpp > 1530859 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/client/windows/SslConnector.cpp > 1530859 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/util.h > PRE-CREATION > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/util.cpp > PRE-CREATION > > Diff: https://reviews.apache.org/r/14570/diff/ > > > Testing > ------- > > Windows 7: Personal/MY store, user created store, pkcs#12 file. Self signed, > chained certs. > > > Thanks, > > Cliff Jansen > >
