Finalized pending ADRs + security enhancements (#1127)

* 📚 docs(VSecM): updated ADRs Signed-off-by: Volkan Özçelik <[email protected]> * minor * added golangci-lint to the mix Signed-off-by: Volkan Özçelik <[email protected]> --------- Signed-off-by: Volkan Özçelik <[email protected]>
vmware-tanzu · Sep 3, 2024 · 58e3b44 · 58e3b44
1 parent e03c7a3
commit 58e3b44
Show file tree

Hide file tree

Showing 20 changed files with 607 additions and 187 deletions.
diff --git a/app/safe/internal/state/stats/stats.go b/app/safe/internal/state/stats/stats.go
@@ -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.
+
 // 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

diff --git a/core/entity/v1/data/status.go b/core/entity/v1/data/status.go
@@ -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++
@@ -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--

diff --git a/core/validation/validation.go b/core/validation/validation.go
@@ -61,7 +61,6 @@ func IsWorkload(spiffeid string) bool {
 					" val: " + env.SpiffeIdPrefixForWorkload() +
 					" trust: " + env.SpiffeTrustDomain(),
 			)
-			return false
 		}
 
 		nrw := env.NameRegExpForWorkload()
@@ -75,7 +74,6 @@ func IsWorkload(spiffeid string) bool {
 					" val: " + env.NameRegExpForWorkload() +
 					" trust: " + env.SpiffeTrustDomain(),
 			)
-			return false
 		}
 
 		match := wre.FindStringSubmatch(spiffeid)
@@ -103,7 +101,6 @@ func IsWorkload(spiffeid string) bool {
 				" val: " + env.NameRegExpForWorkload() +
 				" trust: " + env.SpiffeTrustDomain(),
 		)
-		return false
 	}
 
 	wre, err := regexp.Compile(nrw)
@@ -116,7 +113,6 @@ func IsWorkload(spiffeid string) bool {
 				" val: " + env.NameRegExpForWorkload() +
 				" trust: " + env.SpiffeTrustDomain(),
 		)
-		return false
 	}
 
 	match := wre.FindStringSubmatch(spiffeid)
@@ -165,7 +161,6 @@ func IsSentinel(spiffeid string) bool {
 					" val: " + env.SpiffeIdPrefixForSentinel() +
 					" trust: " + env.SpiffeTrustDomain(),
 			)
-			return false
 		}
 
 		return re.MatchString(spiffeid)
@@ -212,7 +207,6 @@ func IsSafe(spiffeid string) bool {
 					" val: " + env.SpiffeIdPrefixForSafe() +
 					" trust: " + env.SpiffeTrustDomain(),
 			)
-			return false
 		}
 
 		return re.MatchString(spiffeid)

diff --git a/...cture/adrs/20240509-adr-0003-vsecm-will-be-scoped-to-work-on-kubernetes-only.md b/...cture/adrs/20240509-adr-0003-vsecm-will-be-scoped-to-work-on-kubernetes-only.md
@@ -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
@@ -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
 

diff --git a/...tent/documentation/architecture/adrs/20240510-adr-0004-be-practically-secure.md b/...tent/documentation/architecture/adrs/20240510-adr-0004-be-practically-secure.md
@@ -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.
 

diff --git a/...nt/documentation/architecture/adrs/20240510-adr-0005-be-resilient-by-default.md b/...nt/documentation/architecture/adrs/20240510-adr-0005-be-resilient-by-default.md
@@ -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

diff --git a/...on/architecture/adrs/20240510-adr-0011-keep-things-minimal-do-one-thing-well.md b/...on/architecture/adrs/20240510-adr-0011-keep-things-minimal-do-one-thing-well.md
@@ -80,4 +80,3 @@ You can view the ADRs by browsing this following list:
 {{ adrs() }}
 
 {{ edit() }}
-
diff --git a/...chitecture/adrs/20240510-adr-0012-all-files-shall-have-a-spdx-license-header.md b/...chitecture/adrs/20240510-adr-0012-all-files-shall-have-a-spdx-license-header.md
@@ -83,4 +83,3 @@ You can view the ADRs by browsing this following list:
 {{ adrs() }}
 
 {{ edit() }}
-
diff --git a/...0240510-adr-0013-scan-codebase-for-vulnerabilities-and-code-smells-regularly.md b/...0240510-adr-0013-scan-codebase-for-vulnerabilities-and-code-smells-regularly.md
@@ -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`
+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>

diff --git a/...on/architecture/adrs/20240510-adr-0014-get-openssf-best-practices-compliance.md b/...on/architecture/adrs/20240510-adr-0014-get-openssf-best-practices-compliance.md
@@ -103,4 +103,3 @@ You can view the ADRs by browsing this following list:
 {{ adrs() }}
 
 {{ edit() }}
-
diff --git a/...ure/adrs/20240510-adr-0015-keep-source-code-line-length-around-80-characters.md b/...ure/adrs/20240510-adr-0015-keep-source-code-line-length-around-80-characters.md
@@ -118,4 +118,3 @@ You can view the ADRs by browsing this following list:
 {{ adrs() }}
 
 {{ edit() }}
-
diff --git a/...on/architecture/adrs/20240511-adr-0016-centralized-constants-and-magic-words.md b/...on/architecture/adrs/20240511-adr-0016-centralized-constants-and-magic-words.md
@@ -125,4 +125,3 @@ You can view the ADRs by browsing this following list:
 {{ adrs() }}
 
 {{ edit() }}
-