Opened 6 years ago

Closed 4 years ago

#6411 closed enhancement (implemented)

Adding hidden services through control socket

Reported by: kevinevans Owned by: yawning
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.3.19-rc
Severity: Keywords: hidden-service, control, maybe-proposal, tor-hs, globalleaks-wants, nickm-review, 027-triaged-1-out
Cc: lists@…, yawning Actual Points:
Parent ID: #8993 Points:
Reviewer: Sponsor:

Description

Okay, first off, I should say: 1) I'm relatively new to Tor and 2) I don't know C that well.

A while back, I thought that it was a bad idea to have the hidden service hostname/privkey being written to the filesystem, unless it's either a tmpfs or an encrypted volume. For programs like Torchat (or alike), it would seem better to be able to hide the private key/hostname in an encrypted file (for example) versus in a filesystem.

In the patch, I have added an ADDSERVICE command (after it's authenticated), and its arguments are:
[hostname] [private key] [vport0] [rport0] [vport1] [rport1] ... [vport*] [rport*]

I wasn't sure about which status codes to use, so I just used whatever. The code is rather inefficient, frankly because I'm awful at C and I'm probably causing a memory leak by not freeing some memory.

Child Tickets

Attachments (3)

control.c.patch (2.1 KB) - added by kevinevans 6 years ago.
Patch for control.c
rendservice.h.patch (352 bytes) - added by kevinevans 6 years ago.
Patch for rendservice.h
rendservice.c.patch (2.4 KB) - added by kevinevans 6 years ago.
Patch for rendservice.c

Download all attachments as: .zip

Change History (65)

Changed 6 years ago by kevinevans

Attachment: control.c.patch added

Patch for control.c

Changed 6 years ago by kevinevans

Attachment: rendservice.h.patch added

Patch for rendservice.h

Changed 6 years ago by kevinevans

Attachment: rendservice.c.patch added

Patch for rendservice.c

comment:1 Changed 6 years ago by arma

Status: newneeds_review

comment:2 Changed 6 years ago by arma

Milestone: Tor: 0.2.4.x-final
Priority: trivialnormal

comment:3 Changed 6 years ago by nickm

Keywords: hidden-service maybe-proposal added; hidden service removed

comment:4 Changed 6 years ago by nickm

Keywords: tor-hs added

comment:5 Changed 6 years ago by nickm

Component: Tor Hidden ServicesTor

comment:6 Changed 6 years ago by naif

This ticket is very similar to https://trac.torproject.org/projects/tor/ticket/5976 . It may be worth to evaluate if both two complete one each/other during the review-phase of this patch set.

comment:7 Changed 6 years ago by nickm

Status: needs_reviewneeds_revision

This could use a design proposal -- as it stands, I'm not really sure what the command is supposed to do, since there's no documentation for it. At the very least, it needs a patch for control-spec to explain what it does.

Some other issues a proposal should address:

Why does the controller need to generate their own private key?

Why does the controller need to figure out the hostname? Why is it even there -- the code never looks at it except to check its length.

The patch itself has some issues that would need to be fixed, including:

  • It parses a controller line in rendservice.c. By convention, all the controller parsing is done in control.c; rendservice.c should get at most a new "add a service" function.
  • There's a heap overflow if the private key argument is too long.
  • It checks to make sure that there are enough arguments after it's already looked at one of them.
  • It fails badly if there are an odd number of ports, or if parse_port_config fails.
  • The variable "p" doesn't have any purpose.

comment:8 Changed 6 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: unspecified

comment:9 Changed 6 years ago by arma

Parent ID: #8993

comment:10 Changed 4 years ago by naif

Cc: lists@… added

Is it possible to foresee integration of something like that in 0.2.6 ?

It would be extremely useful for proper, clean integration of Tor within GlobaLeaks, being fully managed by the application code.
/cc @meejah

comment:11 Changed 4 years ago by meejah

It seems to me there are two parts here:

  1. create a hidden-service private key (without touching the filesystem). This could be a command-line option to tor (e.g. like --hash-password) that dumps to stdou or a controller command (e.g. "CREATE_HS_KEY" that returns the key-blob).
  1. add an HS private key to a running Tor.

a) To fit in with the other HiddenService* options, it would probably make sense to have something like "HiddenServicePrivateKey" or similar that can take some kind of single-line giant-string representation of the private key. So this would be done via a SETCONF.

OR b) a separate command that sets all the options at once or something for a hidden service (which is sort of what the above is?).

For both the above, you still need some way to get the .onion address -- either the controller has to write compatible code that derives from the private key, OR there's a controller command (e.g. some GETCONF thing?) that can tell you the .onion for a given hiddenservice.


HOWEVER...

I think this probably does warrant further discussion around a proposal on tor-dev -- although a "HiddenServicePrivateKey" option would fit in with the current scheme better, the current scheme is rather clunky for hidden-services, IMO. It is the only real "special case" GETCONF/SETCONF/torrc thing where order matters, *and* it is AFAIK the only user of "Dependant" or "Virtual" as a type.

There is currently a decent chunk of code in txtorcon's torconfig.py classes to deal with the special-case of hidden-services vs. "normal" options, so it might make sense to configure hidden services differently entirely. One such idea:

For filesystem-based HSes you put all the config for your service in the hidden-service-dir in some file ("config"? this would be the ports and such) and put a single option (HiddenService /foo/bar) in torrc

OR you use a controller that knows how to speak the "set up a hidden service" commands, which would all be new. There's then also the matter of "what's the unique name of a hidden service?" which is currently "the hidden service dir" but of course that doesn't make sense if there isn't one at all.

In other words, split the concept of an "ephemeral" (to tor) hidden-service (i.e. where a controller keeps all the config) versus a "permanent" one (i.e. where Tor knows where all the config is on the filesystem). Additionally, for people who want to do things like add/remove client authorizations "on the fly" it might be a lot easier to have controller commands like "HS_REMOTE_CLIENT hsname clientname".

All that said I haven't thought about this too hard and a design proposal does sound like a good idea...

comment:12 in reply to:  10 Changed 4 years ago by nickm

Keywords: goballeaks-wants added

Replying to naif:

Is it possible to foresee integration of something like that in 0.2.6 ?

Probably not; 0.2.6 is already entering feature freeze and this patch set is not up to scratch.

comment:13 Changed 4 years ago by nickm

Keywords: globalleaks-wants added; goballeaks-wants removed

comment:14 Changed 4 years ago by yawning

Cc: yawning added

comment:15 in reply to:  11 Changed 4 years ago by yawning

So, I said I'll look at this the other day in IRC.

Replying to meejah:

  1. create a hidden-service private key (without touching the filesystem). This could be a command-line option to tor (e.g. like --hash-password) that dumps to stdou or a controller command (e.g. "CREATE_HS_KEY" that returns the key-blob).
  1. add an HS private key to a running Tor.

a) To fit in with the other HiddenService* options, it would probably make sense to have something like "HiddenServicePrivateKey" or similar that can take some kind of single-line giant-string representation of the private key. So this would be done via a SETCONF.

OR b) a separate command that sets all the options at once or something for a hidden service (which is sort of what the above is?).

This is "b".

For both the above, you still need some way to get the .onion address -- either the controller has to write compatible code that derives from the private key, OR there's a controller command (e.g. some GETCONF thing?) that can tell you the .onion for a given hiddenservice.

I think for ephemeral hidden services, the key management and generation should be the application's problem. That allows the app to persist it's HS key on it's own if it want's to. Deriving the onion address from the public key is trivial as well.

ypres :: ~ % go get github.com/Yawning/orkey/or-key-tool
ypres :: ~ % ~/.local/go/bin/or-key-tool
File: - N/A (Generated) -
Type: RSA1024 Private Key
Fingerprint: 3B414C1DBF3638F47F946B8906D09D53FBB1F21C
Identity (Base64): O0FMHb82OPR/lGuJBtCdU/ux8hw=
Onion Addr: hnauyhn7gy4pi74u.onion

-----BEGIN RSA PUBLIC KEY-----
 [Redacted Base64 encoded blob]
-----END RSA PUBLIC KEY-----
-----BEGIN RSA PRIVATE KEY-----
 [Redacted Base64 encoded blob]
-----END RSA PRIVATE KEY-----

ypres :: ~ % 

[Snip]

OR you use a controller that knows how to speak the "set up a hidden service" commands, which would all be new. There's then also the matter of "what's the unique name of a hidden service?" which is currently "the hidden service dir" but of course that doesn't make sense if there isn't one at all.

This seems the simplest. I don't see what's wrong with using the onion addr to refer to each HS... For something that's easy to do, and only requires a control spec patch:
For the controller:

  • ADD_EPH_HIDDENSERVICE KEY_TYPE PRIVATE_KEY [virtport targetAddr]... - Initializes a hidden service, using the KEY_TYPE private key PRIVATE_KEY (so, something like "RSA1024 [Base64 encoded blob]"), with the HiddenServicePort list as specified. (Key type is specified as a future-proofing measure).
  • DEL_EPH_HIDDENSERVICE ADDR - Removes the hidden service ADDR.

Open questions:

  • HiddenServiceAuthorizeClient is kind of annoying, so I'm going to ignore it for now.
  • Is it unreasonable to expect people to be able to generate an RSA1024 key, and compute/encode the digest? I'm inclined to think that the added application flexibility here is worth forcing that onto the app code (and crap app code that doesn't generate keys properly get what they deserve).

comment:16 Changed 4 years ago by naif

@yawning Yes, i think that the key generation it's an application problem. There are plenty of TorHS generation tools, what could be useful is to provide some few code-snippets example on how to do it properly. The Tor-side should just validate that they key is good enough.

I also think that, for a first release, the HiddenServiceAuthorizeClient is not needed because it's a feature that nearly no-one use as today.

Yawning, btw i need to offer you a beer or send you some good wine! :*

comment:17 Changed 4 years ago by yawning

Owner: set to yawning
Status: needs_revisionaccepted

Ok, I started poking at this, loosely based off the original patch. Not quite done yet, but getting there (as in, "I need to backfill a bunch of XXX error logging/handling cases, but the core changes should be there", at least for the ADD_EPH_HS command).

Notes so far:

  • The tentative commands are now ADD_EPH_HS and DEL_EPH_HS.
  • The original patch has more things that are wrong:
    • Happy fun times on config reload if more than one HS added via the control port.
    • It leaves a few copies of the private key left splattered on the heap.
    • Either the code has diverged considerably since the patch was written, or a bunch of parameters in the rend_service_t that need to be initialized, aren't.
  • I'm not sure what the behavior for ephemeral hidden services should be on a reload. My patch currently discards them all, since that's the easy thing to do, but preserving them is also possible.
Last edited 4 years ago by yawning (previous) (diff)

comment:18 Changed 4 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.7.x-final

Tentatively giving this a milestone

comment:19 Changed 4 years ago by naif

Discussion happening on txtorcon https://github.com/meejah/txtorcon/issues/13

comment:20 Changed 4 years ago by yawning

Using stem's prompt thing...

>>> foo = controller.msg("ADD_EPH_HS RSA1024 [Base64 blob redacted] 6060 6060")
>>> print foo
OK
ypres :: ~ % torsocks nc [my super sekrit HS].onion 6060                            
GET / HTTP/1.0

HTTP/1.0 200 OK
Date: Tue, 10 Feb 2015 09:05:18 GMT
Content-Type: text/html; charset=utf-8
[Rest of the response redacted]

Notes:

  • I used "or-key-tool" from the example I pasted to generate the key/onion address.
  • "250 OK" isn't a very user friendly response, I might change that to be more useful.
  • When trying to test this I found a torsocks bug, yay me.
  • While a branch exists, don't use it yet as there are a bunch of open design questions, and removal still needs to be implemented. k. thx.

Branch: https://github.com/Yawning/tor/compare/feature6411

Last edited 4 years ago by yawning (previous) (diff)

comment:21 Changed 4 years ago by yawning

Ok, I just went and updated the branch to add DEL_EPH_HS, which does exactly what is expected on the tin. Before I send this off to review, the following questions need discussion/answers:

  • Should ADD_EPH_HS return more than "OK"? If so, what?
  • What should happen to ephemeral hidden services on config reload? The current behavior is to kill off the intro points and remove the HS (this leaves existing clients unaffected, but will disallow new clients), which mimics what happens when a HS is removed during a config reload.
  • Should DEL_EPH_HS be case sensitive for the service id? It currently is not.
  • Should DEL_EPH_HS close only the intro points, or also existing client connections? Current behavior is the former, changing it to the latter is easy-ish.
  • Does DEL_EPH_HS need to scrub the service ID that it removed?

Known issues (Beyond the open design questions, most minor):

  • There's a bit of code duplication between rend_config_services() and rend_service_del_ephemeral(), on the off chance that behavior between the two will diverge. I should carve out a new function if the behavior is going to be the same (as it is now).
  • rend_service_get_by_service_id() doesn't use constant time compares, because it is case insensitive. (Probably ok?).
  • Not much key validation is done in crypto_pk_base64_decode(), the next time I touch it I should add a crypto_pk_check_key() at a minimum...
  • I need to validate that the RSA key is actually 1024 bits...

Going to tag this as needs_information, and set it aside till proper behavior is established, then finish off the known issues. It's basically done at this point, but people still shouldn't use my branch.

Last edited 4 years ago by yawning (previous) (diff)

comment:22 Changed 4 years ago by yawning

Status: acceptedneeds_information

comment:23 Changed 4 years ago by yawning

Status: needs_informationaccepted

After sleeping on it for a bit, and talking to arma:

  • Ephemeral HSes SHOULD persist through config reload (HUPing for log rotation was brought up as an example).
  • DEL_EPH_HS SHOULD be case sensitive for future proofing.
  • DEL_EPH_HS SHOULD NOT kill client connections, but see #14852.
  • I might as well scrub the service id after processing a DEL_EPH_HS in the control code.

Re-accepting, since that covers most of the design questions.

Last edited 4 years ago by yawning (previous) (diff)

comment:24 Changed 4 years ago by naif

Love, love, love!

Yawning are you coming to Tor Dev Meeting in Valencia? Willing to bring Italian Red Wine!!!!!! :-)

comment:25 in reply to:  24 Changed 4 years ago by yawning

Replying to naif:

Yawning are you coming to Tor Dev Meeting in Valencia? Willing to bring Italian Red Wine!!!!!! :-)

Yeah I will. :P

So a minor update, I went and added the code to persist ephemeral HSes through configuration reload to my branch, so basic functionality is done (yay). That said, I talked to nickm regarding the design a bit and he convinced me that allowing tor to (optionally) generate the private key on behalf of the user is a good idea.

The command currently looks like: ADD_EPH_HS keyType keyBlob [VIRTPORT TARGET]... where keyType is RSA1024, Ed25519 etc.

This will most likely change to: ADD_EPH_HS keyType:keyBlob [VIRTPORT TARGET]... where:

  • On success, 250-OnionAddr=address.onion is returned, in addition to the standard 250 OK response.
  • A keyType of known algorithms (Eg: RSA1024, Ed25519) is treated the same as the old behavior, with an opaque blob in the appropriate format supplied by the user as keyBlob.
  • A keyType of NEW will have tor generate a new keypair of the algorithm specified in keyBlob (So, to get a new ephemeral HS with a RSA keypair, one would specify NEW:RSA1024). As an ease of use thing, a keyBlob (in this case acting as the algorithm identifier) of BEST will generate the "best" algorithm supported by the tor instance. On success in addition to the onion address and the OK response, 250-KeyPair=keyType:keyBlob is returned, containing a keyType identifying the keypair algorithm, and keyBlob containing the serialized keypair suitable for future calls to ADD_EPH_HS.

For example:

Adding a new ephemeral HS, with a pre-existing RSA1024 key:

ADD_EPH_HS RSA1024:<opaque blob> 80 127.0.0.1:80
250-OnionAddr=<blah blah blah>.onion
250 OK

Adding a new ephemeral HS, with a newly generated Ed25519 key:

ADD_EPH_HS NEW:Ed25519 22 127.0.0.1:22
250-OnionAddr=<foo bar baz>.onion
250-KeyPair=Ed25519:<opaque blob>
250 OK

Adding a new ephemeral HS, with a newly generated key, using the best supported algorithm:

ADD_EPH_HS NEW:BEST 443 127.0.0.1:443
250-OnionAddr=<hoge hoge hoge>.onion
250-KeyPair=RSA1024:<opaque blob>
250 OK

This will allow:

  • People that know what they are doing to handle keypair generation.
  • People that don't know what they are doing, or don't trust themselves to do things correctly, to have Tor generate a HS keypair (and corresponding address) on their behalf, that they can continue to use in the future if they decide to save it.

Unless anyone has massive objections as to why this is broken, this is what I will implement the next time I work on this.

Note: Ed25519 is used as an example, and won't actually be supported initially since the HS code does not.

Last edited 4 years ago by yawning (previous) (diff)

comment:26 Changed 4 years ago by naif

mmmumble, the only reason i see that application would like to generate it's own private key is to be able to make "vanity name" with in-app rsa key brute-forcing.

But i agree that, when that feature is not required, having Tor pre-generating the key make much more sense as it simplify the process.

Btw ok, sounds like we'll need to load the baggage to fill out enough good wine! Will do that!

comment:27 in reply to:  26 Changed 4 years ago by yawning

Replying to naif:

mmmumble, the only reason i see that application would like to generate it's own private key is to be able to make "vanity name" with in-app rsa key brute-forcing.

Maybe. Being able to load a key that Tor generates for the user is useful (for anyone that wants to run an ephemeral HS that doesn't change it's address, eg: tinfoil hat web hosting setups), and that also happens to allow vanity keys as well as long as the key format is consistent and well documented, so I don't see the harm in providing it.

But i agree that, when that feature is not required, having Tor pre-generating the key make much more sense as it simplify the process.

The code's not that tricky too, it's mostly messing with the control port that's annoying.

comment:28 Changed 4 years ago by meejah

yawning and I were chatting on IRC, and I had a (possibly crazy) idea to tie these ephemeral HSes to the control-connection that added them such that when that connection closes, all the HSes it added go away.

comment:29 in reply to:  28 Changed 4 years ago by yawning

Replying to meejah:

yawning and I were chatting on IRC, and I had a (possibly crazy) idea to tie these ephemeral HSes to the control-connection that added them such that when that connection closes, all the HSes it added go away.

Adding to this a bit, I think this is probably a good idea.

I currently am of the opinion that there should be no command to query a list of all ephemeral hidden services being hosted by a given tor instance. A hypothetical example would be, if I am running a eph. HS based chat application and a file sharing application, neither application has any business what so ever of being able to see each other's HS.

The lack of such a command complicates cleanup (eg: the applciation crashes without cleaning up it's HSes, and is not using it's own tor instance), and restricting eph. HS duration to the control port session's lifetime automates this.

comment:30 Changed 4 years ago by yawning

Ok, done hacking on this for tonight, so I'll leave a note as to the state of the branch.

Done today:

  • Changed the ADD_EPH_HS command format to be that from the discussion with nickm, including key generation (Note: Contrary to what I wrote earlier, 250-ServiceID=<blah blah blah> is returned instead of the onion address on the assumption that adding ".onion" to a string is easy, and for consistency with DEL_EPH_HS).

This means that the branch is feature complete again, unless people want ephemeral HSes to be tied to the originating control port connection (I think this would be good). My plans are to implement this, and then write the control-spec.txt patch and seek to get my branch reviewed.

comment:31 Changed 4 years ago by naif

FYI i just opened #14899 to "Enable Tor to work without using filesystem for cached files" that, together with #13865 would complete the ability to have a fully filesystem-free Tor process managed by a third party application controller.

comment:32 Changed 4 years ago by yawning

The thread: https://lists.torproject.org/pipermail/tor-dev/2015-February/008279.html

Right, so after a productive tor-dev@ thread regarding the design, I went and made some more changes:

  • Ephemeral hidden services are tied to the control port connection that created them. This means, that when the control connection goes away, so does the hidden service intro point. Closing client connections is left as an exercise for the application.
  • DEL_EPH_HS can now only remove ephemeral hidden services created on the same control port connection. Attempts ADD_EPH_HS on one connection, and DEL_EPH_HS on another will result in the tor disavowing knowledge of the service in an error code.
  • ADD_EPH_HS has a new syntax that is hopefully more futureproof.

ADD_EPH_HS SP keyType:keyBlob 1+(SP Port= VIRTPORT [, TARGET]) CRLF

The new syntax allows us to add other arguments to the command in the future more easily, and as a side bonus for callers, TARGET can now be omitted to obtain behavior identical to HiddenServicePort (As in, it will default to 127.0.0.1:VIRTPORT).

The way forward:

  • Write a patch for control-spec.txt, documenting the new commands.
  • Wait for the 0.2.7.x cycle to start.
  • Get my feature branch reviewed, and fix the dumb bugs that are probably lurking.
  • Squash it down and merge.
  • (In the Grim Dark Future) Revisit and think about authenticated HSes.

Tentatively, needs_review-ing this, since the code is done, and appears to work.

comment:33 Changed 4 years ago by yawning

Status: acceptedneeds_review

comment:34 Changed 4 years ago by yawning

Status: needs_reviewneeds_revision

needs-revision-ing this since the branch needs the following minor changes:

  • Rename the command to ADD_ONION/DEL_ONION.
  • Add an extra optional argument to ADD_ONION called DiscardPK (Name not final) that suppresses the 250-Keypair line from the command return chatter (for HSes that are truly oneshot, there is no reason to pass sensitive key material around).
Last edited 4 years ago by yawning (previous) (diff)

comment:35 Changed 4 years ago by yawning

Proposed control port spec changes: https://github.com/Yawning/torspec/compare/feature6411

My branch implements the functionality as documented, so it should be ready for the rebase/squash dance and followed review. I'll kick the documentation to tor-dev@ as well to get some proof reading and see if it can be improved.

comment:36 Changed 4 years ago by atagar

Hi yawning, your spec addition looks great! Delightfully precise, I like. :P

Only thoughts are...

  • Maybe add the following note to the end of ADD_ONION? "Controllers MUST tolerate unrecognized keyword arguments."
  • You might want to consider making 'DiscardPK' a keyword argument instead ('DiscardPK=1'). That's more common for booleans like this than a positional argument.

Out of curiosity why does DEL_ONION disavow knowledge of hidden services not made by this controller connection? I'm guessing there's security reasons behind it but not sure offhand what they are.

