First cut at BoGo shim

First cut at BoGo shim

[ekr-rietveld, 2016-07-31 22:35:46]

Here is my first cut at a shim to let you run NSS under the BoGo test runner. I
am posting this so you can get a look at where I am.

This is marked private because it has the potential to expose vulnerabilities in
NSS.


I will write up some notes soon. The biggest challenge is figuring out how to
deal with tests which don't work yet. There are several reasons why tests don't
work:

- They rely on an argument to the shim that we don't support. This is handled by
returning -89 causing the harness to skip the test.

- There is some other less visible incompatibility (like not supporting ECDSA
keys in NSS). Where I know about these, I also return -89.

- The test is supposed to fail with a specific error code and we don't return
it. This has turned out to be somewhat tricky. In any case where NSS errors are
more fine-grained than BoringSSL we can translate. I have added the
UNTRANSLATED_ERROR error as a stopgap and the runner will ignore it if you pass
-loose-errors. However, there is at least one case where one NSS error is mapped
into two BoringSSL errors and I'm not sure how to fix that except by not
translating at all.

- Bustage in BoGo (for instance, it has some TLS 1.3 spec violations).

Even so, at this point a lot of the TLS 1.3 tests work, as do many of the 1.2
and below tests, so we're making progress. 

PTAL. Also, comments on handle the compat issues welcome. Davidben and I have
thought about it a bit and I'll start an email thread too.

[mt, 2016-08-04 01:28:33]

Seems like a reasonable starting point.

https://codereview.appspot.com/308900043/diff/1/external_tests/manifest.mn
File external_tests/manifest.mn (right):

