Feature/admission controller #226

amitschendel · 2024-06-18T13:18:20Z

Overview

This PR fixes #

Signed Commits

Yes, I signed my commits.

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

github-actions · 2024-06-18T13:32:09Z

Summary:

License scan: failure
Credentials scan: failure
Vulnerabilities scan: success
Unit test: success
Go linting: failure

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

github-actions · 2024-06-19T08:40:18Z

Summary:

License scan: success
Credentials scan: failure
Vulnerabilities scan: success
Unit test: success
Go linting: failure

matthyx

interesting... some clarifications needed, but overall good
did you follow any example for the admission controller that you could document?

matthyx · 2024-06-19T10:52:08Z

main.go

+		admissionController := webhook.New(addr, "/etc/certs/tls.crt", "/etc/certs/tls.key", runtime.NewScheme(), webhook.NewAdmissionValidator(k8sApi, exporter))
+		// Start HTTP REST server for webhook
+		waitGroup.Add(1)
+		go func() {


I saw that @kooomix used https://pkg.go.dev/golang.org/x/sync/errgroup#Group.Go in the backend and I found it super elegant... consider it here maybe?

matthyx · 2024-06-19T10:53:09Z

main.go

@@ -136,6 +142,36 @@ func main() {
 		}(mainHandler)
 	}

+	if operatorConfig.AdmissionControllerEnabled() {
+		// Handle SIGINT and SIGTERM by cancelling the root context
+		admissionContext, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM)


maybe we can move this outside of the flag? why only SIGINT with admission controller?

matthyx · 2024-06-19T10:59:20Z

admission/exporter/http_exporter.go

+		return
+	}
+
+	// discard the body


I am still not convinced this is needed... do you have some pointers (I know we do it in different places, just curious)?

see golang/go#60240

I prefer to do it anyway to avoid potential leaks.

admission/webhook/server.go

matthyx · 2024-06-19T11:05:16Z

admission/webhook/validator.go

+	return true
+}
+
+func (av *AdmissionValidator) validateAdminRoleBinding(attrs admission.Attributes) error {


I think at some point these will reside as separate rules, such as in node-agent?

Maybe at some point but not now.

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

github-actions · 2024-06-19T12:03:24Z

Summary:

License scan: success
Credentials scan: failure
Vulnerabilities scan: success
Unit test: success
Go linting: failure

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

dwertent

I added some comments.

Here are my overall comments:

What are the 'rules'? Why are they not defined as rules, but rather as "if" statements in a large function?
How can the user interact with this system?
This code is missing unit tests.
Some conditions, such as capabilities, are hard-coded. Consider implementing a more flexible solution.

I don't know the full context, but if this is just a POC, I don't see a reason to merge it into the main branch. You should continue working on it, and once it is more complete (at least the basics), we can consider merging it into the main branch.

dwertent · 2024-06-19T11:42:37Z

main.go

+			}()
+
+			cancellationReason := admissionController.Run(serverContext)
+			logger.L().Ctx(ctx).Error("server stopped", helpers.Error(cancellationReason))


Suggested change

logger.L().Ctx(ctx).Error("server stopped", helpers.Error(cancellationReason))

logger.L().Ctx(ctx).Fatal("server stopped", helpers.Error(cancellationReason))

dwertent · 2024-06-19T11:42:59Z

main.go

+				waitGroup.Done()
+			}()
+
+			cancellationReason := admissionController.Run(serverContext)


Suggested change

cancellationReason := admissionController.Run(serverContext)

err := admissionController.Run(serverContext)

dwertent · 2024-06-19T11:45:26Z

main.go

+		admissionContext, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM)
+		defer stop()
+
+		waitGroup := sync.WaitGroup{}


I think we should "kill" the main process if there is an issue initializing the admission controller.
This being said, I don't think we need here a waiting group.

Removed it but I think it's good to have for scaling.

I'm not sure what you mean by the "scaling", what is the scenario?

dwertent · 2024-06-19T11:49:51Z

admission/exporter/http_exporter.go

