Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#3204 closed defect (fixed)

The cli interface of obfsproxy is terrible and makes children cry.

Reported by: asn Owned by: asn
Priority: Medium Milestone:
Component: Obfuscation/Obfsproxy Version:
Severity: Keywords:
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The cli interface of obfsproxy is *awful*.

I asked nickm one day if he knows of any standards for interfaces, and he said "You mean CLI or are you thinking of a GUI?" and I replied "No, just CLI." and then I never got an answer.

I'm pretty sure there are no standards for CLIs, except from the POSIX conventions for command-line options, but really I don't know much about usability.

This trac ticket is to build a nice CLI for obfsproxy.

I started building a getopt() one, but then I remembered that obfsproxy should be running in Windows as well and I got terribly sad.

Child Tickets

Change History (17)

comment:1 Changed 6 years ago by nickm

What do you think the interface should look like? We could try to imitate other tools like stunnel or nc or various socks proxy programs, but I've never really found any of them very intuitive.

comment:2 Changed 6 years ago by asn

I was thinking of an openssl-ish interface:

obfsproxy protocol protocol_opts [ protocol_args ]

where:

  • 'protocol' is the name of the protocol.
  • 'protocol_opts' are protocol specific options,

like whether we are a server or a client.

  • 'protocol_args' are protocol specific arguments,

like setting a shared secret.

For examples:

obfsproxy obfs2 client --shared-secret="supermario"
obfsproxy dummy socks

comment:3 Changed 6 years ago by nickm

So by convention, --arguments usually come before positional arguments.

How would you handle listener and target addresses with this?

comment:4 in reply to:  3 ; Changed 6 years ago by asn

Replying to nickm:

So by convention, --arguments usually come before positional arguments.

True!

How would you handle listener and target addresses with this?

True!
I would say that listener and target addresses belong to the protocol_opts.

So allow me to try again:

obfsproxy --shared-secret="supermario" --dest=10.1.1.1:6666 obfs2 client 127.0.0.1:8888

where the protocol_opts of obfs2 should look like:

<role> <listen address>

and the protocol_args should look like:

--shared-secret=<secret> - set a shared secret
--dest=<destination addport> - set the destination addport, in case of the 'client' or 'server' role.

Yes. It's suddenly starting to look much uglier.

comment:5 in reply to:  4 Changed 6 years ago by asn

In my previous message I said:
Replying to asn:

I would say that listener and target addresses belong to the protocol_opts.

but what I really wanted to say is:

I would say that listener addresses belong to protocol_opts and target addresses belong to the protocol_args. Since, at least for now, listener addresses seem to be mandatory, and target addresses seem to be protocol/role dependent.

comment:6 Changed 6 years ago by nickm

One thing that the above idea doesn't handle is obfsproxy's ability to handle more than one protocol at a time. There's no way to say, "Provide obfs2 on port 5555, and obfs3 on port 5556" with that interface. Not sure if it matters for our needs though.

comment:7 in reply to:  6 Changed 6 years ago by nickm

Replying to nickm:

One thing that the above idea doesn't handle is obfsproxy's ability to handle more than one protocol at a time. There's no way to say, "Provide obfs2 on port 5555, and obfs3 on port 5556" with that interface. Not sure if it matters for our needs though.

One way we could do this is to have some kind of separator such that each set of options is divided from the next. For example,

obfsproxy  --dest=10.1.1.1:6666 obfs2 client 127.0.0.1:8888  --  obfs2 socks 127.0.0.1:8889 -- dummy socks 127.0.0.1:8890

comment:8 Changed 6 years ago by nickm

Or use "+" if "--" is too overloaded

comment:9 Changed 6 years ago by asn

I've been thinking that:

obfsproxy protocol [protocol_args] protocol_opts

looks better and is more intuitive.
Example:

obfsproxy obfs2 --shared-secret=himitsu socks 127.0.0.1:666 + dummy client 127.0.0.1:6667

comment:10 Changed 6 years ago by asn

Status: newneeds_review

'bug3204' branch

comment:11 Changed 6 years ago by asn

Programming question:
atm both network.c and protocols/* stuff are poking into the protocol_params_t structure.
Do you think I should make proto_params_get_* and proto_params_set_* functions for all the elements of protocol_params_t? (like proto_params_set_listen_address_len and proto_params_get_mode)

comment:12 Changed 6 years ago by nickm

IMO no. proto_params_t is basically a data-only structure that's used to pass a bunch of fields at once. It's not an abstract data type.

comment:13 Changed 6 years ago by nickm

Reviewing...

In 1d3d01d5b22e:

It's spelled "SEPARATOR", not "SEPERATOR". Sorry; English orthography is stupid.

is_supported_protocol is weirdly described. The documentation needlessly explains how it's implemented ("Run through all the supported protocols") but not what it actually does (return true if name is the name of a protocol and false otherwise).

Maxing out at MAXPROTOCOLS is silly. C has realloc for a reason.

Please imagine doing tech support on "We don't support crappy protocols, son".

What's with making temporary copies of argv when we could just have indices into it?

Reviewing 67a106ad3bbafe83f1917d8060a1c78bf6d43ff6:

Having the listener_new functions do the argument parsing is a big abstraction layer violation, and makes everything hard to test. Instead, it's better to have the CLI code generate a listener configuration object, and pass that to the listener_new function.

comment:14 Changed 6 years ago by asn

Fixed! Check again!

comment:15 Changed 6 years ago by nickm

Looks better. When I go to merge, though, I get conflicts all over. If you could take a shot at resolving those, that would rock.

Also, have you thought about tests for this?

comment:16 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

merged; thanks!

comment:17 Changed 6 years ago by arma

Component: Pluggable transportObfsproxy
Note: See TracTickets for help on using tickets.