Bug25552 ope draft #150
Bug25552 ope draft #150
Conversation
This is meant for use when encrypting the current time within the period in order to get a monotonically increasing revision counter without actually revealing our view of the time. This scheme is far from the most state-of-the-art: don't use it for anything else without careful analysis by somebody much smarter than I am. See ticket #25552 for some rationale for this logic.
Fix a memory leak.
asn-d6
force-pushed
the
bug25552_ope_draft
branch
2 times, most recently
from
90cb3a1
to
08afd22
To do so for a given descriptor, we use the "seconds since the SR protocol run" started, for the SRV that is relevant to this descriptor. This is guaranteed to be a positive value (since we need an SRV to be able to build a descriptor), and it's also guaranteed to be a small value (since SRVs stop being listed on a consensus after 48 hours). We cannot use the "seconds since the time period started", because for the next descriptor we use the next time period, so the timestamp would end up negative. See [SERVICEUPLOAD] from rend-spec-v3.txt for more details. To do so, we have to introduce a new `is_current` argument to a bunch of functions, because to use "seconds since the SR protocol run" we need to know if we are building the current or the next descriptor, since we use a different SRV for each descriptor.
These were breaking our unittests.
Now that the rev counter depends on the local time, we need to be more careful in the unittests. Some unittests were breaking because they were using consensus values from 1985, but they were not updating the local time appropriately. That was causing the OPE module to complain that it was trying to encrypt insanely large values.
We are not using the state file for rev counters anymore, we just generate them on the fly!
src/common/crypto_ope.h
Outdated
| * the start of an SRV period. SRVs are useful for about 48 hours (that's how | ||
| * long they stick on the consensus). Let's also add 12 hours of drift for | ||
| * clock skewed clients stuck with an old consensus, and we arrive to 60 | ||
| * hours. |
In theory, the "previous" SRV is there to accommodate clock skewed client that thinks it is still the current. But overall, the client never uses the previous SRV so that 48h span is already covering the clock skewed client use case. Not sure the impact of using 60 hours but 48h should be quite enough.
Hmm, that comment might have been a bit misleading. I meant to say "clock
skewed services" and not "clock skewed clients". I pumped it to 60 hours
because I'm concerned about clock skewed services using outdated consensus. In particular, an SRV stays for 48 hours in consensuses, and a service could potentially have an outdated consensus and keep on using it.
In general, 60 hours VS 48 hours doesn't make a huge difference in terms of
OPE, so perhaps we can leave it at 60 and be on the safe side.
Tried to clarify in 87dc47f
src/or/hs_service.c
Outdated
| if (is_current) { | ||
| srv_start = sr_state_get_start_time_of_previous_protocol_run(now); | ||
| } else { | ||
| srv_start = sr_state_get_start_time_of_current_protocol_run(now); |
Is there a mix up here? Seems counter intuitive to have is_current using the previous protocol run start time? If that is correct, we should explain why this is correct.
Not a mixup. It's just the way prop224 spec works. See [SERVICEUPLOAD]:
For is_current (aka first descriptor), Tor always uses previous SRV and hence
the previous protocol run start time:
When a service is in the time segment between a new time period a new SRV
(i.e. the segments drawn with "-"), it uses the previous time period and
previous SRV for uploading its first descriptor
Now if a service is in the time segment between a new SRV and a new time
period (i.e. the segments drawn with "=") it uses the current time period
and the previous SRV for its first descriptor
whereas for !is_current (aka second descriptor), it always uses current SRV and
hence start time of current protocol run.
I added clarifying comment in 2902c2d.
| @@ -1755,7 +1755,6 @@ service_desc_schedule_upload(hs_service_descriptor_t *desc, | |||
| /* Update the given descriptor from the given service. The possible update | |||
General comment: This commit is VERY satisfying :).
src/or/hs_service.c
Outdated
| { /* Now encrypt the revision counter! */ | ||
| crypto_ope_t *ope = NULL; | ||
| ope = crypto_ope_new(key); | ||
| rev_counter = crypto_ope_encrypt(ope, (int) seconds_since_start_of_srv); |
UINT64_MAX is sent back on an "invalid" input. I would BUG() on it for extra safety because if we start doing that but not noticing, we'll wonder for a long time why descriptor are getting refused at the HSDir :S...
Done in 2902c2d
|
A version of this was squashed and merged |
No description provided.
The text was updated successfully, but these errors were encountered: