cpp-netlib server

cpp-netlib server

[ekasper, 2013-06-28 15:32:52]

General comment: I *do* approve seeing a working example, no matter how
incomplete. But I think that perhaps a working example that only you can use is
not the best upstream commit point.

I'd actually suggest that now is a good time to disentangle and restructure the
server code. Perhaps remove all CT-specific code altogether and spend a bit of
time defining the interface and tests for events and request-response flows?
That should make it straightforward to plug the CT code back in later.

https://codereview.appspot.com/10685043/diff/1/src/Makefile
File src/Makefile (right):

https://codereview.appspot.com/10685043/diff/1/src/Makefile#newcode27
src/Makefile:27: INCLUDE += -I /usr/local/include -I
~/software/unpacked/cpp-netlib-0.9.4
Please clean up the local reference.

https://codereview.appspot.com/10685043/diff/1/src/Makefile#newcode33
src/Makefile:33: server/ct-rfc-server
I'd actually suggest we remove the old server altogether (and call this one
ct-server) - but fine to keep both while we're migrating.

https://codereview.appspot.com/10685043/diff/1/src/client/upload_server_cert.sh
File src/client/upload_server_cert.sh (right):

https://codereview.appspot.com/10685043/diff/1/src/client/upload_server_cert....
src/client/upload_server_cert.sh:10: CT_SERVER='localhost:9999'
Debugging leftovers? Presumably you still want this script to work against the
pilot log as advertised?

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc
File src/server/ct-rfc-server.cc (right):

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
src/server/ct-rfc-server.cc:4: #include <boost/network/protocol/http/server.hpp>
This is cpp-netlib "pretending" to be Boost? Maybe add a comment about this?

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
src/server/ct-rfc-server.cc:26: DEFINE_string(port, "9999", "Server port");
Why can't the port be an int?

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
src/server/ct-rfc-server.cc:130: RepeatedEvent(boost::asio::io_service *io,
I find the class hard to read - could you please comment who owns the io, and
how the timer is set up from the io object.

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
src/server/ct-rfc-server.cc:160: class Tick : public RepeatedEvent {
If this is for debugging then it should probably be a unit test somewhere
instead.

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
src/server/ct-rfc-server.cc:307: static KeyError ReadPrivateKey(EVP_PKEY **pkey,
This method looks like it belongs somewhere else, probably in openssl_util.h.

I also wonder if you really need the status here (since it's absorbed by the
CHECK anyway, and it's unlikely an application will be able to do anything smart
about the error). You could return a bool and log errors?

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
src/server/ct-rfc-server.cc:331: response = server::response::stock_reply(
I know I sound like a broken grammophone but instead of printing "hello" I'd
much prefer to see a test.

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
src/server/ct-rfc-server.cc:333: std::cerr << "[" << ip << "]: source = " <<
request.source
Please use VLOG.

[Ben Laurie (Google), 2013-07-01 11:44:44]

I'll update the CL when I have more functionality.

https://codereview.appspot.com/10685043/diff/1/src/Makefile
File src/Makefile (right):

https://codereview.appspot.com/10685043/diff/1/src/Makefile#newcode33
src/Makefile:33: server/ct-rfc-server
On 2013/06/28 15:32:52, ekasper wrote:
> I'd actually suggest we remove the old server altogether (and call this one
> ct-server) - but fine to keep both while we're migrating.

Yes, its there for reference for now.

https://codereview.appspot.com/10685043/diff/1/src/client/upload_server_cert.sh
File src/client/upload_server_cert.sh (right):

https://codereview.appspot.com/10685043/diff/1/src/client/upload_server_cert....
src/client/upload_server_cert.sh:10: CT_SERVER='localhost:9999'
On 2013/06/28 15:32:52, ekasper wrote:
> Debugging leftovers? Presumably you still want this script to work against the
> pilot log as advertised?

Will fix before upload (there will be further code review before then).

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc
File src/server/ct-rfc-server.cc (right):

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
src/server/ct-rfc-server.cc:4: #include <boost/network/protocol/http/server.hpp>
On 2013/06/28 15:32:52, ekasper wrote:
> This is cpp-netlib "pretending" to be Boost? Maybe add a comment about this?

Yes, it is.  Comment added.

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
src/server/ct-rfc-server.cc:26: DEFINE_string(port, "9999", "Server port");
On 2013/06/28 15:32:52, ekasper wrote:
> Why can't the port be an int?

cpp-netlib takes a string for some reason. Seemed easier to change the flag than
to convert an int to a string.

It is a little odd, I agree.

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
src/server/ct-rfc-server.cc:130: RepeatedEvent(boost::asio::io_service *io,
On 2013/06/28 15:32:52, ekasper wrote:
> I find the class hard to read - could you please comment who owns the io, and
> how the timer is set up from the io object. 

This is actually changed in 0.10.0 to a shared_ptr<>, which I think is clear.

Comment added.

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
src/server/ct-rfc-server.cc:160: class Tick : public RepeatedEvent {
On 2013/06/28 15:32:52, ekasper wrote:
> If this is for debugging then it should probably be a unit test somewhere
> instead.

Yeah, it'll be gone, it was just while I was getting the hang of things.

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
src/server/ct-rfc-server.cc:307: static KeyError ReadPrivateKey(EVP_PKEY **pkey,
On 2013/06/28 15:32:52, ekasper wrote:
> This method looks like it belongs somewhere else, probably in openssl_util.h.

Inherited from ct-server, but I can move it.

> I also wonder if you really need the status here (since it's absorbed by the
> CHECK anyway, and it's unlikely an application will be able to do anything
smart
> about the error). You could return a bool and log errors?

This is the way it was in ct-server, but happy to change it.

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
src/server/ct-rfc-server.cc:331: response = server::response::stock_reply(
On 2013/06/28 15:32:52, ekasper wrote:
> I know I sound like a broken grammophone but instead of printing "hello" I'd
> much prefer to see a test.

This is a standard part of cpp-netlib, so I do not intend to write tests for it
(cpp-netlib already tests it). The fleshed out version will be tested in the end
by re-enabling the client/server tests in the test-suite.

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
src/server/ct-rfc-server.cc:333: std::cerr << "[" << ip << "]: source = " <<
request.source
On 2013/06/28 15:32:52, ekasper wrote:
> Please use VLOG.

Done.

[ekasper, 2013-07-01 13:12:34]

Since, as you explained off-thread, you have more work planned before this CL is
ready for a real code review, I'm postponing detailed code comments for now.

(But please do always upload the latest diff as I can't see comments where
you've already added them.)

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc
File src/server/ct-rfc-server.cc (right):

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
src/server/ct-rfc-server.cc:307: static KeyError ReadPrivateKey(EVP_PKEY **pkey,
On 2013/07/01 11:44:44, Ben Laurie (Google) wrote:
> On 2013/06/28 15:32:52, ekasper wrote:
> > This method looks like it belongs somewhere else, probably in
openssl_util.h.
> 
> Inherited from ct-server, but I can move it.

Ah, you've copied it from the Services class.

I understand the migration is painful but it'd be nice if the assorted existing
parts needed to make it work didn't end up duplicated and scattered over this
single file. openssl_util is probably the better place for this particular one,
if you don't mind moving it.

> 
> > I also wonder if you really need the status here (since it's absorbed by the
> > CHECK anyway, and it's unlikely an application will be able to do anything
> smart
> > about the error). You could return a bool and log errors?
> 
> This is the way it was in ct-server, but happy to change it.

Up to you.

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
src/server/ct-rfc-server.cc:331: response = server::response::stock_reply(
On 2013/07/01 11:44:44, Ben Laurie (Google) wrote:
> On 2013/06/28 15:32:52, ekasper wrote:
> > I know I sound like a broken grammophone but instead of printing "hello" I'd
> > much prefer to see a test.
> 
> This is a standard part of cpp-netlib, so I do not intend to write tests for
it
> (cpp-netlib already tests it). The fleshed out version will be tested in the
end
> by re-enabling the client/server tests in the test-suite.

Surely cpp-netlib can't test that the response contains the "Hello" you intended
it to contain.

Like I said, I suggest the direction of this CL should be a structured,
unit-testable request-response handling.

[Eran, 2013-07-01 13:54:33]

drive-by review.
As Emilia pointed out, it's hard to review the structure of the server when it
doesn't actually do anything meaningful (especially figuring out how GET vs.
POST requests are handled).

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc
File src/server/ct-rfc-server.cc (right):

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
src/server/ct-rfc-server.cc:331: response = server::response::stock_reply(
It'd be clearer to see how cpp-netlib handles requests to different URLs in the
server if at least two different actions are implemented.

[Ben Laurie (Google), 2013-07-01 14:37:53]

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc
File src/server/ct-rfc-server.cc (right):

https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
src/server/ct-rfc-server.cc:331: response = server::response::stock_reply(
On 2013/07/01 13:54:34, Eran wrote:
> It'd be clearer to see how cpp-netlib handles requests to different URLs in
the
> server if at least two different actions are implemented.

Fear not, this will do that before it is final. Also, to respond to Emilia, the
"hello" will be gone (that's just a copy of one of cpp-netlib's examples) and
there will be tests for the real URLs.

[Ben Laurie (Google), 2013-07-01 14:38:02]

On 1 July 2013 14:12,  <ekasper@google.com> wrote:
> Since, as you explained off-thread, you have more work planned before
> this CL is ready for a real code review, I'm postponing detailed code
> comments for now.
>
> (But please do always upload the latest diff as I can't see comments
> where you've already added them.)

I've uploaded the latest diff.

>
>
>
> https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc
> File src/server/ct-rfc-server.cc (right):
>
>
https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
> src/server/ct-rfc-server.cc:307: static KeyError ReadPrivateKey(EVP_PKEY
> **pkey,
> On 2013/07/01 11:44:44, Ben Laurie (Google) wrote:
>>
>> On 2013/06/28 15:32:52, ekasper wrote:
>> > This method looks like it belongs somewhere else, probably in
>
> openssl_util.h.
>
>> Inherited from ct-server, but I can move it.
>
>
> Ah, you've copied it from the Services class.
>
> I understand the migration is painful but it'd be nice if the assorted
> existing parts needed to make it work didn't end up duplicated and
> scattered over this single file. openssl_util is probably the better
> place for this particular one, if you don't mind moving it.
>
>
>
>> > I also wonder if you really need the status here (since it's
>
> absorbed by the
>>
>> > CHECK anyway, and it's unlikely an application will be able to do
>
> anything
>>
>> smart
>> > about the error). You could return a bool and log errors?
>
>
>> This is the way it was in ct-server, but happy to change it.
>
>
> Up to you.
>
>
>
https://codereview.appspot.com/10685043/diff/1/src/server/ct-rfc-server.cc#ne...
> src/server/ct-rfc-server.cc:331: response =
> server::response::stock_reply(
> On 2013/07/01 11:44:44, Ben Laurie (Google) wrote:
>>
>> On 2013/06/28 15:32:52, ekasper wrote:
>> > I know I sound like a broken grammophone but instead of printing
>
> "hello" I'd
>>
>> > much prefer to see a test.
>
>
>> This is a standard part of cpp-netlib, so I do not intend to write
>
> tests for it
>>
>> (cpp-netlib already tests it). The fleshed out version will be tested
>
> in the end
>>
>> by re-enabling the client/server tests in the test-suite.
>
>
> Surely cpp-netlib can't test that the response contains the "Hello" you
> intended it to contain.
>
> Like I said, I suggest the direction of this CL should be a structured,
> unit-testable request-response handling.
>
> https://codereview.appspot.com/10685043/

[Ben Laurie (Google), 2013-07-17 16:41:09]

PTAL. Note: no tests yet, probably simplest to test against Eran's new Java
client.

[Eran, 2013-07-18 10:45:14]

Looks good other than the switch statement in src/log/frontend.cc. Good to go
once that's fixed.

https://codereview.appspot.com/10685043/diff/27001/src/client/upload_server_c...
File src/client/upload_server_cert.sh (right):

https://codereview.appspot.com/10685043/diff/27001/src/client/upload_server_c...
src/client/upload_server_cert.sh:7: export PYTHONPATH=$PYTHONPATH:../python
Does sh on FreeBSD do variable name escaping with {} ?
This line could read
export PYTHONPATH=${PYTHONPATH}:../python

https://codereview.appspot.com/10685043/diff/27001/src/log/cert_submission_ha...
File src/log/cert_submission_handler.h (right):

https://codereview.appspot.com/10685043/diff/27001/src/log/cert_submission_ha...
src/log/cert_submission_handler.h:34: // This may change |chain|.
In what way?

https://codereview.appspot.com/10685043/diff/27001/src/log/frontend.cc
File src/log/frontend.cc (right):

https://codereview.appspot.com/10685043/diff/27001/src/log/frontend.cc#newcode34
src/log/frontend.cc:34: SubmitResult result = GetSubmitError(pre_result);
Nit: inline 'result' (matter of personal preference though)

https://codereview.appspot.com/10685043/diff/27001/src/log/frontend.cc#newcode45
src/log/frontend.cc:45: result = NEW;
I find this rather confusing. Is this FrontEndSigner::SubmitResult::NEW? Or
Frontend::SubmitResult::NEW? Accidentally (or not), these two enum values happen
to share the same numeric value, so this switch statement would behave as
desired. This is not documented as a requirement in either of the enums,
however.

https://codereview.appspot.com/10685043/diff/27001/src/log/frontend.cc#newcode54
src/log/frontend.cc:54: UpdateStats(entry.type(), result);
Nit: Seems like the stats are updated on any flow of this method. In which case,
you could have a local variable that calls UpdateStats upon destruction.

[Ben Laurie (Google), 2013-07-18 12:09:31]

PTAL

https://codereview.appspot.com/10685043/diff/27001/src/client/upload_server_c...
File src/client/upload_server_cert.sh (right):

https://codereview.appspot.com/10685043/diff/27001/src/client/upload_server_c...
src/client/upload_server_cert.sh:7: export PYTHONPATH=$PYTHONPATH:../python
On 2013/07/18 10:45:14, Eran wrote:
> Does sh on FreeBSD do variable name escaping with {} ?
> This line could read
> export PYTHONPATH=${PYTHONPATH}:../python

This is shell behaviour, so is common across platforms. In other words: yes.

https://codereview.appspot.com/10685043/diff/27001/src/log/cert_submission_ha...
File src/log/cert_submission_handler.h (right):

https://codereview.appspot.com/10685043/diff/27001/src/log/cert_submission_ha...
src/log/cert_submission_handler.h:34: // This may change |chain|.
On 2013/07/18 10:45:14, Eran wrote:
> In what way?

Well, I didn't want to specify what way, but currently it may truncate it.

https://codereview.appspot.com/10685043/diff/27001/src/log/frontend.cc
File src/log/frontend.cc (right):

https://codereview.appspot.com/10685043/diff/27001/src/log/frontend.cc#newcode34
src/log/frontend.cc:34: SubmitResult result = GetSubmitError(pre_result);
On 2013/07/18 10:45:14, Eran wrote:
> Nit: inline 'result' (matter of personal preference though)

Huh? Its used twice.

https://codereview.appspot.com/10685043/diff/27001/src/log/frontend.cc#newcode45
src/log/frontend.cc:45: result = NEW;
On 2013/07/18 10:45:14, Eran wrote:
> I find this rather confusing. Is this FrontEndSigner::SubmitResult::NEW? Or
> Frontend::SubmitResult::NEW? Accidentally (or not), these two enum values
happen
> to share the same numeric value, so this switch statement would behave as
> desired. This is not documented as a requirement in either of the enums,
> however.

Well spotted. In my defence, I inherited this code :-)

https://codereview.appspot.com/10685043/diff/27001/src/log/frontend.cc#newcode54
src/log/frontend.cc:54: UpdateStats(entry.type(), result);
On 2013/07/18 10:45:14, Eran wrote:
> Nit: Seems like the stats are updated on any flow of this method. In which
case,
> you could have a local variable that calls UpdateStats upon destruction.

That makes me slightly nauseous.

[Eran, 2013-07-18 14:49:38]

LGTM (please have a look at the comment in cert_submission_handler.h)

https://codereview.appspot.com/10685043/diff/27001/src/log/cert_submission_ha...
File src/log/cert_submission_handler.h (right):

https://codereview.appspot.com/10685043/diff/27001/src/log/cert_submission_ha...
src/log/cert_submission_handler.h:34: // This may change |chain|.
On 2013/07/18 12:09:31, Ben Laurie (Google) wrote:
> On 2013/07/18 10:45:14, Eran wrote:
> > In what way?
> 
> Well, I didn't want to specify what way, but currently it may truncate it.

How is the caller affected? Does that mean the CertChain object they passed in
may be unusable? If so, why not take ownership of it and destroy it?

https://codereview.appspot.com/10685043/diff/27001/src/log/frontend.cc
File src/log/frontend.cc (right):

https://codereview.appspot.com/10685043/diff/27001/src/log/frontend.cc#newcode34
src/log/frontend.cc:34: SubmitResult result = GetSubmitError(pre_result);
On 2013/07/18 12:09:31, Ben Laurie (Google) wrote:
> On 2013/07/18 10:45:14, Eran wrote:
> > Nit: inline 'result' (matter of personal preference though)
> 
> Huh? Its used twice.

Right - missed that.

https://codereview.appspot.com/10685043/diff/27001/src/log/frontend.cc#newcode54
src/log/frontend.cc:54: UpdateStats(entry.type(), result);
On 2013/07/18 12:09:31, Ben Laurie (Google) wrote:
> On 2013/07/18 10:45:14, Eran wrote:
> > Nit: Seems like the stats are updated on any flow of this method. In which
> case,
> > you could have a local variable that calls UpdateStats upon destruction.
> 
> That makes me slightly nauseous.

While not a valid technical argument, it is a highly entertaining one, so I
definitely accept it.

Ultimately it is a mechanism to guarantee that statistics are logged from this
method no matter how it exists, but I understand if it's not critical.

https://codereview.appspot.com/10685043/diff/33001/src/log/cert_submission_ha...
File src/log/cert_submission_handler.cc (right):

https://codereview.appspot.com/10685043/diff/33001/src/log/cert_submission_ha...
src/log/cert_submission_handler.cc:161: LogEntry *entry) {
The output parameter type in the previous patchset made more sense, since the
only field of the LogEntry modified is the Precert entry.

[Ben Laurie (Google), 2013-07-18 16:02:47]

On 2013/07/18 14:49:38, Eran wrote:
> LGTM (please have a look at the comment in cert_submission_handler.h)

I think you meant .cc.

> 
>
https://codereview.appspot.com/10685043/diff/27001/src/log/cert_submission_ha...
> File src/log/cert_submission_handler.h (right):
> 
>
https://codereview.appspot.com/10685043/diff/27001/src/log/cert_submission_ha...
> src/log/cert_submission_handler.h:34: // This may change |chain|.
> On 2013/07/18 12:09:31, Ben Laurie (Google) wrote:
> > On 2013/07/18 10:45:14, Eran wrote:
> > > In what way?
> > 
> > Well, I didn't want to specify what way, but currently it may truncate it.
> 
> How is the caller affected? Does that mean the CertChain object they passed in
> may be unusable? If so, why not take ownership of it and destroy it?

It means its changed, so if the caller wants to operate on it again, they'd
better keep a copy. In practice, the caller cares not, and the reason not to
take control is because the caller typically has a local object which you cannot
take control of.

> 
> https://codereview.appspot.com/10685043/diff/27001/src/log/frontend.cc
> File src/log/frontend.cc (right):
> 
>
https://codereview.appspot.com/10685043/diff/27001/src/log/frontend.cc#newcode34
> src/log/frontend.cc:34: SubmitResult result = GetSubmitError(pre_result);
> On 2013/07/18 12:09:31, Ben Laurie (Google) wrote:
> > On 2013/07/18 10:45:14, Eran wrote:
> > > Nit: inline 'result' (matter of personal preference though)
> > 
> > Huh? Its used twice.
> 
> Right - missed that.
> 
>
https://codereview.appspot.com/10685043/diff/27001/src/log/frontend.cc#newcode54
> src/log/frontend.cc:54: UpdateStats(entry.type(), result);
> On 2013/07/18 12:09:31, Ben Laurie (Google) wrote:
> > On 2013/07/18 10:45:14, Eran wrote:
> > > Nit: Seems like the stats are updated on any flow of this method. In which
> > case,
> > > you could have a local variable that calls UpdateStats upon destruction.
> > 
> > That makes me slightly nauseous.
> 
> While not a valid technical argument, it is a highly entertaining one, so I
> definitely accept it.
> 
> Ultimately it is a mechanism to guarantee that statistics are logged from this
> method no matter how it exists, but I understand if it's not critical.
> 
>
https://codereview.appspot.com/10685043/diff/33001/src/log/cert_submission_ha...
> File src/log/cert_submission_handler.cc (right):
> 
>
https://codereview.appspot.com/10685043/diff/33001/src/log/cert_submission_ha...
> src/log/cert_submission_handler.cc:161: LogEntry *entry) {
> The output parameter type in the previous patchset made more sense, since the
> only field of the LogEntry modified is the Precert entry.

[Ben Laurie (Google), 2013-07-18 16:02:59]

https://codereview.appspot.com/10685043/diff/33001/src/log/cert_submission_ha...
File src/log/cert_submission_handler.cc (right):

https://codereview.appspot.com/10685043/diff/33001/src/log/cert_submission_ha...
src/log/cert_submission_handler.cc:161: LogEntry *entry) {
On 2013/07/18 14:49:39, Eran wrote:
> The output parameter type in the previous patchset made more sense, since the
> only field of the LogEntry modified is the Precert entry.

My view is the caller doesn't care, the caller just wants a LogEntry. Besides, I
intend to try to refactor this repetition away, so it will be necessary to pass
a LogEntry.