Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#14555 closed defect (fixed)

Clarification about new CIRC attributes

Reported by: atagar Owned by: arthuredelstein
Priority: Low Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords:
Cc: mikeperry, gk, mcs, arthuredelstein@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

10:37 < atagar> nickm: it would be nice if the socks username/password had a description: https://gitweb.torproject.org/torspec.git/commit/?id=29759745fe1a3a216af7eb9e7ddfa20a10e60f47
10:37 < atagar> example for my unit tests would be nice too if you have one handy
10:49 < nickm> atagar: will try to add.  doing about a million other things.  plase keep bugging me if you can

Can do. Questions I have when I look at the new spec addition is...

  • It's not clear to me what these credentials are for. Please describe what the attributes are.
  • You say 'EscapedUsername' and 'EscapedPassword'. What kind of escaping is at play here? Below instead of 'Escaped*' you say "SocksUsername = SocksPassword = a quoted string" which makes me suspect it's just mislabelled.
  • Please keep in mind that you're saying we can have a username without a password or a password without a username. Not sure if that's intended.
  • Not spec related but if you have an example of this event handy (especially an example that demos the escaping if that's really a thing) I'd love to have it so I can add it to our unit tests.

Cheers! -Damian

Child Tickets

Change History (27)

comment:1 Changed 3 years ago by nickm

Cc: mikeperry gk mcs added

Any chance of a writeup here? It was y'all who needed #8405, so it would be awesome if you could do the writeup.

comment:2 Changed 3 years ago by mcs

Cc: arthuredelstein@… added

comment:3 Changed 3 years ago by atagar

Priority: trivialminor

Gonna raise the priority a tad. I'm waiting on this to add the new attribute to Stem.

comment:4 Changed 3 years ago by nickm

Milestone: Tor: 0.2.6.x-final

comment:5 Changed 3 years ago by arthuredelstein

Owner: set to arthuredelstein
Status: newassigned

comment:6 Changed 3 years ago by arthuredelstein

Status: assignedneeds_review

comment:7 Changed 3 years ago by nickm

Thanks, Arthur! This looks okay to me.

Damian, does it meet your needs?

comment:8 Changed 3 years ago by atagar

Thanks! This still doesn't have the precision other spec fields do. If you replace "quoted strings" with "QuotedString" this would have a very well specified meaning...

https://gitweb.torproject.org/torspec.git/tree/control-spec.txt#n90

Keep in mind though that this means you're saying this have qcontent (which is well defined, but I'd need to look up what exactly it means).


The "SOCKS_USERNAME" and "SOCKS_PASSWORD" fields indicate the credentials
that were used by a SOCKS client to connect to Tor's SOCKS port and
initiate this circuit.

Perfect.


Special characters are escaped.

Again, this leaves some ambiguity. If this conforms with QuotedString fields then lets leave it at that instead.

comment:9 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

comment:10 Changed 3 years ago by arthuredelstein

Here's a new patch specifying the escaping. The strings conform with QuotedString, but have additional escapes as described.

https://github.com/arthuredelstein/torspec/commit/01e7cb79bf3877d2c3e2d8f4f46a0a8bdfadbddf

comment:11 Changed 3 years ago by arthuredelstein

Status: needs_revisionneeds_review

comment:12 Changed 3 years ago by atagar

Spec addition looks good to me - thanks!

That's an odd fashion of escaping. This is now detailed enough that I can write an unescape helper but out of curiosity why does it do that?

comment:13 in reply to:  12 Changed 3 years ago by arthuredelstein

Replying to atagar:

Spec addition looks good to me - thanks!

That's an odd fashion of escaping. This is now detailed enough that I can write an unescape helper but out of curiosity why does it do that?

It's using the pre-existing esc_for_log function in src/common/util.c.

comment:14 Changed 3 years ago by atagar

I see. Nick, does this make sense? Seems like this field should match other QuotedString attributes unless there's a good reason for it to do otherwise.

comment:15 Changed 3 years ago by nickm

Hm, there are a few other places that do esc_for_log or (equivalently) escaped in control.c. I don't mind generating QuotedString here if we can, but it appears that in a few other places where we're supposed to generate it, we use esc_for_log instead. I'm a touch concerned about backward compatibility with changing all of them, unless one format is strictly contained in the other. Better analyze.

