Skip to content

Commit

Permalink
Merge pull request #276 from jfrog/GH-272-fix-malicious-package-can-b…
Browse files Browse the repository at this point in the history 
…e-set-to-false-with-min-severity

Fix validation for malicious_package and min_severity
  • Loading branch information
@alexhung
alexhung authored Nov 19, 2024
2 parents 20d0059 + aef6562 commit 3b3a0ab
Show file tree
Hide file tree
Showing 5 changed files with 192 additions and 50 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down
37 changes: 24 additions & 13 deletions pkg/xray/resource/resource_xray_security_policy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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(
Expand All @@ -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"),
Expand Down
198 changes: 164 additions & 34 deletions pkg/xray/resource/resource_xray_security_policy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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"),
},
},
})
Expand All @@ -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")
Expand Down Expand Up @@ -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.*"),
},
})
}
},
})
}
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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"
Expand All @@ -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 }}
Expand Down Expand Up @@ -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 }}"
Expand Down

0 comments on commit 3b3a0ab

Please sign in to comment.