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

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.

added component::archived/obfsproxy owner::asn priority::medium resolution::fixed status::closed type::defect labels

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.

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

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

How would you handle listener and target addresses with this?

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.

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.

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.

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

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

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

'bug3204' branch

Trac:
Status: new to needs_review

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)

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.

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.

Fixed! Check again!

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?

merged; thanks!

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

Trac:
Component: Pluggable transport to Obfsproxy

closed