See also the madness that #4600 has descended into.

comment:16 Changed 3 years ago by atagar

Ah, forgot about that one. For the purposes of this ticket if we can avoid using esc_for_log for these CIRC attributes and other new additions then that seems ideal. Looking deeper at the existing esc_for_log usage and backward compatability can then be tracked on #4600.

comment:17 Changed 3 years ago by nickm

Is there a function in Tor that encodes QuotedStrings?

If we change in 0.2.6.4, will we break old TBB versions that rely on the old behavior?

comment:18 Changed 3 years ago by atagar

Is there a function in Tor that encodes QuotedStrings?

We have QuotedStrings all throughout the control-spec. What do we do for all the other instances?

If we change in 0.2.6.4, will we break old TBB versions that rely on the old behavior?

I filed this ticket right after the attribute was added to the spec. I'm a tad surprised it's already in a release.

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

Replying to atagar:

Is there a function in Tor that encodes QuotedStrings?

We have QuotedStrings all throughout the control-spec. What do we do for all the other instances?

In some cases, the code uses esc_for_log. Others are input-only.

I looked the other night and couldn't find a function that actually encodes in QuotedString format in our code. That doesn't mean there couldn't be one, but hey

If we change in 0.2.6.4, will we break old TBB versions that rely on the old behavior?

I filed this ticket right after the attribute was added to the spec. I'm a tad surprised it's already in a release.

It's in a TBB alpha, but I don't want to make their lives harder.

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

Replying to nickm:

Replying to atagar:

Is there a function in Tor that encodes QuotedStrings?

We have QuotedStrings all throughout the control-spec. What do we do for all the other instances?

In some cases, the code uses esc_for_log. Others are input-only.

I looked the other night and couldn't find a function that actually encodes in QuotedString format in our code. That doesn't mean there couldn't be one, but hey

If we change in 0.2.6.4, will we break old TBB versions that rely on the old behavior?

I filed this ticket right after the attribute was added to the spec. I'm a tad surprised it's already in a release.

It's in a TBB alpha, but I don't want to make their lives harder.

If you want to change the escaping for #8405, I don't expect it will affect TBB. We're not using any special characters for SOCKS username/password.

comment:21 Changed 3 years ago by atagar

If you want to change the escaping for #8405, I don't expect it will affect TBB. We're not using any special characters for SOCKS username/password.

Ah, good to hear - thanks arthuredelstein!

I looked the other night and couldn't find a function that actually encodes in QuotedString format in our code. That doesn't mean there couldn't be one, but hey

Huh. So options seem to either be to make tor conformant with its spec or continue to mis-escape output. There's also an option 'c' of change the spec specify esc_for_log but I really don't like that option (makes life ickier for controllers, and feels like it's specifying a bug).

comment:22 Changed 3 years ago by nickm

I really don't like that option (makes life ickier for controllers, and feels like it's specifying a bug).

Well, the current behavior is going to keep existing until 0.2.6.3-alpha and earlier are all dead and buried. So controllers' lives are hard, if they want to unescape the strings Tor makes now. What if we add a "BUG NOTE" to the spec, indicate that the bug has existed in some Tor versions, and when it will be fixed? That way the spec won't be lying, and we have a path or migrating to the right thing.

I can make the QuotedString in the socks username and password into a real QuotedString for 0.2.6, but fixing all the other instances will probably have to wait for 0.2.7. (Rationale: this is not a regression) We should survey the controller ecosystem to make sure that nothing is depending on the old behavior.

comment:23 Changed 3 years ago by atagar

Perfect! Sounds like a great plan to me.

comment:24 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged arthuredelstein's thing, added some new text of my own, opened #14999 to track the migration in the future, and #15000 for the issue in #8405. Thanks all!

comment:25 Changed 3 years ago by atagar

Resolution: fixed
Status: closedreopened

Hi Nick, I was actually hoping you *wouldn't* add the bit to the circuit attributes specifying the escaping. Rather, would you mind in the spec as simple QuotedStrings and non-complicance can be covered by #14999?

comment:26 Changed 3 years ago by atagar

Resolution: fixed
Status: reopenedclosed

Oops, didn't spot that #15000 was specifically about this field. Re-closing.

comment:27 Changed 3 years ago by atagar

Note: See TracTickets for help on using tickets.