Add validation rule to ensure security capability depending on security-rule assets by mrodm · Pull Request #820 · elastic/package-spec

mrodm · 2024-10-17T13:43:46Z

What does this PR do?

Add a new validation rule to ensure that security capability is added in the manifest if there is any security-rule asset (in Kibana folder).

Additional changes:

Update junit plugin up to 2.5.0, so skipped tests can be notified too.
Ensure each junit step reports using the required files.
- Depending on the time spent in compliance tests, latest junit step could also download XML files from those compliance tests. Example of this: https://buildkite.com/elastic/package-spec/builds/857

Why is it important?

Currently, there are just two packages using security-rule assets. Both packages have defined the security capability.

This PR ensures that packages containing this kind of security-rule assets are not listed as available to be installed in Observability Serverless projects.

Checklist

I have added test packages to test/packages that prove my change is effective.
I have added an entry in spec/changelog.yml.

Related issues

Closes [Enhancement] Add a new validation rule to ensure capability security is defined #809

This validation rule checks if there is any security-rule asset in kibana folder. If there is any, then security capability must be defined as part of the package manifest.

mrodm · 2024-10-17T14:12:05Z

test integrations

elastic-vault-github-plugin-prod · 2024-10-17T14:19:55Z

Created or updated PR in integrations repository to test this version. Check elastic/integrations#11453

mrodm · 2024-10-17T14:34:02Z

/test

This version allows us to report also skipped tests.

mrodm · 2024-10-17T15:28:55Z

.buildkite/pipeline.yml

-          - junit-annotate#v2.4.1:
-              artifacts: "*.xml"
+          - junit-annotate#v2.5.0:
+              artifacts: "build/test-results/*.xml"


Ensures that this step just downloads and process the XML files from the unit test steps (linux and windows).

mrodm · 2024-10-17T15:44:02Z

code/go/internal/validator/semantic/validate_capabilities_required.go

+	if !slices.Contains(capabilities, "security") {
+		return specerrors.ValidationErrors{
+			specerrors.NewStructuredErrorf("found security rule assets in package but security capability is missing in package manifest"),
+		}
+	}


Should we check that none of the observability capabilities are defined too?https://github.com/elastic/kibana/blob/77102c1854bba7253da5363168e8aafb66a34579/config/serverless.oblt.yml#L145

As this validation is defined now, it just enforces to be present security capability.

Something like:

if slices.Contains(capabilities, "observability") || slices.Contains(capabilities, "apm") || slices.Contains(capabilities, "uptime") { return specerrors.ValidationErrors{ specerrors.NewStructuredErrorf("found security rule assets in package but observability capabilities are defined in package manifest"), } }

The issue here is if those capabilities are modified in Kibana or a new one is added.

Or maybe ensure that just one capability is defined, and that one is "security" ?
Currently, it is just allowed "security":
https://github.com/elastic/kibana/blob/6f4346e4e6f61cf67db50fbe7ce0836c0317cc09/config/serverless.security.yml#L77

That would need to update good_v2 and good_v3 packages (manifests) to just have security capability.

It would be enough with checking that it contains the security capability. If the package has security assets and the deployment has the security capability, it doesn't matter if it also has the observability capability.
If there were a type of project with both capabilities, a package with the security capability should work there, as well as a package with both capabilities.

Perfect, I agree with that too. Thanks!

mrodm · 2024-10-17T16:29:31Z

/test

jsoriano · 2024-10-17T17:16:35Z

code/go/internal/validator/semantic/validate_capabilities_required.go

+	if !slices.Contains(capabilities, "security") {
+		return specerrors.ValidationErrors{
+			specerrors.NewStructuredErrorf("found security rule assets in package but security capability is missing in package manifest"),
+		}
+	}


It would be enough with checking that it contains the security capability. If the package has security assets and the deployment has the security capability, it doesn't matter if it also has the observability capability.
If there were a type of project with both capabilities, a package with the security capability should work there, as well as a package with both capabilities.

jsoriano · 2024-10-17T17:20:30Z

code/go/internal/validator/spec.go

 		{fn: semantic.ValidateKibanaFilterPresent, since: semver.MustParse("3.0.0")},
 		{fn: semantic.ValidateKibanaNoLegacyVisualizations, types: []string{"integration"}, since: semver.MustParse("3.0.0")},
 		{fn: semantic.ValidateDimensionsPresent, types: []string{"integration"}, since: semver.MustParse("3.0.1")},
+		{fn: semantic.ValidateCapabilitiesRequired, since: semver.MustParse("2.10.0")}, // capabilities definition was added in spec version 2.10.0


Using the version where capabilities were introduced looks good to me, but we don't necessarily have to use this version, we could introduce the check on a later version. I guess it will depend if there are packages currently failing with this check.

I set that version to ensure that the expected capabilities are added into the manifest since the first version were those fields were introduced into the manifest (2.10.0).

We could also start testing this validation from 3.0.0. That spec was the minimum one set for Serverless projects. For instance, for observability:
https://github.com/elastic/kibana/blob/68b3267ca2f76d8de4fedef622608d8685e7eceb/config/serverless.security.yml#L78

The current packages defining security-rule assets are:

security_detection_engine: defines spec version 3.0.0

tychon: defines spec version 3.2.2

Currently, there are no other packages failing in integrations because of that.
Do you think it would be better to set since: 3.0.0 to match with versioning used in Serverless rather than 2.10.0?

jsoriano · 2024-10-17T17:22:55Z

code/go/pkg/validator/validator_test.go

 	}
 }

+func TestValidateCapabilitiesRequried(t *testing.T) {


Would it be possible to make this work as part of TestValidateFile? This could be reported as a failure int the manifest.yml file, as the problem would be a missing capability there.

Sure!
I've added the file manifest path into the message error to show that the manifest is invalid. That allows me to move the test for the capabilities to "TestValidateFile".

Added in d16f9fd

mrodm · 2024-10-21T10:32:19Z

test integrations

elastic-vault-github-plugin-prod · 2024-10-21T10:37:13Z

Created or updated PR in integrations repository to test this version. Check elastic/integrations#11453

elasticmachine · 2024-10-21T10:39:44Z

💚 Build Succeeded

Buildkite Build
Commit: d16f9fd

History

💚 Build #864 succeeded 47708d4
💔 Build #863 failed 47708d4
💚 Build #862 succeeded 7dce364
💚 Build #861 succeeded 31316a7
💔 Build #860 failed 0e66179
💔 Build #857 failed 707afa7

cc @mrodm

mrodm added 2 commits October 17, 2024 15:39

Add new validation rule to ensure security capability

82787bb

This validation rule checks if there is any security-rule asset in kibana folder. If there is any, then security capability must be defined as part of the package manifest.

Add test package without security capability

790685a

mrodm self-assigned this Oct 17, 2024

mrodm changed the title ~~Add security rule validation check~~ Add validation rule to ensure security capability depending on security-rule assets Oct 17, 2024

Add changelog entry

707afa7

elastic-vault-github-plugin-prod bot mentioned this pull request Oct 17, 2024

Test branch package-spec#820 - DO NOT MERGE elastic/integrations#11453

Closed

mrodm added 4 commits October 17, 2024 16:44

Avoid downloading compliance xml files

0e66179

Preprocess windown unit test results

31316a7

Update junit plugin to 2.5.0

8f823db

This version allows us to report also skipped tests.

Remove duplicated upload command

7dce364

mrodm commented Oct 17, 2024

View reviewed changes

mrodm marked this pull request as ready for review October 17, 2024 15:45

mrodm requested a review from a team as a code owner October 17, 2024 15:45

Restore some changes

47708d4

jsoriano reviewed Oct 21, 2024

View reviewed changes

Add manifest file path to error as invalid

d16f9fd

mrodm requested a review from jsoriano October 21, 2024 13:36

jsoriano approved these changes Oct 22, 2024

View reviewed changes

mrodm merged commit 4d60460 into elastic:main Oct 23, 2024

mrodm deleted the add_security_rule_validation_check branch October 23, 2024 09:20

Conversation

mrodm commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Why is it important?

Checklist

Related issues

Uh oh!

mrodm commented Oct 17, 2024

Uh oh!

elastic-vault-github-plugin-prod bot commented Oct 17, 2024

Uh oh!

mrodm commented Oct 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrodm commented Oct 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrodm Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrodm commented Oct 21, 2024

Uh oh!

elastic-vault-github-plugin-prod bot commented Oct 21, 2024

Uh oh!

elasticmachine commented Oct 21, 2024

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mrodm commented Oct 17, 2024 •

edited

Loading

mrodm Oct 21, 2024 •

edited

Loading