From 385485d39b096f21347c9aea3dfa6820c59d8b37 Mon Sep 17 00:00:00 2001 From: Dirk Wetter Date: Fri, 24 Jan 2020 11:33:11 +0100 Subject: [PATCH 1/4] More friendly phrased. Incl. soon to follow coding convention --- CONTRIBUTING.md | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 48acd15..540ad04 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,15 +1,17 @@ ### Contributions / participation -is always welcome. +is always welcome! -Note please that following is strongly requested: +Note please the following: -* One PR per feature or bug fix or improvement. -* Document your PR properly, both in the PR and/or commit message and in the code. -* Please test your changes thoroughly as reliability is important for this project. -* Follow the length [coding guideline](https://github.com/drwetter/testssl.sh/wiki/Coding-Style). +* Please read at least the [coding convention](https://github.com/drwetter/testssl.sh/Coding_Convention.md). +* One PR per feature or bug fix or improvement. Please do not mix issues. +* Document your PR, both in the PR and/or commit message and in the code. +* Please test your changes thoroughly as reliability is important for this project. You may want to check different servers with different settings. +* Travis runs automatically when anything is committed/PR'd. You should check any complains from Travis. Beforehand you can check with `prove -v`. +* If it's a new feature please consider writing a unit test for it. You can use e.g. `t/20_baseline_ipv4_http.t` as a template. The general documentation for [Test::More](https://perldoc.perl.org/Test/More.html) is a good start. +* If it's a new feature it would need to be documented in the appropriate section in `help()` and in `~/doc/testssl.1.md` - -If it's a new feature please consider writing a unit test for it. There's a directory ~/t/ which Travis runs automatically when anything is committed. You can use e.g. `20_baseline_ipv4_http.t` as a template. There's also [general documentation for Test::More](https://perldoc.perl.org/Test/More.html). +For questions just open an issue. From 6c892afecd5881011fae39a3ba004c2e9f21ad35 Mon Sep 17 00:00:00 2001 From: Dirk Wetter Date: Fri, 24 Jan 2020 13:09:05 +0100 Subject: [PATCH 2/4] Move from wiki hereto plus sorting+rephrasing --- Coding_Convention.md | 72 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 Coding_Convention.md diff --git a/Coding_Convention.md b/Coding_Convention.md new file mode 100644 index 0000000..445cc49 --- /dev/null +++ b/Coding_Convention.md @@ -0,0 +1,72 @@ +## Coding Style + +### PR + +You'd make the life of the maintainers easier if you submit only _one_ patch with _one_ functional change per PR. + +### General + * Portability is important: Don't use highly system depended binaries (`rpm`, `ip/ifconfig`, ..) as it is not portable. Or it would require lots of efforts (like C's #ifdefs) to be portable plus the code gets ugly. + * Don't use additional binaries. + * If you really, really need to use an additional binary make sure it's available on the system before calling it. (Example: see `timeout`.) + * Keep in mind that binaries might come in different flavors. Especially with ``sed`` you need to be careful as GNU sed is only 80% compatible with BSD sed (`sed -i`,` \n`, `\t`, etc.). + * Avoid checking for the operating system when using a feature of a binary or an OS. E.g. FreeBSD or MacOSX may or may not have GNU binaries installed, so it's better to check a capability of the binary instead. See how `HAS_*` variables are set. + + +### Documentation + +Some people really read that ! New features would need to be documented in the appropriate section in `help()` and in `~/doc/testssl.1.md`. + +### Coding + +#### Shell / bash + +Bash is actually quite powerful -- not only with respect to sockets. It's not as mighty as perl or python, but there are a lot of neat features. Here's how you make use of them. + +* Don't use backticks anymore, use `$(..)` instead +* Use double square `[[]]` instead of single square `[]` brackets +* The [BashPitfalls](http://mywiki.wooledge.org/BashPitfalls) is a good read! +* Whenever possible try to avoid `tr` `sed` `awk` and use bash internal functions instead, see e.g. [bash shell parameter substitution](http://www.cyberciti.biz/tips/bash-shell-parameter-substitution-2.html). It slower as it forks, fopens and pipes back the result. +* `read` often can replace `awk`: `IFS=, read -ra a b c <<< "$line_with_comma"` +* Bash can also deal perfectly with regular expressions, see e.g. [here](https://www.networkworld.com/article/2693361/unix-tip-using-bash-s-regular-expressions.html) and [here](https://unix.stackexchange.com/questions/421460/bash-regex-and-https-regex101-com). You can as well have a look @ `is_ipv4addr()` or `is_ipv6addr()`. +* If you still need to use any of `tr`, `sed` and `awk`: try to avoid a mix of several external binaries e.g. if you can achieve the same with e.g. `awk`. +* Be careful with very advanced bash features. Mac OS X is still using bash version 3 ([http://tldp.org/LDP/abs/html/bashver4.html](differences)) +* Always use a return value for a function/method. 0 means all is fine. +* Make use of [shellcheck](https://github.com/koalaman/shellcheck) if possible + + +#### Shell / testssl.sh specific +* Make use the short functions / methods (code starts from `###### START helper function definitions`) like + * `count_words()` / `count_lines()` / `count_ciphers()` + * `strip_lf()` / `strip_spaces()` + * `toupper()` / `tolower()` + * `newline_to_spaces()` + * `is_number()` / `is_ipv4addr()` + * .. and much more +* Security: + * Watch out for any input especially (but not only) supplied from the server. Input should never be trusted. + * Unless you're really sure where the values come from, variables need to be put in quotes. + * You can use `safe_echo()` when processing input which does some input validation. + * Use ``out()`` or similar output functions when writing something back to the user. +* Use `$OPENSSL` instead of `openssl`. The latter is highly system depended and also $OPENSSL is a binary which capabilities are checked internally before using it, independent whether the supplied one is being used or another one. + +#### Variables +* Use "speaking variables" but don't overdo it with the length +* No camelCase please. We distinguish between lowercase and uppercase only + * Global variables + * use them only when really necessary + * in CAPS + * initialize them + * use ``readonly`` and use typing (variable types) if possible +* Local variables + * are lower case + * declare them before usage (`local`) + * initialize them + +### Misc + +* If you're implementing a new feature a cmd line switch, there has to be also a global ENV variable which can be used without the switch (see e.g. `SNEAKY`, `ASSUME_HTTP` or `ADDITIONAL_CA_FILES`) +* Test before doing a PR! Best if you check with two bad and two good examples which should then work as expected. Maybe compare results e.g. with SSLlabs. +* Unit tests are done automatically done with Perl using Travis. The trigger is `~/.travis.yml`. The general documentation for [Test::More](https://perldoc.perl.org/Test/More.html) is a good start. You are encouraged to write own checks. You can use e.g. `t/20_baseline_ipv4_http.t` as an example. +* If it's an OpenSSL feature you want to use and it could be not available for older OpenSSL versions testssl.sh needs to find out whether OpenSSL has that feature. Best do this with OpenSSL itself and not by checking the version as some vendors do backports. See the examples for `HAS_SSL2` or proxy option check of OpenSSL in `check_proxy()`. +* If a feature of OpenSSL is not available you need to tell this the user by using `pr_warning*()`. Or accordingly with `fatal()` if a continuation of the program doesn't make sense anymore. + From 7d3ff19442a0523fd2588f39f6eb4ee32d6b3a4a Mon Sep 17 00:00:00 2001 From: Dirk Wetter Date: Fri, 24 Jan 2020 14:24:22 +0100 Subject: [PATCH 3/4] Notes wrt [[, references to bash hackers wiki --- Coding_Convention.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Coding_Convention.md b/Coding_Convention.md index 445cc49..933b30c 100644 --- a/Coding_Convention.md +++ b/Coding_Convention.md @@ -20,16 +20,17 @@ Some people really read that ! New features would need to be documented in the a #### Shell / bash -Bash is actually quite powerful -- not only with respect to sockets. It's not as mighty as perl or python, but there are a lot of neat features. Here's how you make use of them. +Bash is actually quite powerful -- not only with respect to sockets. It's not as mighty as perl or python, but there are a lot of neat features. Here's how you make use of them. Besides those short hints here there's a wealth of information of there. One good resource is the [bash hackers wiki](https://wiki.bash-hackers.org/start). * Don't use backticks anymore, use `$(..)` instead -* Use double square `[[]]` instead of single square `[]` brackets +* Use double square `[[]]` brackets (_conditional expressions)_ instead of single square `[]` brackets +* In double square brackets avoid quoting at the right hand side if not necessary, see [bash hackers wiki](https://wiki.bash-hackers.org/syntax/ccmd/conditional_expression). For regex matching (`=~`) you shouldn't quote at all. * The [BashPitfalls](http://mywiki.wooledge.org/BashPitfalls) is a good read! * Whenever possible try to avoid `tr` `sed` `awk` and use bash internal functions instead, see e.g. [bash shell parameter substitution](http://www.cyberciti.biz/tips/bash-shell-parameter-substitution-2.html). It slower as it forks, fopens and pipes back the result. * `read` often can replace `awk`: `IFS=, read -ra a b c <<< "$line_with_comma"` * Bash can also deal perfectly with regular expressions, see e.g. [here](https://www.networkworld.com/article/2693361/unix-tip-using-bash-s-regular-expressions.html) and [here](https://unix.stackexchange.com/questions/421460/bash-regex-and-https-regex101-com). You can as well have a look @ `is_ipv4addr()` or `is_ipv6addr()`. * If you still need to use any of `tr`, `sed` and `awk`: try to avoid a mix of several external binaries e.g. if you can achieve the same with e.g. `awk`. -* Be careful with very advanced bash features. Mac OS X is still using bash version 3 ([http://tldp.org/LDP/abs/html/bashver4.html](differences)) +* Be careful with very advanced bash features. Mac OS X is still using bash version 3 ([differences](http://tldp.org/LDP/abs/html/bashver4.html), see also [bash hackers wiki](https://wiki.bash-hackers.org/scripting/bashchanges)). * Always use a return value for a function/method. 0 means all is fine. * Make use of [shellcheck](https://github.com/koalaman/shellcheck) if possible From 3cdb16a96924cc6c4dcae2e6976ada06f517884a Mon Sep 17 00:00:00 2001 From: Dirk Wetter Date: Fri, 24 Jan 2020 17:42:17 +0100 Subject: [PATCH 4/4] Prepare baseline_ipv4_http as a good example ... ... as indicated in CONTRIBUTING.md / Coding_Convention.md --- t/20_baseline_ipv4_http.t | 68 ++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 29 deletions(-) diff --git a/t/20_baseline_ipv4_http.t b/t/20_baseline_ipv4_http.t index 3e0bba3..b6412b6 100755 --- a/t/20_baseline_ipv4_http.t +++ b/t/20_baseline_ipv4_http.t @@ -1,54 +1,64 @@ #!/usr/bin/env perl -# Just a functional test, whether there are any problems on the client side -# Probably we could also inspect the JSON for any problems for +# baseline test for testssl, screen and JSON output + +# This is referred by the documentation. + +# We could also inspect the JSON for any problems for # "id" : "scanProblem" # "finding" : "Scan interrupted" use strict; use Test::More; use Data::Dumper; -# use JSON; -# if we need JSON we need to comment this and the lines below in +use JSON; my $tests = 0; my $prg="./testssl.sh"; -my $check2run ="-p -s -P --pfs -S -h -U -q --ip=one --color 0"; -my $uri=""; +my $check2run="-p -s -P --pfs -S -h -U -q --ip=one --color 0"; +my $uri="google.com"; my $socket_out=""; my $openssl_out=""; # Blacklists we use to trigger an error: my $socket_regex_bl='(e|E)rror|\.\/testssl\.sh: line |(f|F)atal'; my $openssl_regex_bl='(e|E)rror|(f|F)atal|\.\/testssl\.sh: line |Oops|s_client connect problem'; +my $json_regex_bl='(id".*:\s"scanProblem"|severity".*:\s"FATAL"|"Scan interrupted")'; -# my $socket_json=""; -# my $openssl_json=""; -# $check2run="--jsonfile tmp.json $check2run"; +my $socket_json=""; +my $openssl_json=""; +$check2run="--jsonfile tmp.json $check2run"; die "Unable to open $prg" unless -f $prg; -$uri="google.com"; - -# unlink "tmp.json"; -printf "\n%s\n", "Baseline unit test IPv4 via sockets --> $uri ..."; -$socket_out = `./testssl.sh $check2run $uri 2>&1`; -# $socket_json = json('tmp.json'); -unlike($socket_out, qr/$socket_regex_bl/, ""); -$tests++; - -# unlink "tmp.json"; -printf "\n%s\n", "Baseline unit test IPv4 via OpenSSL --> $uri ..."; -$openssl_out = `./testssl.sh --ssl-native $check2run $uri 2>&1`; -# $openssl_json = json('tmp.json'); -# With Google only we encounter an error as they return a 0 char with openssl, so we white list this pattern here: -$openssl_out =~ s/testssl.*warning: command substitution: ignored null byte in input\n//g; -unlike($openssl_out, qr/$openssl_regex_bl/, ""); -$tests++; - - -done_testing($tests); +# Provide proper start conditions unlink "tmp.json"; +# Title +printf "\n%s\n", "Baseline unit test IPv4 against \"$uri\""; + +#1 +$socket_out = `$prg $check2run $uri 2>&1`; +$socket_json = json('tmp.json'); +unlink "tmp.json"; +unlike($socket_out, qr/$socket_regex_bl/, "via sockets, terminal output"); +$tests++; +unlike($socket_json, qr/$json_regex_bl/, "via sockets JSON output"); +$tests++; + +#2 +$openssl_out = `$prg --ssl-native $check2run $uri 2>&1`; +$openssl_json = json('tmp.json'); +unlink "tmp.json"; +# With Google only we somtimes encounter an error as they return a 0 char with openssl, so we white list this pattern here: +# It should be fixed in the code though so we comment this out +# $openssl_out =~ s/testssl.*warning: command substitution: ignored null byte in input\n//g; +unlike($openssl_out, qr/$openssl_regex_bl/, "via OpenSSL"); +$tests++; +unlike($openssl_json, qr/$json_regex_bl/, "via OpenSSL JSON output"); +$tests++; + +done_testing($tests); +printf "\n"; sub json($) {