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

refactor(repo): Simplify structure #308

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

refactor(repo): Simplify structure #308

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2341576
refactor(specs): Simplify structure for specs
simar7 Jan 4, 2025
7aecd33
add k8s checks readme
simar7 Jan 4, 2025
6ae456c
update architecture.md
simar7 Jan 4, 2025
e281ce0
update go mod
simar7 Jan 4, 2025
b58a18c
Merge branch 'main' into refactor-structure
simar7 Jan 15, 2025
25d4ad7
go mod tidy
simar7 Jan 15, 2025
e7ba465
Merge branch 'main' into refactor-structure
simar7 Jan 17, 2025
9afd9ef
rename pkg/specs -> pkg/compliance
simar7 Jan 18, 2025
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
5 changes: 2 additions & 3 deletions ARCHITECTURE.md
View file
Open in desktop
itaysk marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,9 @@ This document aims to answer the question *Where is the code that does X?*
The directory structure is broken down as follows:

- `cmd/` - These CLI tools are primarily used during development for end-to-end testing without needing to pull the library into trivy/tfsec etc.
itaysk marked this conversation as resolved.
Show resolved Hide resolved
- `checks` - All of the checks are defined in this directory.
- `checks` - All the checks are defined in this directory.
- `commands` - All Node-collector commands are defined in this directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

let's link to node collector docs to make sure the context is undestood

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we want to continue developing node-collector and its associated functionality? I asked it here.

If not we should remove it from trivy-checks if this repo is solely for the purposes of keeping checks.

Copy link
Contributor

@itaysk itaysk Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

yes, node collector is needed for k8s cis compliance, isn't it? in any case, "commands" is not very clear without this context. "node-collector" or something similar will give it more context I think

- `pkg/spec` - Logic to handle standardized specs such as CIS.
- `pkg/rules` - This package exposes internal rules, and imports them accordingly (see _rules.go_).
- `pkg/rules` - This package exposes internal checks, and imports them accordingly (see _rules.go_).
itaysk marked this conversation as resolved.
Show resolved Hide resolved
- `specs/` - Standaridized compliance specs such as CIS.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think specs is oerloaded term. for a newcomer "specs" probably relate more to bdd specs or to standardidation specs. the feature in trivy is called "compliance" shouldn't the directory be called that way too?

Copy link
Contributor

Choose a reason for hiding this comment

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

theres a doc about contributing compliance specs under docs/compliance.md. it's the only doc in the docs dir I thin we should make it a readme in this dir, like kubernetes related info is documented in readme under checks/kubernetes, then we can remove /docs, which should be in trivy anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you've addressed the first comment about renaming specs to compliance but not the second about docs

- `test` - Integration tests and other high-level tests that require a full build of the project.
- `scripts` - Usefule generation scripts for bundle generation and verification purposes.
Copy link
Contributor

Choose a reason for hiding this comment

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

any difference between scripts and cmd? besides the programming languages used

Copy link
Member Author

Choose a reason for hiding this comment

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

No difference besides that. Usually, cmd/ houses small go programs but that's about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think scripts is actually mostly about the bundle isn't it? perhaps that would be a better direction

