dcooper16
commented
3 years ago Hi @polarathene,
I can't really comment on most of this issue. I'm not familiar with bats and it's not clear to me how to bubble up exit codes from the individual tests, given that the individual tests may produce multiple exit codes, but the overall test can only provide one. However, I'd like to comment on the particular scenario above with respect to using the JSON file to detect the error.
I am not a JSON expect, but I can't find any way to append the error message to an existing JSON file that doesn't result in invalid JSON. Whenever I have tried appending new JSON output to an existing JSON file, the JSON checker I have used has flagged as an error the presence of additional data after the contents of the original file.
One solution would be what I tried in #1398, but that PR would simply prevent anything from being written to already existing JSON file. That would prevent the JSON file from being corrupted, but then there would be no indication of the error in the JSON file.
The only alternative I can think of would be to stop scanning entirely. testssl.sh already does this in the case that the command line contains an error. For example, if testssl.txt contained:
--jsonfile-pretty port_587.json --starttls smtp mail_ssl_cipherlists:587
--jsonfile-pretty port_465.json mail_ssl_cipherlists:465 --notanoption
--jsonfile-pretty port_110.json --starttls pop3 mail_ssl_cipherlists:110
--jsonfile-pretty port_995.json mail_ssl_cipherlists:995
--jsonfile-pretty port_143.json --starttls imap mail_ssl_cipherlists:143
--jsonfile-pretty port_993.json mail_ssl_cipherlists:993then testing would stop as soon as the second line was parsed. The child process would for the second line would note that the command line contained an invalid option, print out the --help information, and then signal the parent to stop all testing. The parent process would immediately quit with an exit code indicating that there was an error in one of the child processes.
@drwetter : At the moment, if parse_cmd_line() in a child process find a problem and calls help(), then help() sends a USR1 signal, which causes the parent process to immediately exit. However, in many cases parse_cmd_line() calls fatal() rather than help(), and when that happens, the parent process continues. In addition, as noted above, if json_header(), csv_header(), html_header(), or prepare_logging() encounter a problem they call fatal(), which does not signal the parent process to stop. Do you think we should change this so that any problem caught in a child process by parse_cmd_line(), json_header(), csv_header(), html_header(), or prepare_logging() that results in fatal() being called also results in a signal being sent to the parent for the parent to quit with a $ERR_CHILD exit code?
polarathene
drwetter
Command line / docker command to reproduce
testssl.shcommand:testssl.sh --file /testssl.txt --mode parallelDocker command:
testssl.txt:Errors are not caught like a single test is when using
--file, which was expected, at least with--mode serialthat some indication of a failure could be detected when using testing tools such asbats.JSON output is also incorrectly appended when errors occur, making the data invalid preventing successful query as an alternative failure detection.
More details
The command works well if the json files don't already exist, but unlike a single test (instead of `--file`), the error doesn't seem to get caught by testing tool `bats`'s `run` method (runs command following it via a subshell). The output snippet from bats failure: ```sh -- output differs -- expected (1 lines): "ECDHE-RSA-AES128-SHA ECDHE-RSA-AES256-SHA DHE-RSA-AES128-SHA DHE-RSA-AES256-SHA" actual (3 lines): jq: error (at results/intermediate/port_587.json:632): Cannot iterate over null (null) parse error: Unmatched ']' at line 632, column 39 "ECDHE-RSA-AES128-SHA ECDHE-RSA-AES256-SHA DHE-RSA-AES128-SHA DHE-RSA-AES256-SHA" -- ``` This indicates that `bats` considered the previous `run` with `testssl.sh` successful, despite the error. Presumably because `testssl.sh` handled multiple tests differently by possibly running each test in their own subshells and not bubbling up failure? Errors for each were output to console when testing without `bats` involved, each had the same in output error like when running a single test. I assume the given docker command was considered successful despite at least one (or all) tests returning an error/failure. The next `bats` test following the `testssl.sh` command is a `jq` command that extracts the `cipherorder_TLSv1_1`(by matching id) `finding` value: ```sh local CIPHERLIST_RSA_INTERMEDIATE_TLSv1_1='"ECDHE-RSA-AES128-SHA ECDHE-RSA-AES256-SHA DHE-RSA-AES128-SHA DHE-RSA-AES256-SHA"' run compare_ciphersuites "cipherorder_TLSv1_1" ${RESULTS_FILE} assert_success assert_output ${CIPHERLIST_RSA_INTERMEDIATE_TLSv1_1} # ... function compare_ciphersuites() { local TARGET_CIPHERLIST=$1 local RESULTS_FILE=$2 echo $(docker run --rm --volume "${PWD}:/results" -w "/results" dwdraju/alpine-curl-jq jq '.scanResult[].fs[] | select(.id=="'${TARGET_CIPHERLIST}'") | .finding' ${RESULTS_FILE}) } ``` The failure from that test is related to `testssl.sh` appending invalid JSON to the json output (multiple runs): ```json },{ "id" : "grade_cap_reason_4", "severity" : "INFO", "finding" : "Grade capped to B. TLS 1.0 offered" } ] } ], "scanTime" : 208 } { "id" : "scanProblem", "severity" : "FATAL", "finding" : "non-empty 'port_587.json' exists. Either use '--append' or (re)move it" } ], "scanTime" : "Scan interrupted" } { "id" : "scanProblem", "severity" : "FATAL", "finding" : "non-empty 'port_587.json' exists. Either use '--append' or (re)move it" } ], "scanTime" : "Scan interrupted" } ``` Note how it appends an object `scanProblem` with `],` and `}` closing tags lacking matching opening tags creating invalid JSON. Otherwise I could workaround the lack of error bubbling up for `testssl.sh` by checking for `scanProblem` via `jq`.Expected behavior
When running multiple tests via
--file, there should be a way to allowtestssl.shto bubble up the failure like a single test (without--file) behaves, especially when run with--mode serial. This would be helpful when usingtestssl.shwith test suites.Existing JSON file either should not be modified (toggle via flag?) or not append content that causes malformed JSON data, since that cannot be reliably parsed to detect the failure.
Your system (please complete the following information):
Ubuntu 20.10(Docker Host, Vultr VPS)Linux 5.8.0-38-generic x86_64testssl.sh 3.1dev from https://testssl.sh/dev/(Image ID:fe90513119fb, 5 days ago, digest:sha256:fca6197abd505668a1f5d6557c9652cd83edabe98f97a672c837cb9f9b1ca376)/home/testssl/bin/openssl.Linux.x86_64("OpenSSL 1.0.2-chacha (1.0.2k-dev)" [~183 ciphers] from Jan 18 17:12:17 2019?)Additional context
When I run a single test, assertion correctly fails when the file exists:
drwetter/testssl.sh:3.1dev --jsonfile-pretty port_587.json --starttls smtp mail_ssl_cipherlists:587I am aware of the
--overwriteoption, and the actual test suite correctly removes the files. This was just an error I noticed while writing my tests that I figured I should report. This applies to any error as far as I know, eg an invalid hostname:The docker option
--user "$(id -u):$(id -g)"was required, to match the ownership of the directory(root "0:0") to write to IIRC,testssl.shwas writing as1000:1000and failing to write files due to permission errors. That's more of a docker issue and depends on context rather thantestssl.shthough.