zgrab2 icon indicating copy to clipboard operation
zgrab2 copied to clipboard

Upgrade to go 1.23

Open thecsw opened this issue 1 year ago • 2 comments

Following upon #461, updating the go version to latest, the 1.23 release gives us many new improvements in both the compiler land and tooling land to utilize:

  • Profile Guided Optimizations (PGOs) straight with the compiler allows for 2-7% perf improvements. GA in 1.21.
  • Built-in min, max, so we don't have to constantly implement our own, see: utility.go:129, modules/mssql/connection.go:749, modules/oracle/types_test.go:455, lib/ssh/channel.go:134
  • Structured logging in the standard library!
  • Slices and maps helpers in the standard library.
  • ...and many other improvements all around that we would get for free with all the new tooling to improve zgrab2 further.

How to Test

CI/CD involved should handle it. As Go team noted in all release notes since 1.20 (current version in this project) to now,

As always, the release maintains the Go 1 promise of compatibility. We expect almost all Go programs to continue to compile and run as before.

Compiling and running the program yields no issues.

Notes & Caveats

Only one "breaking" compile-time issue found in modules/amqp091/scanner.go:250, where ChannelMax was expecting an int but got an uint16. Casting here is safe as this is a type promotion with no requirements for this variable to be exactly 16 bits.

Issue Tracking

Add a link to the relevant GitHub issue(s) if the pull request resolves it.

thecsw avatar Sep 03 '24 02:09 thecsw

Hey @thecsw, thanks for this!

Off-hand, though, I'm hesitant to want to bump this to Go 1.23 for backwards-compatibility. We have users using older OS's and don't want to preclude them from being able to compile from source. All the compiler-driven enhancements you mention can still be obtained today by using the latest Go compiler on the existing source code. I don't think the other built-ins standard library enhancements can justify decreasing backwards compatibility, IMO.

phillip-stephens avatar Sep 03 '24 13:09 phillip-stephens

Sure thing and totally valid! With this bump, it would require, at least on major systems, to run at least Big Sur for Mac and windows 10 (some new ports for bsd/etc. introduced)—makes sense for compat

We could possibly leave this open/archived for whenever time comes to upgrade to go1.23, at least

thecsw avatar Sep 03 '24 14:09 thecsw

After the discussion on https://github.com/zmap/zdns/pull/495, I think we can go ahead with this!

phillip-stephens avatar Dec 30 '24 21:12 phillip-stephens

@phillip-stephens wonderful—I noticed there are some new test failures since the last good head—I'll review those and make changes to get this through!

thecsw avatar Dec 31 '24 04:12 thecsw

Okay, rebased onto the upstream's master and should be good to go

thecsw avatar Dec 31 '24 04:12 thecsw

The failures seem to be coming from the python integration tests,

One or more schema validations failed: schema failure@ftp/authtls.json, schema failure@http/https.json, schema failure@ipp/cups-tls.json, schema failure@mssql/2017-linux.json, schema failure@mysql/5.7.json, schema failure@mysql/8.0.json, schema failure@postgres/10.1-ssl.json, schema failure@postgres/9.3-ssl.json, schema failure@postgres/9.4-ssl.json, schema failure@postgres/9.5-ssl.json, schema failure@postgres/9.6-ssl.json

Not super familiar with them or how these are checked, may be transient or coming from upstream

thecsw avatar Jan 19 '25 17:01 thecsw

Looks like all the failures are in the exact same place: ERROR:root:data.ftp.result.tls.handshake_log.server_hello.compression_method: class mismatch for compression_method: expected (<class 'future.types.newint.newint'>,), {u'hex': u'0x00', u'name': u'NULL', u'value': 0} has class dict This does look like a zcrypto change, where compression_method was changed from a uint to a struct here: https://github.com/zmap/zcrypto/commit/c1c1128be414acd7fa84e87733380908944e0829 Though that commit is 2 years old!

Seanstoppable avatar Jan 20 '25 16:01 Seanstoppable