Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#15588 closed enhancement (fixed)

Allow client authorization on control port ADD_ONION services

Reported by: special Owned by: special
Priority: High Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, control, TorCoreTeam201605, TorCoreTeam-postponed-201604
Cc: yawning Actual Points:
Parent ID: #8993 Points: 1
Reviewer: nickm Sponsor:

Description (last modified by special)

We should extend the control port ADD_ONION command from #6411 to support HS client authorization. This would be useful to Ricochet, and probably other projects.

It's also more important to allow changing an existing service when we might want to add or remove authorized clients, so an UPDATE_ONION command would be useful and probably not difficult.

I'd like to see this done before 0.2.7 is final, so I'm going to look into it.

Child Tickets

Change History (35)

comment:1 Changed 3 years ago by special

Milestone: Tor: 0.2.7.x-final

comment:2 Changed 3 years ago by special

Description: modified (diff)
Status: newneeds_review

A specification and implementation for this are on my feature15588 branches of torspec and tor:

https://gitweb.torproject.org/user/special/torspec.git/log/?h=feature15588
https://gitweb.torproject.org/user/special/tor.git/log/?h=feature15588

This currently only implements the "basic" authorization method. "stealth" is slightly more complex, because it has a credential that is private to the service and one that is shared with the client.

comment:3 Changed 3 years ago by nickm

3cee199fc668ff7c06925f899711a265294227a1 : Looks okay.
420f2bde39639fa5f5ad2c966d7b32760d628ce9 : We should have unit tests for these functions.
dcc954f05657054080ac2b67aa2dacb5df537159 : What's the point of having add_onion_helper_clientauth be declared STATIC rather than static?

How tested is this code?

comment:4 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

comment:5 Changed 3 years ago by special

Status: needs_revisionneeds_review

Not tested enough. I found and fixed a few silly mistakes in the control command parsing. I'm mostly confident about it now.

Also rebased (with conflicts), added unit tests for auth cookie en/de-coding, and changed STATIC to static.

comment:6 Changed 3 years ago by nickm

Does anything explode if we take this early in the 0.2.8 timeframe instead of last-minute for 0.2.7?

comment:7 Changed 3 years ago by special

No. Waiting for 0.2.8 is a fine decision.

comment:8 Changed 3 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.8.x-final

comment:9 Changed 3 years ago by nickm

Keywords: 028-triaged added

comment:10 Changed 3 years ago by dgoulet

Keywords: hidden-service control 028-triaged removed
Points: small/medium

comment:11 Changed 3 years ago by nickm

Keywords: hidden-service control 028-triaged added
Points: small/mediumsmall
Priority: normalmajor

comment:12 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

9e44364643eba61567249705374579d58836f832 : looks fine still.

2ac5c5c59cee3ad471eee14995d316988cc909c8 : I'm glad to see tests here now.

  • Suggestion: this code could use base64_encode_nopad() and base64_decode_nopad() to handle the padding-stripping part of the logic.
  • I think the ddecode function needs to check the length of descriptor_cookie_tmp after decoding it? The old code does that, right?
  • descriptor_cookie_tmp should probably be of type uint8_t, yeah?

dce6310a49fb6c0b08a0d5c3220d46834df24d61 : We should add documentation on the type of the new auth_clients argument to rend_service_add_ephemeral, and document that we take ownership of the reference.

11575f3be9705ff571eb24c2506f6e83ae284aa9 : Unit tests wouldn't be too hard to add here, and would be good for ensuring that we got the code right.

I'd be happy to do the above, or you could do it. But if you do it, please do it by adding fixup commits to the branch, rather than by rewriting the branch, so that I can review *only* the part that changed?

(Also, how much of this have you tested in the wild, as client and as server, with actual authorization types?)

comment:13 Changed 3 years ago by nickm

Keywords: pre028-patch added

comment:14 Changed 3 years ago by nickm

Severity: Normal

Also, this needs to compile with --enable-gcc-warnings

comment:15 Changed 3 years ago by nickm

(Special says he's still interested. I'll keep this in 0.2.8, in hopes.)

comment:16 in reply to:  12 Changed 3 years ago by special

Replying to nickm:

  • Suggestion: this code could use base64_encode_nopad() and base64_decode_nopad() to handle the padding-stripping part of the logic.

This function strips 'A=' (not '==') because the auth type is encoded in the high bits of the last input byte. base64_*_nopad would leave the extra character on encode or lose those bits on decode.

  • I think the ddecode function needs to check the length of descriptor_cookie_tmp after decoding it? The old code does that, right?

Done

  • descriptor_cookie_tmp should probably be of type uint8_t, yeah?

Technically yes, but these are being used as char everywhere, and that's what base64_decode expects the buffer to be.

dce6310a49fb6c0b08a0d5c3220d46834df24d61 : We should add documentation on the type of the new auth_clients argument to rend_service_add_ephemeral, and document that we take ownership of the reference.

Done

11575f3be9705ff571eb24c2506f6e83ae284aa9 : Unit tests wouldn't be too hard to add here, and would be good for ensuring that we got the code right.

