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

Implemented validation logic for the webhook #593

Merged

Conversation

sharmapulkit04
Copy link

  • Created a single Validate() function to validate both updating and creating Jenkins CR.
  • Implemented the Validate function to fetch warnings from the API and do security check if
    being enabled.

Changes

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide for more details.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS.

Release Notes

Describe any user facing changes here, or delete this block.

Examples of user facing changes:
- API changes
- Bug fixes
- Any changes in behavior

* Use grep -c flag in check for changes step to fix case when more than 1 website file was modified
@sharmapulkit04 sharmapulkit04 force-pushed the security-validator branch 2 times, most recently from 4027061 to 1725824 Compare July 8, 2021 06:49
Copy link
Collaborator

@prryb prryb left a comment

Choose a reason for hiding this comment

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

Hey, thanks for contributing! I have left some comments.

@@ -41,21 +48,115 @@ var _ webhook.Validator = &Jenkins{}

// ValidateCreate implements webhook.Validator so a webhook will be registered for the type
func (in *Jenkins) ValidateCreate() error {
jenkinslog.Info("validate create", "name", in.Name)
if in.Spec.ValidateSecurityWarnings {
jenkinslog.Info("validate create", "name", in.Name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these logs useful in any way?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah for debugging purposes I was checking if the function is being called and get a look the the reconciller object.

return nil
}

// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
func (in *Jenkins) ValidateUpdate(old runtime.Object) error {
jenkinslog.Info("validate update", "name", in.Name)
if in.Spec.ValidateSecurityWarnings {
jenkinslog.Info("validate update", "name", in.Name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these logs useful in any way?

api/v1alpha2/jenkins_webhook.go Outdated Show resolved Hide resolved
lastVersion = pluginversion // setting default value in case of empty string
}

if pluginversion >= firstVersion && pluginversion <= lastVersion {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the correct way to compare plugin versions?

Copy link
Author

Choose a reason for hiding this comment

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

No, apparently it's not, I sent a message in the group, it's not the correct way to compare semantic versions, I am working on rectifying the algorithm.

api/v1alpha2/jenkins_webhook.go Outdated Show resolved Hide resolved
client := &http.Client{}
request, err := http.NewRequest("GET", pluginURL, nil)
if err != nil {
log.Fatal("unable to create a new request", err.Error())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is log.Fatal the right choice here? It will call os.Exit(1) which exits the whole process. That might not be the wise thing to do.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah you are right it should return an error failing creating a new object but the operator should be able to run.
Thanks for the insightful comments, I will work on it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No problem! Thanks for contributing! 🙂

Comment on lines 158 to 191
func Validate(r Jenkins) error {
basePlugins := plugins.BasePlugins()

for _, plugin := range basePlugins {
name := plugin.Name
version := plugin.Version
hasWarnings, err := CheckSecurityWarnings(name, version)
if err != nil {
return err
}
if hasWarnings {
errormsg := "Security Vulnerabilities detected in base plugin:" + name
return errors.New(errormsg)
}
}

for _, plugin := range r.Spec.Master.Plugins {
name := plugin.Name
version := plugin.Version
hasWarnings, err := CheckSecurityWarnings(name, version)
if err != nil {
return err
}
if hasWarnings {
errormsg := "Security Vulnerabilities detected in the user defined plugin: " + name
return errors.New(errormsg)
}
}

return nil
}

Choose a reason for hiding this comment

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

If you look closely, you are returning with the first warning you encountered, so if there's more than 1 warning you're skipping it. It would be better if you were appending warnings and leave only when you iterate through all the plugins. Right now you wouldn't even start checking user plugins. Please add some unit tests. For those cases too, eg. 2 warnings in user plugins and 2 warnings in user plugins and check if you've gotten them all.

Copy link
Author

Choose a reason for hiding this comment

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

Okay sure, thanks for the suggestion, I can work on that, for unit tests, I will be able to work on it after first phase evaluation, right now I am facing some issues with helm tests, so I am devoting my time there.

Choose a reason for hiding this comment

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

Okay :) You can always write to us about those problems.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah right now, there is some problem with webhook configuration, we can discuss about it in tomorrow's meet if I will stuck by then.

@sharmapulkit04 sharmapulkit04 force-pushed the security-validator branch 3 times, most recently from 1acd52b to 8b19135 Compare July 13, 2021 20:14
kind: Issuer
metadata:
name: selfsigned
namespace: {{ .Values.jenkins.namespace }}

Choose a reason for hiding this comment

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

Suggested change
namespace: {{ .Values.jenkins.namespace }}
namespace: {{ .Release.Namespace }

kind: Certificate
metadata:
name: webhook-certificate
namespace: {{ .Values.jenkins.namespace }}

Choose a reason for hiding this comment

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

Suggested change
namespace: {{ .Values.jenkins.namespace }}
namespace: {{ .Release.Namespace }}

Webhooks should be used per Operator, not per Jenkins and we recommend deploying Operator in separate namespace from Jenkins for better security :)

Makefile Outdated
@@ -96,7 +96,7 @@ e2e: deepcopy-gen manifests ## Runs e2e tests, you can use EXTRA_ARGS

.PHONY: helm-e2e
IMAGE_NAME := $(DOCKER_REGISTRY):$(GITCOMMIT)
helm-e2e: helm container-runtime-build ## Runs helm e2e tests, you can use EXTRA_ARGS
helm-e2e: helm install-cert-manager container-runtime-build ## Runs helm e2e tests, you can use EXTRA_ARGS

Choose a reason for hiding this comment

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

Suggested change
helm-e2e: helm install-cert-manager container-runtime-build ## Runs helm e2e tests, you can use EXTRA_ARGS
helm-e2e: helm install-cert-manager container-runtime-build ## Runs helm e2e tests, you can use EXTRA_ARGS

Also please add a TODO to refactor the logic for deploying cert manager in the helm charts later :)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah sure.

chart/jenkins-operator/templates/webhook.yaml Show resolved Hide resolved
kind: Service
metadata:
name: webhook-service
namespace: {{ .Values.jenkins.namespace }}

Choose a reason for hiding this comment

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

Suggested change
namespace: {{ .Values.jenkins.namespace }}
namespace: {{ .Release.Namespace }}

@sharmapulkit04 sharmapulkit04 force-pushed the security-validator branch 2 times, most recently from cadcf9d to 9659042 Compare July 14, 2021 20:30
- Created a single Validate() function to validate both updating and creating Jenkins CR.
- Implemented the Validate function to fetch warnings from the API and do security check if
  being enabled.
- Updated the helm charts and helm-e2e target to run the helm tests.
Sig00rd and others added 11 commits July 16, 2021 11:45
* Explanation what's backed up and why
Co-authored-by: prryb <prryb@users.noreply.github.com>
Co-authored-by: Sig00rd <Sig00rd@users.noreply.github.com>
…late (jenkinsci#483)

Add GitLFS pull after checkout behaviour to support also repositories which are relying on Git LFS

Close jenkinsci#482
* Link to project's DockerHub in README's section on nightly builds, add paragraph about nightly builds in installation docs

* Fix repositoryURL in sample seedJob configuration with SSH auth

* Slightly expand on jenkinsci#348

* Fix formatting in docs on Jenkins' customization, update plugin versions

* Add notes on Jenkins home Volume in Helm chart values.yaml and docs (jenkinsci#589)
Co-authored-by: Sig00rd <Sig00rd@users.noreply.github.com>
- Reimplemented the validator interface
- Updated manifests to allocate more resources
Sig00rd and others added 3 commits August 5, 2021 17:27
…kinsci#612)

* Update note in installation docs

* Update Helm chart default values.yaml

* Update schema
Co-authored-by: Sig00rd <Sig00rd@users.noreply.github.com>
// Validates security warnings for both updating and creating a Jenkins CR
func Validate(r Jenkins) error {
pluginset := make(map[string]PluginData)
var faultybaseplugins string

Choose a reason for hiding this comment

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

Suggested change
var faultybaseplugins string
var faultyBasePlugins string

func Validate(r Jenkins) error {
pluginset := make(map[string]PluginData)
var faultybaseplugins string
var faultyuserplugins string

Choose a reason for hiding this comment

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

Suggested change
var faultyuserplugins string
var faultyUserPlugins string

…o security-validator

- Refactored code in webhook and main
- Merged changes from master
- Optimized the charts
- Made the webhook optional
- Added cert manager as dependency to be installed while running webhook
@sharmapulkit04 sharmapulkit04 force-pushed the security-validator branch 4 times, most recently from d877bba to 9c820c0 Compare August 20, 2021 06:35
- Completed helm tests for various scenarios
- Disabled startupapi check for cert manager webhook, defined a secret and updated templates
- Made the webhook completely optional
PluginDataFile string
IsCached bool
Attempts int
SleepTime int

Choose a reason for hiding this comment

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

Suggested change
SleepTime int
SleepTime time.Duration

Comment on lines 189 to 197
func NewPluginsDataManager(hosturl string, compressedFilePath string, pluginDataFile string, isCached bool, timeout time.Duration) *PluginDataManager {
return &PluginDataManager{
Hosturl: hosturl,
CompressedFilePath: compressedFilePath,
PluginDataFile: pluginDataFile,
IsCached: isCached,
Timeout: timeout,
}
}

Choose a reason for hiding this comment

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

Suggested change
func NewPluginsDataManager(hosturl string, compressedFilePath string, pluginDataFile string, isCached bool, timeout time.Duration) *PluginDataManager {
return &PluginDataManager{
Hosturl: hosturl,
CompressedFilePath: compressedFilePath,
PluginDataFile: pluginDataFile,
IsCached: isCached,
Timeout: timeout,
}
}
func NewPluginsDataManager(hosturl string, compressedFilePath string, pluginDataFile string, isCached bool, timeout time.Duration, sleepTime time.Duration) *PluginDataManager {
return &PluginDataManager{
Hosturl: hosturl,
CompressedFilePath: compressedFilePath,
PluginDataFile: pluginDataFile,
IsCached: isCached,
Timeout: timeout,
sleepTime: sleepTime,
}
}

Choose a reason for hiding this comment

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

Also move the Hosturl to const in this file. It is a const not a variable.

Copy link
Author

Choose a reason for hiding this comment

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

But we are setting sleeptime it to 1 and 12 by default in the function so what's the point of setting it anyways.

var isDownloaded, isExtracted, isCached bool
var err error
for in.Attempts = 0; in.Attempts < 5; in.Attempts++ {
err = in.download()

Choose a reason for hiding this comment

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

You're losing the errors. Please print it in debug mode, like
if err != nil { in.logger.(V.Debug).Info(err)}
isDowloaded = true
break

it's a pattern to check if the error is nil first and later do something if it's not.

api/v1alpha2/jenkins_webhook.go Show resolved Hide resolved
if isDownloaded {
for in.Attempts = 0; in.Attempts < 5; in.Attempts++ {
err = in.extract()
if err == nil {

Choose a reason for hiding this comment

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

as above. Check if the error is present and log it. Later execute the rest of the logic.

if isExtracted {
for in.Attempts = 0; in.Attempts < 5; in.Attempts++ {
err = in.cache()
if err == nil {

Choose a reason for hiding this comment

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

as above.

Comment on lines 242 to 252
if !in.IsCached {
isInitialized <- isCached
in.IsCached = isCached
}

if isCached {
in.SleepTime = 12
} else {
in.SleepTime = 1
}
time.Sleep(time.Duration(in.SleepTime) * time.Hour)

Choose a reason for hiding this comment

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

Suggested change
if !in.IsCached {
isInitialized <- isCached
in.IsCached = isCached
}
if isCached {
in.SleepTime = 12
} else {
in.SleepTime = 1
}
time.Sleep(time.Duration(in.SleepTime) * time.Hour)
if !in.IsCached {
isInitialized <- isCached
in.IsCached = isCached
in.sleepTime = 1
}
in.sleepTime = 12
time.Sleep(in.sleepTime)

Copy link
Author

Choose a reason for hiding this comment

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

in.IsCached defines that whether we have cached the data file or not,it will always be true once we successfully cached for the first time, isCached defines our current case, so if we are updating our cache and it fails we still are going to run the function after 1 hour.

Namespace: namespace.Name,
},
}
It("Deploys Jenkins operator with webhook enabled along with the default jenkins image", func() {

Choose a reason for hiding this comment

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

Suggested change
It("Deploys Jenkins operator with webhook enabled along with the default jenkins image", func() {
Context("when deploying Helm Chart to cluster", func() {
It("Deploys Jenkins operator with webhook", func() {

Choose a reason for hiding this comment

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

Also I think actually that this test case is for when there is no webhook?

"--set-string", fmt.Sprintf("operator.image=%s", *imageName), "--install")
output, err := cmd.CombinedOutput()
Expect(err).NotTo(HaveOccurred(), string(output))
It("Deploys Jenkins operator along with webhook and cert-manager", func() {

Choose a reason for hiding this comment

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

Add context like "when deploying the operator and jenkins with security warnings validation enabled". what is being done in the test commands. and later there is 'It' so the desired outcome.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah sure,actually I am doing several cases in this it Spec because then I have to redeploy all the resources because of namespace issues


Expect(err).NotTo(HaveOccurred(), string(output))
By("Denies a create request for a Jenkins custom resource with some plugins having security warnings and validation is turned on")

Choose a reason for hiding this comment

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

Suggested change
By("Denies a create request for a Jenkins custom resource with some plugins having security warnings and validation is turned on")
By("Denying the create request if [...etc]")

Choose a reason for hiding this comment

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

Keep it simple. Above is the whole context when this is happening

@sharmapulkit04 sharmapulkit04 force-pushed the security-validator branch 3 times, most recently from 5d435a5 to 13d0665 Compare August 21, 2021 23:18
@@ -5,7 +5,7 @@ Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0
http://www.apache.org/lictenses/LICENSE-2.0

Choose a reason for hiding this comment

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

Please revert this

Suggested change
http://www.apache.org/lictenses/LICENSE-2.0
http://www.apache.org/licenses/LICENSE-2.0

Comment on lines 28 to 30

//"github.com/jenkinsci/kubernetes-operator/pkg/configuration/base/resources"

Choose a reason for hiding this comment

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

Suggested change
//"github.com/jenkinsci/kubernetes-operator/pkg/configuration/base/resources"

Don't leave commented parts in code.

if err == nil {
break
}
jenkinslog.V(1).Info("Cache Plugin Data", "failed to download file", err)

Choose a reason for hiding this comment

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

Suggested change
jenkinslog.V(1).Info("Cache Plugin Data", "failed to download file", err)
jenkinslog.V(log.VDebug).Info("Cache Plugin Data", "failed to download file", err)

We have a constant to not use magic numbers.

if err == nil {
break
}
jenkinslog.V(1).Info("Cache Plugin Data", "failed to extract file", err)

Choose a reason for hiding this comment

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

Suggested change
jenkinslog.V(1).Info("Cache Plugin Data", "failed to extract file", err)
jenkinslog.V(log.VDebug).Info("Cache Plugin Data", "failed to extract file", err)

if err == nil {
break
}
jenkinslog.V(1).Info("Cache Plugin Data", "failed to read plugin data file", err)

Choose a reason for hiding this comment

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

Suggested change
jenkinslog.V(1).Info("Cache Plugin Data", "failed to read plugin data file", err)
jenkinslog.V(log.VDebug).Info("Cache Plugin Data", "failed to read plugin data file", err)

@SylwiaBrant SylwiaBrant merged commit 34c9ee3 into jenkinsci:security-validator Aug 23, 2021
SylwiaBrant pushed a commit that referenced this pull request Aug 30, 2021
* Fix workflow for autogenerating docs (#592)

* Use grep -c flag in check for changes step to fix case when more than 1 website file was modified

* Implemented validation logic for the webhook
- Created a single Validate() function to validate both updating and creating Jenkins CR.
- Implemented the Validate function to fetch warnings from the API and do security check if
  being enabled.
- Updated the helm charts and helm-e2e target to run the helm tests.

* Configure bot for labelling new issues as needing triage (#597)

* Configure bot for managing stale issues (#598)

* Docs: explanation what is backed up and why (#599)

* Explanation what's backed up and why

* Auto-updated docs (#600)

Co-authored-by: prryb <prryb@users.noreply.github.com>

* Docs: clarification of description of get latest command in backup (#601)

* Auto-updated docs (#602)

Co-authored-by: Sig00rd <Sig00rd@users.noreply.github.com>

* Bump seedjobs agent image version to 4.9-1 (#604)

* Add GitLFS pull after checkout behaviour to SeedJob GroovyScript Template (#483)

Add GitLFS pull after checkout behaviour to support also repositories which are relying on Git LFS

Close #482

* Docs: minor fixes (#608)

* Link to project's DockerHub in README's section on nightly builds, add paragraph about nightly builds in installation docs

* Fix repositoryURL in sample seedJob configuration with SSH auth

* Slightly expand on #348

* Fix formatting in docs on Jenkins' customization, update plugin versions

* Add notes on Jenkins home Volume in Helm chart values.yaml and docs (#589)

* Auto-updated docs (#610)

Co-authored-by: Sig00rd <Sig00rd@users.noreply.github.com>

* Reimplemented the validation logic with caching the security warnings
- Reimplemented the validator interface
- Updated manifests to allocate more resources

* Add an issue template for documentation (#613)

* Docs: add info on restricted volumeMounts other than jenkins-home(#612)

* Update note in installation docs

* Update Helm chart default values.yaml

* Update schema

* Auto-updated docs (#616)

Co-authored-by: Sig00rd <Sig00rd@users.noreply.github.com>

* Auto-updated docs (#617)

Co-authored-by: Sig00rd <Sig00rd@users.noreply.github.com>

* Updated Validation logic
- Defined a security manager struct to cache all the plugin data
- Added flag to make validating security warnings optional while deploying the operator

* Helm Chart: Remove empty priorityClassName from Jenkins template (#618)

Also bump Helm Chart version to v0.5.2

* Added unit test cases for webhook

* Updated Helm Charts
- Optimized the charts
- Made the webhook optional
- Added cert manager as dependency to be installed while running webhook

* Updated unit tests, helm charts and validation logic

* Completed helm e2e tests and updated helm charts
- Completed helm tests for various scenarios
- Disabled startupapi check for cert manager webhook, defined a secret and updated templates
- Made the webhook completely optional

* Code optimization and cleanup

* Modified helm tests

* code cleanup and optimization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants