Opened 6 months ago

Closed 3 months ago

#32563 closed defect (fixed)

Merge HSv3 spec fixes we found during onionbalance creation

Reported by: asn Owned by: asn
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, scaling, onionbalance, network-team-roadmap-september, tor-spec, network-team-roadmap-2020Q1
Cc: s7r, gk Actual Points:
Parent ID: #26768 Points: 0.2
Reviewer: dgoulet Sponsor: Sponsor27-must

Description

There are a few issues with the v3 spec that we found out as we were implementing onionbalance. We should merge them.

Examples:

Child Tickets

Change History (11)

comment:1 Changed 6 months ago by asn

Owner: set to asn
Status: newassigned

comment:2 Changed 4 months ago by gaba

Keywords: network-team-roadmap-2020Q1 added

Move s27 must tickets into new roadmap.

comment:3 Changed 4 months ago by asn

More:

  • CONSENSUS_ARRIVED is marked as STATUS_GENERAL but it's actually STATUS_CLIENT
  • Did we document the crosscertification issue?

comment:4 Changed 4 months ago by asn

Disaster SRV computation is:
H("shared-random-disaster" | INT_8(period_length) | INT_8(period_num))
and not the other way around.

comment:5 Changed 4 months ago by asn

[Note to self: This is not a spec change, but I wrote a test vector for hashring computation in bug31648 and this should also get in little-t-tor at some point]

comment:6 in reply to:  5 Changed 4 months ago by teor

Replying to asn:

[Note to self: This is not a spec change, but I wrote a test vector for hashring computation in bug31648 and this should also get in little-t-tor at some point]

Example test vectors are actually a useful part of specs.

comment:7 Changed 3 months ago by asn

Status: assignedneeds_review

Please see https://github.com/torproject/torspec/pull/112 for the spec fixes that came out of onionbalance v3 development.

comment:8 Changed 3 months ago by dgoulet

Reviewer: dgoulet

comment:9 Changed 3 months ago by dgoulet

Status: needs_reviewneeds_revision

I think we need to define the length in bytes for mac_key_len and salt_len ?

Code is forcing it to 8 bytes:

  crypto_digest_add_bytes(digest, (const char *) &mac_len_netorder, 8);
  crypto_digest_add_bytes(digest, (const char *) &salt_len_netorder, 8);

comment:10 in reply to:  9 Changed 3 months ago by asn

Replying to dgoulet:

I think we need to define the length in bytes for mac_key_len and salt_len ?

Code is forcing it to 8 bytes:

  crypto_digest_add_bytes(digest, (const char *) &mac_len_netorder, 8);
  crypto_digest_add_bytes(digest, (const char *) &salt_len_netorder, 8);

Hey. Isn't htonll already specifying that it's a uint64_t and hence 8 bytes?

comment:11 Changed 3 months ago by dgoulet

Resolution: fixed
Status: needs_revisionclosed

Oh hmmm, yes indeed lets go with that since this is not an ABI but rather a hash construction.

Merged to tor-spec!

Note: See TracTickets for help on using tickets.