Done

(Also, how much of this have you tested in the wild, as client and as server, with actual authorization types?)

There isn't an "in the wild" user of this functionality yet; it's just speculative for Ricochet. I haven't found any problems in controlled testing.

Fixups are on the top of my feature15588 branch.

comment:17 Changed 3 years ago by nickm

Status: needs_revisionneeds_review

comment:18 Changed 3 years ago by special

I did a ninja-force-push to get check spaces to pass. If you pulled it already, you might need to reset to 078fd1ffcf9dd70d12314bd3a34e6ff1f11338a1.

comment:19 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_revision

First, this branch has warnings (--enable-gcc-warnings SO useful :):

src/or/control.c: In function ‘add_onion_helper_clientauth’:
src/or/control.c:4147:69: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
       strspn(client->client_name, REND_LEGAL_CLIENTNAME_CHARACTERS) != len) {
                                                                     ^
src/or/control.c:4110:63: error: unused parameter ‘auth_type’ [-Werror=unused-parameter]
 add_onion_helper_clientauth(const char *arg, rend_auth_type_t auth_type,

  • 9e44364643eba61567249705374579d58836f832
  • 2ac5c5c59cee3ad471eee14995d316988cc909c8

In rend_auth_encode_cookie: cookie_out = NULL;, I don't think that's needed.

In rend_auth_decode_cookie: int len = strlen(descriptor_cookie);, it should be size_t.

This memset(descriptor_cookie_tmp, 0, sizeof(descriptor_cookie_tmp)); can be replaced by char descriptor_cookie_tmp[REND_DESC_COOKIE_LEN+2] = {0};

Does that work if descriptor_cookie points to descriptor_cookie_base64ext which has +1 length? If yes, I would comment on why we don't use the right srclen.

+  if (base64_decode(descriptor_cookie_tmp, sizeof(descriptor_cookie_tmp),
+                    descriptor_cookie, REND_DESC_COOKIE_LEN_BASE64+2) < 0) {


While we are at it, I would rename descriptor_cookie_tmp to something more meaningful like descriptor_cookie_decoded.

I know this is a "move to function" thing so let's not change too much but for the record, this is not pretty: if (auth_type_val < 1 || auth_type_val > 2). There, it is on the record :).

  • dce6310a49fb6c0b08a0d5c3220d46834df24d61
  • 11575f3be9705ff571eb24c2506f6e83ae284aa9

Should be size_t here: int len = strlen(client->client_name);

This if (len < 1 || len > 19 should use the define REND_CLIENTNAME_MAX_LEN. I'm not sure where 19 comes from here...

At some point, we should have functions that validate input for auth. because I see some code duplication with rendservice.c ... :(

crypto_rand() can fail. Better check that so we avoid having empty cookies.

---

Done for now.

comment:20 in reply to:  19 Changed 3 years ago by nickm

Replying to dgoulet:

[...]

crypto_rand() can fail. Better check that so we avoid having empty cookies.

FWIW, this is no longer true.

(Thanks for doing code review though!)

comment:21 in reply to:  19 Changed 2 years ago by special

Status: needs_revisionneeds_review

Replying to dgoulet:

First, this branch has warnings (--enable-gcc-warnings SO useful :):

Oof. That must've gotten lost from this build. Sorry about the sloppiness.

In rend_auth_encode_cookie: cookie_out = NULL;, I don't think that's needed.

It is; cookie_out is the return value after the jump.

In rend_auth_decode_cookie: int len = strlen(descriptor_cookie);, it should be size_t.

Done.

This memset(descriptor_cookie_tmp, 0, sizeof(descriptor_cookie_tmp)); can be replaced by char descriptor_cookie_tmp[REND_DESC_COOKIE_LEN+2] = {0};

Done.

Does that work if descriptor_cookie points to descriptor_cookie_base64ext which has +1 length? If yes, I would comment on why we don't use the right srclen.

+  if (base64_decode(descriptor_cookie_tmp, sizeof(descriptor_cookie_tmp),
+                    descriptor_cookie, REND_DESC_COOKIE_LEN_BASE64+2) < 0) {

descriptor_cookie_base64ext has +1 for a null terminator. The source data is always exactly REND_DESC_COOKIE_LEN_BASE64+2 characters here, so we are using the right srclen.

While we are at it, I would rename descriptor_cookie_tmp to something more meaningful like descriptor_cookie_decoded.

Done.

I know this is a "move to function" thing so let's not change too much but for the record, this is not pretty: if (auth_type_val < 1 || auth_type_val > 2). There, it is on the record :).

Yeah :/

This if (len < 1 || len > 19 should use the define REND_CLIENTNAME_MAX_LEN. I'm not sure where 19 comes from here...

I was using the same logic as rend_parse_client_keys, but rend_config_services did it differently. I've combined all of these into a rend_valid_client_name function that uses REND_CLIENTNAME_MAX_LEN.

New commits are 567f76f31c31690f28739fde9cf00da3890da053 and 855e5b72777a165bde86a14422b16adb10815dd0. I'll squash these once we're done with review. Thanks!

comment:22 Changed 2 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final

These seem like features, or like other stuff unlikely to be possible this month. Bumping them to 0.2.9

comment:23 Changed 2 years ago by dgoulet

Keywords: hidden-service 028-triaged pre028-patch removed
Reviewer: dgoulet
Sponsor: None

comment:24 Changed 2 years ago by nickm

Keywords: TorCoreTeam201604 added

Every postponed needs_review ticket should get a review in April

comment:25 Changed 2 years ago by dgoulet

Status: needs_reviewmerge_ready

comment:26 Changed 2 years ago by nickm

Sponsor: None

These tickets had Sponsor == "None". Our convention seems to be Sponsor == "".

comment:27 Changed 2 years ago by nickm

I'm reviewing the diff rather than the patch series, since the history looks long.

(Special, do you know about --autosquash? That's how most folks use the fixup! convention. This FIXUP thing you've been doing is less automatable. No need to change this branch, but it might help for next time.)

  • NM.1 -- the output case of handle_control_add_onion is now possibly inconsistent? It looks like it can output some 250- lines followed by a 551 line. That's not allowed, I think.
  • NM.2 -- If it's not possible for add_onion_helper_clientauth to be called with missing created or err_msg_out parameters, should we maybe assert that they are present?
  • NM.3 -- I'm a little worried that for some functions, err_msg_out includes the status code, and for others it doesn't. That doesn't seem to be documented.
  • NM.4 -- I was about to complain about how awful the rend_auth_decode_cookie code is, but apparently it isn't new code, so I won't complain. (sigh)
  • NM.5 -- rend_auth_encode_cookie should really be using uint8_t, not char, especially since you're looking at the numberic value of the bytes. Probably same with rend_auth_decode_cookie().

Otherwise looks plausible.

comment:28 Changed 2 years ago by nickm

Status: merge_readyneeds_revision

comment:29 Changed 2 years ago by nickm

Oh, one more question: Should this thing that is _like_ base64-nopad, but not actually the same as base64-nopad, get a separate function and/or some documentation explaining what it is? (Or did we document it somewhere and I forgot?)

comment:30 Changed 2 years ago by nickm

Keywords: TorCoreTeam201605 TorCoreTeam-postponed-201604 added; TorCoreTeam201604 removed

April is over; calling these april tickets postponed into may.

comment:31 in reply to:  27 Changed 2 years ago by special

Reviewer: dgouletnickm
Status: needs_revisionneeds_review

Replying to nickm:

I'm reviewing the diff rather than the patch series, since the history looks long.

(Special, do you know about --autosquash? That's how most folks use the fixup! convention. This FIXUP thing you've been doing is less automatable. No need to change this branch, but it might help for next time.)

Sorry about that mess - I'll avoid that in the future. To make the merge easier, I cleaned this one up.

My feature15588 branch is the same as what you reviewed, plus three new commits. The other 5 commits are an _unmodified_ squash of what you reviewed.

The new commits are the two 'fixup!' on top of the branch, plus 896271d5 ("Use uint8_t for rend descriptor_cookie fields"). Again, sorry about the confusion.

  • NM.1 -- the output case of handle_control_add_onion is now possibly inconsistent? It looks like it can output some 250- lines followed by a 551 line. That's not allowed, I think.

Fixed by making rend_auth_encode_cookie never fail and asserting that it doesn't instead of having this error case.

  • NM.2 -- If it's not possible for add_onion_helper_clientauth to be called with missing created or err_msg_out parameters, should we maybe assert that they are present?

Fixed by requiring those parameters, and simplifying the code slightly.

  • NM.3 -- I'm a little worried that for some functions, err_msg_out includes the status code, and for others it doesn't. That doesn't seem to be documented.

This is consistent when read in context. The control.c helper function returns a status code line suitable for the control port. Utility functions from other files do not return anything specific to the control protocol. The documentation of add_onion_helper_clientauth mentions that it returns "a control port error message on failure".

  • NM.4 -- I was about to complain about how awful the rend_auth_decode_cookie code is, but apparently it isn't new code, so I won't complain. (sigh)

This is slightly better now, with less random addition sprinkled throughout. Let me know if there's anything else specific I should do.

  • NM.5 -- rend_auth_encode_cookie should really be using uint8_t, not char, especially since you're looking at the numberic value of the bytes. Probably same with rend_auth_decode_cookie().

896271d525b2b31950572293c512224ca57cee02 changes these fields to uint8_t, and the fixup commits fix the auth_(encode|decode) functions to use uint8_t.

comment:32 Changed 2 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

squashed and merged. (to tor and torspec) Thanks!

comment:33 Changed 2 years ago by atagar

Resolution: implemented
Status: closedreopened

Hi there, just a minor spec issue...

+ [ClientAuth was added in Tor 0.x.x.x.]

Actual version please. :)

comment:34 Changed 2 years ago by nickm

Resolution: fixed
Status: reopenedclosed

good catch; should be fixed now.

comment:35 Changed 2 years ago by isabela

Points: small1
Note: See TracTickets for help on using tickets.