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.

Sign up for GitHub

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

Jump to bottom

Add secret scanning to SCM guide, fixes #488 #489

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Add secret scanning to SCM guide, fixes #488 #489

wants to merge 8 commits into from

Conversation

david-a-wheeler
Copy link
Contributor

No description provided.

@david-a-wheeler
Add secret scanning to SCM guide, fixes #488
748ff78 
Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>
@david-a-wheeler
Fix markdownlint issues
ad69d7c 
Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>
@david-a-wheeler
Copy link
Contributor Author

@SecurityCRob - comments?

I did not try to edit best-practices.yml. I'm not sure what that yml file is doing. Is that the source & the README is generated? I don't see any code to do the generation. If the .yml file is generated from the README, then please run that tool. If they're edited simultaneously, well, yuck :-(. In any case, there should be a clear document somewhere what their relationship is.

ctcpip
ctcpip requested changes May 16, 2024
View reviewed changes
Copy link
Member

@ctcpip ctcpip left a comment

Choose a reason for hiding this comment

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

this is great! looks good, just some minor corrections, and a few consistency suggestions as well

docs/SCM-BestPractices/README.md
@@ -126,6 +126,7 @@ For recommendations only applicable to GitHub or GitLab visit one of the followi
* Repository Should Not Allow Committer Approvals [<img src="https://user-images.githubusercontent.com/287526/230376963-ae9b8a47-4a74-4746-bc83-5b34cc520d40.svg" alt="GitLab" height="20" width="20">](gitlab/project/repository_allows_committer_approvals_policy.md) [GitLab](gitlab/project/repository_allows_committer_approvals_policy.md)
* Webhook Configured Without SSL Verification [<img src="https://user-images.githubusercontent.com/287526/230376963-ae9b8a47-4a74-4746-bc83-5b34cc520d40.svg" alt="GitLab" height="20" width="20">](gitlab/project/project_webhook_doesnt_require_ssl.md) [GitLab](gitlab/project/project_webhook_doesnt_require_ssl.md)
* Project Should Have Fewer Than Three Owners [<img src="https://user-images.githubusercontent.com/287526/230376963-ae9b8a47-4a74-4746-bc83-5b34cc520d40.svg" alt="GitLab" height="20" width="20">](gitlab/project/project_has_too_many_admins.md) [GitLab](gitlab/project/project_has_too_many_admins.md)
* Secret Scanning Should be Enabled [<img src="https://user-images.githubusercontent.com/287526/230375178-2f1f8844-5609-4ef3-b9ac-141c20c43406.svg" alt="GitHub" height="20" width="20">](github/repository/forking_allowed_for_repository.md) [GitHub](github/repository/secret_scanning.md)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Secret Scanning Should be Enabled [<img src="https://user-images.githubusercontent.com/287526/230375178-2f1f8844-5609-4ef3-b9ac-141c20c43406.svg" alt="GitHub" height="20" width="20">](github/repository/forking_allowed_for_repository.md) [GitHub](github/repository/secret_scanning.md)
* Secret Scanning Should be Enabled [<img src="https://user-images.githubusercontent.com/287526/230375178-2f1f8844-5609-4ef3-b9ac-141c20c43406.svg" alt="GitHub" height="20" width="20">](github/repository/secret_scanning.md) [GitHub](github/repository/secret_scanning.md)

docs/SCM-BestPractices/github/repository/secret_scanning.md Outdated Show resolved Hide resolved
docs/SCM-BestPractices/github/repository/secret_scanning.md Outdated Show resolved Hide resolved
docs/SCM-BestPractices/github/repository/secret_scanning.md Outdated Show resolved Hide resolved
docs/SCM-BestPractices/github/repository/secret_scanning.md Outdated Show resolved Hide resolved
docs/SCM-BestPractices/github/repository/secret_scanning.md Outdated Show resolved Hide resolved
docs/SCM-BestPractices/gitlab/project/secret_scanning.md Outdated Show resolved Hide resolved
SecurityCRob and others added 6 commits May 21, 2024 08:48
@SecurityCRob @ctcpip
Update docs/SCM-BestPractices/github/repository/secret_scanning.md
2af81c8 
fixes typo

