Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ci): add trufflehog secrets detection #254

Merged
merged 7 commits into from
Jun 10, 2024

Conversation

McPatate
Copy link
Member

@McPatate McPatate commented Jun 8, 2024

What does this PR do?

Adding a GH action to scan for leaked secrets on each commit.


This change is Reviewable

@Cadene Cadene added the ⚙️ Infra/CI Infra / CI-related label Jun 8, 2024
@Cadene
Copy link
Collaborator

Cadene commented Jun 8, 2024

@McPatate Thanks for this PR! Could you please provide pointer on how it works? :)

@Cadene Cadene requested review from Cadene and aliberts June 8, 2024 16:39
@McPatate
Copy link
Member Author

McPatate commented Jun 8, 2024

IIUC, it will scan the commit that triggered the CI for any token leak. Trufflehog works with a large number of what they call "detectors", each of which will read the text from the commit to see if there is match for a token. For example, the hugging face detector will check for hf tokens and then query our /api/whoami{-v2} endpoint to check if the token is valid. If it detects a valid token, I assume the CI will fail, informing you that you need to rotate the token given it leaked.

More info here:

Comment on lines 6 to 10
permissions:
contents: read
id-token: write
issues: write
pull-requests: write
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this!
I'll add a pre-commit as well (Ideally we want to catch these things before they're even pushed)

Do you know why we need those write permissions though?

Copy link
Member Author

@McPatate McPatate Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a link to the pre-commit hook if you haven't seen it already: https://github.com/marketplace/actions/trufflehog-oss#pre-commit-hook.

I'm checking for the permissions, afaict we could actually remove them. I copied the CI from an internal repo without checking, should've been more thorough here. I think it may be a leftover from another CI pipeline.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.
Yes, I've seen their pre-commit example but I really don't want to use it as - repo: local because it assumes the dev installs it on its end, and I don't want our pre-commits install to be an additional pain point for our contributors.

Ideally I'd like to use something like this:

- repo: https://github.com/trufflesecurity/trufflehog
    rev: v3.78.0
    hooks:
      - id: trufflehog

But it's unclear if this works as intended.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the example:

repos:
  - repo: local
    hooks:
      - id: trufflehog
        name: TruffleHog
        description: Detect secrets in your data.
        entry: bash -c 'trufflehog git file://. --since-commit HEAD --only-verified --fail'
        # For running trufflehog in docker, use the following entry instead:
        # entry: bash -c 'docker run --rm -v "$(pwd):/workdir" -i --rm trufflesecurity/trufflehog:latest git file:///workdir --since-commit HEAD --only-verified --fail'
        language: system
        stages: ["commit", "push"]

They provide the command for running trufflehog in docker, does that not work for you?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That still assumes to have docker installed.
For instance, we develop on the cluster and we don't have access to docker run.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, I think we can merge this in the meantime. Thanks ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand where you're coming from, I wonder if it's possible to run a preinstall step when installing the pre commit hook. You could download the trufflehog binary then.
Otherwise, make install? 😅

@aliberts aliberts enabled auto-merge (squash) June 10, 2024 10:30
Copy link
Collaborator

@aliberts aliberts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @McPatate!

@aliberts aliberts merged commit a065986 into huggingface:main Jun 10, 2024
5 checks passed
@McPatate McPatate deleted the feat/add_trufflehog_ci branch June 10, 2024 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚙️ Infra/CI Infra / CI-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants