Fetch and store raw log entries

Fetch and store raw log entries

[ekasper, 2013-08-13 15:55:06]

Next in a series of cert monitoring CLs: set up get-entries update loop. This
doesn't yet parse the entries; we simply stick them in a temporary data store.
There are lots of #TODO comments in the monitor.py file that should outline
missing steps. The core idea is to first fetch entries to temporary storage and
verify the hash, and only then parse them.

I ran this overnight, and fetched the entire log, plus a couple of incremental
updates, without any problems.

This CL also modifies the STH verification behaviour slightly. I've a) pulled
temporal constraints into a separate method and b) changed EncodingError not to
inherit from VerifyError as this, while convenient, is clearly wrong.

[Eran, 2013-08-14 15:05:40]

Mostly reviewing the monitor code - have not looked at any test code yet.
Overall the code looks straightforward and easy to understand.
The monitor seems to be doing a lot of things - if it can be broken down a bit,
it'll be easier to see the flow of the loop described in the design doc.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
File src/python/ct/client/monitor.py (right):

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:21: self.__temp_db = temp_db
IMHO '__unverified_db' is a more suitable name - makes it easier to notice, when
inspecting the code, if it's incorrectly used.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:56: serialized_state =
new_state.SerializeToString()
nit: By having a state provider you could move all of the file handling code
from this class (the monitor code is big enough even without it).
Said state provider will have the responsibility of writing the state to a file.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:101: # If we have no verified STH, this will
also work with 0-defaults.
The 2nd part of the comment (0-defaults) is unclear.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:111: self.__state.verified_sth, sth_response)
Incorrect indentation

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:129: def __estimate_time(num_new_entries):
Amusing

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:137: def update_entries(self):
Make this method and update_sth protected?
Seems like 'update' should be the only publicly visible method in this class:
The only 'proper' way to drive the monitoring process is by calling the update
method. Clients may need to query the progress of the monitoring (e.g. if
there's a valid, pending STH but no entries yet)

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:143: raise RuntimeError("update_entries called
without a pending STH: "
Use assert?

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:148:
self.__temp_db.drop_storage(self.servername)
The interface has 'drop_table' defined but the squlite_temp_db implementation
has drop_storage, so there's some inconsistency here.
As we discussed offline, I suggest replacing this abstraction with a per-log
instance that will have the following methods:
* drop_all_entries
* write_entries (overrides)
* iterate_over_entries

The fact that this entries storage object is implemented by a database is an
implementation detail, so table name and log name should not matter here.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:154: if (self.__state.pending_sth.tree_size >
num_entries):
Nit: extract the body of the if statement to a separate method.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:165: new_entries, FLAGS.entry_write_batch_size))
nit: I personally find that having flags usage inside the code make it harder to
test and later control. I'd suggest making it an optional c'tor argument. In
this case, though, it may not be critical.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:170: self.__temp_db.drop_table(self.servername)
sqlite_temp_db does not have a drop_table method...

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:174: numbered_batch =
zip(range(next_sequence_number,
I'm not sure I understand why the batching is done here. Is it because the
underlying storage object (__temp_db) works more efficiently with batches? if
that's the case, it can do buffering internally.

If it's because sequence numbers have to be assigned to the entries, that can be
done by creating another generator that takes the new_entries and
next_sequence_number as input and yields pairs of (number, entry) when iterated.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/prober.py
File src/python/ct/client/prober.py (right):

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/prober...
src/python/ct/client/prober.py:34:
self.__monitors.append(monitor.Monitor(client, verifier, db,
That implies the db is thread-safe. Is it?

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/sqlite...
File src/python/ct/client/sqlite_temp_db.py (right):

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/sqlite...
src/python/ct/client/sqlite_temp_db.py:9: class SQLiteTempDB(object):
As discussed offline, perhaps an instance of this class per log with a slightly
different API will be a clearer abstraction of the unverified storage.

[ekasper, 2013-08-15 16:41:37]

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
File src/python/ct/client/monitor.py (right):

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:21: self.__temp_db = temp_db
On 2013/08/14 15:05:41, Eran wrote:
> IMHO '__unverified_db' is a more suitable name - makes it easier to notice,
when
> inspecting the code, if it's incorrectly used.

Done.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:56: serialized_state =
new_state.SerializeToString()
On 2013/08/14 15:05:41, Eran wrote:
> nit: By having a state provider you could move all of the file handling code
> from this class (the monitor code is big enough even without it).
> Said state provider will have the responsibility of writing the state to a
file.

That's a nice suggestion, done.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:101: # If we have no verified STH, this will
also work with 0-defaults.
On 2013/08/14 15:05:41, Eran wrote:
> The 2nd part of the comment (0-defaults) is unclear.

Clarified.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:111: self.__state.verified_sth, sth_response)
On 2013/08/14 15:05:41, Eran wrote:
> Incorrect indentation

Done.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:137: def update_entries(self):
On 2013/08/14 15:05:41, Eran wrote:
> Make this method and update_sth protected?
> Seems like 'update' should be the only publicly visible method in this class:
> The only 'proper' way to drive the monitoring process is by calling the update
> method. Clients may need to query the progress of the monitoring (e.g. if
> there's a valid, pending STH but no entries yet)

Yeah, they're mostly exposed for more modular testing, so I've made them
protected.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:143: raise RuntimeError("update_entries called
without a pending STH: "
On 2013/08/14 15:05:41, Eran wrote:
> Use assert?

Done.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:148:
self.__temp_db.drop_storage(self.servername)
On 2013/08/14 15:05:41, Eran wrote:
> The interface has 'drop_table' defined but the squlite_temp_db implementation
> has drop_storage, so there's some inconsistency here.

Good catch, the sqlite class wasn't inheriting from the interface. Fixed.

> As we discussed offline, I suggest replacing this abstraction with a per-log
> instance that will have the following methods:
> * drop_all_entries
> * write_entries (overrides)

I think that overwriting is highly likely to hide bugs so I've kept the error.

> * iterate_over_entries
> 
> The fact that this entries storage object is implemented by a database is an
> implementation detail, so table name and log name should not matter here.

In general I disagree (you don't need a storage object per entity) but in this
particular case I can't think of any aggregation we'd be doing over unverified
data from multiple logs, so I've changed the interface.

I've also changed the SQLite implementation because if the data is functionally
independent, then it might as well reside in a separate file.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:154: if (self.__state.pending_sth.tree_size >
num_entries):
On 2013/08/14 15:05:41, Eran wrote:
> Nit: extract the body of the if statement to a separate method.

Done.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:165: new_entries, FLAGS.entry_write_batch_size))
On 2013/08/14 15:05:41, Eran wrote:
> nit: I personally find that having flags usage inside the code make it harder
to
> test and later control. I'd suggest making it an optional c'tor argument. In
> this case, though, it may not be critical.

It's in a private method now, so I don't think it's critical.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:170: self.__temp_db.drop_table(self.servername)
On 2013/08/14 15:05:41, Eran wrote:
> sqlite_temp_db does not have a drop_table method...

Renaming bug, fixed.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/monito...
src/python/ct/client/monitor.py:174: numbered_batch =
zip(range(next_sequence_number,
On 2013/08/14 15:05:41, Eran wrote:
> I'm not sure I understand why the batching is done here. Is it because the
> underlying storage object (__temp_db) works more efficiently with batches? if
> that's the case, it can do buffering internally.

Yes, it's because transactions are expensive.

The current approach is to provide high-level transactional API calls. Internal
buffering would require a complete redesign to expose commit and rollback logic
to the caller.

> 
> If it's because sequence numbers have to be assigned to the entries, that can
be
> done by creating another generator that takes the new_entries and
> next_sequence_number as input and yields pairs of (number, entry) when
iterated.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/prober.py
File src/python/ct/client/prober.py (right):

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/prober...
src/python/ct/client/prober.py:34:
self.__monitors.append(monitor.Monitor(client, verifier, db,
On 2013/08/14 15:05:41, Eran wrote:
> That implies the db is thread-safe. Is it? 

It is, though I've not yet implemented handling locking errors due to
concurrency.

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/sqlite...
File src/python/ct/client/sqlite_temp_db.py (right):

https://codereview.appspot.com/12862043/diff/3001/src/python/ct/client/sqlite...
src/python/ct/client/sqlite_temp_db.py:9: class SQLiteTempDB(object):
On 2013/08/14 15:05:41, Eran wrote:
> As discussed offline, perhaps an instance of this class per log with a
slightly
> different API will be a clearer abstraction of the unverified storage.

Done - the SQLiteTempDBFactory is now responsible for dispatching an instance
per log.

[Eran, 2013-08-16 11:06:27]

Overall looks good, see minor comments below.
We could discuss consistency across languages (C++, Python and Java), as the
python code throws exceptions while other two languages use return values for
verification results.

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
File src/python/ct/client/monitor.py (right):

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
src/python/ct/client/monitor.py:31: except state.FileNotFoundError:
Nit: You could have this try/except block in the state_keeper, assuming it's
always OK to start with an empty state (or have a separate method like
readOrEmptyState).

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
src/python/ct/client/monitor.py:138: while True:
Nit: I personally find 'while True' statements unhelpful, because they force me
to go looking in the code for the break statement and figure out the flow
leading to it, rather than stating clearly when the loop should stop. This is
not a 'run forever' loop, so I'd have liked to see the condition for its
continuation clearly stated.

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
src/python/ct/client/monitor.py:155: logging.info("Fetched %d entries" %
len(entry_batch))
Nit: Log this before storing to the DB, so we know how much was fetched even if
writing to the DB failed.

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
File src/python/ct/client/monitor_test.py (right):

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
src/python/ct/client/monitor_test.py:46: _DEFAULT_STH.timestamp =
(long)(time.time()*1000)
Consider using a constant value here, to make the test 100% deterministic. 
(To be clear: I can't see a scenario in the test where it would behave
differently based on the current time, but then again, such cases are usually
surprising :)

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
src/python/ct/client/monitor_test.py:90: verifier = mock.Mock()
Create the verifier in the setUp method, since it seems to be initialized in the
same way for all test cases.

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
src/python/ct/client/monitor_test.py:91: m = monitor.Monitor(client, verifier,
self.db, self.temp_db,
Nit: Consider a helper method for creating the monitor - it seems to be created
the same way in all test cases, save for the client.

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/probe...
File src/python/ct/client/prober.py (right):

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/probe...
src/python/ct/client/prober.py:50: if monitor.update():
This means that a single log failing to respond (timing out) would delay the
whole loop. I guess that's fine for now.
Have a look at Twisted - it simplifies networking and is event-driven. It may
not be useful right now, but worth keeping in mind if the monitor becomes more
complex.

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/probe...
src/python/ct/client/prober.py:65: sleep_time = (self.__last_update_start_time +
sleep_time = max(..., 0) ?

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/probe...
src/python/ct/client/prober.py:67: if sleep_time > 0:
Then this check would be superfluous.

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/crypto/verif...
File src/python/ct/crypto/verify.py (right):

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/crypto/verif...
src/python/ct/crypto/verify.py:95: @error.returns_true_or_raises
Generally, for all the verify methods:
* Returning True does not provide any information to the caller if the method's
promise is to throw if the verification fails.

* I'd expect methods to throw only if something truly unexpected happened during
execution. Since the data we deal with here is untrusted, verification failure
should be considered a part of normal operation, rather than exceptional
circumstances.
Notice that a lot of the client code using this verification method does:
try:
   verifySomething:
except SomeError:
   # log failure
   return False

This pattern repeats sometimes several times in the same method. Checking a
return value seems more appropriate in this case (that's what the C++ code does
as well as my Java client).

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/dashboard/da...
File src/python/ct/dashboard/dashboard.py (right):

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/dashboard/da...
src/python/ct/dashboard/dashboard.py:24: gflags.DEFINE_string("ct_sqlite_db",
"/tmp/ct", "Location of the CT database")
nit: Have one base directory and derive db, temp db and state dirs from that.

[ekasper, 2013-08-16 11:55:50]

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
File src/python/ct/client/monitor.py (right):

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
src/python/ct/client/monitor.py:31: except state.FileNotFoundError:
On 2013/08/16 11:06:27, Eran wrote:
> Nit: You could have this try/except block in the state_keeper, assuming it's
> always OK to start with an empty state (or have a separate method like
> readOrEmptyState).

See the TODO one line below.

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
src/python/ct/client/monitor.py:138: while True:
On 2013/08/16 11:06:27, Eran wrote:
> Nit: I personally find 'while True' statements unhelpful, because they force
me
> to go looking in the code for the break statement and figure out the flow
> leading to it, rather than stating clearly when the loop should stop. This is
> not a 'run forever' loop, so I'd have liked to see the condition for its
> continuation clearly stated.

I've made it a bit stricter, and added a comment.

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
src/python/ct/client/monitor.py:155: logging.info("Fetched %d entries" %
len(entry_batch))
On 2013/08/16 11:06:27, Eran wrote:
> Nit: Log this before storing to the DB, so we know how much was fetched even
if
> writing to the DB failed.

Done.

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
File src/python/ct/client/monitor_test.py (right):

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
src/python/ct/client/monitor_test.py:46: _DEFAULT_STH.timestamp =
(long)(time.time()*1000)
On 2013/08/16 11:06:27, Eran wrote:
> Consider using a constant value here, to make the test 100% deterministic. 
> (To be clear: I can't see a scenario in the test where it would behave
> differently based on the current time, but then again, such cases are usually
> surprising :)

I didn't want this to become susceptible to possible stale data checks but there
are currently none in place, so I've removed this.

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
src/python/ct/client/monitor_test.py:90: verifier = mock.Mock()
On 2013/08/16 11:06:27, Eran wrote:
> Create the verifier in the setUp method, since it seems to be initialized in
the
> same way for all test cases.

Done.

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
src/python/ct/client/monitor_test.py:91: m = monitor.Monitor(client, verifier,
self.db, self.temp_db,
On 2013/08/16 11:06:27, Eran wrote:
> Nit: Consider a helper method for creating the monitor - it seems to be
created
> the same way in all test cases, save for the client.

I think the tests are easier to read when the monitor dependencies are not
hidden in a helper.

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/probe...
File src/python/ct/client/prober.py (right):

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/probe...
src/python/ct/client/prober.py:50: if monitor.update():
On 2013/08/16 11:06:27, Eran wrote:
> This means that a single log failing to respond (timing out) would delay the
> whole loop. I guess that's fine for now.
> Have a look at Twisted - it simplifies networking and is event-driven. It may
> not be useful right now, but worth keeping in mind if the monitor becomes more
> complex.

Will keep in mind. I don't think there's point in spending time on event-driven
monitoring before we have more than one log to monitor.

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/probe...
src/python/ct/client/prober.py:65: sleep_time = (self.__last_update_start_time +
On 2013/08/16 11:06:27, Eran wrote:
> sleep_time = max(..., 0) ?

Done.

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/probe...
src/python/ct/client/prober.py:67: if sleep_time > 0:
On 2013/08/16 11:06:27, Eran wrote:
> Then this check would be superfluous.

Removed.

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/crypto/verif...
File src/python/ct/crypto/verify.py (right):

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/crypto/verif...
src/python/ct/crypto/verify.py:95: @error.returns_true_or_raises
On 2013/08/16 11:06:27, Eran wrote:
> Generally, for all the verify methods:
> * Returning True does not provide any information to the caller if the
method's
> promise is to throw if the verification fails.
> 
> * I'd expect methods to throw only if something truly unexpected happened
during
> execution. Since the data we deal with here is untrusted, verification failure
> should be considered a part of normal operation, rather than exceptional
> circumstances.
> Notice that a lot of the client code using this verification method does:
> try:
>    verifySomething:
> except SomeError:
>    # log failure
>    return False
> 
> This pattern repeats sometimes several times in the same method. Checking a
> return value seems more appropriate in this case (that's what the C++ code
does
> as well as my Java client).

Python is not C++ or Java (in our C++ exceptions are forbidden).

This is a conscious design decision, based on my own experience in crypto API
(mis)use, and discussions with colleagues who use existing Python crypto APIs.
Return codes are a terrible thing for crypto, even more so in Python where the
return value can be 1, 0, -1, "horse", None, [], or Sheep().

Please read ct/crypto/error.py. Philosophically, I argue that a cryptographic
verification error _is_ an exceptional situation - when I call verify() I
*expect* the input to verify and when it doesn't, I should stop doing whatever I
was doing (displaying the user pictures of cats) and switch to exception
handling (scary browser interstitials).

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/dashboard/da...
File src/python/ct/dashboard/dashboard.py (right):

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/dashboard/da...
src/python/ct/dashboard/dashboard.py:24: gflags.DEFINE_string("ct_sqlite_db",
"/tmp/ct", "Location of the CT database")
On 2013/08/16 11:06:27, Eran wrote:
> nit: Have one base directory and derive db, temp db and state dirs from that.

I've added a TODO as I'd like to set up a nice config and not rush it.

[Eran, 2013-08-19 09:17:02]

Overall looks good based on your replies to my comments - can't verify as I only
see patchset #5 without some changes you've mentioned (for example the change to
the 'while True' loop in monitor.py).
Once I can see that, would be good to go.

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
File src/python/ct/client/monitor_test.py (right):

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
src/python/ct/client/monitor_test.py:91: m = monitor.Monitor(client, verifier,
self.db, self.temp_db,
On 2013/08/16 11:55:50, ekasper wrote:
> On 2013/08/16 11:06:27, Eran wrote:
> > Nit: Consider a helper method for creating the monitor - it seems to be
> created
> > the same way in all test cases, save for the client.
> 
> I think the tests are easier to read when the monitor dependencies are not
> hidden in a helper.

That does mean that changing the behaviour of the class-under-test is more
cumbersome - many call sites to change.

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/crypto/verif...
File src/python/ct/crypto/verify.py (right):

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/crypto/verif...
src/python/ct/crypto/verify.py:95: @error.returns_true_or_raises
We're definitely in agreement that designing crypto APIs is hard. If it wasn't,
we'd have seen a good solution a long time ago..
Basically the problem is that there's no way to force the caller to deal with
verification failure - Exceptions are not checked in Python and it's perfectly
valid to ignore a function's return value.
I'm willing to see if this approach leads to less mistakes when using the API.

in error.py you mention that the 'True' return value is for tests to verify
there's no return path in the code that's not covered.
In this case, I'd suggest not documenting this return value - because the user
isn't expected to check for it (so let's not make it a part of the contract for
these verification methods).
The same goal (all return paths covered) can be achieved by coverage checking,
rather than a return value.

On 2013/08/16 11:55:50, ekasper wrote:
> On 2013/08/16 11:06:27, Eran wrote:
> > Generally, for all the verify methods:
> > * Returning True does not provide any information to the caller if the
> method's
> > promise is to throw if the verification fails.
> > 
> > * I'd expect methods to throw only if something truly unexpected happened
> during
> > execution. Since the data we deal with here is untrusted, verification
failure
> > should be considered a part of normal operation, rather than exceptional
> > circumstances.
> > Notice that a lot of the client code using this verification method does:
> > try:
> >    verifySomething:
> > except SomeError:
> >    # log failure
> >    return False
> > 
> > This pattern repeats sometimes several times in the same method. Checking a
> > return value seems more appropriate in this case (that's what the C++ code
> does
> > as well as my Java client).
> 
> Python is not C++ or Java (in our C++ exceptions are forbidden).
> 
> This is a conscious design decision, based on my own experience in crypto API
> (mis)use, and discussions with colleagues who use existing Python crypto APIs.
> Return codes are a terrible thing for crypto, even more so in Python where the
> return value can be 1, 0, -1, "horse", None, [], or Sheep().
> 
> Please read ct/crypto/error.py. Philosophically, I argue that a cryptographic
> verification error _is_ an exceptional situation - when I call verify() I
> *expect* the input to verify and when it doesn't, I should stop doing whatever
I
> was doing (displaying the user pictures of cats) and switch to exception
> handling (scary browser interstitials).

[ekasper, 2013-08-20 09:52:04]

On 2013/08/19 09:17:02, Eran wrote:
> Overall looks good based on your replies to my comments - can't verify as I
only
> see patchset #5 without some changes you've mentioned (for example the change
to
> the 'while True' loop in monitor.py).
> Once I can see that, would be good to go.

The 'while True' change is in patchset #4...

> 
>
https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
> File src/python/ct/client/monitor_test.py (right):
> 
>
https://codereview.appspot.com/12862043/diff/10001/src/python/ct/client/monit...
> src/python/ct/client/monitor_test.py:91: m = monitor.Monitor(client, verifier,
> self.db, self.temp_db,
> On 2013/08/16 11:55:50, ekasper wrote:
> > On 2013/08/16 11:06:27, Eran wrote:
> > > Nit: Consider a helper method for creating the monitor - it seems to be
> > created
> > > the same way in all test cases, save for the client.
> > 
> > I think the tests are easier to read when the monitor dependencies are not
> > hidden in a helper.
> 
> That does mean that changing the behaviour of the class-under-test is more
> cumbersome - many call sites to change.
> 
>
https://codereview.appspot.com/12862043/diff/10001/src/python/ct/crypto/verif...
> File src/python/ct/crypto/verify.py (right):
> 
>
https://codereview.appspot.com/12862043/diff/10001/src/python/ct/crypto/verif...
> src/python/ct/crypto/verify.py:95: @error.returns_true_or_raises
> We're definitely in agreement that designing crypto APIs is hard. If it
wasn't,
> we'd have seen a good solution a long time ago..
> Basically the problem is that there's no way to force the caller to deal with
> verification failure - Exceptions are not checked in Python and it's perfectly
> valid to ignore a function's return value.
> I'm willing to see if this approach leads to less mistakes when using the API.
> 
> in error.py you mention that the 'True' return value is for tests to verify
> there's no return path in the code that's not covered.
> In this case, I'd suggest not documenting this return value - because the user
> isn't expected to check for it (so let's not make it a part of the contract
for
> these verification methods).
> The same goal (all return paths covered) can be achieved by coverage checking,
> rather than a return value.
> 
> On 2013/08/16 11:55:50, ekasper wrote:
> > On 2013/08/16 11:06:27, Eran wrote:
> > > Generally, for all the verify methods:
> > > * Returning True does not provide any information to the caller if the
> > method's
> > > promise is to throw if the verification fails.
> > > 
> > > * I'd expect methods to throw only if something truly unexpected happened
> > during
> > > execution. Since the data we deal with here is untrusted, verification
> failure
> > > should be considered a part of normal operation, rather than exceptional
> > > circumstances.
> > > Notice that a lot of the client code using this verification method does:
> > > try:
> > >    verifySomething:
> > > except SomeError:
> > >    # log failure
> > >    return False
> > > 
> > > This pattern repeats sometimes several times in the same method. Checking
a
> > > return value seems more appropriate in this case (that's what the C++ code
> > does
> > > as well as my Java client).
> > 
> > Python is not C++ or Java (in our C++ exceptions are forbidden).
> > 
> > This is a conscious design decision, based on my own experience in crypto
API
> > (mis)use, and discussions with colleagues who use existing Python crypto
APIs.
> > Return codes are a terrible thing for crypto, even more so in Python where
the
> > return value can be 1, 0, -1, "horse", None, [], or Sheep().
> > 
> > Please read ct/crypto/error.py. Philosophically, I argue that a
cryptographic
> > verification error _is_ an exceptional situation - when I call verify() I
> > *expect* the input to verify and when it doesn't, I should stop doing
whatever
> I
> > was doing (displaying the user pictures of cats) and switch to exception
> > handling (scary browser interstitials).

[ekasper, 2013-08-20 09:52:36]

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/crypto/verif...
File src/python/ct/crypto/verify.py (right):

https://codereview.appspot.com/12862043/diff/10001/src/python/ct/crypto/verif...
src/python/ct/crypto/verify.py:95: @error.returns_true_or_raises
On 2013/08/19 09:17:02, Eran wrote:
> We're definitely in agreement that designing crypto APIs is hard. If it
wasn't,
> we'd have seen a good solution a long time ago..
> Basically the problem is that there's no way to force the caller to deal with
> verification failure - Exceptions are not checked in Python and it's perfectly
> valid to ignore a function's return value.
> I'm willing to see if this approach leads to less mistakes when using the API.

Thanks!

> 
> in error.py you mention that the 'True' return value is for tests to verify
> there's no return path in the code that's not covered.
> In this case, I'd suggest not documenting this return value - because the user
> isn't expected to check for it (so let's not make it a part of the contract
for
> these verification methods).
> The same goal (all return paths covered) can be achieved by coverage checking,
> rather than a return value.

Actually, returning True allows me to use the defensive decorator transparently
- it is a noop, unless there is a bug. But it is part of the contract regardless
of whether the decorator is used to enforce it, so I don't see why we should
hide it/not test it.

> 
> On 2013/08/16 11:55:50, ekasper wrote:
> > On 2013/08/16 11:06:27, Eran wrote:
> > > Generally, for all the verify methods:
> > > * Returning True does not provide any information to the caller if the
> > method's
> > > promise is to throw if the verification fails.
> > > 
> > > * I'd expect methods to throw only if something truly unexpected happened
> > during
> > > execution. Since the data we deal with here is untrusted, verification
> failure
> > > should be considered a part of normal operation, rather than exceptional
> > > circumstances.
> > > Notice that a lot of the client code using this verification method does:
> > > try:
> > >    verifySomething:
> > > except SomeError:
> > >    # log failure
> > >    return False
> > > 
> > > This pattern repeats sometimes several times in the same method. Checking
a
> > > return value seems more appropriate in this case (that's what the C++ code
> > does
> > > as well as my Java client).
> > 
> > Python is not C++ or Java (in our C++ exceptions are forbidden).
> > 
> > This is a conscious design decision, based on my own experience in crypto
API
> > (mis)use, and discussions with colleagues who use existing Python crypto
APIs.
> > Return codes are a terrible thing for crypto, even more so in Python where
the
> > return value can be 1, 0, -1, "horse", None, [], or Sheep().
> > 
> > Please read ct/crypto/error.py. Philosophically, I argue that a
cryptographic
> > verification error _is_ an exceptional situation - when I call verify() I
> > *expect* the input to verify and when it doesn't, I should stop doing
whatever
> I
> > was doing (displaying the user pictures of cats) and switch to exception
> > handling (scary browser interstitials).
>

[Eran, 2013-08-20 10:50:27]

LGTM.
Re True return value from verification methods:
Having the method return True is useful as it allows you to use the decorator,
just as you've stated.
What I was suggesting is to point out in the documentation that the method's
return value need not be checked by the caller since the decorator already does
that.

[ekasper, 2013-08-20 10:54:18]

On 2013/08/20 10:50:27, Eran wrote:
> LGTM.
> Re True return value from verification methods:
> Having the method return True is useful as it allows you to use the decorator,
> just as you've stated.
> What I was suggesting is to point out in the documentation that the method's
> return value need not be checked by the caller since the decorator already
does
> that.

Sure, that sounds reasonable. I'll add that before committing upstream.