[tor-bugs] #22781 [Core Tor/Tor]: hs: Unify link specifier API/ABI

[Tor Bug Tracker & Wiki]

#22781: hs: Unify link specifier API/ABI
---------------------------------------+-----------------------------------
 Reporter:  dgoulet                    |          Owner:  dgoulet
     Type:  enhancement                |         Status:  needs_information
 Priority:  Medium                     |      Milestone:  Tor:
                                       |  0.3.3.x-final
Component:  Core Tor/Tor               |        Version:
 Severity:  Normal                     |     Resolution:
 Keywords:  tor-cell, tor-relay, ipv6  |  Actual Points:
Parent ID:  #24403                     |         Points:  1
 Reviewer:                             |        Sponsor:  SponsorV-can
---------------------------------------+-----------------------------------

Comment (by teor):

 Replying to [comment:6 dgoulet]:
 > I want to make sure we nail down an interface that we like/want before
 going further.
 >
 > 1. EXTEND cell: The `extend_cell_t` object contains basically all the
 possible ways to extend. Through `extend_cell_format()`, we take what's in
 it and create `link_specifier_t` (trunnel).
 >
 >  The relationship is:
 >  - `extend_cell_format(): extend_cell_t -> link_specifier_t`
 >  - `extend_cell_parse(): link_specifier_t -> extend_cell_t` (Note that
 this is for EXTEND2).

 What about CREATE[2] cells?
 extend_info_t is used for them, too.

 > 2. HSv3: We have `hs_desc_link_specifier_t` which represent only one
 possible way to extend (IPv4, IPv6, Legacy ID or ed25519).
 >
 >  Relationship is:
 >  - `encode_link_specifiers(): hs_desc_link_specifier_t ->
 link_specifier_t`
 >  - `decode_link_specifiers(): link_specifier_t ->
 hs_desc_link_specifier_t`
 >
 > That is what we have right now using link specifiers interface (trunnel)
 and some intermediate object. I think a good move forward would be so
 abstract the high level link specifier object to something generic that
 every subsystem could use. In other words, replace
 `hs_desc_link_specifier_t` and `extend_cell_t` to be the same object.
 >
 > They differ right now because the HS one contains *one* specific link
 specifier and the extend cell object contains *all* possible link
 specifiers. In the spirit of reducing memory allocation, I think it is
 reasonable to have one generic object containing all possible link
 specifiers. Something like:
 >
 > {{{
 > tor_addr_port_t addrport_v4;
 > tor_addr_port_t addrport_v6;
 > uint8_t legacy_id[DIGEST_LEN];
 > ed25519_public_key_t ed_id;
 > }}}
 >
 > We mandate that v4 and legacy_id be present which means we could either
 check for "mem zero" on IPv6 and ed_id to decide to include them or we add
 a flag saying "v6 defined" to maybe be more efficient. Parsing cells is
 certainly part of our fast path in my opinion so any optimization is a
 win.

 I'm all for avoiding premature optimisations.
 Let's define accessor functions that do the zero-checks and then profile
 CPU and RAM.
 Then we can add flags, and profile again.

 > Downside here is if we ever want to include more than one type of
 specifiers in a cell/descriptor that is for example 2 different IPv4?

 The Prop224 spec says that we should pass on all link specifiers, even
 unknown link specifier types.
 So I think we need a final member `extra_spec_list` that's a smartlist
 pointer.
 On the fast path, it doesn't get used, because it's NULL.

 This could handle extra addresses, too, if we ever did that.

 > My original branch (`ticket22781_032_01`) has already some work started
 there with `lspec_t` being a generic higher level object that encodes to a
 `link_specifier_t` and vice versa. However, it would need to contain all
 the possible links.
 >
 > Am I on a path that we like?

 I like this idea.

 I think this makes it easier to push all the details of choosing addresses
 deep down the stack. Then we can choose the remote address just before we
 go to connect to it. It makes a whole lot of code much easier, and much
 more generic. And we can finally implement tickets like #6772, and try
 another ORPort if the first one fails.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/22781#comment:7>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online