Presently the controller has full access over adding/removing hidden services. Unless I'm missing something with this change we're explicitly making use cases like "Connect to the control port and shut down all existing hidden services" impossible.

comment:37 in reply to:  36 Changed 4 years ago by yawning

Replying to atagar:

Hi yawning, your spec addition looks great! Delightfully precise, I like. :P

Only thoughts are...

  • Maybe add the following note to the end of ADD_ONION? "Controllers MUST tolerate unrecognized keyword arguments."

This is for the response right? Sure, I'll add that, since part of the motivation for the "NEW:BEST" method of keypair generation is to allow old controller libraries to work with newer tor instances that support new and exciting key types (the value of PrivateKey= can just be passed back into ADD_ONION as an opqaue blob at a later date, even if the controller can't parse it to recreate a given HS).

  • You might want to consider making 'DiscardPK' a keyword argument instead ('DiscardPK=1'). That's more common for booleans like this than a positional argument.

Hm, it's tied to key generation so having the arg be next to KeyType:KeyBlob sort of makes sense to me (the only reason why it's not grouped with that arg is to avoid adding restrictions to the serialized blob beyond "no whitespace").

Maybe something like: [SP "Flags=" Flag *("," Flag)] might be even better here, with DiscardPK being one of the flags, if it were to be non-positional?

Out of curiosity why does DEL_ONION disavow knowledge of hidden services not made by this controller connection? I'm guessing there's security reasons behind it but not sure offhand what they are.

Presently the controller has full access over adding/removing hidden services. Unless I'm missing something with this change we're explicitly making use cases like "Connect to the control port and shut down all existing hidden services" impossible.

I made the decision to tie HSes added this way to the controller connection to ensure that things get cleaned up if the application ever terminates abruptly, misbehaves, etc. My rationale here currently is that, if the use-case you envision was allowed, there will be applications out there that blindly connect and kill off all hidden services, even ones that they shouldn't in the name of cleanup.

We could add something like a Detach flag that overrides this behavior, and expanding DEL_ONION to be able to kill off HSes associated with the control port along with Detached ones (along with LIST_ONIONS that provides a list of ServiceIDs for such a purpose). The code here would be easy, and is actually a reasonable argument for switching to the Flags=DiscardPK,Detach type syntax.

comment:38 Changed 4 years ago by atagar

This is for the response right?

Oh baka, I'm being stupid. Controller inputs change so infrequently it completely slipped my mind that I wasn't looking at a reply.

I made the decision to tie HSes added this way to the controller connection to ensure that things get cleaned up if the application ever terminates abruptly, misbehaves, etc.

Actually, on reflection I really like this design.

All points I brought up are incorrect. Sorry about the confusion. :)

comment:39 in reply to:  38 ; Changed 4 years ago by yawning

Replying to atagar:

This is for the response right?

Oh baka, I'm being stupid. Controller inputs change so infrequently it completely slipped my mind that I wasn't looking at a reply.

I made the decision to tie HSes added this way to the controller connection to ensure that things get cleaned up if the application ever terminates abruptly, misbehaves, etc.

Actually, on reflection I really like this design.

All points I brought up are incorrect. Sorry about the confusion. :)

No worries. I went and changed the syntax to Flags=DiscardPK in both the code and the spec. Since enough people asked about it, I'm severely tempted to go and add the Detach flag now as well, and LIST_ONIONS since it covers the use case you detailed (and a lot of other people initially ask about, till I explain why things are this way).

Applications/users that chose to use Detach, and get their HSes snuffled out like a guttering candle amidst a typhoon, will be treated to a performance of "The Requiem Mass in D minor" on my world's smallest violin.

Last edited 4 years ago by yawning (previous) (diff)

comment:40 in reply to:  39 Changed 4 years ago by yawning

Replying to yawning:

No worries. I went and changed the syntax to Flags=DiscardPK in both the code and the spec. Since enough people asked about it, I'm severely tempted to go and add the Detach flag now as well, and LIST_ONIONS since it covers the use case you detailed (and a lot of other people initially ask about, till I explain why things are this way).

I went and did this, though I ended up calling the command "GET_ONIONS". The spec has been updated as appropriate. At this point I can't think of anything else this needs for the 1.0 implementation. I'll notify tor-dev@ that I went and changed a bunch of stuff, but I'm not sure if there's really more that people want out of a "first pass" of this.

comment:41 Changed 4 years ago by naif

Now that this ticket is implemented, i'd suggest using it to test properly "Make tor support starting with 10.000 Tor Hidden Services" https://trac.torproject.org/projects/tor/ticket/15251 that's required to implement OnionFlare in Tor2web https://github.com/globaleaks/Tor2web/issues/228

comment:42 Changed 4 years ago by yawning

Status: needs_revisionneeds_review

As threatened, I fixed one harmless warning (int -> size_t), and did the rebase + squash dance.

Spec: https://github.com/Yawning/torspec/compare/feature6411
Branch: https://github.com/Yawning/tor/compare/feature6411_v2

comment:43 Changed 4 years ago by special

A few minor comments on the spec:

  • The spec repeatedly uses 'Onion Services', but all other documentation still says 'Hidden services'. The reader may wonder what the difference is between these 'Onion Services' and the hidden services they can create via the configuration.
  • GET_ONIONS could be a GETINFO command instead, but I can't think of any particular benefit to changing it
  • Because GET_ONIONS doesn't return services from other control connections, there is no way to comprehensively list services published by tor. They're not confidential either, though: those service names can (I think) appear in asynchronous events.

Nothing stands out to me as wrong in the code branch, but I'm not qualified to properly review it either.

comment:44 Changed 4 years ago by kevinevans

Thank you guys for doing this. Cool to see this feature being added to Tor!

comment:45 Changed 4 years ago by dgoulet

Status: needs_reviewneeds_revision

