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

Issue #89: Update documentation for detect-secrets #95

Merged
merged 16 commits into from
Jun 10, 2023

Conversation

perryzjc
Copy link
Member

@perryzjc perryzjc commented May 12, 2023

Purpose

  • The purpose of this Pull Request is to improve automated checking for general sensitive information within Git. It provides documentation, architecture graphs, and configurations to solve the needs proposed by the JPL community at the issue ticket discussion.

Proposed Changes

  • [ADD] Starter kit for detect-secrets under Static Application Security Testing section of continuous-testing/starter-kits/README.md

Issues

Testing

  • Sample use cases available in the documentation.
  • Tested on the operating system :
    • macOS Big Sur, Version 11.1, Chip Apple M1
      • Layer 1: Full scan (client-side)
      • Layer 2: Git commit scan (client-side)
      • Ubuntu 20.04.2 ARM64
        • Layer 1: Full scan (client-side)
        • Layer 2: Git commit scan (client-side)
    • Windows 10 & 11
      • Layer 1: Full scan (client-side)
        • Although it is able to generate the baseline file, audit feature is unavailable on both Windows 10 & 11 during my testing. Error message mentions UnableToReadBaselineError. When I use the default Yelp/detect-secrets, the problem still exists. I will be further investigating this issue and open an issue ticket to Yelp/detect-secrets.
      • Layer 2: Git commit scan (client-side)
        • Similar to Layer 1, due to problem of reading baseline file, pre-commit hook is not working on Windows 10 & 11, which involves comparing the current commit with the baseline file.
    • Independent on operating system

Additional Notes

After discussion, I am using the forked detect-secrets for this documentation. Compared to the original detect-secrets from Yelp, my solution have
additional plugins which are being reviewed by the Yelp/detect-secrets community. There are two pull requests for my customized plugins

Other plugins are experimental version for now, and I will be further developing in the next two weeks and have them reviewed.
Once all the plugins are reviewed and merged to the Yelp/detect-secrets, I will update the documentation to use the original Yelp/detect-secrets.

Check out the documentation for more details :)

@perryzjc perryzjc marked this pull request as ready for review May 19, 2023 10:08
Copy link
Collaborator

@riverma riverma left a comment

Choose a reason for hiding this comment

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

Excellent job here @perryzjc - this is a wonderful feature you've proposed and added and it is going to be invaluable for many projects. Your documentation was very clear. There's a few problems I encountered that you may want to help on, and some suggestions. Excited to get this integrated soon.

continuous-testing/starter-kits/README.md Outdated Show resolved Hide resolved
continuous-testing/starter-kits/README.md Show resolved Hide resolved
continuous-testing/starter-kits/README.md Show resolved Hide resolved
continuous-testing/starter-kits/README.md Outdated Show resolved Hide resolved
continuous-testing/starter-kits/README.md Show resolved Hide resolved
continuous-testing/starter-kits/README.md Outdated Show resolved Hide resolved
continuous-testing/starter-kits/README.md Show resolved Hide resolved

```
Starter Kit:
1. Create a workflow file `detect-secrets.yaml` in `.github/workflows` directory from your repository root.
Copy link
Collaborator

Choose a reason for hiding this comment

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

After running this on a fresh repository on GitHub, I received the following in the GH Actions workflow log (see trace here):

Run # scripts to scan repository for new secrets
cp: cannot stat '.secrets.baseline': No such file or directory
Error: Process completed with exit code 1.

Is there a way you can check for and create a blank .secrets.baseline file if being run for the first time on a repo that has never used detect secrets before?

Copy link
Contributor

@jpl-jengelke jpl-jengelke Jun 7, 2023

Choose a reason for hiding this comment

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

try touch .secrets.baseline && <additional_command> which will ensure it's always there.

Copy link
Member Author

Choose a reason for hiding this comment

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

try touch .secrets.baseline && <additional_command> which will ensure it's always there.

Yep, I have a similar idea. But due to the format requirement of the baseline file (detect-secrets version can be different), my new solution is to generate the blank baseline file (but with the correct config) by scanning the result on an empty folder.

      - name: Create a blank .secrets.baseline (but with correct configuration) if it does not exist
        run: |
          if [ ! -f .secrets.baseline ]; then
            # The generated baseline file will only be available temporarily on the GitHub side and will not appear on the user's local files
            # Scanning an empty folder ensures no initial secrets are included
            echo "⚠️ No existing .secrets.baseline file detected. Creating a new blank baseline file."
            mkdir empty-dir
            detect-secrets scan empty-dir > .secrets.baseline
            echo "✅ Blank .secrets.baseline file created successfully."
            rm -r empty-dir
          else
            echo "✅ Existing .secrets.baseline file detected. No new baseline file will be created."
          fi

Copy link
Contributor

Choose a reason for hiding this comment

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

Touch does exactly that. If it's not there, it creates an empty file.

You may want to use >> so if a file exists it doesn't replace its contents with an empty file.

Copy link
Member Author

@perryzjc perryzjc Jun 7, 2023

Choose a reason for hiding this comment

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

You may want to use >> so if a file exists it doesn't replace its contents with an empty file.

I have a clarification.

To run detect-secrets for the new secret successfully,
detect-secrets scan --baseline .secrets.new --exclude-files '.secrets.' --exclude-files '.git'

the baseline file should not be entirely empty but should contain some information, even if no secrets are detected. Here's an example of the required format for the baseline file:

image

And, this format of the content generated can vary based on the version of detect-secrets. So I use this command to help with the compatibility
detect-secrets scan empty-dir > .secrets.baseline

My solution uses if-else approach to determine whether we need to create the "initial" baseline file. And even though the file got created by the GitHub Action server, the change is not committed, so it will not affect the project code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @perryzjc - thanks for getting the GitHub Action going. Working now in my test run! Though please see two suggestions below:

Suggestions:

  1. The sample workflow file mentioned in your Usage section does not work as-is for me. I received an invalid workflow file exception (see here and here). I had to remove the "description" and the "branding" parameters to get it to work. Can you resolve this?
  2. I'm not clear on the "Use latest version" button of your GH action page and how your action will support inline use within the "steps" section of a GitHub workflow file. Seems like the way you have it configured, your GitHub action can only work as a single workflow file, and not within a workflow file that contains multiple steps. Might be good to clarify that in the description of your GitHub action.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note @perryzjc - I've proposed a PR to resolve #1 above: perryzjc/detect-secrets-action#1

Copy link
Collaborator

Choose a reason for hiding this comment

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

After chatting with @perryzjc - let's go forth with reverting to the previous GitHub Action strategy, namely: embedding the Workflow file contents inline into this documentation for users to copy/paster. The GH marketplace strategy is complicated and will require more investigation.

Copy link
Member Author

Choose a reason for hiding this comment

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

@riverma I just made some updates based on the conversation.

continuous-testing/starter-kits/README.md Show resolved Hide resolved
@perryzjc
Copy link
Member Author

perryzjc commented Jun 8, 2023

I also have some updates on my findings about the "bug" discussed in my meeting with @riverma

Background

Below are the cases we discussed during the meeting. Rishi and I both got the same results for each case.

1. Three secret types (one built-in plugin, two my custom plugins)

built-in plugin:

  1. KeywordDetector (sample test file available here)

my custom plugins:

  1. EmailAddressDetector (sample test file available here)
  2. AWSSensitiveInfoDetectorExperimental (sample test file available here)

sample file:

e-mail: [email protected]

username: myuser
password: mypass

sg-12345678

After running command

detect-secrets scan ./ --all-files --exclude-files '.secrets.*' --exclude-files '.git*' > .secrets.baseline

the baseline file only showed two results (supposed to show all three) from my custom plugin:
image

2. Two secret types (one built-in plugin, one my custom plugins)

built-in plugin:

  1. KeywordDetector (sample test file available here)

my custom plugins:

  1. AWSSensitiveInfoDetectorExperimental (sample test file available here)

sample file:

username: myuser
password: mypass

sg-12345678

After running the same command,
the baseline file only showed one result from my custom plugin:
image

3. One secret type (one built-in plugin)

built-in plugin:

  1. KeywordDetector (sample test file available here)

sample file:

username: myuser
password: mypass

After running the same command,
the baseline file only showed one result from the built-in plugin:
image

Thoughts

During the meeting, we thought the "bug" came from my custom plugins based on the above results.
After the meeting and more test cases, I think it's a common issue, which also occurs for built-in plugins only case

More tests

4. Two secret types (two built-in plugins)

built-in plugin:

  1. KeywordDetector (sample test file available here)
  2. PrivateKeyDetector (sample test file available here)

sample file:

username: myuser
password: mypass

'-----BEGIN RSA PRIVATE KEY-----\n'
'super secret private key here\n'
'-----END RSA PRIVATE KEY-----'

After running the same command,
the baseline file only showed one result from the built-in plugin (PrivateKeyDetector):
image

5. Two secret types (one built-in plugin and one custom plugin)

built-in plugin:

  1. PrivateKeyDetector (sample test file available here)

my custom plugins:

  1. AWSSensitiveInfoDetectorExperimental (sample test file available here)

sample file:

sg-12345678

'-----BEGIN RSA PRIVATE KEY-----\n'
'super secret private key here\n'
'-----END RSA PRIVATE KEY-----'

After running the same command,
the baseline file successfully showed both results
image

6. Two secret types (one built-in plugin and one custom plugin)

built-in plugin:

  1. GitHubTokenDetector (sample test file available here)

my custom plugins:

  1. AWSSensitiveInfoDetectorExperimental (sample test file available here)

sample file:

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

sg-12345678

After running the same command,
the baseline file successfully showed both results
image

Assumption and Conclusion

Based on further tests 5 and 6, detect-secrets successfully showed all secret types even if there is a custom plugin.
So far, all the buggy test examples occur when there is a KeywordDetector

It can probably be a bug from the official 'Yelp/detect-secrets` or a feature.
But anyway, this part is what I need to ask 'Yelp/detect-secrets' to improve

Although this "bug" exists at the moment, detect-secret is still able to tell us where there is a Keyword detected. The only inconvenience is that we need to let it be the only secret type existing in a single file. But as we clean the otehr secret types further, we will find it eventually.

continuous-testing/starter-kits/README.md Outdated Show resolved Hide resolved
@riverma
Copy link
Collaborator

riverma commented Jun 9, 2023

I also have some updates on my findings about the "bug" discussed in my meeting with @riverma

Great analysis @perryzjc! This clarifies some unexpected behavior (we think) from detect secrets. Not too worried, since this is only relevant to a single file, and resolving one of the secrets in code should make the other plugins catch other secrets within the same file. However, this discussion is great to point developers at detect secrets to, for clarification whether a bug ticket should be filed.

riverma added a commit to riverma/detect-secrets-action that referenced this pull request Jun 9, 2023
As mentioned in [this comment](NASA-AMMOS/slim#95 (comment)), the two lines cause failures.
Copy link
Collaborator

@riverma riverma left a comment

Choose a reason for hiding this comment

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

Great work @perryzjc - if you can make the GH action fix described in the comments, this is good to merge from my side.

Copy link
Collaborator

@riverma riverma left a comment

Choose a reason for hiding this comment

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

@perryzjc - just re-tested the GitHub Action, and it works great! Just as expected, it was able to find secrets and after removing them it was able to let the commit proceed.

You've done a fantastic job here. Congratulations! LGTM and +1 for merge.

@jpl-jengelke jpl-jengelke dismissed their stale review June 10, 2023 02:06

Timely delivery is meaningful. Some changes addressed concerns. More changes wouldn't be constructive at this moment.

@jpl-jengelke
Copy link
Contributor

+1 Let's merge this PR...

Copy link
Contributor

@jpl-jengelke jpl-jengelke left a comment

Choose a reason for hiding this comment

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

Let's merge this project. It looks great and is a very meaningful contribution to our documentation stack.

Thank you @perryzjc!

@riverma
Copy link
Collaborator

riverma commented Jun 10, 2023

Approved by @riverma and @jpl-jengelke. Merging.

@riverma riverma merged commit 3d0548f into NASA-AMMOS:main Jun 10, 2023
riverma added a commit that referenced this pull request Jun 10, 2023
Recently discovered a rendering issue with respect to #95. Resolving with the use of an in line image.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants