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

Show secrets detected at PR scan #793

Merged
merged 4 commits into from
Dec 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@


---
## 🗝️ Secret Detected

---
| Severity | Finding |
| :---------------------: | :-----------------------------------: |
| Medium | Secret keys were found |
attiasas marked this conversation as resolved.
Show resolved Hide resolved

---
### Full description

---
Storing hardcoded secrets in your source code or binary artifact could lead to several risks.

If the secret is associated with a wide scope of privileges, attackers could extract it from the source code or binary artifact and use it maliciously to attack many targets. For example, if the hardcoded password gives high-privilege access to an AWS account, the attackers may be able to query/modify company-wide sensitive data without per-user authentication.

## Best practices

Use safe storage when storing high-privilege secrets such as passwords and tokens, for example -

* ### Environment Variables

Environment variables are set outside of the application code, and can be dynamically passed to the application only when needed, for example -
`SECRET_VAR=MySecret ./my_application`
This way, `MySecret` does not have to be hardcoded into `my_application`.

Note that if your entire binary artifact is published (ex. a Docker container published to Docker Hub), the value for the environment variable must not be stored in the artifact itself (ex. inside the `Dockerfile` or one of the container's files) but rather must be passed dynamically, for example in the `docker run` call as an argument.

* ### Secret management services

External vendors offer cloud-based secret management services, that provide proper access control to each secret. The given access to each secret can be dynamically modified or even revoked. Some examples include -

* [Hashicorp Vault](https://www.vaultproject.io)
* [AWS KMS](https://aws.amazon.com/kms) (Key Management Service)
* [Google Cloud KMS](https://cloud.google.com/security-key-management)

## Least-privilege principle

Storing a secret in a hardcoded manner can be made safer, by making sure the secret grants the least amount of privilege as needed by the application.
For example - if the application needs to read a specific table from a specific database, and the secret grants access to perform this operation **only** (meaning - no access to other tables, no write access at all) then the damage from any secret leaks is mitigated.
That being said, it is still not recommended to store secrets in a hardcoded manner, since this type of storage does not offer any way to revoke or moderate the usage of the secret.
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@

## 🗝️ Secret Detected
<div align='center'>

| Severity | Finding |
| :---------------------: | :-----------------------------------: |
| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableMediumSeverity.png)<br> Medium | Secret keys were found |

</div>

<details>
<summary> <b>Full description</b> </summary>
<br>

Storing hardcoded secrets in your source code or binary artifact could lead to several risks.

If the secret is associated with a wide scope of privileges, attackers could extract it from the source code or binary artifact and use it maliciously to attack many targets. For example, if the hardcoded password gives high-privilege access to an AWS account, the attackers may be able to query/modify company-wide sensitive data without per-user authentication.

## Best practices

Use safe storage when storing high-privilege secrets such as passwords and tokens, for example -

* ### Environment Variables

Environment variables are set outside of the application code, and can be dynamically passed to the application only when needed, for example -
`SECRET_VAR=MySecret ./my_application`
This way, `MySecret` does not have to be hardcoded into `my_application`.

Note that if your entire binary artifact is published (ex. a Docker container published to Docker Hub), the value for the environment variable must not be stored in the artifact itself (ex. inside the `Dockerfile` or one of the container's files) but rather must be passed dynamically, for example in the `docker run` call as an argument.

* ### Secret management services

External vendors offer cloud-based secret management services, that provide proper access control to each secret. The given access to each secret can be dynamically modified or even revoked. Some examples include -

* [Hashicorp Vault](https://www.vaultproject.io)
* [AWS KMS](https://aws.amazon.com/kms) (Key Management Service)
* [Google Cloud KMS](https://cloud.google.com/security-key-management)

## Least-privilege principle

Storing a secret in a hardcoded manner can be made safer, by making sure the secret grants the least amount of privilege as needed by the application.
For example - if the application needs to read a specific table from a specific database, and the secret grants access to perform this operation **only** (meaning - no access to other tables, no write access at all) then the damage from any secret leaks is mitigated.
That being said, it is still not recommended to store secrets in a hardcoded manner, since this type of storage does not offer any way to revoke or moderate the usage of the secret.


</details>
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@


---
## 🗝️ Secret Detected

---
| Severity | Finding | Status |
| :---------------------: | :-----------------------------------: | :-----------------------------------: |
| Medium | Secret keys were found | Active |

---
### Full description

---
Storing hardcoded secrets in your source code or binary artifact could lead to several risks.

If the secret is associated with a wide scope of privileges, attackers could extract it from the source code or binary artifact and use it maliciously to attack many targets. For example, if the hardcoded password gives high-privilege access to an AWS account, the attackers may be able to query/modify company-wide sensitive data without per-user authentication.

## Best practices

Use safe storage when storing high-privilege secrets such as passwords and tokens, for example -

* ### Environment Variables

Environment variables are set outside of the application code, and can be dynamically passed to the application only when needed, for example -
`SECRET_VAR=MySecret ./my_application`
This way, `MySecret` does not have to be hardcoded into `my_application`.

Note that if your entire binary artifact is published (ex. a Docker container published to Docker Hub), the value for the environment variable must not be stored in the artifact itself (ex. inside the `Dockerfile` or one of the container's files) but rather must be passed dynamically, for example in the `docker run` call as an argument.

* ### Secret management services

External vendors offer cloud-based secret management services, that provide proper access control to each secret. The given access to each secret can be dynamically modified or even revoked. Some examples include -

* [Hashicorp Vault](https://www.vaultproject.io)
* [AWS KMS](https://aws.amazon.com/kms) (Key Management Service)
* [Google Cloud KMS](https://cloud.google.com/security-key-management)

## Least-privilege principle

Storing a secret in a hardcoded manner can be made safer, by making sure the secret grants the least amount of privilege as needed by the application.
For example - if the application needs to read a specific table from a specific database, and the secret grants access to perform this operation **only** (meaning - no access to other tables, no write access at all) then the damage from any secret leaks is mitigated.
That being said, it is still not recommended to store secrets in a hardcoded manner, since this type of storage does not offer any way to revoke or moderate the usage of the secret.
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@

## 🗝️ Secret Detected
<div align='center'>

| Severity | Finding | Status |
| :---------------------: | :-----------------------------------: | :-----------------------------------: |
| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableMediumSeverity.png)<br> Medium | Secret keys were found | Active |

</div>

<details>
<summary> <b>Full description</b> </summary>
<br>

Storing hardcoded secrets in your source code or binary artifact could lead to several risks.

If the secret is associated with a wide scope of privileges, attackers could extract it from the source code or binary artifact and use it maliciously to attack many targets. For example, if the hardcoded password gives high-privilege access to an AWS account, the attackers may be able to query/modify company-wide sensitive data without per-user authentication.

## Best practices

Use safe storage when storing high-privilege secrets such as passwords and tokens, for example -

* ### Environment Variables

Environment variables are set outside of the application code, and can be dynamically passed to the application only when needed, for example -
`SECRET_VAR=MySecret ./my_application`
This way, `MySecret` does not have to be hardcoded into `my_application`.

Note that if your entire binary artifact is published (ex. a Docker container published to Docker Hub), the value for the environment variable must not be stored in the artifact itself (ex. inside the `Dockerfile` or one of the container's files) but rather must be passed dynamically, for example in the `docker run` call as an argument.

* ### Secret management services

External vendors offer cloud-based secret management services, that provide proper access control to each secret. The given access to each secret can be dynamically modified or even revoked. Some examples include -

* [Hashicorp Vault](https://www.vaultproject.io)
* [AWS KMS](https://aws.amazon.com/kms) (Key Management Service)
* [Google Cloud KMS](https://cloud.google.com/security-key-management)

## Least-privilege principle

Storing a secret in a hardcoded manner can be made safer, by making sure the secret grants the least amount of privilege as needed by the application.
For example - if the application needs to read a specific table from a specific database, and the secret grants access to perform this operation **only** (meaning - no access to other tables, no write access at all) then the damage from any secret leaks is mitigated.
That being said, it is still not recommended to store secrets in a hardcoded manner, since this type of storage does not offer any way to revoke or moderate the usage of the secret.


</details>
19 changes: 19 additions & 0 deletions utils/comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const (
ApplicableComment ReviewCommentType = "Applicable"
IacComment ReviewCommentType = "Iac"
SastComment ReviewCommentType = "Sast"
SecretComment ReviewCommentType = "Secrets"

RescanRequestComment = "rescan"
commentRemovalErrorMsg = "An error occurred while attempting to remove older Frogbot pull request comments:"
Expand Down Expand Up @@ -201,6 +202,12 @@ func getNewReviewComments(repo *Repository, issues *IssuesCollection) (commentsT
for _, sast := range issues.Sast {
commentsToAdd = append(commentsToAdd, generateReviewComment(SastComment, sast.Location, generateSourceCodeReviewContent(SastComment, sast, writer)))
}
if !repo.Params.PullRequestSecretComments {
return
}
for _, secret := range issues.Secrets {
attiasas marked this conversation as resolved.
Show resolved Hide resolved
commentsToAdd = append(commentsToAdd, generateReviewComment(SecretComment, secret.Location, generateSourceCodeReviewContent(SecretComment, secret, writer)))
}
return
}

Expand Down Expand Up @@ -252,6 +259,18 @@ func generateSourceCodeReviewContent(commentType ReviewCommentType, issue format
issue.CodeFlow,
writer,
), writer)
case SecretComment:
applicability := ""
if issue.Applicability != nil {
applicability = issue.Applicability.Status
}
return outputwriter.GenerateReviewCommentContent(outputwriter.SecretReviewContent(
issue.Severity,
issue.Finding,
issue.ScannerDescription,
applicability,
writer,
), writer)
}
return
}
Expand Down
83 changes: 76 additions & 7 deletions utils/comment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,12 @@ func TestGetFrogbotReviewComments(t *testing.T) {
}

func TestGetNewReviewComments(t *testing.T) {
repo := &Repository{OutputWriter: &outputwriter.StandardOutput{}}
writer := &outputwriter.StandardOutput{}
testCases := []struct {
name string
issues *IssuesCollection
expectedOutput []ReviewComment
name string
generateSecretsComments bool
issues *IssuesCollection
expectedOutput []ReviewComment
}{
{
name: "No issues for review comments",
Expand Down Expand Up @@ -92,6 +93,73 @@ func TestGetNewReviewComments(t *testing.T) {
},
expectedOutput: []ReviewComment{},
},
{
name: "Secret review comments",
generateSecretsComments: true,
issues: &IssuesCollection{
Vulnerabilities: []formats.VulnerabilityOrViolationRow{
{
Summary: "summary-2",
Applicable: "Applicable",
IssueId: "XRAY-2",
ImpactedDependencyDetails: formats.ImpactedDependencyDetails{
SeverityDetails: formats.SeverityDetails{Severity: "low"},
ImpactedDependencyName: "component-C",
},
Cves: []formats.CveRow{{Id: "CVE-2023-4321"}},
Technology: techutils.Npm,
},
},
Secrets: []formats.SourceCodeRow{
{
SeverityDetails: formats.SeverityDetails{
Severity: "High",
SeverityNumValue: 13,
},
Finding: "secret finding",
Applicability: &formats.Applicability{Status: "Inactive"},
Location: formats.Location{
File: "index.js",
StartLine: 5,
StartColumn: 6,
EndLine: 7,
EndColumn: 8,
Snippet: "access token exposed",
},
},
},
},
expectedOutput: []ReviewComment{
{
Location: formats.Location{
File: "index.js",
StartLine: 5,
StartColumn: 6,
EndLine: 7,
EndColumn: 8,
Snippet: "access token exposed",
},
Type: SecretComment,
CommentInfo: vcsclient.PullRequestComment{
CommentInfo: vcsclient.CommentInfo{
Content: outputwriter.GenerateReviewCommentContent(outputwriter.SecretReviewContent("High", "secret finding", "", "Inactive", writer), writer),
},
PullRequestDiff: vcsclient.PullRequestDiff{
OriginalFilePath: "index.js",
OriginalStartLine: 5,
OriginalStartColumn: 6,
OriginalEndLine: 7,
OriginalEndColumn: 8,
NewFilePath: "index.js",
NewStartLine: 5,
NewStartColumn: 6,
NewEndLine: 7,
NewEndColumn: 8,
},
},
},
},
},
{
name: "With issues for review comments",
issues: &IssuesCollection{
Expand Down Expand Up @@ -156,7 +224,7 @@ func TestGetNewReviewComments(t *testing.T) {
Type: ApplicableComment,
CommentInfo: vcsclient.PullRequestComment{
CommentInfo: vcsclient.CommentInfo{
Content: outputwriter.GenerateReviewCommentContent(outputwriter.ApplicableCveReviewContent("Low", "", "", "CVE-2023-4321", "summary-2", "component-C:", "", repo.OutputWriter), repo.OutputWriter),
Content: outputwriter.GenerateReviewCommentContent(outputwriter.ApplicableCveReviewContent("Low", "", "", "CVE-2023-4321", "summary-2", "component-C:", "", writer), writer),
},
PullRequestDiff: vcsclient.PullRequestDiff{
OriginalFilePath: "file1",
Expand Down Expand Up @@ -184,7 +252,7 @@ func TestGetNewReviewComments(t *testing.T) {
Type: IacComment,
CommentInfo: vcsclient.PullRequestComment{
CommentInfo: vcsclient.CommentInfo{
Content: outputwriter.GenerateReviewCommentContent(outputwriter.IacReviewContent("High", "Missing auto upgrade was detected", "", repo.OutputWriter), repo.OutputWriter),
Content: outputwriter.GenerateReviewCommentContent(outputwriter.IacReviewContent("High", "Missing auto upgrade was detected", "", writer), writer),
},
PullRequestDiff: vcsclient.PullRequestDiff{
OriginalFilePath: "file1",
Expand Down Expand Up @@ -212,7 +280,7 @@ func TestGetNewReviewComments(t *testing.T) {
Type: SastComment,
CommentInfo: vcsclient.PullRequestComment{
CommentInfo: vcsclient.CommentInfo{
Content: outputwriter.GenerateReviewCommentContent(outputwriter.SastReviewContent("High", "XSS Vulnerability", "", [][]formats.Location{}, repo.OutputWriter), repo.OutputWriter),
Content: outputwriter.GenerateReviewCommentContent(outputwriter.SastReviewContent("High", "XSS Vulnerability", "", [][]formats.Location{}, writer), writer),
},
PullRequestDiff: vcsclient.PullRequestDiff{
OriginalFilePath: "file1",
Expand All @@ -233,6 +301,7 @@ func TestGetNewReviewComments(t *testing.T) {
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
repo := &Repository{OutputWriter: writer, Params: Params{Git: Git{PullRequestSecretComments: tc.generateSecretsComments}}}
output := getNewReviewComments(repo, tc.issues)
assert.ElementsMatch(t, tc.expectedOutput, output)
})
Expand Down
24 changes: 13 additions & 11 deletions utils/consts.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,21 @@ const (
JfrogConfigProfileEnv = "JF_CONFIG_PROFILE"

// Git environment variables
GitProvider = "JF_GIT_PROVIDER"
GitRepoOwnerEnv = "JF_GIT_OWNER"
GitRepoEnv = "JF_GIT_REPO"
GitProjectEnv = "JF_GIT_PROJECT"
GitUsernameEnv = "JF_GIT_USERNAME"
GitUseLocalRepositoryEnv = "JF_USE_LOCAL_REPOSITORY"
GitProvider = "JF_GIT_PROVIDER"
GitRepoOwnerEnv = "JF_GIT_OWNER"
GitRepoEnv = "JF_GIT_REPO"
GitProjectEnv = "JF_GIT_PROJECT"
GitUsernameEnv = "JF_GIT_USERNAME"
GitUseLocalRepositoryEnv = "JF_USE_LOCAL_REPOSITORY"
UseMostCommonAncestorAsTargetEnv = "JF_USE_MOST_COMMON_ANCESTOR_AS_TARGET"

// Git naming template environment variables
BranchNameTemplateEnv = "JF_BRANCH_NAME_TEMPLATE"
CommitMessageTemplateEnv = "JF_COMMIT_MESSAGE_TEMPLATE"
PullRequestTitleTemplateEnv = "JF_PULL_REQUEST_TITLE_TEMPLATE"
PullRequestCommentTitleEnv = "JF_PR_COMMENT_TITLE"
UseMostCommonAncestorAsTargetEnv = "JF_USE_MOST_COMMON_ANCESTOR_AS_TARGET"
BranchNameTemplateEnv = "JF_BRANCH_NAME_TEMPLATE"
CommitMessageTemplateEnv = "JF_COMMIT_MESSAGE_TEMPLATE"
PullRequestTitleTemplateEnv = "JF_PULL_REQUEST_TITLE_TEMPLATE"
PullRequestCommentTitleEnv = "JF_PR_COMMENT_TITLE"
//#nosec G101 -- not a secret
PullRequestSecretCommentsEnv = "JF_PR_SHOW_SECRETS_COMMENTS"

// Repository environment variables - Ignored if the frogbot-config.yml file is used
InstallCommandEnv = "JF_INSTALL_DEPS_CMD"
Expand Down
Loading
Loading