Here is what I have after a first review. Very awesome feature I might add :).

1) crypto_pk_base64_decode()

  • size_t der_len should be checked against <= 0 because base64_decode() can return negative value.

2) handle_control_add_onion() --> HUGE :)

  • tor_parse_long(port_str, 10, 1, 65536, NULL, ..., shouldn be 65535 here else 65536 is accepted and currently abort if used.
  • I think TARGET needs to be validated because this makes tor die (crazy IP):

ADD_ONION NEW:BEST Port=65535,181.121.123.1116:655

Quite related to previous port bug. Didn't investiguate more but seems service->ports is invalid in some way.

  • if (!strcasecmp("RSA1024", key_type)) {, I would continue the good practice of using "static const char *" like you did with Flags= and Port=. Same for BEST and NEW.

3) handle_control_del_onion()

  • if (onion_services == NULL || idx == -1) {, the "idx == -1" doesn't seem useful.
  • In this function, the given service ID is not validated so it's blindly passed to rend_service_del_ephemeral which validates it and can return an error. The comment This should *NEVER* fail, is wrong in that sense. I think would be good if you could inform the user that the service ID is malformed in the first place instead of a generic "Failed to remove Onion Service" error?

4) rend_config_services

  • This comment is a bit confusing. Can you clarify why you are doing that?
    /* Pull all the ephemeral services out of old_service_list and add them
     * into rend_service_list so they remain active and surviving_service_list
     * so the intro points don't get closed.
     */
    

5) rend_service_add_ephemeral

  • Documenting the returned value would be desirable. (same for del_ephemeral)
  • s->directory = NULL; /* This indicates the service is ephemeral. * I would also document that in the structure definition in rendservice.c at the top.
  • This function has some code duplication from rend_config_services(). I would advocate for a kind of "rend_service_create" function that would do the allocation and initialization. We can even split that into "create()" and "init()" if you prefer. You even add this comment in del_ephemeral() ;) for which I agree and should be done.
     * XXX: As with the comment in rend_config_services(), a nice abstraction
     * would be ideal here, but for now just duplicate the code.
    

comment:46 in reply to:  45 Changed 4 years ago by yawning

Replying to dgoulet:

Here is what I have after a first review. Very awesome feature I might add :).

Wow, that was fast thanks.

1) crypto_pk_base64_decode()

  • size_t der_len should be checked against <= 0 because base64_decode() can return negative value.

Fixed.

2) handle_control_add_onion() --> HUGE :)

  • tor_parse_long(port_str, 10, 1, 65536, NULL, ..., shouldn be 65535 here else 65536 is accepted and currently abort if used.
  • I think TARGET needs to be validated because this makes tor die (crazy IP):

ADD_ONION NEW:BEST Port=65535,181.121.123.1116:655

Quite related to previous port bug. Didn't investiguate more but seems service->ports is invalid in some way.

I'll think of a nice way to fix this. Probably involving exposing parse_port_config().

  • if (!strcasecmp("RSA1024", key_type)) {, I would continue the good practice of using "static const char *" like you did with Flags= and Port=. Same for BEST and NEW.

Fixed.

3) handle_control_del_onion()

  • if (onion_services == NULL || idx == -1) {, the "idx == -1" doesn't seem useful.

Fixed.

  • In this function, the given service ID is not validated so it's blindly passed to rend_service_del_ephemeral which validates it and can return an error. The comment This should *NEVER* fail, is wrong in that sense. I think would be good if you could inform the user that the service ID is malformed in the first place instead of a generic "Failed to remove Onion Service" error?

Kind of, did you read the rest of the comment? There's no way that a malformed service ID will be on either list, so a "552 Unknown Onion Service id" error is going to be returned (onion_services will be NULL, idx will be -1, so rend_service_del_ephemeral() will never get called).

rend_service_del_ephemeral(service_id) failing here is a bug, because it indicates that there's an inconsistency between the per-control-connection/global detached HS lists, and the HSes that are actually running.

I will add a call to rend_valid_service_id() before going through the lists for clarity and the sake of a more descriptive error, but I believe the current code is correct.

4) rend_config_services

  • This comment is a bit confusing. Can you clarify why you are doing that?
    /* Pull all the ephemeral services out of old_service_list and add them
     * into rend_service_list so they remain active and surviving_service_list
     * so the intro points don't get closed.
     */
    

That comment is utter failboat. Replaced with:

    /* Preserve the existing ephemeral services.
     *
     * This is the ephemeral service equivalent of the "Copy introduction
     * points to new services" block, except there's no copy required since
     * the service structure isn't regenerated.
     *
     * After this is done, all ephemeral services will be:
     *  * Removed from old_service_list, so the equivalent non-ephemeral code
     *    will not attempt to preserve them.
     *  * Added to the new rend_service_list (that previously only had the
     *    services listed in the configuration).
     *  * Added to surviving_services, which is the list of services that
     *    will NOT have their intro point closed.
     */

Fixed.

5) rend_service_add_ephemeral

  • Documenting the returned value would be desirable. (same for del_ephemeral)

