fix: Updated MissingMinVersionTLS query to check go version #29
Conversation
|
|
|
Thanks, gonna review soon. |
|
Nice work! Although it looks like the tests are failing. I've also reproduced the failures locally, so I think there's still some work to be done. I just updated the testing functionality in this repo, which should make it easier to have a consistent environment both locally and CI: #30. So hopefully that helps 👍 |
|
@mschwager yeah so codeql was giving some problems on my local setup so i couldn't properly test things on my end. |
|
Yeah, we've been fighting the tests for a while too. CodeQL makes this difficult. One thing to consider, when upgrading our Go queries to the latest dependency versions ( So perhaps this is related to the issues you're seeing. That is, the latest dependency versions are removing 3 expected test results. That's something we need to look into, but it could be affecting your testing too. |
Apostlex0
force-pushed
the
go-version-aware-tls-minversion
branch
from
33a2d7d to
65fcd82
Compare
i ran the tests with CodeQL command-line toolchain release 2.23.8 and the tests passed without issues, though i had to make some changes to the MissingMinVersionTLS.expected to fit some necessary conditions. |
| | MissingMinVersionTLS.go:135:10:135:49 | struct literal | TLS.Config.MinVersion is never set for variable $@. | MissingMinVersionTLS.go:136:3:136:3 | c | c | | ||
| | MissingMinVersionTLS.go:142:23:142:62 | struct literal | TLS.Config.MinVersion is never set for variable $@. | MissingMinVersionTLS.go:142:3:142:3 | c | c | | ||
| | MissingMinVersionTLS.go:149:23:149:62 | struct literal | TLS.Config.MinVersion is never set for variable $@. | MissingMinVersionTLS.go:149:3:149:3 | c | c | | ||
| | MissingMinVersionTLS.go:159:23:161:5 | struct literal | TLS.Config.MinVersion is never set for variable $@. | MissingMinVersionTLS.go:157:3:157:8 | client | client | |
There was a problem hiding this comment.
It looks like the test results begin to change here. Not sure why the diff isn't displaying that correctly.
I believe some of these shifted by a single line because the // BAD for Go < 1.18 comment was added. That's fine, but it looks like there are also 3 additional findings. I think line 13, 16, and 17 are new. Is that expected?
There was a problem hiding this comment.
yeah, the one-line shifts are from the added comment, and the 3 extra findings are expected-
One new row is for the block labeled // BAD for Go < 1.18: config used for a client. After the fix in this pr we now flag client-side TLSClientConfig when go.mod allows Go < 1.18, because clients defaulted to TLS 1.0 before 1.18 the old query excluded client configs, so it didn’t show up before.
The other two new rows come from the case where the same config literal flows into c and then into both TLSClientConfig: c(client) and TLSConfig:c (server) and since results are emitted per (struct literal-- variable), we get additional findings.
There was a problem hiding this comment.
Okay, I think these could be considered duplicate results in some cases, but I understand why they are written the way they are in the updated query. I think this query could use a number of improvements (e.g. move to path-problem, change the lengthy where clause to class types with restrictive characteristic predicates, update to the latest go-all dependency version which currently breaks the tests, etc.), but I don't want to block this PR on those changes. So I think this is in good enough shape to move forward with the PR.
mschwager
left a comment
mschwager
left a comment
There was a problem hiding this comment.
Nice work! Thank you for improving this query, I know it takes a lot of work!
| | MissingMinVersionTLS.go:135:10:135:49 | struct literal | TLS.Config.MinVersion is never set for variable $@. | MissingMinVersionTLS.go:136:3:136:3 | c | c | | ||
| | MissingMinVersionTLS.go:142:23:142:62 | struct literal | TLS.Config.MinVersion is never set for variable $@. | MissingMinVersionTLS.go:142:3:142:3 | c | c | | ||
| | MissingMinVersionTLS.go:149:23:149:62 | struct literal | TLS.Config.MinVersion is never set for variable $@. | MissingMinVersionTLS.go:149:3:149:3 | c | c | | ||
| | MissingMinVersionTLS.go:159:23:161:5 | struct literal | TLS.Config.MinVersion is never set for variable $@. | MissingMinVersionTLS.go:157:3:157:8 | client | client | |
There was a problem hiding this comment.
Okay, I think these could be considered duplicate results in some cases, but I understand why they are written the way they are in the updated query. I think this query could use a number of improvements (e.g. move to path-problem, change the lengthy where clause to class types with restrictive characteristic predicates, update to the latest go-all dependency version which currently breaks the tests, etc.), but I don't want to block this PR on those changes. So I think this is in good enough shape to move forward with the PR.
No problem ser!👍 |
4 participants
resolves #27
This PR updates the
MissingMinVersionTLSquery to check the project's Go version (fromgo.mod) and only flag TLS configurations when the defaultMinVersionis actually insecure, based on the Go version in use.