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

[runtime-security] Execute runtime security functional tests using Kitchen #6328

Merged
merged 24 commits into from Oct 9, 2020

Conversation

lebauce
Copy link
Contributor

@lebauce lebauce commented Sep 7, 2020

What does this PR do?

Execute the runtime security agent functional tests using Kitchen

Motivation

Test the runtime security agent on multiple OS / kernel versions

Copy link
Contributor

@KSerrania KSerrania left a comment

Choose a reason for hiding this comment

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

You may want to make the agent-security team owner of /test/kitchen/site-cookbooks/dd-agent-runtime-security-check/ and /test/kitchen/kitchen-azure-runtime-security-test.yml in the CODEOWNERS file (you can add agent-platform as joint owners if you want to).

.gitlab-ci.yml Outdated Show resolved Hide resolved
albertvaka
albertvaka previously approved these changes Sep 7, 2020
Copy link
Contributor

@albertvaka albertvaka left a comment

Choose a reason for hiding this comment

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

LGTM assuming ^TestMkdir$ does what it seems it does and all paths are correct.

Note that until now on kitchen tests we only cared about the OS they ran on, and not the kernel version. If you want to test the EBPF code against several kernels, you might want to use something more specific than .kitchen_os_centos, .kitchen_os_ubuntu, etc.

But that can be a second iteration, this is much better already than where we were before :D

Edit: +1 to Kylian's comments above ^

@albertvaka albertvaka dismissed their stale review September 7, 2020 12:58

Kylian found something

@lebauce lebauce force-pushed the lebauce/kitchen-functional-tests branch from c8cceb8 to 568a030 Compare September 8, 2020 15:03
@lebauce lebauce requested a review from a team as a code owner September 8, 2020 15:03
@lebauce lebauce force-pushed the lebauce/kitchen-functional-tests branch 9 times, most recently from 9173989 to 2a180b6 Compare September 11, 2020 12:27
@lebauce lebauce force-pushed the lebauce/kitchen-functional-tests branch 7 times, most recently from 034e840 to 222f3ef Compare September 18, 2020 09:52
@lebauce lebauce force-pushed the lebauce/kitchen-functional-tests branch 3 times, most recently from 3ab077e to 7c37116 Compare October 9, 2020 12:59
Copy link

@guedou guedou left a comment

Choose a reason for hiding this comment

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

Nice PR. I made some minor comments.

for i := 0; i != maxEnableRetry; i++ {
if err = m.EnableKprobe(secName, 512); err == nil {
break
func (m *Module) tryEnableKprobe(secName string) error {
Copy link

Choose a reason for hiding this comment

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

Shall we have a metric associated to retries to ease debugging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can discuss it with the team but I don't think this should be part of the PR

package_name 'glibc.i686'
when 'ubuntu', 'debian'
package_name 'libc6-i386'
# when 'suse'
Copy link

Choose a reason for hiding this comment

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

Is there a reason why this is commented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, to not forget to do it when we'll be able to install packages on SUSE

it 'displays PASS and returns 0' do
output = `sudo /tmp/security-agent/testsuite -test.v 1>&2`
retval = $?
expect(retval).to eq(0)
Copy link

Choose a reason for hiding this comment

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

Do you want to also add expect(output).not_to include("FAIL") like in the following describe ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is useful. If one test fails, it will return something different from 0

@lebauce lebauce force-pushed the lebauce/kitchen-functional-tests branch from 7c37116 to ecaab51 Compare October 9, 2020 13:26
.gitlab-ci.yml Outdated
@@ -143,6 +145,9 @@ variables:
.if_not_test_kitchen_deploy: &if_not_test_kitchen_deploy
if: $CI_COMMIT_BRANCH != "master" && $CI_COMMIT_TAG == null && $DEPLOY_AGENT != "true" && $CI_PIPELINE_SOURCE != "web" && $CI_PIPELINE_SOURCE != "api"

.if_test_kitchen_security_agent: &if_test_kitchen_security_agent
Copy link
Contributor

@albertvaka albertvaka Oct 9, 2020

Choose a reason for hiding this comment

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

Maybe if_agent_security_team_label would be more descriptive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not used any more (I wanted to trigger the kitchen tests based on labels on the PR but they are not available). Removed

@lebauce lebauce force-pushed the lebauce/kitchen-functional-tests branch from ecaab51 to 4ef191d Compare October 9, 2020 14:21
@lebauce lebauce merged commit d3ee653 into master Oct 9, 2020
@lebauce lebauce deleted the lebauce/kitchen-functional-tests branch October 9, 2020 18:59
@lebauce lebauce restored the lebauce/kitchen-functional-tests branch October 15, 2020 13:24
@lebauce lebauce deleted the lebauce/kitchen-functional-tests branch October 15, 2020 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants