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

Initial other-checks benchmark #431

Merged
merged 37 commits into from
Jul 6, 2022
Merged

Conversation

rajlearner17
Copy link
Contributor

Checklist

  • Issue(s) linked

@rajlearner17 rajlearner17 changed the title Extra checks design2 Initial other-checks benchmark Jun 29, 2022
@rajlearner17 rajlearner17 marked this pull request as draft June 29, 2022 14:49
Base automatically changed from release/v0.37 to main June 30, 2022 06:45
@rajlearner17 rajlearner17 changed the base branch from main to release/v0.38 June 30, 2022 14:58
Copy link
Contributor

@cbruno10 cbruno10 left a comment

Choose a reason for hiding this comment

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

@rajlearner17 I know this PR is still in draft, but I took an initial look as I know we're releasing this week. Please see comments, if any are already changed, please disregard, thanks!

@@ -0,0 +1,44 @@
with all_lb_details as (
select
arn,
Copy link
Contributor

Choose a reason for hiding this comment

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

@rajlearner17 Why is this query using tabs (along with others)?

}

benchmark "other" {
title = "Other"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
title = "Other"
title = "Other Compliance Checks"

@@ -0,0 +1 @@
We are adding additional checks to improve the information gathering around other AWS compliance best practices, these checks are out of the scope of any predefined benchmarks for AWS but we consider them very helpful.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm currently writing some additional content for this doc, I'll add it later today

@khushboo9024 khushboo9024 marked this pull request as ready for review July 6, 2022 05:41
with control_panel_audit_logging as (
select
distinct arn,
log -> 'Types' as log_type
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log -> 'Types' as log_type
log -> 'Types' as log_type

jsonb_array_elements(logging -> 'ClusterLogging') as log
where
log ->> 'Enabled' = 'true'
and (log -> 'Types') @> '["api","audit", "authenticator", "controllerManager","scheduler"]'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
and (log -> 'Types') @> '["api","audit", "authenticator", "controllerManager","scheduler"]'
and (log -> 'Types') @> '["api", "audit", "authenticator", "controllerManager", "scheduler"]'

case
when l.security_groups is null then l.title || ' does not have security group attached.'
when s.num = 0 then l.title || ' does not have outbound rule.'
else l.title || ' have outbound rule(s).'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add how many outbound rules are present?

aws_ec2_classic_load_balancer as c,
jsonb_array_elements(instances) as e
left join aws_ec2_instance as i on i.instance_id = e ->> 'InstanceId'

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

@@ -0,0 +1,20 @@
select
-- Required Columns
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
-- Required Columns
-- Required Columns

Copy link
Contributor

Choose a reason for hiding this comment

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

@khushboo9024 Could we update the alignment of the queries to maintain consistency?

else 'alarm'
end as "status",
case
when encryption_at_rest is not null and encryption_at_rest ->> 'CatalogEncryptionMode' != 'DISABLED' then 'enabled glue data catalog metadata encryption in ' || region
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
when encryption_at_rest is not null and encryption_at_rest ->> 'CatalogEncryptionMode' != 'DISABLED' then 'enabled glue data catalog metadata encryption in ' || region
when encryption_at_rest is not null and encryption_at_rest ->> 'CatalogEncryptionMode' != 'DISABLED' then 'glue data catalog metadata encryption is enabled in ' || region

end as "status",
case
when encryption_at_rest is not null and encryption_at_rest ->> 'CatalogEncryptionMode' != 'DISABLED' then 'enabled glue data catalog metadata encryption in ' || region
else 'disabled glue data catalog metadata encryption in ' || region
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
else 'disabled glue data catalog metadata encryption in ' || region
else 'glue data catalog metadata encryption is disabled in ' || region

else 'alarm'
end as "status",
case
when connection_password_encryption is not null and connection_password_encryption ->> 'ReturnConnectionPasswordEncrypted' != 'false' then 'enabled glue data catalog connection password encryption in ' || region
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make changes as per the above suggestion

else 'alarm'
end as "status",
case
when e is not null and e ->> 'S3EncryptionMode' != 'DISABLED' then d.title || ' s3 encryption enabled.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
when e is not null and e ->> 'S3EncryptionMode' != 'DISABLED' then d.title || ' s3 encryption enabled.'
when e is not null and e ->> 'S3EncryptionMode' != 'DISABLED' then d.title || ' S3 encryption enabled.'

end as "status",
case
when e is not null and e ->> 'S3EncryptionMode' != 'DISABLED' then d.title || ' s3 encryption enabled.'
else d.title || ' s3 encryption disabled.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
else d.title || ' s3 encryption disabled.'
else d.title || ' S3 encryption disabled.'

else 'alarm'
end as status,
case
when e is not null and e ->> 'S3EncryptionMode' != 'DISABLED' then j.title || ' enabled s3 encryption.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update the reason to follow a single pattern as mentioned in -
https://github.com/turbot/steampipe-mod-aws-compliance/pull/431/files#r914552035

else 'ok'
end as status,
case
when fu.user_id is not null then u.name || ' custom policies allows STS Role assumption.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
when fu.user_id is not null then u.name || ' custom policies allows STS Role assumption.'
when fu.user_id is not null then u.name || ' custom policies allow STS Role assumption.'

end as status,
case
when au.user_id is null then u.name || ' does not have administrator access.'
when au.user_id is not null and u.mfa_enabled then u.name || ' have MFA token enabled.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
when au.user_id is not null and u.mfa_enabled then u.name || ' have MFA token enabled.'
when au.user_id is not null and u.mfa_enabled then u.name || ' has MFA token enabled.'

@cbruno10 cbruno10 merged commit 44ae3ee into release/v0.38 Jul 6, 2022
@cbruno10 cbruno10 deleted the extra-checks-design2 branch July 6, 2022 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants