feat(misconf): Adds CloudFront standard logging v2 support to AVD-AWS-0010#10848
Conversation
Signed-off-by: Aakarsh Maurya <mauryaaakarsh11@gmail.com>
Signed-off-by: Aakarsh Maurya <mauryaaakarsh11@gmail.com>
Signed-off-by: Aakarsh Maurya <mauryaaakarsh11@gmail.com>
Signed-off-by: Aakarsh Maurya <mauryaaakarsh11@gmail.com>
Signed-off-by: Aakarsh Maurya <mauryaaakarsh11@gmail.com>
Signed-off-by: Aakarsh Maurya <mauryaaakarsh11@gmail.com>
Signed-off-by: Aakarsh Maurya <mauryaaakarsh11@gmail.com>
| logType := source.GetAttribute("log_type").AsStringValueOrDefault("", source) | ||
| if logType.Value() != "ACCESS_LOGS" { |
There was a problem hiding this comment.
This can be simplified to:
| logType := source.GetAttribute("log_type").AsStringValueOrDefault("", source) | |
| if logType.Value() != "ACCESS_LOGS" { | |
| if !source.GetAttribute("log_type").Equals("ACCESS_LOGS") { |
| } | ||
| } | ||
|
|
||
| func Test_adaptDistributionV2(t *testing.T) { |
There was a problem hiding this comment.
Could you add a negative case where the delivery source references the distribution and a delivery exists, but log_type is not ACCESS_LOGS (e.g. ERROR_LOGS) → expect false? Right now the log_type filter branch isn't covered, so a regression there would go unnoticed.
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Two cases missing here:
LogTypeother thanACCESS_LOGS→ expect false (covers theLogTypefilter).DeliverySourceNamegiven as!Ref CloudFrontAccessLogsDeliverySource(logical ID) instead of the literal name → expect true.
| for _, delivery := range deliveries { | ||
| deliverySourceName := delivery.GetStringProperty("DeliverySourceName") | ||
| if deliverySourceName.Value() == sourceName.Value() || deliverySourceName.Value() == source.ID() { | ||
| return iacTypes.Bool(true, distribution.Metadata()) |
There was a problem hiding this comment.
The metadata for LoggingV2.Enabled points at the distribution. When the chain is detected it would be more accurate to point it at the aws_cloudwatch_log_delivery resource (the AWS::Logs::Delivery on the CF side), since that's the link that actually completes the chain - a source on its own does not enable logging.
| "$ref": "#/definitions/github.qkg1.top.aquasecurity.trivy.pkg.iac.providers.azure.appservice.Identity" | ||
| }, | ||
| "platform": { | ||
| "resource": { |
There was a problem hiding this comment.
Just flagging that this schema diff in azure.appservice.Service is unrelated to CloudFront. It's correct though — the Service model already uses Resource, so the committed schema was out of sync and the regeneration fixed it. Worth mentioning in the PR description so it doesn't look accidental.
|
Before this is merged, we should verify the updated check works end to end with these model changes by running the integration tests. Since the Rego check lives in trivy-checks and relies on the new |
Co-authored-by: Nikita Pivkin <nikita.pivkin@smartforce.io>
Signed-off-by: Aakarsh Maurya <mauryaaakarsh11@gmail.com>
Signed-off-by: Aakarsh Maurya <mauryaaakarsh11@gmail.com>
Signed-off-by: Aakarsh Maurya <mauryaaakarsh11@gmail.com>
Signed-off-by: Aakarsh Maurya <mauryaaakarsh11@gmail.com>
Signed-off-by: Aakarsh Maurya <mauryaaakarsh11@gmail.com>
|
|
||
| deliveries := modules.GetReferencingResources(source, "aws_cloudwatch_log_delivery", "delivery_source_name") | ||
| if len(deliveries) > 0 { | ||
| return types.Bool(true, metadata) |
There was a problem hiding this comment.
Just as in CF, it's best to return the aws_cloudwatch_log_delivery metadata.
There was a problem hiding this comment.
Until now it was under assumption that merely an existence of a aws_cloudwatch_log_delivery would confirm the V2 to be true, here it didn't reference the specific resource explicitly, so in case at least one resource exists(usual case) should i do return types.Bool(true, deliveries[0].GetMetadata()). Is this the correct approach?
There was a problem hiding this comment.
Yes, that's fine, since there's supposed to be at least one delivery resource here.
There was a problem hiding this comment.
also just a note for both CF and Terraform in case of false, distribution metadata is returned
…eturns true Signed-off-by: Aakarsh Maurya <mauryaaakarsh11@gmail.com>
|
I believe at Trivy-checks certain integration tests are bound to fail if they are checked as intended for now atleast, until the PRs are not merged. I have manually imported schemas, rego is working fine with them. |
nikpivkin
left a comment
There was a problem hiding this comment.
LGTM. I reviewed the adapter changes and verified them e2e together with the corresponding trivy-checks PR aquasecurity/trivy-checks#580.
While testing this I simplified local development of the integration test so unreleased trivy changes can be validated against a locally built binary without building and pushing an image. That is in aquasecurity/trivy-checks#581.
Description
This PR adds v2 logging support to AVD-AWS-0010.
Modified data model to include v2 logging struct
V2 delivery chains detection added to cloudformation and terraform
hasV2Loggingas discussed as a helper to ensure smooth integration with original adapter.types.BoolValueused instead ofiacTypes.BoolValuein Terraform to follow the previous implementations in the file.hasV2Logginghelper at cloudformation as well s a clean solution to ensure minimal disruption to other implementations.getDistributionsAdded respective tests
Additional Notes
Lint fixed a few other lint related issues in the repo as well.
Related issues
Related PRs
feat(misconf): Adds CloudFront standard logging v2 support to AVD-AWS-0010
Checklist