https://codereview.appspot.com/308900043/diff/1/external_tests/manifest.mn#ne...
external_tests/manifest.mn:15: nss_shim \
This needs a better name.  If it's a shim for the BoGo runner, then maybe
bogo_shim.

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/Makefile
File external_tests/nss_shim/Makefile (left):

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/Makef...
external_tests/nss_shim/Makefile:53: endif
Is this intentional?

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/confi...
File external_tests/nss_shim/config.cc (right):

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/confi...
external_tests/nss_shim/config.cc:41: return res;
I really don't understand this little piece of code.

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/config.h
File external_tests/nss_shim/config.h (right):

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/confi...
external_tests/nss_shim/config.h:24: virtual bool parse(std::queue<char*>* args)
= 0;
const char* ?

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/confi...
external_tests/nss_shim/config.h:27: bool parse_int(std::queue<char*>* args,
std::string *out);
int == internal rather than integer?  Also, naming convention is TitleCamel

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/confi...
external_tests/nss_shim/config.h:36: template <typename T> class ConfigEntry:
public ConfigEntryBase {
ConfigEntry : public

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/confi...
external_tests/nss_shim/config.h:45: bool parse(std::queue<char*>* args) {
Parse()

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/confi...
external_tests/nss_shim/config.h:47: }
whacky indent

Maybe we should run clang-format here.

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/manif...
File external_tests/nss_shim/manifest.mn (right):

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/manif...
external_tests/nss_shim/manifest.mn:18: EXTRA_LIBS =
$(DIST)/lib/$(LIB_PREFIX)softokn.$(LIB_SUFFIX)
just softokn, not ssl?

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/nss_s...
File external_tests/nss_shim/nss_shim.cc (right):

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/nss_s...
external_tests/nss_shim/nss_shim.cc:72: static bool Create(const Config& cfg,
std::unique_ptr<TestAgent>* agentp) {
This could return the unique_ptr.

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/nss_s...
external_tests/nss_shim/nss_shim.cc:73: std::unique_ptr<TestAgent> agent(new
TestAgent(cfg));
auto agent = std::make_unique<TestAgent>(cfg);

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/nss_s...
external_tests/nss_shim/nss_shim.cc:83: bool Init() {
This one could be split.  It's pretty long.

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/nss_s...
external_tests/nss_shim/nss_shim.cc:89: return false;
Aren't these once-per-process calls?

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/nss_s...
external_tests/nss_shim/nss_shim.cc:234: if (rv < 0) {
PR_WOULD_BLOCK?

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/nss_s...
external_tests/nss_shim/nss_shim.cc:321: agent.reset();  // Force close.
code repetition

[ekr-rietveld, 2016-08-06 22:04:51]

https://codereview.appspot.com/308900043/diff/1/external_tests/manifest.mn
File external_tests/manifest.mn (right):

https://codereview.appspot.com/308900043/diff/1/external_tests/manifest.mn#ne...
external_tests/manifest.mn:15: nss_shim \
On 2016/08/04 01:28:32, mt wrote:
> This needs a better name.  If it's a shim for the BoGo runner, then maybe
> bogo_shim.

Theirs is called bssl_shim, so...

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/Makefile
File external_tests/nss_shim/Makefile (left):

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/Makef...
external_tests/nss_shim/Makefile:53: endif
On 2016/08/04 01:28:32, mt wrote:
> Is this intentional?

Yeah, I think it's some kind of git move confusion. Notice where it lives.

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/confi...
File external_tests/nss_shim/config.cc (right):

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/confi...
external_tests/nss_shim/config.cc:41: return res;
On 2016/08/04 01:28:32, mt wrote:
> I really don't understand this little piece of code.

Bit rot.

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/config.h
File external_tests/nss_shim/config.h (right):

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/confi...
external_tests/nss_shim/config.h:24: virtual bool parse(std::queue<char*>* args)
= 0;
On 2016/08/04 01:28:32, mt wrote:
> const char* ?

Done.

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/confi...
external_tests/nss_shim/config.h:27: bool parse_int(std::queue<char*>* args,
std::string *out);
On 2016/08/04 01:28:32, mt wrote:
> int == internal rather than integer?  Also, naming convention is TitleCamel

Done.

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/confi...
external_tests/nss_shim/config.h:36: template <typename T> class ConfigEntry:
public ConfigEntryBase {
On 2016/08/04 01:28:32, mt wrote:
> ConfigEntry : public

Done.

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/confi...
external_tests/nss_shim/config.h:45: bool parse(std::queue<char*>* args) {
On 2016/08/04 01:28:32, mt wrote:
> Parse()

Done.

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/confi...
external_tests/nss_shim/config.h:47: }
On 2016/08/04 01:28:32, mt wrote:
> whacky indent
> 
> Maybe we should run clang-format here.

Done.

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/manif...
File external_tests/nss_shim/manifest.mn (right):

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/manif...
external_tests/nss_shim/manifest.mn:18: EXTRA_LIBS =
$(DIST)/lib/$(LIB_PREFIX)softokn.$(LIB_SUFFIX)
On 2016/08/04 01:28:32, mt wrote:
> just softokn, not ssl?

ssl is statically linked

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/nss_s...
File external_tests/nss_shim/nss_shim.cc (right):

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/nss_s...
external_tests/nss_shim/nss_shim.cc:73: std::unique_ptr<TestAgent> agent(new
TestAgent(cfg));
On 2016/08/04 01:28:33, mt wrote:
> auto agent = std::make_unique<TestAgent>(cfg);

Sadly only in C++14

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/nss_s...
external_tests/nss_shim/nss_shim.cc:83: bool Init() {
On 2016/08/04 01:28:32, mt wrote:
> This one could be split.  It's pretty long.

Split it a little bit

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/nss_s...
external_tests/nss_shim/nss_shim.cc:89: return false;
On 2016/08/04 01:28:32, mt wrote:
> Aren't these once-per-process calls?

good point. I wrote this before I realized I needed to do two inits, one for
resumption.

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/nss_s...
external_tests/nss_shim/nss_shim.cc:234: if (rv < 0) {
On 2016/08/04 01:28:32, mt wrote:
> PR_WOULD_BLOCK?

These sockets are set to nonblocking.

https://codereview.appspot.com/308900043/diff/1/external_tests/nss_shim/nss_s...
external_tests/nss_shim/nss_shim.cc:321: agent.reset();  // Force close.
On 2016/08/04 01:28:32, mt wrote:
> code repetition

Refactored

[ekr-rietveld, 2016-08-12 14:50:57]

MT, revised. This passed with HOT.

I suggest we land this and then have someone add support in CI

[wtc1, 2016-08-13 22:57:59]

Eric: this is just a supplemental review.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/Makefile (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/Makefile:51: CXXFLAGS += -std=c++0x
1. Nit: this should go to section 3 or 4. I think section 4 is good because
CFLAGS is also there.

2. Just curious: why don't we use -std=c++11?

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/config.cc (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.cc:10: #include <algorithm>
Nit: <cinttypes> and <algorithm> don't seem necessary.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.cc:14: bool
ConfigEntryBase::ParseInternal(std::queue<const char*>* args, std::string *out)
{
Nit: this line seems too long.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.cc:40: return "";
Nit: arg.empty()

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/config.h (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.h:17: class ConfigEntryBase {
Nit: this file has no comments.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.h:27: bool ParseInternal(std::queue<const
char*>* args, std::string *out);
Nit: the placement of "*" is not consistent.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.h:55: enum Status { OK, UnknownFlag,
MalformedArgument, MissingValue };
Nit: in the naming convention of the Google C++ Style Guide, these enumerators
should start with "k":

https://google.github.io/styleguide/cppguide.html#Enumerator_Names

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.h:85: #endif
Nit: add a comment: // config_h_

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/manifest.mn (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/manifest.mn:18: EXTRA_LIBS =
$(DIST)/lib/$(LIB_PREFIX)softokn.$(LIB_SUFFIX)
I am surprised that we need libdbm and libsoftokn.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/nss_bogo_shim.cc (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nss_bogo_shim.cc:230: if (rv !=
static_cast<int>(len)) {
You can declare |len| as int32_t and also declare
the for loop variable |i| as int32_t. Then you don't
need the static_cast<int> here.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nss_bogo_shim.cc:311: return false;
BUG: the return value of the main() function is the exit
status of the process. Its success/failure codes are opposite
to true and false. On error exit, main() should return 1 or
call exit(1). (Any nonzero integer <= 255 is fine.)

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/nsskeys.h (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nsskeys.h:15: #include "pk11pub.h"
Nit: you only need to include the headers that define
the SECKEYPrivateKey and CERTCertificate types.

[ekr-rietveld, 2016-08-13 23:03:51]

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/manifest.mn (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/manifest.mn:18: EXTRA_LIBS =
$(DIST)/lib/$(LIB_PREFIX)softokn.$(LIB_SUFFIX)
On 2016/08/13 22:57:59, wtc1 wrote:
> I am surprised that we need libdbm and libsoftokn.

We might not.They were defined in ssl_gtest. I will check.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/nss_bogo_shim.cc (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nss_bogo_shim.cc:311: return false;
On 2016/08/13 22:57:59, wtc1 wrote:
> BUG: the return value of the main() function is the exit
> status of the process. Its success/failure codes are opposite
> to true and false. On error exit, main() should return 1 or
> call exit(1). (Any nonzero integer <= 255 is fine.)

Thanks for catching this. This code moved from Init() and I missed the return
value change.

[mt, 2016-08-15 03:27:10]

LGTM modulo nits

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/Makefile (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/Makefile:23: 
extra line

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/config.cc (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.cc:38: std::string Config::xform_flag(const
std::string& arg) {
What is the naming convention here.

This should be static or const.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.cc:49: std::queue<const char*> args;
std::queue<std::string> might be better.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/config.h (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.h:35: 
extra line

Maybe you can just add this to the list of directories in Bug 1293528.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.h:77: ConfigEntryBase *entry(const
std::string& key) const {
Should this return const ConfigEntryBase& instead?

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/nss_bogo_shim.cc (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nss_bogo_shim.cc:62: bool Init() {
I think that this could be split up:

ConnectTcp(), which does the address/port/importFD

SetupVersion()

SetupResumption(), which does SSL_NO_CACHE,
SSL_SetURL/ConfigServerSessionIDCache.

SetupKeys(), which loads the cert/private key and installs the various hooks.

SetupCipherSuites()

Then you can call SSL_ResetHandshake().

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nss_bogo_shim.cc:149: return false;
Is the intent of this to enable all cipher suites, why not just enumerate and
enable all of them then (well at least the non-exportable ones).

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nss_bogo_shim.cc:213: SECStatus ReadWrite() {
You should at least comment here saying that this is essentially an echo
server/client, which flips all the bits on the message it receives and writes it
back out again.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nss_bogo_shim.cc:221: if (!rv)
nit: rv > 0

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nss_bogo_shim.cc:292: std::unique_ptr<TestAgent>
agent(TestAgent::Create(*cfg.get()));
Why isn't *cfg sufficient?  Calling .get() is a red flag you should avoid.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/nsskeys.cc (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nsskeys.cc:77: PORT_ErrorToString(PORT_GetError())
<< "\n";
Do we have a bug on file for not being able to import an ECDSA private key?

[ekr-rietveld, 2016-08-15 14:29:43]

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/Makefile (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/Makefile:23: 
On 2016/08/15 03:27:09, mt wrote:
> extra line

Done.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/Makefile:51: CXXFLAGS += -std=c++0x
On 2016/08/13 22:57:59, wtc1 wrote:
> 1. Nit: this should go to section 3 or 4. I think section 4 is good because
> CFLAGS is also there.
> 
> 2. Just curious: why don't we use -std=c++11?

Done.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/config.cc (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.cc:10: #include <algorithm>
On 2016/08/13 22:57:59, wtc1 wrote:
> Nit: <cinttypes> and <algorithm> don't seem necessary.

Done.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.cc:14: bool
ConfigEntryBase::ParseInternal(std::queue<const char*>* args, std::string *out)
{
On 2016/08/13 22:57:59, wtc1 wrote:
> Nit: this line seems too long.

Done.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.cc:38: std::string Config::xform_flag(const
std::string& arg) {
On 2016/08/15 03:27:10, mt wrote:
> What is the naming convention here.
> 
> This should be static or const.

Done.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.cc:40: return "";
On 2016/08/13 22:57:59, wtc1 wrote:
> Nit: arg.empty()

Done.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.cc:49: std::queue<const char*> args;
On 2016/08/15 03:27:09, mt wrote:
> std::queue<std::string> might be better.

Given that this is just local, I figured I would save the alloc and memcpy

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/config.h (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.h:17: class ConfigEntryBase {
On 2016/08/13 22:57:59, wtc1 wrote:
> Nit: this file has no comments.

Done.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.h:27: bool ParseInternal(std::queue<const
char*>* args, std::string *out);
On 2016/08/13 22:57:59, wtc1 wrote:
> Nit: the placement of "*" is not consistent.

Done.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.h:55: enum Status { OK, UnknownFlag,
MalformedArgument, MissingValue };
On 2016/08/13 22:57:59, wtc1 wrote:
> Nit: in the naming convention of the Google C++ Style Guide, these enumerators
> should start with "k":
> 
> https://google.github.io/styleguide/cppguide.html#Enumerator_Names

OK

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/config.h:77: ConfigEntryBase *entry(const
std::string& key) const {
On 2016/08/15 03:27:10, mt wrote:
> Should this return const ConfigEntryBase& instead?

No, because I need to be able to return nullptr

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/manifest.mn (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/manifest.mn:18: EXTRA_LIBS =
$(DIST)/lib/$(LIB_PREFIX)softokn.$(LIB_SUFFIX)
On 2016/08/13 23:03:51, ekr-webrtc wrote:
> On 2016/08/13 22:57:59, wtc1 wrote:
> > I am surprised that we need libdbm and libsoftokn.
> 
> We might not.They were defined in ssl_gtest. I will check.

Removed.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/nss_bogo_shim.cc (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nss_bogo_shim.cc:62: bool Init() {
On 2016/08/15 03:27:10, mt wrote:
> I think that this could be split up:
> 
> ConnectTcp(), which does the address/port/importFD
> 
> SetupVersion()
> 
> SetupResumption(), which does SSL_NO_CACHE,
> SSL_SetURL/ConfigServerSessionIDCache.
> 
> SetupKeys(), which loads the cert/private key and installs the various hooks.
> 
> SetupCipherSuites()
> 
> Then you can call SSL_ResetHandshake().

Split it a bit differently.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nss_bogo_shim.cc:149: return false;
On 2016/08/15 03:27:10, mt wrote:
> Is the intent of this to enable all cipher suites, why not just enumerate and
> enable all of them then (well at least the non-exportable ones).

Done

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nss_bogo_shim.cc:213: SECStatus ReadWrite() {
On 2016/08/15 03:27:10, mt wrote:
> You should at least comment here saying that this is essentially an echo
> server/client, which flips all the bits on the message it receives and writes
it
> back out again.

Done.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nss_bogo_shim.cc:221: if (!rv)
On 2016/08/15 03:27:10, mt wrote:
> nit: rv > 0

I am going to take this to mean rv == 0, because rv > 0 we handle below.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nss_bogo_shim.cc:221: if (!rv)
On 2016/08/15 03:27:10, mt wrote:
> nit: rv > 0

Done.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nss_bogo_shim.cc:230: if (rv !=
static_cast<int>(len)) {
On 2016/08/13 22:57:59, wtc1 wrote:
> You can declare |len| as int32_t and also declare
> the for loop variable |i| as int32_t. Then you don't
> need the static_cast<int> here.

Done.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nss_bogo_shim.cc:292: std::unique_ptr<TestAgent>
agent(TestAgent::Create(*cfg.get()));
On 2016/08/15 03:27:10, mt wrote:
> Why isn't *cfg sufficient?  Calling .get() is a red flag you should avoid.

Done.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nss_bogo_shim.cc:311: return false;
On 2016/08/13 23:03:51, ekr-webrtc wrote:
> On 2016/08/13 22:57:59, wtc1 wrote:
> > BUG: the return value of the main() function is the exit
> > status of the process. Its success/failure codes are opposite
> > to true and false. On error exit, main() should return 1 or
> > call exit(1). (Any nonzero integer <= 255 is fine.)
> 
> Thanks for catching this. This code moved from Init() and I missed the return
> value change.

Done.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/nsskeys.cc (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nsskeys.cc:77: PORT_ErrorToString(PORT_GetError())
<< "\n";
On 2016/08/15 03:27:10, mt wrote:
> Do we have a bug on file for not being able to import an ECDSA private key?

We do now.

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
File external_tests/nss_bogo_shim/nsskeys.h (right):

https://codereview.appspot.com/308900043/diff/20001/external_tests/nss_bogo_s...
external_tests/nss_bogo_shim/nsskeys.h:15: #include "pk11pub.h"
On 2016/08/13 22:57:59, wtc1 wrote:
> Nit: you only need to include the headers that define
> the SECKEYPrivateKey and CERTCertificate types.

Done.

[wtc1, 2016-08-15 16:15:15]

Eric: it seems that you didn't upload a new patch set.
Patch set 2 does not show many of the changes you made.

[ekr-rietveld, 2016-09-03 18:21:29]

Done.