+	}
+	// create the HTTPAlertsList struct
+	httpAlertsList := HTTPAlertsList{
+		Kind:       "RuntimeAlerts",


Is this runtimeAlerts? I dont think so.
I believe this should have a different kind so it can be treated as admission controller alerts.

It is a RuntimeAlert the admission kind is passed in other place.

This is the same kind of alert as coming from the node-agent, in other words, we will not know "down the line" the source of the alert. And I think we will want to know...

dwertent · 2024-06-19T11:53:14Z

admission/exporter/http_exporter.go

+func (exporter *HTTPExporter) SendAdmissionAlert(admissionAttrs *admission.Attributes, ruleID string, RuleDescription string) {
+	isLimitReached := exporter.checkAlertLimit()
+	if isLimitReached {
+		exporter.sendAlertLimitReached()


This sends an alert every time the limit is reached. Instead of sending multiple alerts for each instance that reaches the limit, we should send a single alert indicating that the limit has been reached. This can be managed by wrapping the alert logic in a boolean condition to ensure the 'limit reached' alert is sent only once.

dwertent · 2024-06-19T12:00:58Z

admission/webhook/validator.go

+func (av *AdmissionValidator) validatePods(attrs admission.Attributes) error {
+	var errs error
+
+	// Check if the request is for pod exec or attach.


This is not "pod validation". You should have a different function for this

dwertent · 2024-06-19T12:01:15Z

admission/webhook/validator.go

+		errs = errors.Join(errs, admission.NewForbidden(attrs, fmt.Errorf("exec to pod is audited")))
+	}
+
+	if attrs.GetSubresource() == "attach" {


Same, not pod validation

admission/webhook/validator.go

dwertent · 2024-06-19T12:02:05Z

admission/webhook/validator.go

+	}
+
+	// Check if the request is for port-forwarding.
+	if attrs.GetSubresource() == "portforward" {


Same, not pod validation

admission/webhook/validator.go

github-actions · 2024-06-19T12:11:35Z

Summary:

License scan: success
Credentials scan: failure
Vulnerabilities scan: success
Unit test: success
Go linting: failure

amitschendel · 2024-06-19T12:34:41Z

@dwertent

What are the 'rules'? Why are they not defined as rules, but rather as "if" statements in a large function?

Do you think we should use the rules packages from the node-agent?

How can the user interact with this system?

Which kind of interaction do you expect? rule bindings?

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

github-actions · 2024-06-19T12:53:55Z

Summary:

License scan: success
Credentials scan: failure
Vulnerabilities scan: success
Unit test: success
Go linting: failure

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

github-actions · 2024-06-19T13:05:40Z

Summary:

License scan: success
Credentials scan: failure
Vulnerabilities scan: success
Unit test: success
Go linting: failure

dwertent · 2024-06-20T07:44:11Z

@amitschendel

Do you think we should use the rules packages from the node-agent?

Not really, because here this is a different set of rules, but I will expect to see a similar code structure, there are different restrictions so I don't think there is a need for a Factory design pattern, but I think there should be a package with rules etc.
Consider using the same rule IDs as the regolibrary, meaning the isPrivileged should have the same control ID as we have with compliance scanning. This should be considered, I'm not sure we want it.

Which kind of interaction do you expect? rule bindings?

Possibly. What were the requirements and design for the admission controller?

Also, as I wrote, I'm unfamiliar with the big picture, but I believe we will add rules and possibly block user actions. I don't see how all this can "fit" in the current code structure without the need of refactoring it all in the future, so I will recommend writing the admission controller infrastructure in a way that future modifications and requirements could be met without refactoring. This being said, if the scope here is a POC and not actual product development, I don't see a need to merge it into the main branch.

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

github-actions · 2024-07-02T15:13:01Z

Summary:

License scan: success
Credentials scan: failure
Vulnerabilities scan: success
Unit test: success
Go linting: failure

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

github-actions · 2024-07-04T07:12:06Z

Summary:

License scan: success
Credentials scan: failure
Vulnerabilities scan: success
Unit test: success
Go linting: failure

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

matthyx

fix the conflicts with go.mod
remove github.com/zeebo/assert and use testify instead (as everywhere)
improve some of the test coverage (for example the cache)
I wonder if we should have a component test or something testing the full functionality of the webhook (outside of the system tests that you will add)

matthyx · 2024-07-10T06:55:01Z

admission/rulebinding/cache/cache_test.go

+	"github.com/stretchr/testify/assert"
+)
+
+func TestNewCache(t *testing.T) {


lol, maybe we could add useful tests?

matthyx · 2024-07-10T06:55:30Z

admission/rulebinding/cache/helpers_test.go

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestResourcesToWatch(t *testing.T) {


same here, maybe use real data?

matthyx · 2024-07-10T06:56:57Z

admission/rules/v1/r2001_portforward_test.go

+import (
+	"testing"
+
+	"github.com/zeebo/assert"


why we're not using testify?

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

gitguardian · 2024-07-14T11:35:14Z

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
13049693	Triggered	Generic Private Key	`7c7c360`	admission/webhook/testdata/key.pem	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secret safely. Learn here the best practices.
Revoke and rotate this secret.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

github-actions · 2024-07-14T11:50:26Z

Summary:

License scan: success
Credentials scan: failure
Vulnerabilities scan: success
Unit test: success
Go linting: failure

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

github-actions · 2024-07-15T15:54:37Z

Summary:

License scan: success
Credentials scan: failure
Vulnerabilities scan: success
Unit test: success
Go linting: failure

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

Done

github-actions · 2024-07-16T09:30:39Z

Summary:

License scan: success
Credentials scan: failure
Vulnerabilities scan: success
Unit test: success
Go linting: failure

amitschendel added 6 commits June 18, 2024 13:16

Adding admission pkg

92f58fe

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

Adding support for admission controller in the main server init

931c296

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

Adding admission config

cab0884

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

Adding testing json

b343ce6

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

Adding needed pkgs

92d9d2d

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

Resolved conflicts

e3cf97b

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

amitschendel added 2 commits June 19, 2024 08:23

Bumping go to 1.22

8482107

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

Bumping go to 1.22

172c924

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

amitschendel marked this pull request as ready for review June 19, 2024 08:29

amitschendel mentioned this pull request Jun 19, 2024

Adding helm chart for admission controller kubescape/helm-charts#458

Merged

amitschendel requested review from matthyx and dwertent June 19, 2024 08:43

matthyx requested changes Jun 19, 2024

View reviewed changes

Removing some rules

8435848

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

Changing context to notify context

5fa777a

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

dwertent previously requested changes Jun 19, 2024

View reviewed changes

amitschendel added 2 commits June 19, 2024 12:44

Adding some CR fixes

cf805f7

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

go mod tidy

f4e85a0

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

Fixing exporter to not alert multiple times on alert limit reached

3052023

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

dwertent marked this pull request as draft June 20, 2024 08:56

amitschendel added 2 commits June 22, 2024 10:32

Adding skeleton of bindings

f2dc5d5

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

Adding packages

b51c633

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

amitschendel added 2 commits July 2, 2024 14:01

go mod tidy

104df0f

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

Pretty code

7628a13

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

amitschendel requested review from matthyx and dwertent July 2, 2024 14:10

amitschendel marked this pull request as ready for review July 2, 2024 14:11

amitschendel added 2 commits July 4, 2024 06:54

Adding portforward rule

c3fc344

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

Adding timestamp

3b0d474

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

Fixing timestamp

4c0e92b

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

matthyx requested changes Jul 10, 2024

View reviewed changes

amitschendel added 4 commits July 14, 2024 07:59

Adding tests for cache and helpers

787ccc6

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

Fixing conflicts

32289db

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

Changing import pkg of node-agent

2511183

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

Adding server tests

7c7c360

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

amitschendel requested a review from matthyx July 14, 2024 11:54

matthyx previously approved these changes Jul 15, 2024

View reviewed changes

Fixing conflicts

8d4af97

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

amitschendel dismissed matthyx’s stale review via 8d4af97 July 15, 2024 15:16

amitschendel requested a review from matthyx July 16, 2024 07:27

amitschendel added the release Create release label Jul 16, 2024

Removing comments

f17c936

Signed-off-by: Amit Schendel <amitschendel@gmail.com>

matthyx approved these changes Jul 16, 2024

View reviewed changes

amitschendel merged commit 0ae2750 into main Jul 16, 2024
5 of 7 checks passed

amitschendel deleted the feature/admission-controller branch July 16, 2024 09:27

	logger.L().Ctx(ctx).Error("server stopped", helpers.Error(cancellationReason))
	logger.L().Ctx(ctx).Fatal("server stopped", helpers.Error(cancellationReason))

	cancellationReason := admissionController.Run(serverContext)
	err := admissionController.Run(serverContext)

Feature/admission controller #226

Feature/admission controller #226

Conversation

amitschendel commented Jun 18, 2024 • edited Loading

Overview

github-actions bot commented Jun 18, 2024

github-actions bot commented Jun 19, 2024

matthyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amitschendel Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jun 19, 2024

dwertent left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 19, 2024

amitschendel commented Jun 19, 2024 • edited Loading

github-actions bot commented Jun 19, 2024

github-actions bot commented Jun 19, 2024

dwertent commented Jun 20, 2024 • edited Loading

github-actions bot commented Jul 2, 2024

github-actions bot commented Jul 4, 2024

matthyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitguardian bot commented Jul 14, 2024 • edited Loading

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

github-actions bot commented Jul 14, 2024

github-actions bot commented Jul 15, 2024

github-actions bot commented Jul 16, 2024

amitschendel commented Jun 18, 2024 •

edited

Loading

amitschendel Jun 19, 2024 •

edited

Loading

dwertent left a comment •

edited

Loading

amitschendel commented Jun 19, 2024 •

edited

Loading

dwertent commented Jun 20, 2024 •

edited

Loading

gitguardian bot commented Jul 14, 2024 •

edited

Loading