Mbed-TLS / mbedtls

An open source, portable, easy to use, readable and flexible TLS library, and reference implementation of the PSA Cryptography API. Releases are on a varying cadence, typically around 3 - 6 months between releases.
https://www.trustedfirmware.org/projects/mbed-tls/
Other
5.5k stars 2.6k forks source link

Some programs no longer work when invoked with no arguments #7100

Open gilles-peskine-arm opened 1 year ago

gilles-peskine-arm commented 1 year ago

https://github.com/Mbed-TLS/mbedtls/pull/6998 has changed many programs to error out when invoked with no argument. This should only be done for programs that don't do anything useful when invoked with no arguments!

I don't think the condition if (argc == 0) was ever right: it doesn't make sense to distinguish argc == 0 (no arguments, not even a program name) and argc == 1 (no arguments apart from the program name). But in many cases, the correct fix would have been to only print the usage information if there is a wrong parameter, plus fix any missing initialization or out-of-bounds access to argv.

We should fix this before the next release, since it's a regression from the previous release.

tom-cosgrove-arm commented 1 year ago

We should fix this before the next release, since it's a regression from the previous release.

This is a change in the behaviour of (some of the) example programs, as opposed to a change in the library's ABI or API, so we don't really have to do that.

Also, there is no documentation of the example programs that I can see, apart from their code, and the usage statements that the code emits.

Many of these follow the standard C idiom of "check argc, if nothing given, print a usage statement". This is very useful behaviour for an example program. Even if it does have defaults for parameters, until you've run it and got the usage you don't know what the parameters are. I don't see --help or even help listed for these, so it does seem that "run with no arguments" was intended to give usage.

So: to me, the fact that these programs used to do something useful when run without arguments was more of a bug than a feature!

(As evidence: the code with if (argc == 0) USAGE() that would otherwise be pretty nonsensical - this is clearly intended to print usage in the face of no arguments. If it wasn't intended to do that, a for-loop running for (int i = 1; i < argc; i++) is perfectly safe when argc is just 0 or 1.)

We might want to add At least one parameter must be given to the usages, at which point the example programs are fully internally consistent.

gilles-peskine-arm commented 1 year ago

The lack of documentation and testing of the example programs is not a good argument for breaking them.

the standard C idiom of "check argc, if nothing given, print a usage statement".

That is not at all a standard idiom. A standard idiom is to do something sensible when given nothing, print a long usage statement when given --help, and print a short usage statement if given invalid parameters.

this is clearly intended to print usage in the face of no arguments

No. Maybe the first time it was written, but not in the programs that are meant to work fine when invoked without arguments, of which there are several (not sure if it's a majority).

daverodgman commented 1 year ago

These are example programs, not aimed at using in production, so a change in behaviour is not breaking our compatibility promises. IMO it is more important for these to be accessible and user-friendly than to be efficient to use.

I also think that from a security point of view, it seems poor practice for something like dh_genprime to generate a key of unspecified size; users would normally want a key of a specific size as opposed to a not-obviously-visible default size. It doesn't seem like a style of use we should encourage.

gilles-peskine-arm commented 1 year ago

These are example programs

Yes, so

it is more important for these to be accessible and user-friendly than to be efficient to use

yes, so when you use a program according to its documented usage, you shouldn't get a generic error message!

from a security point of view, it seems poor practice for something like dh_genprime to generate a key of unspecified size; users would normally want a key of a specific size as opposed to a not-obviously-visible default size

Actually, from a security point of view, the general assumption is that we know better about cryptographic considerations like choosing a key size. So the program should have a secure default. (However, in the particular case of dh_genprime, a conservatively secure default would be very slow, so as a demonstration, a smaller key size would be better.)

dh_genprime isn't the only program. I haven't made a list, but what made me initially notice was ssl_server2. That's not a program intended for users, but when testing something related to SSL, I often run ssl_server2 with no arguments and the client with whatever arguments are needed to force a cipher suite, protocol version, etc. And when I did that yesterday I got a mysterious error message.

tom-cosgrove-arm commented 1 year ago

And when I did that yesterday I got a mysterious error message.

"Mysterious" being something that had usage: ssl_server2 param=<>... very close to the top? i.e. you run it with a bunch of parameters (and it doesn't even say usage: ssl_server2 [param=<>]...!)

These are example programs. The main intended user of example programs should be people who aren't very familiar with the library, so giving a bunch of help when run with no arguments still seems perfectly reasonable to me.

And I still think the intent is clear that this is what the programs are meant to be doing: you can run a program with no arguments, or with some. These programs, since the beginning, have processed their arguments, and had code to print a usage when given argc == 0. Now, you have to go through some hoops to actually run a program with argc == 0 on a typical system (Linux, Darwin, Windows), so that clearly can't have been the intent (to only give usage when run with argc == 0 as opposed to giving usage when the program was run with no arguments).

I'm sorry we broke your workflow (add link to obligatory XKCD here) but someone like yourself can easily add a simple parameter (e.g. server_port=4433) to get it working again. And while we might use the example programs, I don't think we are the primary target for them.

gilles-peskine-arm commented 1 year ago

Mysterious as in, what on earth can have go wrong that ssl_server2 breaks even on the simplest possible usage, namely no parameters at all?

someone like yourself can easily add a simple parameter (e.g. server_port=4433) to get it working again

Sure. That took me about 10 seconds. After maybe 10 minutes to figure out what the problem was.

But I don't understand your insistence that we should spend those extra 15 seconds each time. And that each person who's new to the program should spend the 10 minutes at least once.

The programs' reaction to an invalid command line is bad: they just spout usage information instead of explaining what's wrong. We should fix that (and as usual, I have an old bit-rotten pull request that does it to some extent), but I don't think it's our highest-priority technical debt item. If we fixed that, the no-argument case would clearly be wrong for programs like ssl_server2 — “Error: no arguments given, but no arguments are required”.

gilles-peskine-arm commented 1 year ago

I still think the intent is clear that this is what the programs are meant to be doing: you can run a program with no arguments, or with some. These programs, since the beginning, have processed their arguments, and had code to print a usage when given argc == 0

I don't know the intent. But we can see the history, and that's no how it went. The first commit to add a check on argc == 0 was 9caf2d2d38835cf3d53954d4ce64dea09b9f12d0, and it changed the existing program ssl_client2 to support some command line arguments. After that there was no related change for a long time, and in the meantime there was no practical way to display the usage, but ssl_client2 with no parameters kept working. It's not clear to me whether Paul intended ssl_client2 with no arguments to keep working (a thing that Paul probably did regularly when debugging) and intended a different method (like help) to print the usage, or whether Paul did intend ssl_client2 with no arguments to print the usage and ssl_client2 to require at least one argument (even though all arguments have a sensible default).

But anyway, I'm far less concerned by the intent of Paul 13 years ago than the usability of the programs now. Sure, it's not a high-priority task, but why should we make it worse?

gilles-peskine-arm commented 11 months ago

I'm reopening this issue because I ran into it again. Several of our programs don't conform to their documentation or to any reasonable user expectation, and require at least one argument even when all arguments have defaults.