-
Notifications
You must be signed in to change notification settings - Fork 390
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
Print connection SSL/TLS version #157
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this, I’m uncomfortable with the (unsecure) parenthetical, but I’m fine with adjusting the ConnectdVia default from “” to “plaintext”
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please address the linking error by adding the appropriate comment to disable the static check rule on that line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, staticcheck is still unhappy
main.go:297:3: malformed linter directive; missing the required reason field? (compile)
main.go:297:8: tls.VersionSSL30 has been deprecated since Go 1.13 because it shouldn't be used: SSLv3 is cryptographically broken, and is no longer supported by this package. See golang.org/issue/32716. (SA1019)
Error: Process completed with exit code 1.
main.go
Outdated
if resp.TLS != nil { | ||
switch resp.TLS.Version { | ||
//lint:ignore SA1019 | ||
case tls.VersionSSL30: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try
case tls.VersionSSL30: //lint:ignore SA1019
It might require a comment after the ignore SA1019 but I cannot think of one. Honestly I'm more tempted to adjust the TLSConfig settings to never negotiate this setting so we don't have to check for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Please retest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dpanic if your last change doesn't work, maybe try the syntax shown here:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thank you, waiting for you guys to check CI/CD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed SSLv3.
Added TSLv1.0 as minimum version for connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davecheney probably you missed my commits. Please take look at it now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@atc0005 this is done. Please review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dpanic I'm an observer of the project (actively learning/improving Go skills) and not directly associated with it. I thought I saw a potential opportunity to assist and spoke up. Apologies if I gave an impression otherwise.
Add minimum version TLSv1.0
Changes done. Please review |
Okay, I perfectly understand. Thank you. Waiting for Dave then :-)
…On Wed, Aug 4, 2021 at 1:21 PM Adam Chalkley ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In main.go
<#157 (comment)>:
> @@ -289,6 +289,25 @@ func visit(url *url.URL) {
log.Fatalf("failed to read response: %v", err)
}
+ // Print SSL/TLS version which is used for connection
+ connectedVia := "plaintext"
+ if resp.TLS != nil {
+ switch resp.TLS.Version {
+ //lint:ignore SA1019
+ case tls.VersionSSL30:
@dpanic <https://github.com/dpanic> I'm an observer of the project
(actively learning/improving Go skills) and not directly associated with
it. I thought I saw a potential opportunity to assist and spoke up.
Apologies if I gave an impression otherwise.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#157 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACUTPPYS2I6NHITBDMHGYDT3EPDHANCNFSM5A364BJQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
main.go
Outdated
@@ -272,6 +272,7 @@ func visit(url *url.URL) { | |||
ServerName: host, | |||
InsecureSkipVerify: insecure, | |||
Certificates: readClientCert(clientCertFile), | |||
MinVersion: tls.VersionTLS10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this simpler and set the min TLS version to 1.2 then we only have to worry about 1.2 vs 1.3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any updates? Do I need anything else to do?
Added support for printing SSL/TLS version of connection.
This is important to know, because TLSv1.3 has faster handshake than the others.