Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Phrasing of reason for STARTTLS grading improved #2564

Merged
merged 3 commits into from
Sep 7, 2024
Merged

Conversation

drwetter
Copy link
Owner

@drwetter drwetter commented Sep 7, 2024

... added a comment added in the description.

Unfortunately I couldn't get the line wrapping working.

What is your pull request about?

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Typo fix
  • Documentation update
  • Update of other files

If it's a code change please check the boxes which are applicable

  • For the main program: My edits contain no tabs and the indentation is five spaces
  • I've read CONTRIBUTING.md and Coding_Convention.md
  • I have tested this fix against >=2 hosts and I couldn't spot a problem
  • I have tested this new feature against >=2 hosts which show this feature and >=2 host which does not (in order to avoid side effects) . I couldn't spot a problem
  • For the new feature I have made corresponding changes to the documentation and / or to help()
  • If it's a bigger change: I added myself to CREDITS.md (alphabetical order) and the change to CHANGELOG.md

... a a comment added in the desciption.

Unfortunately I couldn't get the line wrapping working.
@drwetter
Copy link
Owner Author

drwetter commented Sep 7, 2024

Hey @magnuslarsen : Why does preformatting the string not work? I tried variations of set_grade_cap "T" "$(out_row_aligned_max_width "STARTTLS encryption is not mandatory for clients. STARTTLS can only be secured clientside" " " $TERM_WIDTH)

cc @dcooper16

@drwetter drwetter changed the title Phrasing of STARTTLS grading improved Phrasing of reason for STARTTLS grading improved Sep 7, 2024
@drwetter drwetter merged commit be3e765 into 3.2 Sep 7, 2024
3 checks passed
@drwetter drwetter deleted the starttls_phrasing branch September 7, 2024 15:08
@magnuslarsen
Copy link
Contributor

magnuslarsen commented Sep 8, 2024 via email

drwetter added a commit that referenced this pull request Sep 8, 2024
... also in the man pages. See also #2564.
@jbiosca78
Copy link

The aclaration is ok, the grade is capped to T, ok.

But why key exchange score is 0? why protocol score is 0? why all the scores are 0?

I think we sould get these scores with starttls. Some of us have secured clients that check for server certificate and want to check these scores.

@drwetter
Copy link
Owner Author

@jbiosca78 : Because the key exchange doesn't matter and the protocol doesn't matter if there's potentially a man in the middle. We can also set it to T maybe.

But think of a server which has SSLv2 / v3 enabled but at the same time a TLS 1.2 / TLS 1.3 with AEAD ciphers and good kx. How should testssl.sh supposed to deal with that if somebody claims: "hey, I have a secured client and that's not supporting SSLv2/3 at all. I want to check the score for TLS 1.2"

@jbiosca78
Copy link

Then, what's the point of the parámeter -t=smtp and do all the checks if the result will be always grade T and 0 score?
If we have -t=smtp, we already know that we are dealing with STARTTLS server, and we want to know all the details and final score so we can improve what we can.
In my case we have multiple servers that need to be startssl for compatibility issues and we sort them by score to periodically improve them.

@drwetter
Copy link
Owner Author

The point of -t=smtp is that you can test STARTTLS SMTP servers in the first place, so you can test whether you did everything possible on your side.

"Everything possible" translates to the whole output, not to the rating. A common thing is to provide a self-signed or otherwise invalid certificate for a mail server. Which in turn makes the internet a less safe place as for reliable mail delivery as sending servers need either to accept ALL not trusted certificates or configure a manual exception for each server which doesn't scale well or is practically almost impossible.

I've seen this server side misconfiguration 1 or 2 years ago even for government and government-like organizations. Picture the consequences...

@magnuslarsen
Copy link
Contributor

Hey @magnuslarsen : Why does preformatting the string not work? I tried variations of set_grade_cap "T" "$(out_row_aligned_max_width "STARTTLS encryption is not mandatory for clients. STARTTLS can only be secured clientside" " " $TERM_WIDTH)

What issue do you see? I had to use single quotes in the inner body, then it parsed correctly (seemingly anyway):

 Rating (experimental)

 Rating specs (not complete)  SSL Labs's 'SSL Server Rating Guide' (version 2009q from 2020-01-30)
 Specification documentation  https://github.com/ssllabs/research/wiki/SSL-Server-Rating-Guide
 Protocol Support (weighted)  0 (0)
 Key Exchange     (weighted)  0 (0)
 Cipher Strength  (weighted)  0 (0)
 Final Score                  0
 Overall Grade                T
 Grade cap reasons            Grade capped to T. STARTTLS encryption is not mandatory for clients. STARTTLS can only be secured clientside
                              Grade capped to T. Issues with the chain of trust (chain incomplete)
                              Grade capped to A. HSTS is not offered

 Done 2024-09-17 08:25:44 [  75s] -->> 104.154.89.105:443 (untrusted-root.badssl.com) <<--



mzla@LDKCPH05372 ~/zgit/testssl.sh (3.2) ❯ git diff
diff --git a/testssl.sh b/testssl.sh
index 4068440..fd991a3 100755
--- a/testssl.sh
+++ b/testssl.sh
@@ -7691,6 +7691,7 @@ determine_trust() {
                fi
                fileout "${jsonID}${json_postfix}" "CRITICAL" "failed $code. $addtl_warning"
                set_grade_cap "T" "Issues with the chain of trust $code"
+               set_grade_cap "T" "$(out_row_aligned_max_width 'STARTTLS encryption is not mandatory for clients. STARTTLS can only be secured clientside' '                            ' $TERM_WIDTH)"
           else
                # alt least one ok and other(s) not ==> display the culprit store(s)
                if "$some_ok"; then

mzla@LDKCPH05372 ~/zgit/testssl.sh (3.2) ❯                                         

@jbiosca78
Copy link

Ok. And what if we add a option for include the server certificate (CA issuer, certificate hash or full certificate) and if we can validate it we remove the grade cap?

@drwetter
Copy link
Owner Author

@jbiosca78 : did you read what I wrote?

@drwetter
Copy link
Owner Author

@magnuslarsen

What issue do you see? I had to use single quotes in the inner body, then it parsed correctly (seemingly anyway):

Thanks. Don't know however whether just your example didn't quite fit or my point was not coming across as I wanted. The line starting with Grade capped to T. STARTTLS encryption didn't wrap in your example.

Repository owner locked as resolved and limited conversation to collaborators Sep 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants