HSTS preload list check against API#1809
Conversation
|
Sorry, about the late reply. First: much appreciated, thanks! I read and tried that in December. I can't get to it and need a few days for a consolidate answer! |
| [[ -z "$key" ]] && return 0 | ||
|
|
||
| # Check if key exists in response. If not, the API may have changed or something. | ||
| ! grep -Fiaqw "$key" $tmpfile && debugme echo "HSTS preloadlist key unrecognized: $key" && return 21 |
There was a problem hiding this comment.
This grep is legacy code. Can't this be changed into something like [[ "$tmpfile" =~ \ ${key}\ ]] ? (please doublecheck)
| ! grep -Fiaqw "$key" $tmpfile && debugme echo "HSTS preloadlist key unrecognized: $key" && return 21 | ||
|
|
||
| # Check if there is a match, return 10 if there is, 20 if not | ||
| grep -Fiaqw "\"$key\": $value" $tmpfile && return 10 || return 20 |
| out "$indent"; pr_bold " HSTS preload list " | ||
|
|
||
| # Check if the domain is also the preloadedDomain. If so, it *may* be correct that the server response header does not have 'preloaded' included. | ||
| check_hsts_preloadlist_match "$NODE" "preloadedDomain" "\"$NODE\"" |
There was a problem hiding this comment.
Is there any reason you use here and other places string why you pass the double quotes here? If they are needed at all I would rather handle this in the function and not in the call.
| debugme echo "Temporary lookupvariable: $preloadcombined" | ||
|
|
||
| # Determine and show the outcome | ||
| case "$(check_hsts_preloadlist_value "$NODE" "status")" in |
There was a problem hiding this comment.
for a plain string there should be no need to enclose it in quotes
| case $key in | ||
| "status") values=("\"unknown\"" "\"pending\"" "\"rejected\"" "\"preloaded\"") ;; | ||
| "bulk") values=("true" "false") ;; | ||
| *) return 1 ;; # No supported key provided. |
There was a problem hiding this comment.
Please double check here the usage of quotes. I believe for status and bulk they can be removed
For the array values I am not sure whether they need contain quotes.
| check_hsts_preloadlist_value() { | ||
| local domain="$1" | ||
| local key="$2" | ||
| local values |
There was a problem hiding this comment.
local -a value=() would declare the type better.
|
Good work! You spend a lot of time for the lookup table and the decision table you provided is quite impressive. The information on the screen for both easy cases (HSTS and no preload anywhere, HSTS and preload eerywhere)
I miss use case to test every variant of this. If I sleep over it I probably can test 1-2 more than the basic variants. If you could address the minor points in the code; I'll do a few more tests and then it should be set. Thanks for your work! |
|
@drwetter @tosticated — #1248 and this PR are still labeled @tosticated, thanks for the original implementation and the decision table — full credit to you; if you're not actively working on it, I'm happy to revive it as a fresh PR against My plan: rebase onto current
To resolve what stalled this (verifying every variant of the preload decision table), I'd add a @drwetter — before I finalize: are you happy with the existing decision-table approach ( |
|
@potato-20 : would be great! Allow me some time for feedback |
It looked fine like it was to me, see picture . And: unit tests are always appreciated ;-) |
|
I've opened #3060 which revives this onto current |
|
Cool, thanks a lot! I'll get to it next week. |
Revives and rebases testssl#1809 by @tosticated (Jim Blankendaal) onto 3.3dev. When --phone-out is set, run_hsts now queries https://hstspreload.org/api/v2/status and reports whether the domain is on the browser HSTS preload list (preloaded/pending/rejected/unknown), cross-referenced with the served header, the same-domain check and the bulk flag. Addresses the review comments on testssl#1809: the API-response matching uses native bash string matching instead of forking grep, the JSON quoting is handled inside check_hsts_preloadlist_match() so callers pass plain values, and the value arrays use 'local -a'. The output decision table is kept as-is (per maintainer feedback). Adds t/53_hsts_preload.t. Original design and decision table by @tosticated.
3 participants
First attempt at addressing #1248: adding support for HSTS preload list lookups.
Feedback is welcome. Especially regarding the way the results are displayed. The idea is that the check is done even when the HSTS preload header is missing. This would show unintended and/or undesired situations such as having a list addition rejected because the 'preload' header itself is missing. Based on my understanding of how the list should work, I have currently assigned the following:
Explanation of some columns:
As I was writing this check, it grew from just a simple 'status' to this entire 'lookup table'. If this is too much, it can also be changed back to just viewing the status without any severity and advice.