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

Finalized pending ADRs + security enhancements #1127

Merged
merged 3 commits into from
Sep 3, 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
7 changes: 4 additions & 3 deletions app/safe/internal/state/stats/stats.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,15 @@ var CurrentState = data.Status{
K8sQueueLen: 0,
K8sQueueCap: 0,
NumSecrets: 0,
Lock: sync.RWMutex{},
}

var currentStateLock sync.RWMutex // Protects access to the CurrentState object.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

having a mutex as a member of struct was raising a go vet warning.


// Stats returns a copy of the CurrentState Status object, providing a snapshot
// of the current status of VSecM.
func Stats() data.Status {
CurrentState.Lock.RLock()
defer CurrentState.Lock.RUnlock()
currentStateLock.RLock()
defer currentStateLock.RUnlock()

// Note that this is a side effect.
// Another alternative would be to update these values in a
Expand Down
11 changes: 6 additions & 5 deletions core/entity/v1/data/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,15 @@ type Status struct {
K8sQueueLen int
K8sQueueCap int
NumSecrets int
Lock sync.RWMutex
}

var statusLock sync.RWMutex // Protects access to the Status struct.

// Increment is a method for the Status struct that increments the NumSecrets
// field by 1 if the provided secret name is not found in the in-memory store.
func (s *Status) Increment(name string, loader func(name any) (any, bool)) {
s.Lock.Lock()
defer s.Lock.Unlock()
statusLock.Lock()
defer statusLock.Unlock()
_, ok := loader(name)
if !ok {
s.NumSecrets++
Expand All @@ -47,8 +48,8 @@ func (s *Status) Increment(name string, loader func(name any) (any, bool)) {
// Decrement is a method for the Status struct that decrements the NumSecrets
// field by 1 if the provided secret name is found in the in-memory store.
func (s *Status) Decrement(name string, loader func(name any) (any, bool)) {
s.Lock.Lock()
defer s.Lock.Unlock()
statusLock.Lock()
defer statusLock.Unlock()
_, ok := loader(name)
if ok {
s.NumSecrets--
Expand Down
6 changes: 0 additions & 6 deletions core/validation/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ func IsWorkload(spiffeid string) bool {
" val: " + env.SpiffeIdPrefixForWorkload() +
" trust: " + env.SpiffeTrustDomain(),
)
return false
}

nrw := env.NameRegExpForWorkload()
Expand All @@ -75,7 +74,6 @@ func IsWorkload(spiffeid string) bool {
" val: " + env.NameRegExpForWorkload() +
" trust: " + env.SpiffeTrustDomain(),
)
return false
}

match := wre.FindStringSubmatch(spiffeid)
Expand Down Expand Up @@ -103,7 +101,6 @@ func IsWorkload(spiffeid string) bool {
" val: " + env.NameRegExpForWorkload() +
" trust: " + env.SpiffeTrustDomain(),
)
return false
}

wre, err := regexp.Compile(nrw)
Expand All @@ -116,7 +113,6 @@ func IsWorkload(spiffeid string) bool {
" val: " + env.NameRegExpForWorkload() +
" trust: " + env.SpiffeTrustDomain(),
)
return false
}

match := wre.FindStringSubmatch(spiffeid)
Expand Down Expand Up @@ -165,7 +161,6 @@ func IsSentinel(spiffeid string) bool {
" val: " + env.SpiffeIdPrefixForSentinel() +
" trust: " + env.SpiffeTrustDomain(),
)
return false
}

return re.MatchString(spiffeid)
Expand Down Expand Up @@ -212,7 +207,6 @@ func IsSafe(spiffeid string) bool {
" val: " + env.SpiffeIdPrefixForSafe() +
" trust: " + env.SpiffeTrustDomain(),
)
return false
}

return re.MatchString(spiffeid)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,6 @@ title = "ADR-0003: VSecM Will Be Scoped to Work on Kubernetes Only"
weight = 3
+++

{{
/*
| Protect your secrets, protect your sensitive data.
: Explore VMware Secrets Manager docs at https://vsecm.com/
</
<>/ keep your secrets... secret
>/
<>/' Copyright 2023-present VMware Secrets Manager contributors.
>/' SPDX-License-Identifier: BSD-2-Clause
*/
}}

- Status: accepted
- Date: 2024-05-09
- Tags: integration
Expand Down Expand Up @@ -63,8 +51,7 @@ and there are not many major features to implement.
## Decision Outcome

Chosen option: Option 1, because of increased scope not matching our limited time
and resources; and also because we’d rather keep the project secure and
well-tested.
and resources; and also because we’d rather keep the project secure and well-tested.

### Positive Consequences

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,25 +13,13 @@ title = "ADR-0004: Be Practically Secure"
weight = 4
+++

{{
/*
| Protect your secrets, protect your sensitive data.
: Explore VMware Secrets Manager docs at https://vsecm.com/
</
<>/ keep your secrets... secret
>/
<>/' Copyright 2023-present VMware Secrets Manager contributors.
>/' SPDX-License-Identifier: BSD-2-Clause
*/
}}

- Status: accepted
- Date: 2024-05-11
- Tags: guidelines, principles

## Context and Problem Statement

Corollary: Do not be annoyingly secure.
**Corollary**: Do not be annoyingly secure.

