Overhaul npm detector #2264

rgmz · 2023-12-28T18:37:15Z

Description:

This fixes #1455. It matches new npm tokens (npm_xxx...), old npm tokens (NpmToken.0000-..., 0000-...), and "non-standard" tokens such as Artifactory using a JWT or GitHub packages using a PAT. Basic auth is a separate can of worms, so is out of scope for this PR.

The high-level flow for all detectors is:

flowchart TD
    A[Got token] -->|Search for associated URL| B{URL found?}
    B -->|Yes| C[Verify against URL]
    B -->|No| D[Search for any potential URLs]
    D --> E[Verify against all URLs]
    
    C-->Z[END]
    E-->Z

Checklist:

Tests passing (make test-community)?
Lint passing (make lint this requires golangci-lint)?

rgmz

This went through several iterations so there may be some leftover cruft or confusing bits.

rgmz · 2023-12-28T18:39:02Z

pkg/detectors/npm/npm_token_v1_test.go

+	}
+}
+
+func BenchmarkFromDataV1(benchmark *testing.B) {


The main downside of consolidating everything under one package is that you can't have BenchmarkFromData multiple times.

pkg/detectors/npm/registry.go

rgmz · 2023-12-28T18:42:03Z

pkg/detectors/npm/registry.go

+	// Skip the first two indices: 1 is the entire string, 2 is the protocol.
+	index, uri := firstNonEmptyMatch(matches, 2)
+	info := &registryInfo{
+		RegistryType: registryType(index - 1),


This is spooky black magic that relies on knownRegistryPat having the same number of capture groups as registryType has values. Unfortunately, it doesn't seem possible to programmatically get enum values so this can't be verified at compile time.

Would it be feasible to create some sort of registry struct that contains a pattern and a name and a const int, and then smoosh them all together into a list in one place? That way there wouldn't have to be these multiple lists maintained across multiple files. (If not, then I think some loud comments in all the places that are magically correlated are warranted.)

I considered doing that but was concerned that looping through n patterns would be much less efficient. In the worst case, you'd need to loop through all the "known" patterns, find all "generic" registry matches, and do a secondary lookup to determine the scheme for each registry.

Hm, I'm either misunderstanding something or being unclear. I'm just proposing merging the three lists of registries you have (const ints, string names, and patterns) into one list of structs, each of which contains three fields. Why would that add any additional loops?

Perhaps I'm misunderstanding. Can you provide an example of what you mean?

type registry struct { // maybe an int too, i don't know if it's necessary name string pattern string } // somewhere, maybe at a top level registries := []registry { { name: "npm", pattern: `(registry\.(?:npmjs\.org|yarnpkg\.com))` }, { name: "artifactoryCloud", pattern: `([a-z0-9-]+\.jfrog\.io/(?:artifactory|[a-z0-9._-]+)/api/npm/[a-z][a-z0-9._-]+)` {, // etc }

(This could also be some kind of map. I see that some of the patterns are pretty long and decomposable, so maybe they get built out of a function somehow.) Building the big monster pattern is a matter of looping through the slice and just joining all the patterns together. This would eliminate the spooky magic that ties together the big pattern and the registry list.

pkg/engine/ahocorasick/ahocorasickcore.go

rgmz · 2023-12-28T18:43:24Z

pkg/detectors/npm/npm_token_generic.go

+	detectors.Versioner
+} = (*ScannerGeneric)(nil)
+
+func (s ScannerGeneric) Version() int { return 0 }


Version 0 might not be the most idiomatic way to approach this.

@mcastorina is there any reason not to use an explicit version 0 when also using versioned detectors?

iirc the way we implemented filtering depends on the version being non-zero, so you wouldn't be able to exclude/ignore just the v0 detector.

I've added a test here to codify that assumption.

rosecodym

This is a ton of work, thanks for diving into it. I added some questions inline.

rosecodym · 2024-01-02T15:35:29Z

pkg/detectors/npm/common.go

+	req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
+	res, err := client.Do(req)
+	if err != nil {
+		// A |tls.RecordHeaderError| likely means that the server is using HTTP, not HTTPS.


Am I paranoid to be concerned by an automatic fallback here? It seems like we should be trying to the scheme in the original URI and failing if https is expected but not present. Am I thinking about this right?

In some circumstances it isn't possible to determine the scheme of the registry URI. For example, //registry.host.com/npm/:_authToken=... does not contain a scheme but we can do a secondary lookup to tell that it should be HTTP.

//registry.host.com/npm/:_authToken=... registry=http://registry.host.com/npm/

If that information (registry=http://...) isn't present in the chunk then the scheme is unknown. HTTPS is a reasonable default but may not necessarily be correct. Trying to connect to a HTTP server with HTTPS seems to reliably result in a tls.RecordHeaderError; conversely, I haven't found a reliable indicator that you're to connect to a HTTPS port with HTTP.

We could limit the automation fallback to cases where the scheme is unknown. Personally, I don't think there's any risk in having it.

rosecodym · 2024-01-02T15:39:02Z

pkg/detectors/npm/common.go

+			_ = res2.Body.Close()
+
+			if res2.StatusCode == http.StatusOK {
+				return false, nil, nil


Tell me if I've got this right:

If the whoami user is anonymous,

And the request succeeds even without the token sent at all,

Then we conclude that the token isn't valid and the endpoint is just bad at communicating that fact

But if the request fails after removing the token, then we conclude that the token legitimately has no user attached

If I got that correctly, then it took me a bit to understand it. (And if I didn't, then I definitely didn't understand it!) I think I would have figured it out faster if this logic were factored out into a named function, but maybe there's another approach that would clarify it as well.

If I got that correctly, then it took me a bit to understand it. (And if I didn't, then I definitely didn't understand it!) I think I would have figured it out faster if this logic were factored out into a named function, but maybe there's another approach that would clarify it as well.

Your understanding is correct, but the verification logic is probably going to get even more convoluted, so it would make sense to extract this into its own function.

Some registries do not implement the whoami endpoint and return a 404. Others, like npm.fontawesome.com, have a no-op whoami endpoint that always returns 200 {"username": null}. That's not to mention that some registries don't implement either /-/whoami or /-/all but do implement /-/v1/search or only support direct package references.

e.g., I have encountered dozens of Azure and Google registry URLs but they all return 404. I don't know if that means the tokens are invalid, the registry doesn't implement the endpoint, or the registry no longer exists.

pkg/detectors/npm/common.go

rosecodym · 2024-01-02T15:49:31Z

pkg/detectors/npm/npm_token_generic.go

+	detectors.Versioner
+} = (*ScannerGeneric)(nil)
+
+func (s ScannerGeneric) Version() int { return 0 }


@mcastorina is there any reason not to use an explicit version 0 when also using versioned detectors?

rosecodym · 2024-01-02T15:50:33Z

pkg/detectors/npm/npm_token_generic.go

+func (s ScannerGeneric) Version() int { return 0 }
+
+// genericKeyPat should match all possible values for .npmrc auth tokens.
+// TODO: Ensure this works with Yarn and UPM configurations.


Could this PR degrade any existing functionality?

I've done some further testing and having dedicated scanners for each pattern type (.npmrc, .yarnrc, .upmconfig.toml, etc.) seems to produce fewer false positives. However, I'm not sure how to best organize them. Are "versions" meant to have any semantic meaning/order?

i.e., is it confusing to have one detector type with numerical versions that are completely unrelated (npm version 1 & 2 are siblings, version 3 is yarn, version 4 is rennovate, version 5 is a hypothetical update to the yarn config, etc.)

rosecodym · 2024-01-02T15:52:58Z

pkg/detectors/npm/npm_token_generic_test.go

+		},
+		"_authToken/scoped/artifactory": {
+			input: `registry=https://artifactory.example.com/artifactory/api/npm/npm/
+//artifactory.example.com/artifactory/api/npm/npm/:_authToken=eyJ2ZXIiOiIyIiwidHlwIjoiSldUIiwiYWxnIjoiUlMyNTYiLCJraWQiOiJSZ25DcnBEVXlKOV9yNElVRnNSU2hqU0E0aGpibEpjZ0M2bnJhN3ZqcTNNIn0.eyJzdWIiOiJqZnJ0QDAxY2pwdDc0N3ZyNHo0MTU4MHNiN3MxYW14XC91c2Vyc1wvYXJ0dXJvLmNhbXBvcyIsInNjcCI6ImFwcGxpZWQtcGVybWlzc2lvbnNcL2dyb3VwczpyZWFkZXJzLGRlcGxveS1kZXYtbnBtLGRlcGxveS1sb2NhbCIsImF1ZCI6ImpmcnRAMDFjanB0NzQ3dnI0ejQxNTgwc2I3czFhbXgiLCJpc3MiOiJqZnJ0QDAxY2pwdDc0N3ZyNHo0MTU4MHNiN3MxYW14XC91c2Vyc1wvYXJ0dXJvLmNhbXBvcyIsImV4cCI6MTY1NjAwNDUxOSwiaWF0IjoxNjU2MDAwOTE5LCJqdGkiOiJjOWZhM2VhNS01MzE0LTQwMzUtYjNiYy03OGNjMDJmYmM1NWMifQ.EIEPLzz9H2oQ3rKLKI_t1qtxi-G9ym6P6J5z7xWLsq79aHK3XV_E8_yuUK8giaOOdl_CaITOjl8Bt2aUVXTZKFKIOiQMLscBM4B6qGj_n4Qq8jiMwcKjnD_iWx8Wo-aNHHxgjvrRdWf-2UPIm6lSc77oZbUNAjhA5Q-W3uQRG7d50FGZpq_EEZsfbOcD7EMU2ZnvfYNTgTtmhZWfLefzB6xUF8WHgiDAVHJKQ2fKLX45Z9trc2SkKQmPBxaS-pBtKBhK15kQZ3x625KtLRr2ZwgOaJKHcg4SwuGOpyTF48nTk53SDorSj6fqlypTavVQRi-5cuSGTPrqLObk6lwpRg`,


Out of curiosity, how did you generate your test file secrets?

rosecodym · 2024-01-02T15:56:28Z

pkg/detectors/npm/npm_token_v2_test.go

+		"invalid/placeholder_x": {
+			input: `//registry.npmjs.org/:_authToken=npm_XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX`,
+		},
+		"invalid/word_boundary": {


❤️ all your tests

rosecodym · 2024-01-02T16:08:59Z

pkg/detectors/npm/registry.go

+	// Skip the first two indices: 1 is the entire string, 2 is the protocol.
+	index, uri := firstNonEmptyMatch(matches, 2)
+	info := &registryInfo{
+		RegistryType: registryType(index - 1),


Would it be feasible to create some sort of registry struct that contains a pattern and a name and a const int, and then smoosh them all together into a list in one place? That way there wouldn't have to be these multiple lists maintained across multiple files. (If not, then I think some loud comments in all the places that are magically correlated are warranted.)

pkg/engine/engine.go

rosecodym · 2024-01-02T16:22:59Z

pkg/detectors/npm/common.go

+		for _, registry := range registries {
+			verified, extraData, err = doVerification(ctx, s.client, registry, token)
+			if verified {
+				return true, extraData, err
+			}
+			if err != nil {
+				errs = append(errs, err)
+			}
+		}
+		return false, nil, errors.Join(errs...)


Tell me if I'm reading this right: Any verification errors returned from any registry will propagate to the final result, even if any (other) individual registry returns a determinate failure (i.e. no error.) This means that if any registry reports indeterminate verification, then either the token will be verified or indeterminately unverified.

If I'm right, this is an issue, because it means that tokens can plausibly be impossible to determinately revoke. (A similar problem exists with the JDBC detector.)

Sort of. The 'happy path' is that a token is associated with a single known registry(1) and we get either a determinate verification or failure. If the token is associated with a single registry but it isn't a 'known' pattern(2), it's possible to encounter an indeterminate failure caused by implementation-specific differences.

# 1 //artifactory.corp.com/artifactory/api/npm/npm/:_authToken=*abcdef123456789* # 2 .npmrc //packages.corp.net/custom_layout/:_authToken=... # 2 CLI flag npm config set registry https://packages.corp.net/custom_layout/

The worst-case scenario is that a token cannot be correlated with a single registry, and there are multiple potential registries¹. In that case, it can be difficult to tell whether something determinately or indeterminately failed. Some implementations are extremely sensitive, with things as small as a trailing slash being the difference between a 200 API response and a generic 400 HTML error page.

So you can either report failures for all attempted URLs, or report none of them and just mark the token as unverified². Realistically, this is a problem when any multi-part secret has multiple potential matches. For example, Azure is a combination of secret (distinct) + clientId (uuid) + tenantId (uuid); if it fails and you have multiple UUIDs is it better to report ALL possible combinations or none of them?

Footnotes

Honestly, the detector that's most likely to trigger this right now is the existing npm v1 one that just searches for npm.+{uuid}. It's prone to false positives — even GitHub Advanced Security flags random UUIDs as npm tokens. ↩

prior versions had more logic to handle invalid registries (e.g., being redirected to https://landing.jfrog.com/reactivate-server/test or a HTML 404 page). That was a bit complicated to refactor so I ended up dropping it. ↩

CLAassistant · 2024-01-04T01:29:32Z

All committers have signed the CLA.

0x1 · 2024-01-05T18:56:06Z

pkg/detectors/npm/common.go

+			if err != nil {
+				return false, nil, fmt.Errorf("request failed for %s: %w", reqUrl, err)
+			}
+			_, _ = io.Copy(io.Discard, res.Body)


should this be res2.Body?

Absolutely. 😅

0x1 · 2024-01-05T19:25:15Z

pkg/detectors/npm/registry.go

+		// language=regexp
+		`(?i)(//%s(?:/[a-z0-9._-]+)*)/:(?:_auth(?:Token)?|_password).{1,20}%s`, hostPat, token))
+	matches := registryAuthPat.FindStringSubmatch(data)
+	if len(matches) == 0 {


shouldn't this be len(matches) < 2 since we're checking matches[1] next?

I don't believe so. If a regex pattern matches, it always returns a fixed number of match groups. If you have mutually exclusive match groups like (?:(foo)|(bar)) then the non-matching group will just be an empty string.

func main() { pat := regexp.MustCompile(`(?i)hello (?:(john)|(anne)|(world)|(.+))`) matches := pat.FindStringSubmatch("Hello Anne") fmt.Printf("Matches return %d groups: %s\n", len(matches), strings.Join(matches, ", ")) } // Matches return 5 groups: Hello Anne, , Anne, ,

https://go.dev/play/p/cdPwcqZwDcT

TL;DR len(matches) should always be 0 or > 2.

I suppose you could make the argument that it prevents failure, but if your pattern changes I think it's better to explicitly fail than silently break.

e.g., stuff like this will silently break if the pattern is updated to add another match group.

trufflehog/pkg/detectors/aha/aha.go

Lines 40 to 42 in 5e78ad4

if len(match) != 2 {

continue

}

rgmz requested a review from a team as a code owner December 28, 2023 18:37

rgmz force-pushed the feat/update-npm branch 2 times, most recently from f583bf3 to 0080bf4 Compare December 28, 2023 18:44

rgmz commented Dec 28, 2023

View reviewed changes

rgmz force-pushed the feat/update-npm branch from 0080bf4 to a67a1c4 Compare December 28, 2023 18:47

feat(npm): overhaul detector

a62d713

rgmz force-pushed the feat/update-npm branch from a67a1c4 to a62d713 Compare December 28, 2023 18:47

rosecodym reviewed Jan 2, 2024

View reviewed changes

chore: remove accidental changes

8e3cc43

rgmz force-pushed the feat/update-npm branch from 346eac9 to 8e3cc43 Compare January 4, 2024 01:29

0x1 reviewed Jan 5, 2024

View reviewed changes

rgmz mentioned this pull request Jan 7, 2024

Add Google oauth2 token detector #2274

Merged

2 tasks

rgmz mentioned this pull request Jan 26, 2024

Update the template detector #2342

Merged

2 tasks

rgmz mentioned this pull request Feb 16, 2024

add version to extra data + moving existing versioned detectors into subdirectory format #2471

Merged

2 tasks

rgmz marked this pull request as draft April 11, 2024 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul npm detector #2264

Overhaul npm detector #2264

rgmz commented Dec 28, 2023 •

edited

Loading

rgmz left a comment

rgmz Dec 28, 2023

rgmz Dec 28, 2023

rosecodym Jan 2, 2024

rgmz Jan 4, 2024

rosecodym Jan 5, 2024

rgmz Jan 5, 2024

rosecodym Jan 8, 2024

rgmz Dec 28, 2023

rosecodym Jan 2, 2024

mcastorina Jan 4, 2024

mcastorina Jan 5, 2024

rosecodym left a comment

rosecodym Jan 2, 2024

rgmz Jan 4, 2024 •

edited

Loading

rosecodym Jan 2, 2024

rgmz Jan 4, 2024 •

edited

Loading

rosecodym Jan 2, 2024

rosecodym Jan 2, 2024

rgmz Jan 3, 2024

rosecodym Jan 2, 2024

rosecodym Jan 2, 2024

rosecodym Jan 2, 2024

rosecodym Jan 2, 2024

rgmz Jan 4, 2024 •

edited

Loading

CLAassistant commented Jan 4, 2024 •

edited

Loading

0x1 Jan 5, 2024

rgmz Jan 6, 2024

0x1 Jan 5, 2024

rgmz Jan 5, 2024 •

edited

Loading

rgmz Jan 5, 2024

Overhaul npm detector #2264

Are you sure you want to change the base?

Overhaul npm detector #2264

Conversation

rgmz commented Dec 28, 2023 • edited Loading

Description:

Checklist:

rgmz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosecodym left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgmz Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgmz Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgmz Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Footnotes

CLAassistant commented Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgmz Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgmz commented Dec 28, 2023 •

edited

Loading

rgmz Jan 4, 2024 •

edited

Loading

rgmz Jan 4, 2024 •

edited

Loading

rgmz Jan 4, 2024 •

edited

Loading

CLAassistant commented Jan 4, 2024 •

edited

Loading

rgmz Jan 5, 2024 •

edited

Loading