Web hook secret used

[laurentsimon]

See if we can add a check to verify that web hooks defined in the repo have a secret configured for authentication https://docs.github.com/en/developers/webhooks-and-events/webhooks/creating-webhooks#secret

Unfortunately, I don't think the APIs let us query them without a PAT https://docs.github.com/en/rest/reference/webhooks#repository-webhooks :/

Note, however, that the PAT only needs to be read-only.

cc @azeemsgoogle @jeffmendoza

[laurentsimon]

cc @justaugustus this is another one probably better for allstar

[cpanato]

this is something that is open for contribution? if so can I take it?

[cpanato]

@laurentsimon ^

i went ahead and start some implementation/testing

[laurentsimon]

thanks. We're not entirely sure this is do-able using the default workflow's GITHUB_TOKEN? Could you verify that?
There's a good chance we may be moving towards the default token and away from the PAT in the future.

I suppose there are 2 ways we could use your code anyway:

1. Have additional features if a PAT is provided
2. Use your code in allstar instead

@azeemsgoogle @naveensrinivasan @jeffmendoza feel free to chime in

[cpanato]

I tested with a PAT but i will confirm wich permission is needed.
I will test and comment out tomorrow my morning, I'm phasing out today :)

[cpanato]

made a test: generate a new token with the public_repo permission only and run the check, and got the same results as using a full permission token.

[justaugustus]

made a test: generate a new token with the public_repo permission only and run the check, and got the same results as using a full permission token.

@cpanato -- Curious...
Did you test this on a repo you have elevated access to? I'd imagine that that might work on a repo you have access to dump endpoints on. If that's the case, could you also try it against a repo you don't have write++ access to?

[laurentsimon]

Another thing to test: does it work with the default secrets.GITHUB_TOKEN in a workflow?

[laurentsimon]

#1675 looks great!

Please let us know whether the webhook can be queried via secrets.GITHUB_TOKEN or not. If we ask users to use the default token in the future (safer), would it work?

What do folks think about having certain checks not available with certain tokens types?

[cpanato]

made a test: generate a new token with the public_repo permission only and run the check, and got the same results as using a full permission token.

@cpanato -- Curious... Did you test this on a repo you have elevated access to? I'd imagine that that might work on a repo you have access to dump endpoints on. If that's the case, could you also try it against a repo you don't have write++ access to?

yes, and does not work, and I think this is a correct approach, otherwise, I can inspect webhook endpoints and use that for malicious things.

for example, in a repo that I don't have access

$ ./scorecard --repo github.com/falcosecurity/falco --checks Webhooks
Starting [Webhooks]
Finished [Webhooks]

RESULTS
-------
Aggregate score: ?

Check scores:
|-------|----------|---------------------------------------------------------|---------------------------------------------------------------------------------------------------------|
| SCORE |   NAME   |                         REASON                          |                                        DOCUMENTATION/REMEDIATION                                        |
|-------|----------|---------------------------------------------------------|---------------------------------------------------------------------------------------------------------|
| ?     | Webhooks | internal error: Client.Repositories.ListWebhooks:       | https://github.com/ossf/scorecard/blob/bfd93e77b00dca4b60917653498ebbd547cfbc45/docs/checks.md#webhooks |
|       |          | error during webhookHandler.setup:                      |                                                                                                         |
|       |          | error during ListHooks: GET                             |                                                                                                         |
|       |          | https://api.github.com/repos/falcosecurity/falco/hooks: |                                                                                                         |
|       |          | 404 Not Found []                                        |                                                                                                         |
|-------|----------|---------------------------------------------------------|---------------------------------------------------------------------------------------------------------|

[laurentsimon]

made a test: generate a new token with the public_repo permission only and run the check, and got the same results as using a full permission token.

@cpanato -- Curious... Did you test this on a repo you have elevated access to? I'd imagine that that might work on a repo you have access to dump endpoints on. If that's the case, could you also try it against a repo you don't have write++ access to?

yes, and does not work, and I think this is a correct approach, otherwise, I can inspect webhook endpoints and use that for malicious things.

What sort of malicious things? :-)

[cpanato]

@laurentsimon i can fetch the endpoint and then use that to do some DDoS attack or other things

[azeemshaikh38]

yes, and does not work, and I think this is a correct approach, otherwise, I can inspect webhook endpoints and use that for malicious things.

Hmm, I think this is a pretty strong reason to consider adding this check as part of AllStar instead of Scorecard. Scorecard hopes to make the security posture of a repo easily visible and understandable so that OSS consumers can know the risks they are undertaking. deps.dev is a great example of how someone might consume Scorecard data. If its a check that cannot/should not be exposed, it should not belong in Scorecard (my opinion).

AllStar on the other hand is meant to be like your personal security bot that either automatically remediates the security issue for you or can privately raise security concerns with the maintainers so that they can tackle it. A similar usecase to this that AllStar tackles is 2FA - maintainers need to be notified that 2FA is not enabled for all maintainers, but at the same time this information should not be made public.

I know you have put in a lot of time and effort behind #1675 @cpanato, so I apologize that we did not discuss this thoroughly beforehand. @jeffmendoza @laurentsimon @justaugustus @naveensrinivasan thoughts?

[cpanato]

@azeemshaikh38 no worries!
but if we continue on this we can add other checks like validating if the endpoint is using HTTPS and if the Enable SSL verification is also set.

[laurentsimon]