Provide a delightful user experience while taking security seriously.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,6 @@ title = "ADR-0005: Be Resilient by Default"
weight = 5
+++

{{
/*
| Protect your secrets, protect your sensitive data.
: Explore VMware Secrets Manager docs at https://vsecm.com/
</
<>/ keep your secrets... secret
>/
<>/' Copyright 2023-present VMware Secrets Manager contributors.
>/' SPDX-License-Identifier: BSD-2-Clause
*/
}}

- Status: accepted
- Date: 2024-05-11
- Tags: quality, stability
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,3 @@ You can view the ADRs by browsing this following list:
{{ adrs() }}

{{ edit() }}

Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,3 @@ You can view the ADRs by browsing this following list:
{{ adrs() }}

{{ edit() }}

Original file line number Diff line number Diff line change
Expand Up @@ -13,40 +13,99 @@ title = "ADR-0013: Scan the Codebase for Vulnerabilities and Code Smells Regular
weight = 13
+++

- Status: draft
- Date: 2024-05-12
- Status: accepted
- Date: 2024-09-02
- Tags: security, static-analysis, quality

## Context and Problem Statement

As our software project grows in complexity and scale, the risk of introducing
security vulnerabilities and code smells increases. Currently, our codebase
lacks a consistent and systematic approach to identifying these issues early
in the development cycle, leading to higher maintenance costs and potential
As our software project grows in complexity and scale, the risk of introducing
security vulnerabilities and code smells increases. Currently, our codebase
lacks a consistent and systematic approach to identifying these issues early
in the development cycle, leading to higher maintenance costs and potential
security breaches in production.

This ADR is in a draft state, we will update it with a selection of tools and
processes to scan the codebase for vulnerabilities and code smells regularly.

## Decision Drivers

- TBD
- Improve code quality and security
- Detect potential vulnerabilities early in the development process
- Ensure consistent code analysis across the project
- Integrate seamlessly with existing CI/CD pipelines

## Considered Options

- TBD
1. Use `go vet` and `govulncheck`
2. Use `go vet`, `govulncheck`, and Snyk
3. Use `go vet`, `govulncheck`, `codesweep`, and `gosec`
4. Use `go vet`, `govulncheck`, Snyk, and `golangci-lint`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the golangci-lint suggestion @gurkanguray .

5. Maintain status quo (no regular scanning)

## Decision Outcome

TBD.
Chosen option: "Use go vet, govulncheck, Snyk, and golangci-lint", because this
combination of tools provides a comprehensive set of tools for code analysis,
vulnerability detection, and security monitoring while also including a
powerful linter for Go code.

### Implementation Details

1. `go vet`:
- Run `go vet ./...` as part of the CI/CD pipeline to catch common coding
mistakes.
- Before release: Manually run `go vet ./...` and review results.

2. `govulncheck`:
- Install: `go install golang.org/x/vuln/cmd/govulncheck@latest`
- Run: `govulncheck ./...` in the CI/CD pipeline to check for known
vulnerabilities.
- Before release: Manually run `govulncheck ./...` and review results.

3. Snyk:
- Install Snyk CLI: `npm install -g snyk`
- Navigate to the project directory: `cd $WORKSPACE/secrets-manager`
- Authenticate (if not already done): `snyk auth`
- Run: `snyk monitor` in the CI/CD pipeline.
- Before release: Manually run `snyk test` and review results.
- Monitor projects on https://app.snyk.io/org/$username/projects
(replace $username with your actual username)

4. `golangci-lint`:
- Install `golangci-lint` by following the official documentation
- Create a configuration file `.golangci.yml` in the project root with
desired linters and settings.
- Run: `golangci-lint run` before every release cut and review results.

5. Pre-release manual check:
- Before cutting a release, a designated team member should run all the above
commands manually and review the results.
- Any new vulnerabilities or issues found should be addressed before proceeding
with the release.
- Document the results of this manual check in the release notes.

### Positive Consequences

- TBD
- Early detection of potential vulnerabilities and code smells
- Improved overall code quality and security
- Consistent code analysis across the project
- Comprehensive linting with `golangci-lint` catches a wide range of issues

### Negative Consequences

- TBD
- Slight increase in release preparation time due to additional scanning steps
during release cuts
- Potential false positives that may require manual review
- Initial setup time for configuring `golangci-lint`

## Additional Notes

- Consider implementing codesweep and gosec in the future if more comprehensive
static analysis is needed.
- Regularly review and update the scanning tools to ensure they remain effective
and relevant.
- Provide documentation and training on how to interpret and act on the results
of these scans.
- Periodically review and update the `.golangci.yml` configuration to ensure it
aligns with project needs and best practices.

<p>&nbsp;</p>
<p>&nbsp;</p>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,4 +103,3 @@ You can view the ADRs by browsing this following list:
{{ adrs() }}

{{ edit() }}

Original file line number Diff line number Diff line change
Expand Up @@ -118,4 +118,3 @@ You can view the ADRs by browsing this following list:
{{ adrs() }}

{{ edit() }}

Original file line number Diff line number Diff line change
Expand Up @@ -125,4 +125,3 @@ You can view the ADRs by browsing this following list:
{{ adrs() }}

{{ edit() }}

Loading