Hey @WDoranWMF - we can likely schedule the review for this quarter upon the qualification that a production-ready codebase has been completed by mid-quarter. I had a couple of quick questions though:

A Go service that supports persistence of Image Suggestions data

I assume the home for this service is intended to be Wikimedia production and not wmcs or somewhere else?

Currently, https://gitlab.wikimedia.org/repos/generated-data-platform/datasets/image-suggestions but will move to Gerrit

Back to Gerrit? Is this mainly to leverage pipelinelib for CI/CD since that's still an unanswered question for Gitlab?

Hello @WDoranWMF and @Eevans, I will be in charge of this security review.

For now, I would like to know if you have any estimate as to when you might have a stable branch as close to production-deployable as possible (small changes and features might still be worked upon up until a production deployment) to be reviewed.

Thanks

@mmartorana https://gerrit.wikimedia.org/g/generated-data-platform/datasets/image-suggestions is -to the best of my knowledge- feature complete.

Hey @Eevans, I just wanted to have confirmation that the main branch is stable and I can work with the most recent commit, and that there shouldn't be any more substantial contributions before deployment.

If this is the case, I can proceed with my review.

Thanks

In T304885#7868962, @mmartorana wrote:

Hey @Eevans, I just wanted to have confirmation that the main branch is stable and I can work with the most recent commit, and that there shouldn't be any more substantial contributions before deployment.

If this is the case, I can proceed with my review.

Thanks

Yes; The only change pending will be to serve a static (yaml) file -an openapi specification- from the root url (/openapi or similar). Nothing else is planned at this time.

Perfect, thank you.

Hi @Eevans, we have also found these two extensions:

https://gerrit.wikimedia.org/g/mediawiki/services/image-suggestion-api/+/refs/heads/master

https://gerrit.wikimedia.org/g/mediawiki/extensions/ImageSuggestions/+/refs/heads/master

Are these two out of the scope of this review?

In T304885#7885163, @mmartorana wrote:

Hi @Eevans, we have also found these two extensions:

https://gerrit.wikimedia.org/g/mediawiki/services/image-suggestion-api/+/refs/heads/master

https://gerrit.wikimedia.org/g/mediawiki/extensions/ImageSuggestions/+/refs/heads/master

Are these two out of the scope of this review?

The former is a proof-of-concept (this service will replace it), the latter will (at most) consume from this service; They're out of scope, yes.

Ok, thank you.

Security Review Summary - T304885 - 2022-06-28
Last commit reviewed: I38128ed804e5e9d1fd05f16aae303ac479b684cb

Summary

Overall, the extension looks pretty good. There are a limited amount of findings, and they are easy to fix. A decent number of outdated dependencies have been found, with an overall risk rating of: low.

Vulnerable Packages

Outdated Packages
As reported via go-mod-outdated:
(no explicit vulnerabilities reported, simply noting for completeness' sake.)

Static Analysis Findings
snyk returned no results. low risk
gosec generated results. medium

semgrep which was run with the following rule sets:

1. p/ci: This policy returned no results. low risk
2. p/insecure-transport: This policy returned no results. low risk
3. p/jwt: This policy returned no results. low risk
4. p/secrets: This policy returned no results. low risk
5. p/r2c: This policy returned no results. low risk
6. p/supply-chain: This policy returned no results. low risk

The following semgrep policies all generated results, which I've included as supplemental files at the end of this review.

1. p/golang-correctness
2. p/golang
3. p/owasp
4. p/security-audit
5. p/xss

The general summary of the above tools and policy results are:

1. ioutil.ReadFile is deprecated. (CWE-22: Potential file inclusion via variable) medium
2. Detected directly writing in http.ResponseWriter.write(). This bypasses HTML escaping that prevents XSS vulnerabilities. Instead, use the 'html/template' package and render data using template.Execute(). medium
3. Use net.JoinHostPort instead of fmt.Sprintf(config.Address, config.Port). low risk
4. Found an HTTP server without TLS. Is it possible to use http.ListenAndServeTLS instead? low risk
5. Errors unhandled in main.go:152. low risk
6. Errors unhandled in openapi.go:75. low risk
7. Errors unhandled in healthz.go:60. low risk
8. Errors unhandled in healthz.go:57. low risk

Supplemental Materials
Please see attached:

1. {F35284622}
2. {F35284630}
3. {F35284629}
4. {F35284628}
5. {F35284627}
6. gosec.txt1 KBDownload
7. nancy.txt76 KBDownload

Hi @WDoranWMF and @Eevans - even though we rated the reviews as low risk, do you have any actual plans to address the security issues and vulnerable dependencies?

In T304885#8054986, @mmartorana wrote:

Hi @WDoranWMF and @Eevans - even though we rated the reviews as low risk, do you have any actual plans to address the security issues and vulnerable dependencies?

Hi @mmartorana,

First off: Many thanks for the review, it is appreciated!

With a couple of exceptions, the list of outdated modules are all transitive dependencies of third-party libraries. Fixing them will require us to engage with those third-party maintainers to update their code, and release new versions.

Most of the semgrep feedback seems reasonable and straightforward. One exception is (2), which comes from cassandra-http-gateway where it returns JSON serialized results from the database verbatim. It's done this way to keep the data model opaque to the framework, and if it were to ever return anything other than well formed JSON query results, it'd be a bug in Cassandra and not a code error here.

Regarding (4), TLS could be added but (thus far) the service is only accessible to internal clients, access is unauthenticated, and it only exposes otherwise public data. Consensus seemed to be that TLS didn't provide any benefits here, and would only make it more difficult to access. A valid counter-argument of course would be that if any of the above were to change in the future (even inadvertently) then we could create unintended exposure.

So: I propose to follow this up with a) an upgrade of the direct dependencies, and b) to address the semgrep output. The rest I will defer to @WDoranWMF to schedule/prioritize as necessary (both the coordination with upstream maintainers, and/or revisiting whether to run TLS).

/cc @mark

Change 811783 had a related patch set uploaded (by Eevans; author: Eevans):

[generated-data-platform/datasets/image-suggestions@main] Security review

https://gerrit.wikimedia.org/r/811783

Change 811783 merged by jenkins-bot:

[generated-data-platform/datasets/image-suggestions@main] Security review

https://gerrit.wikimedia.org/r/811783

All that remains is for this be deployed, @hnowlan is this something you can help with (there is no hurry)? I tagged this 1.0.0 so you can deploy as docker-registry.wikimedia.org/wikimedia/generated-data-platform-datasets-image-suggestions:1.0.0.

Change 813242 had a related patch set uploaded (by Hnowlan; author: Hnowlan):

[operations/deployment-charts@master] image-suggestion: Bump to latest version

https://gerrit.wikimedia.org/r/813242

Change 813242 merged by jenkins-bot:

[operations/deployment-charts@master] image-suggestion: Bump to latest version

https://gerrit.wikimedia.org/r/813242

Version 1.0.0 has been deployed across all clusters.

Since there are no longer vulnerable packages, outdated modules are significantly reduced, and the main SAST issues are fixed; I am considering this review completed.

Feel free to comment if you have further questions.