I think we need to decide whether we'd be OK having checks that are only available via PAT. @azeemshaikh38 makes a good point that we may not want to make the data public to reduce possibility of abuse (which @cpanato has already mentioned).

This means we'd have to be careful in the scorecard action, to not expose the results of these checks publicly, in particular when sending the results of these checks to our servers for badge generation. cc @rohankh532

I can see a use case for PAT-only checks for advanced users who may want to scan their own repo/orgs via the command line (or during a pen test exercise). This could be a good way for users to test the tool and see the value of it, and a way to show value and convince their orgs to adopt scorecard/allstar. Also, advanced users often find bugs, which is beneficial for us.

Note that the Branch Protection check already has some dependency on the type of token: we cannot retrieve all settings unless the token has admin privileges.

[laurentsimon]

Please chime in if you have an opinion on this, so that we unblock @cpanato

cc @oliverchang

[laurentsimon]

friendly ping if you have an opinion.

[jeffmendoza]

I can see a use case for PAT-only checks for advanced users who may want to scan their own repo/orgs via the command line (or during a pen test exercise). This could be a good way for users to test the tool and see the value of it, and a way to show value and convince their orgs to adopt scorecard/allstar. Also, advanced users often find bugs, which is beneficial for us.

Agree with this, I think it makes sense.

[laurentsimon]

we've discussed this in the by-weekly call. There was consensus there is a use case for this, and

1. we need to be careful around UX (failing early if we can when the token does not have the right permissions)
2. confidentiality of data (GitHub action should not have it turned on by default unless users want, since we're going to stream the results to our backend for the badges)
3. documentation: explain permissions needed for each check (of buckets of checks) cc @olivekl

@cpanato please feel free to finish the PR and get it merged. Just gate access to the check via an environment variable like WEBHOOK_ENABLE=1 (which was already mentioned in the PR), and we'll work on rolling out safely after that.

Thanks all for the comments, suggestions and feedback.

[naveensrinivasan]

we've discussed this in the by-weekly call. There was consensus there is a use case for this, and

1. we need to be careful around UX (failing early if we can when the token does not have the right permissions)
2. confidentiality of data (GitHub action should not have it turned on by default unless users want, since we're going to stream the results to our backend for the badges)
3. documentation: explain permissions needed for each check (of buckets of checks) cc @olivekl

@cpanato please feel free to finish the PR and get it merged. Just gate access to the check via an environment variable like WEBHOOK_ENABLE=1 (which was already mentioned in the PR), and we'll work on rolling out safely after that.

Thanks all for the comments, suggestions and feedback.

I have a suggestion instead of WEBHOOK_ENABLE=1 can we rather have something like SCORECARD_EXPERIMENTAL=1 similar to COSIGN_EXPERIMENTAL=1? This will make it easier to test newer features in the future.

[justaugustus]

I have a suggestion instead of WEBHOOK_ENABLE=1 can we rather have something like SCORECARD_EXPERIMENTAL=1 similar to COSIGN_EXPERIMENTAL=1? This will make it easier to test newer features in the future.

+1 to this, as I mentioned in today's meeting.
Having the env vars namespaced with SCORECARD_ also means can filter for them and they're less likely to collide with other programs.

[laurentsimon]

+1 to have the env start with SCORECARD_.
Let's name them SCORECARD_XX or SCORECARD_EXPERIMENTAL_XX with XX being the feature: this gives us more flexibility to turn on/off a specific subset of features. Wdut?

[naveensrinivasan]

+1 to have the env start with SCORECARD_.
Let's name them SCORECARD_XX or SCORECARD_EXPERIMENTAL_XX with XX being the feature: this gives us more flexibility to turn on/off a specific subset of features. Wdut?

👍

[cpanato]

@laurentsimon i will finish this over the weekend, thanks for all the reviews and feedback

[laurentsimon]

SG, just ping me on the PR when you need a review

[cpanato]

@laurentsimon just to give some status, i'm a bit busy with company work, but I'm planning to finish asap, my deadline is early next week

[evverx]

I ran the "webhook" check the other day and among other things it complained about inactive webhooks. I'm not sure they should be included there. Other than that the documentation seems weird to me:

If there is no support for token authentication, consider implementing it by following these directions.

While I agree ideally it should be implemented I'm pretty sure most webhooks are provided by third-party services that are aware of those issues but in some cases aren't willing to fix it. I think unless scorecard (or whoever uses scorecard) is willing to spend some time trying to convince those third-party services that their one-way webhooks should be fixed that paragraph isn't exactly helpful.

[evverx]

I'd add that this particular check can easily be misled by putting secret keys that aren't actually used anywhere.

[laurentsimon]

I ran the "webhook" check the other day and among other things it complained about inactive webhooks. I'm not sure they should be included there. Other than that the documentation seems weird to me:

Interesting. Can you give an example of a repo we can try out to reproduce your results? (Feel free to create an issue)

If there is no support for token authentication, consider implementing it by following these directions.

Will file an issue for this, thanks

While I agree ideally it should be implemented I'm pretty sure most webhooks are provided by third-party services that are aware of those issues but in some cases aren't willing to fix it. I think unless scorecard (or whoever uses scorecard) is willing to spend some time trying to convince those third-party services that their one-way webhooks should be fixed that paragraph isn't exactly helpful.

Let's address this by fixing the documentation.

[evverx]

@laurentsimon having played with this check a bit more I don't think it makes sense to keep it because regardless of what scorecard says all webhooks have to be audited manually to make sure that secrets are actually used on the receiving end. I think it's helpful in terms of raising awareness so to speak though