search

okigan / awscurl

curl-like access to AWS resources with AWS Signature Version 4 request signing.
MIT License
737 stars 91 forks source link

Fix: Booleans verify and insecure are not interchangable. #103

Closed beyarkay closed 3 years ago

beyarkay commented 3 years ago

The --help indicates that the -k --insecure flag is by default True, however this was not actually the case.

The confusion is that the --help calls the flag --insecure, but this flag is passed to the make_request method where the parameter is named verify. Naturally verify should be equal to not insecure, and this change implements that.

This change flips the --insecure flag at the make_request method call so that its value is in line with what the method expects, as well as keeping the semantics consistent wherever verify or insecure is mentioned.

okigan commented 3 years ago

Nice!

want to confirm this still would behave consistent with curl:

% curl --help 
Usage: curl [options...] <url>
     --abstract-unix-socket <path> Connect via abstract Unix domain socket
...
     -k, --insecure      Allow insecure server connections when using SSL
beyarkay commented 3 years ago

Thanks! Fair point, it's getting quite late for me now but I'll confirm consistency with curl in the morning (about 12 hours)

beyarkay commented 3 years ago

I have verified that the behavior is consistent with curl, as well as fixed a related bug that caused the default value of the --insecure flag to be True instead of False. In a nutshell, if a user passed in the --insecure flag, then this would get flipped twice before any processing was done on it which didn't result in any functional issues, except that the --help would show a default value of True when the actual default value was False.

In detail: The wording in the python argparse docs isn't great when it comes to action='store_true' or action='store_false' which was likely the source of the problem, but the previous behavior for awscurl meant that if you added an --insecure flag to the command, then it would set args.insecure to be False (which is not what we want).

This error wasn't spotted because later down the line, the value of args.insecure was passed directly to a method parameter called verify, where verify is expected to be the boolean negation of insecure. And since we mistakenly flipped the value of args.insecure, and verify should be equal to not args.insecure, this error didn't actually cause any harm except for the --help being incorrect.

Hope that clears things up!

okigan commented 3 years ago

Sounds reasonable (I'll take your word for it)