Disallow using and delete fallback file on server 401/403

[apazzolini]

If the Doppler server is accessible but returns a 401 or 403 when fetching secrets, we should not use the fallback file. Additionally, if it exists, we want to delete it from the user's FS.

[apazzolini]

Since the commit messages will be included in the changelog, could you update server 401/403 to client error.

Done. Sorry for missing that.

[apazzolini]

@Piccirello have you seen the associated Salus failure before? Looks like an underlying issue with the scanner but we're pinned to a specific version so it's surprising to see it suddenly.

[Piccirello]

@Piccirello have you seen the associated Salus failure before? Looks like an underlying issue with the scanner but we're pinned to a specific version so it's surprising to see it suddenly.

Yes :/ https://github.com/coinbase/salus/issues/828

I can force merge this, though can you first run Salus locally? You can run it via docker or install it via brew. I suspect it may fail due to a lack of error checking on os.Remove.

[apazzolini]

The unhandled os.Remove (which was on purpose since there's nothing to do in that case) errors on the default Salus settings, but not when I run it with our custom configuration. There are two other errors though, but those are from unchanged code. LMK what you want me to do.

docker run --rm -t -v $(pwd):/home/repo --env SALUS_CONFIGURATION=file://salus-config.yaml coinbase/salus

==== Salus Scan v3.2.5 for CLI

==== Gosec v2.15.0: FAILED in 251.09s

 ~~ Scanner Logs:

  {
        "Golang errors": {},
        "Issues": [
                {
                        "severity": "MEDIUM",
                        "confidence": "HIGH",
                        "cwe": {
                                "id": "703",
                                "url": "https://cwe.mitre.org/data/definitions/703.html"
                        },
                        "rule_id": "G307",
                        "details": "Deferring unsafe method \"Close\" on type \"io.ReadCloser\"",
                        "file": "/home/repo/pkg/http/http.go",
                        "code": "266: \tif response != nil {\n267: \t\tdefer response.Body.Close()\n268: \t}\n",
                        "line": "267",
                        "column": "3",
                        "nosec": false,
                        "suppressions": null
                },
                {
                        "severity": "MEDIUM",
                        "confidence": "HIGH",
                        "cwe": {
                                "id": "703",
                                "url": "https://cwe.mitre.org/data/definitions/703.html"
                        },
                        "rule_id": "G307",
                        "details": "Deferring unsafe method \"Close\" on type \"io.ReadCloser\"",
                        "file": "/home/repo/pkg/http/http.go",
                        "code": "228: \t\t\tif resp != nil {\n229: \t\t\t\tdefer resp.Body.Close()\n230: \t\t\t}\n",
                        "line": "229",
                        "column": "5",
                        "nosec": false,
                        "suppressions": null
                }
        ],
        "Stats": {
                "files": 80,
                "lines": 12003,
                "nosec": 20,
                "found": 2
        },
        "GosecVersion": "2.15.0"
  }

==== GoOSV: PASSED in 3.57s

==== GoPackageScanner: PASSED in 0.0s

==== GoVersionScanner: PASSED in 0.32s

==== PatternSearch v0.9.0: PASSED in 0.0s

==== RepoNotEmpty: PASSED in 0.0s

==== ReportGoDep: PASSED in 0.03s

==== Semgrep v1.15.0: PASSED in 54.0s

==== Trufflehog v3.29.1: PASSED in 54.96s

==== Salus Configuration Files Used:

  file:///salus-config.yaml

Overall scan status: FAILED in 251.74s

┌──────────────────┬──────────────┬──────────┬────────┐
│ Scanner          │ Running Time │ Required │ Passed │
├──────────────────┼──────────────┼──────────┼────────┤
│ Gosec            │ 251.09s      │ yes      │ no     │
│ GoOSV            │ 3.57s        │ yes      │ yes    │
│ GoPackageScanner │ 0.0s         │ yes      │ yes    │
│ GoVersionScanner │ 0.32s        │ yes      │ yes    │
│ PatternSearch    │ 0.0s         │ yes      │ yes    │
│ RepoNotEmpty     │ 0.0s         │ yes      │ yes    │
│ ReportGoDep      │ 0.03s        │ yes      │ yes    │
│ Semgrep          │ 54.0s        │ yes      │ yes    │
│ Trufflehog       │ 54.96s       │ yes      │ yes    │
└──────────────────┴──────────────┴──────────┴────────┘

[nmanoogian]

Do the issues go away if you assign the error to underscore?

diff --git a/pkg/cmd/run.go b/pkg/cmd/run.go
index 4cc3f36..42420ff 100644
--- a/pkg/cmd/run.go
+++ b/pkg/cmd/run.go
@@ -414,9 +414,9 @@ func fetchSecrets(localConfig models.ScopedOptions, enableCache bool, fallbackOp
        canUseFallback := statusCode != 401 && statusCode != 403
        if !canUseFallback {
            utils.LogDebug(fmt.Sprintf("Received 401/403. Deleting (if exists) %v", fallbackOpts.path))
-           os.Remove(fallbackOpts.path)
+           _ = os.Remove(fallbackOpts.path)
            utils.LogDebug(fmt.Sprintf("Received 401/403. Deleting (if exists) %v", fallbackOpts.legacyPath))
-           os.Remove(fallbackOpts.legacyPath)
+           _ = os.Remove(fallbackOpts.legacyPath)
        }

        if fallbackOpts.enable && canUseFallback {

[apazzolini]

Do the issues go away if you assign the error to underscore?

The issues are related to defer response.Body.Close() in the http module, not the new code.