Add "make lint" target and add to "make test" target

[bnevis-i]

Should enable golangci-lint with default linters + gosec.

See edgexfoundry/edgex-go#3565

[AlexCuse]

I have been thinking about this as well, there are a fair number of lint issues in the project but mostly easy stuff. Not sure we have any warnings now but some of the gosimple recommendations are pretty handy. might be able to get to this by the end of the month.

Been wondering what we might get out of using one of the various 'golang.mk' templates that are out there as a base too. I will try and find the one that was my favorite for some work stuff, it has done us well but we were also starting way closer to zero :)

[AlexCuse]

https://github.com/AlexCuse/edgex-watermill/blob/master/golang.mk

This is the one I usually use, it installs all its own dependencies in /bin

[AlexCuse]

Looking at our Makefile it seems simple enough. Could probably crib the dependency installation stuff from here so no need to document the linter install @bnevis-i

One problem with the approach is may need to specify different targets for CI to use but I don't think it turns out that bad usually. I also have not really looked for a better way to solve it yet.

[bnevis-i]

@AlexCuse No need to worry about having the linter installed for CI. Since we did it for edgex-go, its already installed and available in CI for everyone else. The comments (in edgex-go) are for the developers, to warn them if they don't have the linter installed their PRs may fail when they get pushed.

[AlexCuse]

Understood. Do you think auto-installing what's needed would make it easier to contribute in general?

[AlexCuse]

I have a note on my desk for awhile to try and fix the lint issues before next code freeze but haven't been using the security linter - does it bring many extra warnings to the table @bnevis-i

[bnevis-i]

I honestly haven't run it for app-functions-sdk-go yet. There's also no requirement to do this prior to Jakarta release.

[AlexCuse]

Ran it this morning and only got a few new issues from the security linter. Just added the target/config you used in edgex-go:

pkg/secure/mqttfactory.go:89:3: G402: TLS InsecureSkipVerify may be true. (gosec)
                InsecureSkipVerify: factory.skipCertVerify,
                ^
pkg/transforms/encryption.go:24:2: G505: Blocklisted import crypto/sha1: weak cryptographic primitive (gosec)
        "crypto/sha1"
        ^
pkg/transforms/encryption.go:85:10: G401: Use of weak cryptographic primitive (gosec)
        hash := sha1.New()
                ^
pkg/transforms/encryption_test.go:23:2: G505: Blocklisted import crypto/sha1: weak cryptographic primitive (gosec)
        "crypto/sha1"
        ^
pkg/transforms/encryption_test.go:53:10: G401: Use of weak cryptographic primitive (gosec)
        hash := sha1.New()

I need to figure out a few things about the state of my system though it looks like I have gotten go 1.17.2 via an OS update and I'm seeing some fmt/vet issues on untouched files.

[bnevis-i]

In mqttfactory, configureMQTTClientForAuth needs to be setting the minimum TLS version to TLSv1.2 (surprised this was not reported as well).

We should exempt InsecureSkipVerify as a false positive (// nolint:gosec) because we are taking that parameter from configuration.

encryption.go is giving me some pause. First of all, I don't see that it is being referenced anywhere else in the code other than the test. Not being a professional cryptographer, I can't say whether it is broken or not, but the following are concerning to me:

1. Using a simple SHA1 to derive an encryption key from a password (use of broken crypto)
2. Not using an encrypt-then-MAC construction when using AES-CBC.
3. There appears to be serious problems with the IV (it is supplied by the caller (i.e. not guaranteed random nor unique), as a string, that is truncated to the block size).
4. Uses weak key size (128 bits)

I wonder if someone is willing to vouch for these, and if not, perhaps we should consider removing this implementation.

[lenny-goodell]

@bnevis-i , this is the encryption implementation that was brought over from the old export service in the original App SDK impl. Please add this to the tomorrows Security WG meeting.

[AlexCuse]

I have all this stuff done except encryption.go in my main branch. Also fixed the gofmt issues that have cropped up since I updated go.

Once you figure out what to do with encryption.go I can take care of that, add lint to the test target and bring a PR.

[bnevis-i]

@AlexCuse This one? https://github.com/golangci/golangci-lint/blob/master/install.sh Where does it say that it got deprecated?

[AlexCuse]

Its actually the godownloader script that is referenced: goreleaser/godownloader#207

[bnevis-i]

Its actually the godownloader script that is referenced: goreleaser/godownloader#207

It seems to me that godownloader is the code generator for install.sh, and the project sill recommends install.sh as one of the preferred installation methods (https://golangci-lint.run/usage/install/#local-installation).

It doesn't really bother me that much since:
(1) Its up to golangci-lint to decide whether or not they want to continue to support it
(2) The way I implemented it in edgex-go was to just document the script but not actually run it on the user's behalf.

But if we think the script is going away, maybe we want to refer people to https://golangci-lint.run/usage/install instead of directly to the install.sh script.

[AlexCuse]

I am not worried about it either @bnevis-i just noticed it on a colleague's machine who didn't have lint installed. I deleted my comment when I realized it was in the script that golangci-lint references - agree we should continue to follow their recommendation :)