hs: Unify link specifier API/ABI

In hs_descriptor.{c|h}, link specifiers are used to encode the introduction points in the descriptor. The decoded version is represented by the hs_desc_link_specifier_t object which is used to build a trunnel object that is then base64 encoded and put in the descriptor.

With the service and client implementation of prop224, link specifiers are also used in the INTRODUCE1 cell. Client will encode them in the cell and service decodes them (INTRODUCE2 cell).

This ticket is to unify the link specifier API/ABI used between cell and descriptor for both encoding and decoding in order to avoid code duplication.

Furthermore, future work could expend this to the EXTEND2 cell that also requires link specifiers (extend_cell_from_extend2_cell_body() and extend_cell_format().

changed milestone to %Tor: 0.4.1.x-final

added 034-removed-20180328 034-triage-20180328 actualpoints::0.1 component::core tor/tor ipv6 milestone::Tor: 0.4.1.x-final owner::teor parent::23576 points::1 priority::medium resolution::fixed reviewer::teor severity::normal status::closed tor-cell tor-relay type::enhancement labels

This is important groundwork for #17242 (moved) and #20657 (moved) which both uses link specifiers so they are using this unified API.

Branch is in ticket22781_032_01. https://gitlab.com/dgoulet/tor/merge_requests/32

Trac:
Status: new to needs_review
Parent: N/A to #21888 (moved)

nvm the above, this needs some changes.

Trac:
Status: needs_review to needs_revision

This is not prop224 groundwork afterall.

Trac:
Sponsor: SponsorR-must to SponsorR-can
Parent: #21888 (moved) to N/A
Keywords: N/A deleted, prop224-extra added

Trac:
Milestone: Tor: 0.3.2.x-final to Tor: 0.3.3.x-final
Sponsor: SponsorR-can to N/A
Keywords: prop224, prop224-extra, tor-hs deleted, tor-relay, tor-cell added

This will help with IPv6 ORPort reachability checks in #24403 (moved).

Trac:
Keywords: N/A deleted, ipv6 added
Parent: N/A to #24403 (moved)
Sponsor: N/A to SponsorV-can

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).

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. 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?

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?

Trac:
Status: needs_revision to needs_information

Replying to 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 (moved), and try another ORPort if the first one fails.

Trac:
Reviewer: N/A to teor
Status: needs_information to needs_revision

Replying to teor:

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

Right the goal would be to put the "link specifier" generic object in extend_cell_t leaving also the create_cell_t in there.

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.

Sounds good. Simple first!

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.

Hmmm well the generic object I'm talking about is the one we decode to and encode from link_specifier_t which means that if you don't recognize one, you ignore. Thus the object doesn't really need to keep "extra unknown specifier(s)" because you can really decode and encode what you don't know.

This is more a matter of ignoring unknown link spec type.

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 (moved), and try another ORPort if the first one fails.

Great! I'll get to it asap! Thanks for the feedback teor!

I've picked up the branch and modified it with what we discussed. See branch ticket22781_033_01.

It still needs a bit more unit test but at least we got the extend cell format unit test testing a lot of it.

One of the commit make the extend_cell_t use the new link specifier object. It could be really improved in terms of refactoring but for now I went for an "inplace" change and then with future commit we can refactor this a bit nicer.

Also, you'll notice that I've explicitly made that we can't put an IPv6 link spec in an EXTEND cell for now even though with this patch, we get it for free.

Replying to dgoulet:

Replying to teor:

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.

Hmmm well the generic object I'm talking about is the one we decode to and encode from link_specifier_t which means that if you don't recognize one, you ignore. Thus the object doesn't really need to keep "extra unknown specifier(s)" because you can really decode and encode what you don't know.

As long as you know the length, you can transmit unknown link specifier types without parsing them.

This is more a matter of ignoring unknown link spec type.

Hang on, that's not what the spec says:

The hidden service SHOULD NOT reject any LSTYPE fields which it doesn't recognize; instead, it should use them verbatim in its EXTEND request to the rendezvous point.

https://gitweb.torproject.org/torspec.git/tree/proposals/224-rend-spec-ng.txt#n1689

But the spec is inconsistent: it specifies this for service to rend, but not client to intro. I think we should do it for both, because it means that we automatically upgrade when a new link specifier comes along.

Also, we should add something about how to deal with link specifiers that are too long:

Any link specifier that would make the total length exceed the length of an EXTEND cell MUST be ignored. (This is important for HS descriptor to intro EXTEND, because the descriptor link specifiers aren't limited to the size of a cell.)

Or, we can just ignore unknown link specifiers, but that introduces a version distinguisher, and makes upgrades slower.

Replying to teor: [snip]

But the spec is inconsistent: it specifies this for service to rend, but not client to intro.

Intro do not parse link specifiers from the INTRO cell, that part is encrypted for the service else they would learn the RP and that is no good :).

So HS service is really the only place I see that tor will take a "blob" of link specifiers even thought it doesn't know about it and will extend to the RP.

Am I correct?

The client reads link specifiers from the descriptor and uses them to extend to the intro:

https://gitweb.torproject.org/torspec.git/tree/proposals/224-rend-spec-ng.txt#n1322

Trac:
Priority: Medium to Very High

The 0.3.3 freeze deadline has passed, all these children of #24403 (moved) belong in 0.3.4

Trac:
Milestone: Tor: 0.3.3.x-final to Tor: 0.3.4.x-final

Trac:
Keywords: N/A deleted, 034-triage-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

Trac:
Keywords: N/A deleted, 034-removed-20180328 added

These needs_revision, tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if somebody does the necessary revision.

Trac:
Milestone: Tor: 0.3.4.x-final to Tor: unspecified

Trac:
Priority: Very High to Medium

I think this is mostly fixed in #23576 (moved), by removing hs_descriptor_link_specifier_t. Because there is only one type, link_specifier_t, I didn't need a higher-level encode/decode API.

We should open a new ticket for any remaining changes.

Trac:
Status: needs_revision to assigned
Owner: dgoulet to teor
Actualpoints: N/A to 0.1
Sponsor: SponsorV-can to N/A
Parent: #24403 (moved) to #23576 (moved)

Trac:
Resolution: N/A to duplicate
Status: assigned to closed

Really this should be fixed

Trac:
Resolution: duplicate to N/A
Milestone: Tor: unspecified to Tor: 0.4.1.x-final
Status: closed to reopened

Trac:
Resolution: N/A to fixed
Status: reopened to closed

closed

changed time estimate to 8h

added 48m of time spent

mentioned in issue #22810 (moved)

mentioned in issue #23576 (moved)

mentioned in issue #23759 (moved)

moved to tpo/core/tor#22781 (closed)

mentioned in issue tpo/core/tor#23759 (closed)