Ok, will do.

  • s->directory = NULL; /* This indicates the service is ephemeral. * I would also document that in the structure definition in rendservice.c at the top.

Ok, will do.

  • This function has some code duplication from rend_config_services(). I would advocate for a kind of "rend_service_create" function that would do the allocation and initialization. We can even split that into "create()" and "init()" if you prefer. You even add this comment in del_ephemeral() ;) for which I agree and should be done.
     * XXX: As with the comment in rend_config_services(), a nice abstraction
     * would be ideal here, but for now just duplicate the code.
    

Hmm. I could add rend_service_new() or something, but to properly minimize duplication (to the point where it makes sense to me), rend_config_services() would need a lot of changes. I'd like to avoid doing that in this patch if possible to minimize the impact on the rest of the HS code.

The common parts that are immediately obviously extracted are:

       service = tor_malloc_zero(sizeof(rend_service_t));
       service->ports = smartlist_new();
       service->intro_period_started = time(NULL);
       service->n_intro_points_wanted = NUM_INTRO_POINTS_DEFAULT;

Is that worth it?

TODO:

  • Add validation to handle_del_onion() for user friendliness.
  • Figure out what to do with Port.
  • Add documentation comments for the rendservice.c stuff (return values, directory == NULL).

comment:47 Changed 4 years ago by yawning

Status: needs_revisionneeds_review

Ok, everything except the rend_config_services() code duplication is fixed. Not sure if that's worth doing yet. The Port TARGET bug turned out to be something incredibly stupid and simple, so I left the code as is, and added a sanity check so the command will return an error instead of crash.

The latest commit in the branch is a bit overloaded since it fixes the TARGET sillyness, changes return values of rend_service_add_ephemeral() (so that -1 is a generic failure), and adds the comments you wanted. Sorry.

Tentatively needs_review-ing this again, so we can argue about rend_config_services(), and since everything else is addressed.

comment:48 in reply to:  43 Changed 4 years ago by yawning

Replying to special:

  • The spec repeatedly uses 'Onion Services', but all other documentation still says 'Hidden services'. The reader may wonder what the difference is between these 'Onion Services' and the hidden services they can create via the configuration.

Maybe. I figured we had to start using the new and improved terminology somewhere.

  • GET_ONIONS could be a GETINFO command instead, but I can't think of any particular benefit to changing it

If this would be more consistent, this could easily be changed. If people who do control port stuff think this should be folded into GETINFO, I'll change it.

  • Because GET_ONIONS doesn't return services from other control connections, there is no way to comprehensively list services published by tor. They're not confidential either, though: those service names can (I think) appear in asynchronous events.

This is somewhat future proof in that the reply format returns flags. So this could easily be modified/extended in the future to return all the things. The only reason this currently exists is so people hopefully manage "Detach"ed Onion Services correctly (though I'd prefer if people ignored the fact that it's possible to create those in the first place).

Maybe the spec should mandate ignoring unknown flags in the GET_ONIONS response?

comment:49 Changed 4 years ago by yawning

After some thought on the matter, "GET_ONIONS" is now "GETINFO onions/current" and "GETINFO onions/detached", that just returns a dot encoded, NL delimited list of Onions with the queried ownership. This is simpler, and consistent with every other information query that's not serviced by "GETCONF".

Still needs-review, spec and my v2 branch have been updated appropriately.

comment:50 Changed 4 years ago by special

There's no way to change the target ports for a service other than calling DEL_ONION and ADD_ONION again, which has side effects (like getting all new IPs, disruptions). This becomes even more relevant if we add client authentication data later, for example.

Use case: I was thinking about modifying onionwrap[1] to monitor ports bound by its child process and forward all of them. It would sometimes need to add new ports.

The obvious option is to allow ADD_ONION to update the properties (ports, detach?) of an existing service, but this is a problem for fully ephemeral services where the controller didn't even get a PK.

But, it seems excessive to add CHANGE_ONION just for this case.

It's also acceptable to ignore this problem, and if someone later thinks that we need a better solution than DEL/ADD, they can discuss and implement it.

Thoughts?

[1] https://github.com/Yawning/onionwrap

comment:51 in reply to:  50 Changed 4 years ago by yawning

Replying to special:

There's no way to change the target ports for a service other than calling DEL_ONION and ADD_ONION again, which has side effects (like getting all new IPs, disruptions). This becomes even more relevant if we add client authentication data later, for example.

Use case: I was thinking about modifying onionwrap[1] to monitor ports bound by its child process and forward all of them. It would sometimes need to add new ports.

The obvious option is to allow ADD_ONION to update the properties (ports, detach?) of an existing service, but this is a problem for fully ephemeral services where the controller didn't even get a PK.

But, it seems excessive to add CHANGE_ONION just for this case.

It's also acceptable to ignore this problem, and if someone later thinks that we need a better solution than DEL/ADD, they can discuss and implement it.

Thoughts?

I'm leaning towards deferring this for future revisions, but I would lean towards CHANGE_ONION (or similar, name irrelevant) because eventually this functionality should have support for the various authentication mechanisms, which could/should be dynamically modifiable as well.

There isn't a problem of not getting a private key back to handle ADD_ONION supporting modification if that's desired (Eg: the keyType:keyBlob could be something like SERVICEID:<ServiceID>, which would explicitly indicate that modification is wanted).

Modifying ADD_ONION may be easier in the short run, but I'd like to see how HS authentication fits into the picture before commiting to that path.

Ps: The onionwrap code is a tech demo held together by ducttape, chewing gum and bits of string.

comment:52 Changed 4 years ago by dgoulet

Status: needs_reviewneeds_information

Review round 2! :)

  • in rend_service_del_ephemeral(), nitpick, consistent with code, this should be on the same line. Same in handle_control_add_onion().
    +  }
    +  SMARTLIST_FOREACH_END(circ);
    
  • comment on top of rend_service_add_ephemeral() should mention what it does with service_id_out. Simply that on success, this is set with the newly created service_id value and it's the caller responsability to free it and on error, that value is still set to NULL also thus modified.
  • Being *very* picky, I would say that DiscardPK and Detach should also be in a static const.
  • Following the above, the good advantage of having them in a single declaration is that you can also use them in the error/log message. Like these (might have missed some):

connection_printf_to_buf(conn, "512 Invalid 'Flags' argument\r\n");
use "flags_prefix" instead of 'Flags'

connection_printf_to_buf(conn, "512 Missing 'Port' argument\r\n");
use "port_prefix" instead of 'Port'

connection_printf_to_buf(conn, "551 Failed to generate RSA key\r\n");
use "key_type_rsa1024", this one you are going to love it when ecc keys appears and you can just use the right static string without changing all error message hardcoded strings :).

Ok I'm quite happy with this! Works well! If you have some cycles, some test(s) would be quite awesome ;). Once in Stem, we'll probably have regression test but unit test are always useful.

Let me know about the next step but I would say this is ready for upstream (or at least for nickm to look at it). If you prefer a third review after some tweaks, feel free to ask! Good job!

comment:53 Changed 4 years ago by yawning

Status: needs_informationneeds_review

Everything should be fixed with my latest commits, minus the 'Flags' and 'Port' for display purposes.

What remains to be done:

  • I shall rebase/squash down the branches onto feature6411_v3 (tor)/feature6411_v2 (spec).
  • Engage in glorious mortal combat with dgoulet over who gets 3.26. in control-spec.txt.
  • nickm time?

Tagging as needs_review, the "camera ready" branches are at:

Last edited 4 years ago by yawning (previous) (diff)

comment:54 Changed 4 years ago by yawning

Keywords: nickm-review added

comment:55 Changed 4 years ago by nickm

Status: needs_reviewneeds_revision

Notes:

  • <span class="darth-vader">I find your lack of tests....disturbing.</span>
  • What motivates the memwipe in crypto_pk_base64_encode() ?
  • Can we split the command-parsing part of add_onion handling into its own function, so that it can be tested separately? It's pretty bulky...
  • Let's turn the return values from rend_service_add_ephemeral into an enum.
  • I'm not fond of naming variables l, because of I and 1.
  • The new return value for rend_add_service needs to get documented.
  • What happens if two control connections try to create an ephemeral hidden service with the same identity? Is that case covered?
  • Is there a reason not to allow the GETINFO command to enumerate hidden services configured with the torrc file?

comment:56 in reply to:  55 Changed 4 years ago by yawning

Quickly replying to stuff that doesn't require code changes, I'll fix the other things.

Replying to nickm:

  • What motivates the memwipe in crypto_pk_base64_encode() ?

Because I'm doing "encode into newline delineated Base64, then strip out the newlines in place". So the tail of the buffer will have some keying material past the nul terminator. It's not a lot, of keying material, since it's Base64 encoded, and only the number of chars I'm stripping out, but better safe than sorry.

  • What happens if two control connections try to create an ephemeral hidden service with the same identity? Is that case covered?

Yes, I check for duplicates across all rend_services, on add, even ones from the torrc/setconf. The only case that's not handled is duplicate identities across torrc HSes.

  • Is there a reason not to allow the GETINFO command to enumerate hidden services configured with the torrc file?

I didn't want to complicate the patch further than what it already does (since it's rather large) by adding a way to iterate through rend_service_list. It's something that can easily be added later as a separate patch, and in the mean time, GETCONF exists.

comment:57 Changed 4 years ago by nickm

Keywords: 027-triaged-1-out added

Marking triaged-out items from first round of 0.2.7 triage.

comment:58 in reply to:  55 ; Changed 4 years ago by yawning

To keep this on people's radar... I pushed a bunch more commits to the _v3 branch, that I shall squash/rebase once it's passed review.

Replying to nickm:

Notes:

  • <span class="darth-vader">I find your lack of tests....disturbing.</span>

This should be the last thing required.

  • What motivates the memwipe in crypto_pk_base64_encode() ?

aa5af50a32719c2ade2d9b86b1c0ae26686e92c4 - adds a comment. The correct fix requires #15652.

  • Can we split the command-parsing part of add_onion handling into its own function, so that it can be tested separately? It's pretty bulky...

9d2b42fd61e63c9b9e9ea1bdcb3b77a58a72eb20 - Carves out the key argument parsing stuff.
8c31713d34287232461f21d4f7d02992b2c25aa7 - Makes add_onion use the rend_service port config parser.

It's still a tad bulky, but significantly less so, and more testable. Reducing it further is possible, but all the control code tends to be a gigantic ball of bulk, so I don't feel too bad about it in it's current state.

  • Let's turn the return values from rend_service_add_ephemeral into an enum.

1d183d640140606e030e269b6271d81e2c982239

  • I'm not fond of naming variables l, because of I and 1.

ac6431d70b9cd612033cf392d84a0160f09b1552

  • The new return value for rend_add_service needs to get documented.

28e7e0d1d47d76ba5f7da25a98ff0614466ce602

Just needs tests at this point. I'll write ones for the new stuff in common/crypto.c and the testable parts of the add_onion handler.

comment:59 in reply to:  58 Changed 4 years ago by yawning

Status: needs_revisionneeds_review

Replying to yawning:

Just needs tests at this point. I'll write ones for the new stuff in common/crypto.c and the testable parts of the add_onion handler.

7bdb379dded7f80d97f2d0272b3021629321afa0 - common/crypto.c
5e641a2ff4a3300ab7cda4ff7342f19969f205f5 - Fix a dumb that the tests caught (Yay).
0dcc2df20055c02b869b83e7ef0243ccc0bcfd6a - The rend_service_port_config parser and key generator/parser.

There may be a better place for the rend_service_port_config stuff, but it wasn't immediately obvious.

comment:60 Changed 4 years ago by yawning

Spec: https://github.com/Yawning/torspec/compare/feature6411_v3
Branch: https://github.com/Yawning/tor/compare/feature6411_v4

Changes since the last comment:

  • Squashed/rebased vs latest master of both trees.
  • crypto_pk_base64_encode() uses the new b64 encoding routines.

This should be merge ready.

comment:61 Changed 4 years ago by nickm

Merged it!

comment:62 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged the spec change too.

Note: See TracTickets for help on using tickets.