Adds github client and PAT inspection #138

timmyteo · 2023-10-03T14:19:50Z

Implements feature for #100

prabhu · 2023-10-03T15:42:59Z

depscan/lib/github.py

+class GitHub:
+    # The GitHub personal access token required scopes for depscan
+    DEPSCAN_REQUIRED_TOKEN_SCOPES = [
+        RequiredTokenScope("read:packages", "write:packages")


Do we need either of these scopes? We need the token only to access the public vulnerabilities.

According to the depscan documentation, read:packages is the one required scope on tokens. The scope of write:packages is a parent of read, such that if a token has write scope, it will also inherit read scope on packages.

The GitHub API will only return one or the other scope which the token has: read:packages or write:packages. So although we only need read, the write will technically work for depscan to operate as well. It will just be an over-permissioning problem at that point if the token has write.

@timmyteo, could you kindly do some testing? The documentation might be outdated. Could you try using a token with no scope and see if the database is getting filled with advisories? We can definitely not request write:packages so that we operate with the least privilege.

prabhu · 2023-10-03T15:44:51Z

depscan/cli.py

+
+            if not github_client.authenticate():
+                LOG.error("The GitHub personal access token supplied appears to be invalid or expired. Please see: https://github.com/owasp-dep-scan/dep-scan#github-security-advisory")
+            elif github_client.get_token_scopes() is None:


Is depscan not working with fine grained tokens?

I saw this note in GitHub documentation for the packages API endpoint: you must authenticate using a personal access token (classic). If classic tokens are the only ones allowed, then the new fine-grained tokens will not work.

This needs to be tested as well to prevent users from getting stuck with a legacy token.

cerrussell · 2023-10-03T19:14:07Z

@timmyteo I know you're using Windows, so let me look into why the tests are failing for linux and mac.

cerrussell · 2023-10-03T23:30:15Z

Ok, so there are two discrete issues causing the checks to fail.

The built-in GITHUB_TOKEN we're putting in the env for some workflow steps does not work in Ubuntu when we try to authenticate. Since I don't really like the idea of us generating a new token with expanded privileges, We must instead catch the exception.
The type hints for get_token_scopes and get_token_extra_scopes is screwing up runs using python 3.8. Instead of list[str] should just be list.

We ultimately can't actually test the condition exists to output a warning from within our own repo without implementing the same poor security practice we intend to warn against, as @prabhu describes below - unless we want to set up a separate repo with such overly permissive permissions and run a test there.

A number of users of AppThreat pass the default GITHUB_TOKEN with overly generous read/write permissions across their repos and packages. It would be nice to detect this and flag up a warning.

I have therefore made minor changes to the pr to address these two issues, as well as to stop all our workflows from running when the only thing that's changed is another workflow.

@timmyteo Do you think you could set up a dummy repo with poor security to test this out?

cerrussell

Will approve pending confirmation that this works in a repo with an overly permissive GITHUB_TOKEN.

timmyteo · 2023-10-04T03:24:43Z

Thanks @prabhu and @cerrussell for your feedback and help!

@prabhu I looked at AppThreat VDB and it looks like it calls the https://api.github.com/graphql with a query for securityAdvisories. I tried calling the endpoint directly with various tokens, and found that although a token is required (classic or fine-grained both work), the token doesn't require any permissions to be granted. I am updating this PR to reflect that discovery, including:

Update README with guidance on fine-grained and classic token usage
Code is simpler because any permissions granted is overly permissive and a warning will be issued for any permission greater than minimum
Note: The warning only works for classic token types. As detailed here, there is no way presently to see the permissions of a fine-grained token.

@cerrussell Thanks for helping with the failing tests. I am not understanding your ask for creating a repo with the overly permissive token. Can you please elaborate more? For example, how could I do this in my existing fork? Also, is this a temporary setup, or should this be everlasting to continuously demonstrate this works?

prabhu · 2023-10-10T13:05:17Z

@cerrussell @timmyteo What is left in this PR?

cerrussell · 2023-10-11T23:33:21Z

@prabhu I think I had wanted a demo to show this works, which we can't do by scanning from our own repo.

prabhu · 2023-10-12T13:02:01Z

@cerrussell, you can run it using a workflow on AppThreat org, which uses such overly generous GITHUB_TOKEN by default.

Consolidate exceptions Signed-off-by: Caroline Russell <caroline@appthreat.dev> Output exception Signed-off-by: Caroline Russell <caroline@appthreat.dev> Handle GitHubException Signed-off-by: Caroline Russell <caroline@appthreat.dev> Don't fail-fast version_tests2 Signed-off-by: Caroline Russell <caroline@appthreat.dev> Fix type hints, unnecessary test runs Signed-off-by: Caroline Russell <caroline@appthreat.dev>

* Adds 3.12 tests Signed-off-by: Prabhu Subramanian <prabhu@appthreat.com> * Adds 3.12 tests Signed-off-by: Prabhu Subramanian <prabhu@appthreat.com> --------- Signed-off-by: Prabhu Subramanian <prabhu@appthreat.com>

cerrussell · 2023-10-16T22:32:02Z

@prabhu @timmyteo I am quite possibly missing something here, but I'm getting that the token is expired or invalid rather than the expected message on the test workflow.

Maybe the built-in GITHUB_TOKEN is not a classic token?

timmyteo · 2023-10-19T18:55:31Z

@cerrussell Interesting! Both classic and newer fine grained tokens should work, so I don't think that is the problem here. One of these two exceptions are being thrown. Your Github token may be expired, although if it is being used in this workflow, it seems unlikely. Maybe this catch for the two exceptions needs to be separated out for describing different error scenarios, especially if a different issue is occurring here besides expired token.

cerrussell · 2023-10-19T18:58:03Z

@timmyteo The GITHUB_TOKEN is automatically generated by GitHub when the workfow is run, so it must be something else.

timmyteo · 2023-10-24T01:55:11Z

Thanks for these details @cerrussell. It seems the tokens for Actions have a reduced scope by default, which was not suitable for calling the users API. I reworked the code to not call the API for the user details, and instead just call the base API. Testing with classic tokens and fine-grained access tokens show no regression from expected functionality, and I believe this will work now for Action tokens as well.

cerrussell · 2023-10-24T13:02:08Z

@timmyteo Thanks! I just tried it out, but still couldn't get anything in CI. However, I was able to get it locally with a token I generated, so that's good. I guess we just won't be able to do this in CI.

cerrussell

@timmyteo Thanks so much for your contribution!

timmyteo · 2023-10-24T13:59:28Z

Thanks @cerrussell! The excess permission token warning will only work with classic tokens, as those are the only type that return the associated permission scopes when talking to the GitHub API.

cerrussell · 2023-10-24T14:30:07Z

@timmyteo Confused - I thought that might be it, but I mentioned this upthread and and your reply to that was they should both work. Am I missing something?

timmyteo · 2023-10-24T14:45:08Z

@cerrussell Sorry for the confusion! Let me know if this explanation helps. GitHub has these 3 main token types:

Classic
Fine-Grained
Auto-generated for Actions

The first two can be long lived and are generated by the developer and associated to their account. The third is short lived, lesser privileged, and generally only lives as long the action does.

This pull request adds two pieces of functionality with respect to these three token types:

Log an error if the token returns a 401 Unauthorized from the GitHub API (applicable to all 3 token types)
Log a warning if oauth scopes are returned from the GitHub API, signaling that the token has more permission than is necessary for depscan. (Only the classic token type returns oauth scopes from the GitHub API)

cerrussell · 2023-10-24T15:02:01Z

@timmyteo I understand that, I meant your comment in response to where I said "Maybe the built-in GITHUB_TOKEN is not a classic token"

@cerrussell Interesting! Both classic and newer fine grained tokens should work, so I don't think that is the problem here. One of these two exceptions are being thrown. Your Github token may be expired, although if it is being used in this workflow, it seems unlikely. Maybe this catch for the two exceptions needs to be separated out for describing different error scenarios, especially if a different issue is occurring here besides expired token.

timmyteo · 2023-10-24T15:21:49Z

Ah yes, I was misunderstanding how the workflow was configured. My first guess was that a personal access token (classic or fine-grained) was generated and manually set as a workflow environment variable GITHUB_TOKEN. But I learned as part of our conversation thanks to your help that there is the option for the additional token type of auto-generated token, which you are utilizing here. So sorry for the confusion and this is just one of many examples that you learn something new everyday :)

