From 311eb06376a6395ee3c9a514353b6f0ba003b2c8 Mon Sep 17 00:00:00 2001 From: Alex Hung Date: Mon, 18 Nov 2024 14:53:18 -0800 Subject: [PATCH 1/2] Fix validation for malicious_package and min_severity --- .../resource/resource_xray_security_policy.go | 37 ++-- .../resource_xray_security_policy_test.go | 198 +++++++++++++++--- 2 files changed, 188 insertions(+), 47 deletions(-) diff --git a/pkg/xray/resource/resource_xray_security_policy.go b/pkg/xray/resource/resource_xray_security_policy.go index 3a8a50f1..88ea5c4b 100644 --- a/pkg/xray/resource/resource_xray_security_policy.go +++ b/pkg/xray/resource/resource_xray_security_policy.go @@ -4,7 +4,6 @@ import ( "context" "regexp" - "github.com/hashicorp/terraform-plugin-framework-validators/boolvalidator" "github.com/hashicorp/terraform-plugin-framework-validators/float64validator" "github.com/hashicorp/terraform-plugin-framework-validators/listvalidator" "github.com/hashicorp/terraform-plugin-framework-validators/setvalidator" @@ -388,15 +387,7 @@ var securityPolicyCriteriaAttrs = map[string]schema.Attribute{ Description: "Mark to skip CVEs that are not applicable in the context of the artifact. The contextual analysis operation might be long and affect build time if the `fail_build` action is set.\n\n~>Only supported by JFrog Advanced Security", }, "malicious_package": schema.BoolAttribute{ - Optional: true, - Validators: []validator.Bool{ - boolvalidator.ConflictsWith( - path.MatchRelative().AtParent().AtName("min_severity"), - ), - boolvalidator.ConflictsWith( - path.MatchRelative().AtParent().AtName("cvss_range"), - ), - }, + Optional: true, Description: "Generating a violation on a malicious package.", }, "vulnerability_ids": schema.SetAttribute{ @@ -495,11 +486,27 @@ func (r SecurityPolicyResource) ValidateConfig(ctx context.Context, req resource attrs := criteria.Elements()[0].(types.Object).Attributes() fixVersionDependant := attrs["fix_version_dependant"].(types.Bool).ValueBool() + minSeverity := attrs["min_severity"].(types.String).ValueString() + cvssRange := attrs["cvss_range"].(types.List) maliciousPackage := attrs["malicious_package"].(types.Bool).ValueBool() - packageName := attrs["package_name"].(types.String) - packagType := attrs["package_type"].(types.String) - packageVersions := attrs["package_versions"].(types.Set) + if maliciousPackage && minSeverity != "" { + resp.Diagnostics.AddAttributeError( + path.Root("rules").AtSetValue(rule).AtName("criteria").AtSetValue(criteria.Elements()[0]).AtName("malicious_package"), + "Invalid Attribute Configuration", + "malicious_package cannot be set to 'true' if min_severity is set", + ) + return + } + + if maliciousPackage && !cvssRange.IsNull() { + resp.Diagnostics.AddAttributeError( + path.Root("rules").AtSetValue(rule).AtName("criteria").AtSetValue(criteria.Elements()[0]).AtName("malicious_package"), + "Invalid Attribute Configuration", + "malicious_package cannot be set to 'true' if cvss_range is set", + ) + return + } if maliciousPackage && fixVersionDependant { resp.Diagnostics.AddAttributeError( @@ -510,6 +517,10 @@ func (r SecurityPolicyResource) ValidateConfig(ctx context.Context, req resource return } + packageName := attrs["package_name"].(types.String) + packagType := attrs["package_type"].(types.String) + packageVersions := attrs["package_versions"].(types.Set) + if fixVersionDependant && (!packageName.IsNull() || !packagType.IsNull() || !packageVersions.IsNull()) { resp.Diagnostics.AddAttributeError( path.Root("rules").AtSetValue(rule).AtName("criteria").AtSetValue(criteria.Elements()[0]).AtName("fix_version_dependant"), diff --git a/pkg/xray/resource/resource_xray_security_policy_test.go b/pkg/xray/resource/resource_xray_security_policy_test.go index 1f03bd39..c09a8281 100644 --- a/pkg/xray/resource/resource_xray_security_policy_test.go +++ b/pkg/xray/resource/resource_xray_security_policy_test.go @@ -567,7 +567,7 @@ func TestAccSecurityPolicy_createMaliciousPackageFail(t *testing.T) { }) } -func TestAccSecurityPolicy_createMaliciousPackageCvssMinSeverityFail(t *testing.T) { +func TestAccSecurityPolicy_createMaliciousPackageFailMinSeverity(t *testing.T) { _, fqrn, resourceName := testutil.MkNames("policy-", "xray_security_policy") testData := sdk.MergeMaps(testDataSecurity) @@ -582,8 +582,30 @@ func TestAccSecurityPolicy_createMaliciousPackageCvssMinSeverityFail(t *testing. ProtoV6ProviderFactories: acctest.ProtoV6ProviderFactories, Steps: []resource.TestStep{ { - Config: util.ExecuteTemplate(fqrn, securityPolicyCVSSMinSeverityMaliciousPkg, testData), - ExpectError: regexp.MustCompile("(?s).*Invalid Attribute Combination.*cvss_range.*cannot be specified when.*malicious_package.*is specified.*"), + Config: util.ExecuteTemplate(fqrn, securityPolicyMaliciousPkgMinSeverityDep, testData), + ExpectError: regexp.MustCompile("malicious_package cannot be set to 'true' if min_severity is set"), + }, + }, + }) +} + +func TestAccSecurityPolicy_createCvssMaliciousPackageFail(t *testing.T) { + _, fqrn, resourceName := testutil.MkNames("policy-", "xray_security_policy") + testData := sdk.MergeMaps(testDataSecurity) + + testData["resource_name"] = resourceName + testData["policy_name"] = fmt.Sprintf("terraform-security-policy-6-%d", testutil.RandomInt()) + testData["rule_name"] = fmt.Sprintf("test-security-rule-6-%d", testutil.RandomInt()) + testData["malicious_package"] = "true" + testData["min_severity"] = "High" + + resource.Test(t, resource.TestCase{ + CheckDestroy: acctest.VerifyDeleted(fqrn, "", acctest.CheckPolicy), + ProtoV6ProviderFactories: acctest.ProtoV6ProviderFactories, + Steps: []resource.TestStep{ + { + Config: util.ExecuteTemplate(fqrn, securityPolicyCVSSMaliciousPkg, testData), + ExpectError: regexp.MustCompile("malicious_package cannot be set to 'true' if cvss_range is set"), }, }, }) @@ -604,13 +626,35 @@ func TestAccSecurityPolicy_createCvssMinSeverityFail(t *testing.T) { ProtoV6ProviderFactories: acctest.ProtoV6ProviderFactories, Steps: []resource.TestStep{ { - Config: util.ExecuteTemplate(fqrn, securityPolicyCVSSMinSeverityMaliciousPkg, testData), + Config: util.ExecuteTemplate(fqrn, securityPolicyCVSSMinSeverity, testData), ExpectError: regexp.MustCompile("(?s).*Invalid Attribute Combination.*cvss_range.*cannot be specified when.*min_severity.*is specified.*"), }, }, }) } +func TestAccSecurityPolicy_createCvssVulnerabilityIDsFail(t *testing.T) { + _, fqrn, resourceName := testutil.MkNames("policy-", "xray_security_policy") + testData := sdk.MergeMaps(testDataSecurity) + + testData["resource_name"] = resourceName + testData["policy_name"] = fmt.Sprintf("terraform-security-policy-6-%d", testutil.RandomInt()) + testData["rule_name"] = fmt.Sprintf("test-security-rule-6-%d", testutil.RandomInt()) + testData["malicious_package"] = "false" + testData["vulnerability_ids"] = "High" + + resource.Test(t, resource.TestCase{ + CheckDestroy: acctest.VerifyDeleted(fqrn, "", acctest.CheckPolicy), + ProtoV6ProviderFactories: acctest.ProtoV6ProviderFactories, + Steps: []resource.TestStep{ + { + Config: util.ExecuteTemplate(fqrn, securityPolicyCVSSVulnerabilityIDs, testData), + ExpectError: regexp.MustCompile("(?s).*Invalid Attribute Combination.*cvss_range.*cannot be specified when.*vulnerability_ids.*is specified.*"), + }, + }, + }) +} + // Min severity criteria, allow downloading of unscanned and active func TestAccSecurityPolicy_createBlockDownloadFalseMinSeverity(t *testing.T) { _, fqrn, resourceName := testutil.MkNames("policy-", "xray_security_policy") @@ -773,43 +817,37 @@ func TestAccSecurityPolicy_vulnerabilityIdsIncorrectCVEFails(t *testing.T) { } } -func TestAccSecurityPolicy_conflictingAttributesFail(t *testing.T) { +func TestAccSecurityPolicy_exporsureConflictingAttributesFail(t *testing.T) { _, fqrn, resourceName := testutil.MkNames("policy-", "xray_security_policy") testData := sdk.MergeMaps(testDataSecurity) - testAttributes := []string{ + conflictingAttributes := []string{ "vulnerability_ids = [\"CVE-2022-12345\", \"CVE-2021-67890\", \"XRAY-1234\"]", "cvss_range {\nfrom = 1 \nto = 3\n}", - "malicious_package = true", + "malicious_package = false", "min_severity = \"High\"", - "exposures {\nmin_severity = \"High\" \nsecrets = true \n applications = true \n services = true \n iac = true\n}", } - for _, testAttribute := range testAttributes { - for _, conflictingAttribute := range testAttributes { - if testAttribute == conflictingAttribute { - continue - } - testData["resource_name"] = resourceName - testData["policy_name"] = fmt.Sprintf("terraform-security-policy-9-%d", testutil.RandomInt()) - testData["rule_name"] = fmt.Sprintf("test-security-rule-9-%d", testutil.RandomInt()) - testData["block_unscanned"] = "false" - testData["block_active"] = "false" - testData["test_attribute"] = testAttribute - testData["malicious_package"] = "true" - testData["conflicting_attribute"] = conflictingAttribute - - resource.Test(t, resource.TestCase{ - CheckDestroy: acctest.VerifyDeleted(fqrn, "", acctest.CheckPolicy), - ProtoV6ProviderFactories: acctest.ProtoV6ProviderFactories, - Steps: []resource.TestStep{ - { - Config: util.ExecuteTemplate(fqrn, securityPolicyVulnIdsConflict, testData), - ExpectError: regexp.MustCompile("(?s).*Invalid Attribute Combination.*cvss_range.*cannot be specified when.*vulnerability_ids.*is specified.*"), - }, + for _, conflictingAttribute := range conflictingAttributes { + testData["resource_name"] = resourceName + testData["policy_name"] = fmt.Sprintf("terraform-security-policy-9-%d", testutil.RandomInt()) + testData["rule_name"] = fmt.Sprintf("test-security-rule-9-%d", testutil.RandomInt()) + testData["block_unscanned"] = "false" + testData["block_active"] = "false" + testData["malicious_package"] = "false" + testData["exposures"] = "{\nmin_severity = \"High\" \nsecrets = true \n applications = true \n services = true \n iac = true\n}" + testData["conflicting_attribute"] = conflictingAttribute + + resource.Test(t, resource.TestCase{ + CheckDestroy: acctest.VerifyDeleted(fqrn, "", acctest.CheckPolicy), + ProtoV6ProviderFactories: acctest.ProtoV6ProviderFactories, + Steps: []resource.TestStep{ + { + Config: util.ExecuteTemplate(fqrn, securityPolicyVulnIdsConflict, testData), + ExpectError: regexp.MustCompile("(?s).*Invalid Attribute Combination.*cvss_range.*cannot be specified when.*vulnerability_ids.*is specified.*"), }, - }) - } + }, + }) } } @@ -1150,7 +1188,13 @@ const securityPolicyVulnIdsConflict = `resource "xray_security_policy" "{{ .reso name = "{{ .rule_name }}" priority = 1 criteria { - {{ .test_attribute }} + exposures { + min_severity = "High" + secrets = true + applications = true + services = true + iac = true + } {{ .conflicting_attribute }} } actions { @@ -1249,7 +1293,7 @@ const securityPolicyTwoRules = `resource "xray_security_policy" "{{ .resource_na } }` -const securityPolicyCVSSMinSeverityMaliciousPkg = `resource "xray_security_policy" "{{ .resource_name }}" { +const securityPolicyCVSSMinSeverity = `resource "xray_security_policy" "{{ .resource_name }}" { name = "{{ .policy_name }}" description = "{{ .policy_description }}" type = "security" @@ -1258,6 +1302,65 @@ const securityPolicyCVSSMinSeverityMaliciousPkg = `resource "xray_security_polic priority = 1 criteria { min_severity = "{{ .min_severity }}" + cvss_range { + from = {{ .cvss_from }} + to = {{ .cvss_to }} + } + } + actions { + block_release_bundle_distribution = {{ .block_release_bundle_distribution }} + block_release_bundle_promotion = {{ .block_release_bundle_promotion }} + fail_build = {{ .fail_build }} + notify_watch_recipients = {{ .notify_watch_recipients }} + notify_deployer = {{ .notify_deployer }} + create_ticket_enabled = {{ .create_ticket_enabled }} + build_failure_grace_period_in_days = {{ .grace_period_days }} + block_download { + unscanned = {{ .block_unscanned }} + active = {{ .block_active }} + } + } + } +}` + +const securityPolicyCVSSVulnerabilityIDs = `resource "xray_security_policy" "{{ .resource_name }}" { + name = "{{ .policy_name }}" + description = "{{ .policy_description }}" + type = "security" + rule { + name = "{{ .rule_name }}" + priority = 1 + criteria { + vulnerability_ids = ["CVE-2022-12345", "CVE-2021-67890", "XRAY-1234"] + cvss_range { + from = {{ .cvss_from }} + to = {{ .cvss_to }} + } + } + actions { + block_release_bundle_distribution = {{ .block_release_bundle_distribution }} + block_release_bundle_promotion = {{ .block_release_bundle_promotion }} + fail_build = {{ .fail_build }} + notify_watch_recipients = {{ .notify_watch_recipients }} + notify_deployer = {{ .notify_deployer }} + create_ticket_enabled = {{ .create_ticket_enabled }} + build_failure_grace_period_in_days = {{ .grace_period_days }} + block_download { + unscanned = {{ .block_unscanned }} + active = {{ .block_active }} + } + } + } +}` + +const securityPolicyCVSSMaliciousPkg = `resource "xray_security_policy" "{{ .resource_name }}" { + name = "{{ .policy_name }}" + description = "{{ .policy_description }}" + type = "security" + rule { + name = "{{ .rule_name }}" + priority = 1 + criteria { malicious_package = {{ .malicious_package }} cvss_range { from = {{ .cvss_from }} @@ -1392,6 +1495,33 @@ const securityPolicyMaliciousPkgFixVersionDep = `resource "xray_security_policy" } }` +const securityPolicyMaliciousPkgMinSeverityDep = `resource "xray_security_policy" "{{ .resource_name }}" { + name = "{{ .policy_name }}" + description = "{{ .policy_description }}" + type = "security" + rule { + name = "{{ .rule_name }}" + priority = 1 + criteria { + malicious_package = "{{ .malicious_package }}" + min_severity = "{{ .min_severity }}" + } + actions { + block_release_bundle_distribution = {{ .block_release_bundle_distribution }} + block_release_bundle_promotion = {{ .block_release_bundle_promotion }} + fail_build = {{ .fail_build }} + notify_watch_recipients = {{ .notify_watch_recipients }} + notify_deployer = {{ .notify_deployer }} + create_ticket_enabled = {{ .create_ticket_enabled }} + build_failure_grace_period_in_days = {{ .grace_period_days }} + block_download { + unscanned = {{ .block_unscanned }} + active = {{ .block_active }} + } + } + } +}` + const securityPolicyPackagesFixVersionDep = `resource "xray_security_policy" "{{ .resource_name }}" { name = "{{ .policy_name }}" description = "{{ .policy_description }}" From aef65627d5c92771327a0d068ed1e0c5f6568812 Mon Sep 17 00:00:00 2001 From: Alex Hung Date: Tue, 19 Nov 2024 10:15:19 -0800 Subject: [PATCH 2/2] Update CHANGELOG Downgrade shared module so we can continue to use Resty 2.15.3 --- CHANGELOG.md | 1 + go.mod | 2 +- go.sum | 4 ++-- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a5adb353..c5dff4a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ BREAKING CHANGES: BUG FIXES: * resource/xray_license_policy: Switch `allowed_licenses` and `banned_licenses` attribute type from `TypeSet` to `TypeList` to resolve performance issue with large number of licenses. Add validation to ensure `allowed_licenses` and `banned_licenses` attributes cannot be set at the same time. Issue: [#262](https://github.com/jfrog/terraform-provider-xray/issues/262) and [#271](https://github.com/jfrog/terraform-provider-xray/issues/271) PR: [#274](https://github.com/jfrog/terraform-provider-xray/issues/274) +* resource/xray_security_policy: Fix validation not allowing `malicious_package` set to `false` when `min_severity` is set. Issue: [#272](https://github.com/jfrog/terraform-provider-xray/issues/272) PR: [#276](https://github.com/jfrog/terraform-provider-xray/issues/276) ## 2.13.2 (November 11, 2024). Tested on Artifactory 7.98.8 and Xray 3.104.18 with Terraform 1.9.8 and OpenTofu 1.8.5 diff --git a/go.mod b/go.mod index 6e4b4136..83f87cc9 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,7 @@ require ( github.com/hashicorp/terraform-plugin-framework-validators v0.15.0 github.com/hashicorp/terraform-plugin-go v0.25.0 github.com/hashicorp/terraform-plugin-testing v1.10.0 - github.com/jfrog/terraform-provider-shared v1.27.0 + github.com/jfrog/terraform-provider-shared v1.26.0 github.com/samber/lo v1.47.0 golang.org/x/exp v0.0.0-20230809150735-7b3493d9a819 golang.org/x/text v0.20.0 diff --git a/go.sum b/go.sum index 2ed22725..1bc591c7 100644 --- a/go.sum +++ b/go.sum @@ -130,8 +130,8 @@ github.com/imdario/mergo v0.3.15 h1:M8XP7IuFNsqUx6VPK2P9OSmsYsI/YFaGil0uD21V3dM= github.com/imdario/mergo v0.3.15/go.mod h1:WBLT9ZmE3lPoWsEzCh9LPo3TiwVN+ZKEjmz+hD27ysY= github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99 h1:BQSFePA1RWJOlocH6Fxy8MmwDt+yVQYULKfN0RoTN8A= github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99/go.mod h1:1lJo3i6rXxKeerYnT8Nvf0QmHCRC1n8sfWVwXF2Frvo= -github.com/jfrog/terraform-provider-shared v1.27.0 h1:ivXga2hsXnIJF/gevPZyXXqDm82v5PQ3sy/x3kDk+nA= -github.com/jfrog/terraform-provider-shared v1.27.0/go.mod h1:LpjcFuDzCW5+gdQs1LAjgMESKuYd3ZqJazoIGt0uv9Q= +github.com/jfrog/terraform-provider-shared v1.26.0 h1:xfJfKcgejlFkIyo6VLJPzNtEVfbTYIiGKD2PWysdgw4= +github.com/jfrog/terraform-provider-shared v1.26.0/go.mod h1:IPwXN48K3uzJNDmT2x6zFGa5IS0KG2AK7jnQR2H4G1A= github.com/jhump/protoreflect v1.15.1 h1:HUMERORf3I3ZdX05WaQ6MIpd/NJ434hTp5YiKgfCL6c= github.com/jhump/protoreflect v1.15.1/go.mod h1:jD/2GMKKE6OqX8qTjhADU1e6DShO+gavG9e0Q693nKo= github.com/kevinburke/ssh_config v1.2.0 h1:x584FjTGwHzMwvHx18PXxbBVzfnxogHaAReU4gf13a4=