Co-authored-by: Chris de Almeida <ctcpip@users.noreply.github.com>
Signed-off-by: CRob <69357996+SecurityCRob@users.noreply.github.com>
@SecurityCRob @ctcpip
Update docs/SCM-BestPractices/github/repository/secret_scanning.md
7959141 
better word choice

Co-authored-by: Chris de Almeida <ctcpip@users.noreply.github.com>
Signed-off-by: CRob <69357996+SecurityCRob@users.noreply.github.com>
@SecurityCRob @ctcpip
Update docs/SCM-BestPractices/github/repository/secret_scanning.md
3447e0f 
"quotes"

Co-authored-by: Chris de Almeida <ctcpip@users.noreply.github.com>
Signed-off-by: CRob <69357996+SecurityCRob@users.noreply.github.com>
@SecurityCRob @ctcpip
Update docs/SCM-BestPractices/github/repository/secret_scanning.md
76cc1ad 
"quotes"

Co-authored-by: Chris de Almeida <ctcpip@users.noreply.github.com>
Signed-off-by: CRob <69357996+SecurityCRob@users.noreply.github.com>
@SecurityCRob @ctcpip
Update docs/SCM-BestPractices/github/repository/secret_scanning.md
5d73b59 
"quotes"

Co-authored-by: Chris de Almeida <ctcpip@users.noreply.github.com>
Signed-off-by: CRob <69357996+SecurityCRob@users.noreply.github.com>
@SecurityCRob @ctcpip
Update docs/SCM-BestPractices/gitlab/project/secret_scanning.md
6aa08a6 
better word choice

Co-authored-by: Chris de Almeida <ctcpip@users.noreply.github.com>
Signed-off-by: CRob <69357996+SecurityCRob@users.noreply.github.com>
SecurityCRob
SecurityCRob approved these changes May 21, 2024
View reviewed changes
Copy link
Contributor

@SecurityCRob SecurityCRob left a comment

Choose a reason for hiding this comment

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

I like the addition. If we could get Chris' open comment resolved I think we can move this forward

@SecurityCRob
Copy link
Contributor

@SecurityCRob - comments?

I did not try to edit best-practices.yml. I'm not sure what that yml file is doing. Is that the source & the README is generated? I don't see any code to do the generation. If the .yml file is generated from the README, then please run that tool. If they're edited simultaneously, well, yuck :-(. In any case, there should be a clear document somewhere what their relationship is.

I think Randall put that in place. I'm also unsure how that works. We should ask him to start.

@balteravishay
Copy link
Contributor

@SecurityCRob - comments?
I did not try to edit best-practices.yml. I'm not sure what that yml file is doing. Is that the source & the README is generated? I don't see any code to do the generation. If the .yml file is generated from the README, then please run that tool. If they're edited simultaneously, well, yuck :-(. In any case, there should be a clear document somewhere what their relationship is.

I think Randall put that in place. I'm also unsure how that works. We should ask him to start.

I think that might relate to how the Legitify was used to produce these recommendations. maybe @noamd-legit can comment on this and suggest how to add new ones?

@noamd-legit
Copy link
Contributor

The best-practices.yml is generated automatically and does not require manual edits. In fact, I believe we can remove it entirely from this repository.

To update the document, simply edit the markdown file. If we need to regenerate the base version, I will handle any differences that arise.

@david-a-wheeler
Copy link
Contributor Author

In that case, I suggest removing the .yml file (in a separate pull request). We generally don't want generated files in the source repo.

@noamd-legit - would you mind creating that PR?

@david-a-wheeler david-a-wheeler mentioned this pull request May 24, 2024
Open
@noamd-legit
Copy link
Contributor

@david-a-wheeler
Removed the file here: #508

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ctcpip ctcpip ctcpip requested changes

@SecurityCRob SecurityCRob SecurityCRob approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Enhancement Product: SCM Guide
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@david-a-wheeler @SecurityCRob @balteravishay @noamd-legit @ctcpip