cerrussell · 2023-10-24T15:40:20Z

@timmyteo Gotcha, no problem, just wanted to make sure I was on the same page.

prabhu

Please merge at your convenience. Great PR!

prabhu reviewed Oct 3, 2023

View reviewed changes

prabhu requested a review from cerrussell October 3, 2023 15:45

cerrussell reviewed Oct 3, 2023

View reviewed changes

timmyteo and others added 4 commits October 16, 2023 18:04

Adds github client and PAT inspection

fe3608b

Addressing PR concerns

c11243d

Adds 3.12 tests (owasp-dep-scan#137)

95d8e57

* Adds 3.12 tests Signed-off-by: Prabhu Subramanian <prabhu@appthreat.com> * Adds 3.12 tests Signed-off-by: Prabhu Subramanian <prabhu@appthreat.com> --------- Signed-off-by: Prabhu Subramanian <prabhu@appthreat.com>

cerrussell force-pushed the feature/issue-100 branch from 9c97f7f to 95d8e57 Compare October 16, 2023 22:07

Update to work with GitHub Actions tokens

01b70ce

cerrussell requested review from cerrussell and prabhu October 24, 2023 13:03

cerrussell approved these changes Oct 24, 2023

View reviewed changes

prabhu approved these changes Oct 24, 2023

View reviewed changes

cerrussell merged commit 54dd761 into owasp-dep-scan:master Oct 25, 2023
38 checks passed

timmyteo deleted the feature/issue-100 branch October 25, 2023 15:09

cerrussell linked an issue Nov 14, 2023 that may be closed by this pull request

Warn users about overly generous github permissions #100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds github client and PAT inspection #138

Adds github client and PAT inspection #138

timmyteo commented Oct 3, 2023

prabhu Oct 3, 2023 •

edited

Loading

timmyteo Oct 3, 2023

timmyteo Oct 3, 2023 •

edited

Loading

prabhu Oct 3, 2023

prabhu Oct 3, 2023

timmyteo Oct 3, 2023 •

edited

Loading

prabhu Oct 3, 2023

cerrussell commented Oct 3, 2023

cerrussell commented Oct 3, 2023 •

edited

Loading

cerrussell left a comment

timmyteo commented Oct 4, 2023

prabhu commented Oct 10, 2023

cerrussell commented Oct 11, 2023

prabhu commented Oct 12, 2023

cerrussell commented Oct 16, 2023 •

edited

Loading

timmyteo commented Oct 19, 2023

cerrussell commented Oct 19, 2023

timmyteo commented Oct 24, 2023

cerrussell commented Oct 24, 2023

cerrussell left a comment

timmyteo commented Oct 24, 2023

cerrussell commented Oct 24, 2023

timmyteo commented Oct 24, 2023

cerrussell commented Oct 24, 2023 •

edited

Loading

timmyteo commented Oct 24, 2023

cerrussell commented Oct 24, 2023

prabhu left a comment

Adds github client and PAT inspection #138

Adds github client and PAT inspection #138

Conversation

timmyteo commented Oct 3, 2023

prabhu Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

timmyteo Oct 3, 2023

Choose a reason for hiding this comment

timmyteo Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

prabhu Oct 3, 2023

Choose a reason for hiding this comment

prabhu Oct 3, 2023

Choose a reason for hiding this comment

timmyteo Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

prabhu Oct 3, 2023

Choose a reason for hiding this comment

cerrussell commented Oct 3, 2023

cerrussell commented Oct 3, 2023 • edited Loading

cerrussell left a comment

Choose a reason for hiding this comment

timmyteo commented Oct 4, 2023

prabhu commented Oct 10, 2023

cerrussell commented Oct 11, 2023

prabhu commented Oct 12, 2023

cerrussell commented Oct 16, 2023 • edited Loading

timmyteo commented Oct 19, 2023

cerrussell commented Oct 19, 2023

timmyteo commented Oct 24, 2023

cerrussell commented Oct 24, 2023

cerrussell left a comment

Choose a reason for hiding this comment

timmyteo commented Oct 24, 2023

cerrussell commented Oct 24, 2023

timmyteo commented Oct 24, 2023

cerrussell commented Oct 24, 2023 • edited Loading

timmyteo commented Oct 24, 2023

cerrussell commented Oct 24, 2023

prabhu left a comment

Choose a reason for hiding this comment

prabhu Oct 3, 2023 •

edited

Loading

timmyteo Oct 3, 2023 •

edited

Loading

timmyteo Oct 3, 2023 •

edited

Loading

cerrussell commented Oct 3, 2023 •

edited

Loading

cerrussell commented Oct 16, 2023 •

edited

Loading

cerrussell commented Oct 24, 2023 •

edited

Loading