13 changes: 13 additions & 0 deletions checks/kubernetes/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,19 @@ wget https://github.com/aquasecurity/trivy-checks/raw/main/test/testdata/kuberne
conftest test denied.yaml --policy myPolicy/ --namespace builtin.kubernetes.KSV008
```

# Kubernetes checks classification
There are several Kubernetes checks that target various subsystems. They are loosely classified as follows.

| Target | Description |
|----------------|------------------------------------------------------------------------------------------------------------------------------------|
| Network | Checks primarily targeting the networking stack |
| Dynamic | Checks that evaluate deprecated and removed APIs |
Copy link
Contributor

Choose a reason for hiding this comment

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

either the description is not accurate or the dir name is bad. I think the intention was that these checks rely on environmental context for evaluation. for example, the deprecated API checks rely on information of which k8s version the user is running/evaluating against in order to decide if outdated or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree the name isn't very meaningful but then again I didn't want to change it in this PR as there maybe users who rely on this structure.

I couldn't come up with a better description, so I'm open to ideas. Would the following work based on what you said?

"Checks whose decision outcome is dynamically determined based on the environmental context."

Copy link
Contributor

Choose a reason for hiding this comment

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

do you think the structure of checks could break users? I mean aren't the checks save/loaded from embedded/bundle in their complete hierarchy? is changing the internal hierachy problemattic?

about the description, it's something like:
"Checks that cannot evaluate based on input alone, and depend on environmental or user-provided context"

| CIS Benchmarks | Checks that are recommended by the CIS Benchmarks. The checks inside are targeted per each subsystem (e.g. apiserver, cni, etc.) |
Copy link
Contributor

Choose a reason for hiding this comment

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

If this dir is serving only CIS, and CIS is using only this dir then fine, but I will challange us abit here: aren't these checks valid k8s checks also outside CIS context? aren't CIS reports include k8s checks besides these checks (i presume something like non privileged pod check).

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right in the sense that these checks are ordinary checks even outside of CIS context.

I'm open to re-organizing all k8s checks in general (maybe based on another type) but we just need to confirm first with the stakeholders that this won't cause any breakage as in the past some users did rely on the structure. Can we do this in another PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes I think we need to reorganize the checks which is part of the motivation for this PR. sure if you think it needs another pr that's fine

| Advanced | Checks that are recommended for the advanced uesrs of Kubernetes |
Copy link
Contributor

Choose a reason for hiding this comment

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

how do we define advanced users

| GKE | Checks specific to Google Kubernetes Engine |
| PSS | Checks pertaining to Pod Security Standards |
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as CIS



# Standards and best practices
This GitHub repository has controls that cover both [PodSecurityPolicy](https://kubernetes.io/docs/concepts/policy/pod-security-policy/) (PSP) and the Kubernetes [Pod Security Standards](https://kubernetes.io/docs/concepts/security/pod-security-standards/) (PSS), plus additional best practices.

Expand Down
5 changes: 1 addition & 4 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ module github.com/aquasecurity/trivy-checks
go 1.23.4

require (
github.com/aquasecurity/trivy v0.58.1-0.20250109050215-f9a6a7192722
github.com/aquasecurity/trivy v0.58.1-0.20250111034400-243e5a3af99e
github.com/aws-cloudformation/rain v1.21.0
github.com/hashicorp/hcl/v2 v2.23.0
github.com/liamg/iamgo v0.0.9
Expand Down Expand Up @@ -407,6 +407,3 @@ require (
tags.cncf.io/container-device-interface v0.8.0 // indirect
tags.cncf.io/container-device-interface/specs-go v0.8.0 // indirect
)

// needed until the new name is used everywhere
replace github.com/aquasecurity/trivy-policies v0.10.0 => github.com/aquasecurity/trivy-checks v0.10.2-0.20240417031955-932169bbd75f
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,8 +355,8 @@ github.com/aquasecurity/testdocker v0.0.0-20240730042311-4642e94c7fc8 h1:b43UVqY
github.com/aquasecurity/testdocker v0.0.0-20240730042311-4642e94c7fc8/go.mod h1:wXA9k3uuaxY3yu7gxrxZDPo/04FEMJtwyecdAlYrEIo=
github.com/aquasecurity/tml v0.6.1 h1:y2ZlGSfrhnn7t4ZJ/0rotuH+v5Jgv6BDDO5jB6A9gwo=
github.com/aquasecurity/tml v0.6.1/go.mod h1:OnYMWY5lvI9ejU7yH9LCberWaaTBW7hBFsITiIMY2yY=
github.com/aquasecurity/trivy v0.58.1-0.20250109050215-f9a6a7192722 h1:+YTyne1Ge2I8bpbqkClvMKCdK0U1qmQySjlS3cC4ih4=
github.com/aquasecurity/trivy v0.58.1-0.20250109050215-f9a6a7192722/go.mod h1:pDTEekX8yWaSI094UO3dTBnwI2xSZhwekFCbTSog8Jk=
github.com/aquasecurity/trivy v0.58.1-0.20250111034400-243e5a3af99e h1:d5JKS4O/5IO7lPweDsoH3DaEe3F96mMHwK9FqZeC2WY=
github.com/aquasecurity/trivy v0.58.1-0.20250111034400-243e5a3af99e/go.mod h1:pDTEekX8yWaSI094UO3dTBnwI2xSZhwekFCbTSog8Jk=
github.com/aquasecurity/trivy-db v0.0.0-20241209111357-8c398f13db0e h1:O5j5SeCNBrXApgBTOobO06q4LMxJxIhcSGE7H6Y154E=
github.com/aquasecurity/trivy-db v0.0.0-20241209111357-8c398f13db0e/go.mod h1:gS8VhlNxhraiq60BBnJw9kGtjeMspQ9E8pX24jCL4jg=
github.com/aquasecurity/trivy-java-db v0.0.0-20240109071736-184bd7481d48 h1:JVgBIuIYbwG+ekC5lUHUpGJboPYiCcxiz06RCtz8neI=
Expand Down
23 changes: 0 additions & 23 deletions pkg/spec/spec.go
View file
Open in desktop

This file was deleted.

0 pkg/specs/compliance/aws-cis-1.2.yaml → pkg/specs/aws-cis-1.2.yaml
View file
Open in desktop
File renamed without changes.
0 pkg/specs/compliance/aws-cis-1.4.yaml → pkg/specs/aws-cis-1.4.yaml
View file
Open in desktop
File renamed without changes.
0 pkg/specs/compliance/docker-cis-1.6.0.yaml → pkg/specs/docker-cis-1.6.0.yaml
View file
Open in desktop
File renamed without changes.
0 pkg/specs/compliance/eks-cis-1.4.yaml → pkg/specs/eks-cis-1.4.yaml
View file
Open in desktop
File renamed without changes.
0 pkg/specs/compliance/k8s-cis-1.23.yaml → pkg/specs/k8s-cis-1.23.yaml
View file
Open in desktop
File renamed without changes.
0 pkg/specs/compliance/k8s-nsa-1.0.yaml → pkg/specs/k8s-nsa-1.0.yaml
View file
Open in desktop
File renamed without changes.
0 ...pecs/compliance/k8s-pss-baseline-0.1.yaml → pkg/specs/k8s-pss-baseline-0.1.yaml
View file
Open in desktop
File renamed without changes.
0 ...cs/compliance/k8s-pss-restricted-0.1.yaml → pkg/specs/k8s-pss-restricted-0.1.yaml
View file
Open in desktop
File renamed without changes.
28 changes: 22 additions & 6 deletions pkg/specs/loader.go
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be in trivy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not quite because then users will need to import trivy as a library to load compliance specs which isn't ideal as it's a much bigger dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

users needing to load compliance checks will also need to run them, which will require trivy no? is there a use case for loading compliance and not running them?

Original file line number Diff line number Diff line change
Expand Up @@ -10,24 +10,40 @@ import (
"gopkg.in/yaml.v3"
)

const ComplianceFolder = "compliance"
// Loader access compliance specs
type Loader interface {
GetSpecByName(name string) string
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a need for this interface?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not but since we already have it, removal of it would be a breaking change that I don't want to make in this PR.

}

type specLoader struct {
}

// NewSpecLoader instansiate spec loader
func NewSpecLoader() Loader {
return &specLoader{}
}

// GetSpecByName get spec name and return spec data
func (sl specLoader) GetSpecByName(name string) string {
return GetSpec(name)
}

var (
//go:embed compliance
complainceFS embed.FS
//go:embed *.yaml
complianceFS embed.FS
)

var complianceSpecMap map[string]string

// Load compliance specs
func init() {
dir, _ := complainceFS.ReadDir(ComplianceFolder)
complianceSpecMap = make(map[string]string, 0)
dir, _ := complianceFS.ReadDir(".")
complianceSpecMap = make(map[string]string)
for _, r := range dir {
if !strings.Contains(r.Name(), ".yaml") {
continue
}
file, err := complainceFS.Open(fmt.Sprintf("%s/%s", ComplianceFolder, r.Name()))
file, err := complianceFS.Open(fmt.Sprintf("%s", r.Name()))
if err != nil {
panic(err)
}
Expand Down
18 changes: 9 additions & 9 deletions pkg/specs/loader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ func TestLoadSpecs(t *testing.T) {
specName string
wantSpecPath string
}{
{name: "nsa spec", specName: "k8s-nsa-1.0", wantSpecPath: "./compliance/k8s-nsa-1.0.yaml"},
{name: "k8s cis bench", specName: "k8s-cis-1.23", wantSpecPath: "./compliance/k8s-cis-1.23.yaml"},
{name: "k8s pss baseline", specName: "k8s-pss-baseline-0.1", wantSpecPath: "./compliance/k8s-pss-baseline-0.1.yaml"},
{name: "k8s pss restricted", specName: "k8s-pss-restricted-0.1", wantSpecPath: "./compliance/k8s-pss-restricted-0.1.yaml"},
{name: "awscis1.2", specName: "aws-cis-1.2", wantSpecPath: "./compliance/aws-cis-1.2.yaml"},
{name: "awscis1.4", specName: "aws-cis-1.4", wantSpecPath: "./compliance/aws-cis-1.4.yaml"},
{name: "docker cis bench", specName: "docker-cis-1.6.0", wantSpecPath: "./compliance/docker-cis-1.6.0.yaml"},
{name: "awscis1.2 by filepath", specName: "@./compliance/aws-cis-1.2.yaml", wantSpecPath: "./compliance/aws-cis-1.2.yaml"},
{name: "nsa spec", specName: "k8s-nsa-1.0", wantSpecPath: "./k8s-nsa-1.0.yaml"},
{name: "k8s cis bench", specName: "k8s-cis-1.23", wantSpecPath: "./k8s-cis-1.23.yaml"},
{name: "k8s pss baseline", specName: "k8s-pss-baseline-0.1", wantSpecPath: "./k8s-pss-baseline-0.1.yaml"},
{name: "k8s pss restricted", specName: "k8s-pss-restricted-0.1", wantSpecPath: "./k8s-pss-restricted-0.1.yaml"},
{name: "awscis1.2", specName: "aws-cis-1.2", wantSpecPath: "./aws-cis-1.2.yaml"},
{name: "awscis1.4", specName: "aws-cis-1.4", wantSpecPath: "./aws-cis-1.4.yaml"},
{name: "docker cis bench", specName: "docker-cis-1.6.0", wantSpecPath: "./docker-cis-1.6.0.yaml"},
{name: "awscis1.2 by filepath", specName: "@./aws-cis-1.2.yaml", wantSpecPath: "./aws-cis-1.2.yaml"},
{name: "bogus spec", specName: "foobarbaz"},
}

Expand All @@ -30,7 +30,7 @@ func TestLoadSpecs(t *testing.T) {
wantSpecData, err := os.ReadFile(tt.wantSpecPath)
assert.NoError(t, err)
gotSpecData := GetSpec(tt.specName)
assert.Equal(t, gotSpecData, string(wantSpecData))
assert.Equal(t, string(wantSpecData), gotSpecData)
} else {
assert.Empty(t, GetSpec(tt.specName), tt.name)
}
Expand Down
0 pkg/specs/compliance/rke2-cis-1.24.yaml → pkg/specs/rke2-cis-1.24.yaml
View file
Open in desktop
File renamed without changes.
Loading