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.

Sign up for GitHub

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

Jump to bottom

feat(aws/cis): fix config query #325

Merged
merged 2 commits into from
Nov 26, 2021
Merged

feat(aws/cis): fix config query #325

merged 2 commits into from
Nov 26, 2021

Conversation

ecktom
Copy link
Contributor

@ecktom ecktom commented Nov 22, 2021

This is a follow-up of #279 and also fixes #26.

@rajlearner17
Copy link
Contributor

@ecktom appreciate the PR, we are checking to get it in. Thank you

@bigdatasourav bigdatasourav changed the base branch from main to release/v0.21 November 24, 2021 06:30
@rajlearner17
Copy link
Contributor

@ecktom thanks for the updated query, help me in understanding this. As per CIS v 1.4.0 (3.5)

2. Evaluate the output to ensure that there's at least one recorder for which
recordingGroup object includes "allSupported": true AND
"includeGlobalResourceTypes": true

Along with the above point, below must be also satisfying

4. In the output, find recorders with name key matching the recorders that met the criteria
in step 2. Ensure that at least one of them includes "recording": true and
"lastStatus": "SUCCESS"

Our interpretation was, whichever (at least one) region IncludeGlobalResourceTypes = true and same region must be recording as well i.e. "recording": true then, we consider them to be a satisfying condition.

See the image as attached (executed by the modified query in this PR), ap-south-1 is enabled with IncludeGlobalResourceTypes but the Recording is disabled

ap-south-1 **IncludeGlobalResourceTypes enabled**, AllSupported enabled, Recording disabled and LastStatus is SUCCESS
Should we still consider moving us-east-1 to OK? let me know your thoughts. This is interesting one like many others too, appreciate tracking it
OK us-east-1 IncludeGlobalResourceTypes disabled, AllSupported enabled, Recording enabled and LastStatus is SUCCESS.
image

@bigdatasourav bigdatasourav deleted the branch turbot:release/v0.22 November 24, 2021 10:07
@khushboo9024 khushboo9024 reopened this Nov 24, 2021
@khushboo9024 khushboo9024 changed the base branch from release/v0.21 to main November 24, 2021 11:22
@ecktom
Copy link
Contributor Author

ecktom commented Nov 25, 2021

Hi @rajlearner17, Thanks for looking into this!

So this again is something which is not very well defined within the CIS Benchmark. Based on the Audit/Remediation I think it would be fine if you have a single region fulfilling:

allSupported=true
includeGlobalResourceTypes=true
recording=true
lastStatus=SUCCESS

Thus we could put this control on an OK state, if we only found a single region on which this combination applies, even though all other regions might be fully misconfigured.

However, the Control also clearly states "It is recommended AWS Config be enabled in all regions". From my point of view, logically, this implies that we should also check for at leat recording=true and lastStatus=SUCCESS even though this is not clearly defined in their Remediation.

I agree that we might want to make allSupported an optional thing.

So more like

with global_recorders as (
  select
    count(*) as global_config_recorders
  from
    aws_config_configuration_recorder
  where
    recording_group -> 'IncludeGlobalResourceTypes' = 'true'
    and recording_group -> 'AllSupported' = 'true'
 )
 select
  -- Required columns
  'arn:aws::' || a.region || ':' || a.account_id as resource,
  case
    when
      g.global_config_recorders >= 1
      and status ->> 'Recording' = 'true'
      and status ->> 'LastStatus' = 'SUCCESS'
    then 'ok'
    else 'alarm'

What do you think?

@rajlearner17
Copy link
Contributor

@ecktom I think the suggestion is appropriate to freeze our interpretation for the time being, not sure if any other edge cases can come up to check further, pls take a look below query and share us your feedback

-- pgFormatter-ignore
-- Get count for any region with all matching criteria
with global_recorders as (
  select
    count(*) as global_config_recorders
  from
    aws_config_configuration_recorder
  where
    recording_group -> 'IncludeGlobalResourceTypes' = 'true'
    and recording_group -> 'AllSupported' = 'true'
    and status ->> 'Recording' = 'true'
    and status ->> 'LastStatus' = 'SUCCESS'
 )
select
  -- Required columns
  'arn:aws::' || a.region || ':' || a.account_id as resource,
  case
  -- When any of the region satisfies with above CTE
  -- In left join of <aws_config_configuration_recorder> table, regions now having
  -- 'Recording' and 'LastStatus' matching criteria can be considered as OK
    when
      g.global_config_recorders >= 1
      and status ->> 'Recording' = 'true'
      and status ->> 'LastStatus' = 'SUCCESS'
    then 'ok'
    else 'alarm'
  end as status,
  -- Below cases are for citing respective reasons for control state
  case
    when recording_group -> 'IncludeGlobalResourceTypes' = 'true' then a.region || ' IncludeGlobalResourceTypes enabled,'
    else a.region || ' IncludeGlobalResourceTypes disabled,'
  end ||
  case
    when recording_group -> 'AllSupported' = 'true' then ' AllSupported enabled,'
    else ' AllSupported disabled,'
  end ||
  case
    when status ->> 'Recording' = 'true' then ' Recording enabled'
    else ' Recording disabled'
  end ||
  case
    when status ->> 'LastStatus' = 'SUCCESS' then ' and LastStatus is SUCCESS.'
    else ' and LastStatus is not SUCCESS.'
  end as reason,
  -- Additional columns
  a.region,
  a.account_id
from
  global_recorders as g,
  aws_region as a
  left join aws_config_configuration_recorder as r
    on r.account_id = a.account_id and r.region = a.name;

@ecktom
Copy link
Contributor Author

ecktom commented Nov 25, 2021

@rajlearner17 your query looks good and should do the trick. I also checked it in our environment and it worked as expected 👍

@khushboo9024 khushboo9024 changed the base branch from main to release/v0.22 November 26, 2021 06:13
@khushboo9024
Copy link
Contributor

Output:

with global_recorders as (
  select
    count(*) as global_config_recorders
  from
    aws_config_configuration_recorder
  where
    recording_group -> 'IncludeGlobalResourceTypes' = 'true'
    and recording_group -> 'AllSupported' = 'true'
    and status ->> 'Recording' = 'true'
    and status ->> 'LastStatus' = 'SUCCESS'
 )
select
  -- Required columns
  'arn:aws::' || a.region || ':' || a.account_id as resource,
  case
  -- When any of the region satisfies with above CTE
  -- In left join of <aws_config_configuration_recorder> table, regions now having
  -- 'Recording' and 'LastStatus' matching criteria can be considered as OK
    when
      g.global_config_recorders >= 1
      and status ->> 'Recording' = 'true'
      and status ->> 'LastStatus' = 'SUCCESS'
    then 'ok'
    else 'alarm'
  end as status,
  -- Below cases are for citing respective reasons for control state
  case
    when recording_group -> 'IncludeGlobalResourceTypes' = 'true' then a.region || ' IncludeGlobalResourceTypes enabled,'
    else a.region || ' IncludeGlobalResourceTypes disabled,'
  end ||
  case
    when recording_group -> 'AllSupported' = 'true' then ' AllSupported enabled,'
    else ' AllSupported disabled,'
  end ||
  case
    when status ->> 'Recording' = 'true' then ' Recording enabled'
    else ' Recording disabled'
  end ||
  case
    when status ->> 'LastStatus' = 'SUCCESS' then ' and LastStatus is SUCCESS.'
    else ' and LastStatus is not SUCCESS.'
  end as reason,
  -- Additional columns
  a.region,
  a.account_id
from
  global_recorders as g,
  aws_region as a
  left join aws_config_configuration_recorder as r
    on r.account_id = a.account_id and r.region = a.name;
+--------------------------------------+--------+------------------------------------------------------------------------------------------------------------------------------+----------------+-----------
| resource                             | status | reason                                                                                                                       | region         | account_id
+--------------------------------------+--------+------------------------------------------------------------------------------------------------------------------------------+----------------+-----------
| arn:aws::us-west-1:9123456789      | alarm  | us-west-1 IncludeGlobalResourceTypes disabled, AllSupported disabled, Recording disabled and LastStatus is not SUCCESS.      | us-west-1      | 235678967
| arn:aws::eu-west-2:9123456789      | alarm  | eu-west-2 IncludeGlobalResourceTypes disabled, AllSupported disabled, Recording disabled and LastStatus is not SUCCESS.      | eu-west-2      | 235678967
| arn:aws::eu-north-1:9123456789     | alarm  | eu-north-1 IncludeGlobalResourceTypes disabled, AllSupported disabled, Recording disabled and LastStatus is not SUCCESS.     | eu-north-1     | 235678967
| arn:aws::af-south-1:9123456789     | alarm  | af-south-1 IncludeGlobalResourceTypes disabled, AllSupported disabled, Recording disabled and LastStatus is not SUCCESS.     | af-south-1     | 235678967
| arn:aws::eu-central-1:9123456789   | alarm  | eu-central-1 IncludeGlobalResourceTypes disabled, AllSupported disabled, Recording disabled and LastStatus is not SUCCESS.   | eu-central-1   | 235678967
| arn:aws::us-east-1:9123456789      | ok     | us-east-1 IncludeGlobalResourceTypes disabled, AllSupported enabled, Recording enabled and LastStatus is SUCCESS.            | us-east-1      | 235678967
| arn:aws::us-east-2:9123456789      | alarm  | us-east-2 IncludeGlobalResourceTypes disabled, AllSupported disabled, Recording disabled and LastStatus is not SUCCESS.      | us-east-2      | 235678967
| arn:aws::ap-northeast-3:9123456789 | alarm  | ap-northeast-3 IncludeGlobalResourceTypes disabled, AllSupported disabled, Recording disabled and LastStatus is not SUCCESS. | ap-northeast-3 | 235678967
| arn:aws::eu-south-1:9123456789     | alarm  | eu-south-1 IncludeGlobalResourceTypes disabled, AllSupported disabled, Recording disabled and LastStatus is not SUCCESS.     | eu-south-1     | 235678967
| arn:aws::us-west-2:9123456789      | alarm  | us-west-2 IncludeGlobalResourceTypes disabled, AllSupported disabled, Recording disabled and LastStatus is not SUCCESS.      | us-west-2      | 235678967
| arn:aws::eu-west-1:9123456789      | alarm  | eu-west-1 IncludeGlobalResourceTypes disabled, AllSupported disabled, Recording disabled and LastStatus is not SUCCESS.      | eu-west-1      | 235678967
| arn:aws::sa-east-1:9123456789      | alarm  | sa-east-1 IncludeGlobalResourceTypes disabled, AllSupported disabled, Recording disabled and LastStatus is not SUCCESS.      | sa-east-1      | 235678967
| arn:aws::ap-east-1:9123456789      | alarm  | ap-east-1 IncludeGlobalResourceTypes disabled, AllSupported disabled, Recording disabled and LastStatus is not SUCCESS.      | ap-east-1      | 235678967
| arn:aws::ap-northeast-2:9123456789 | alarm  | ap-northeast-2 IncludeGlobalResourceTypes disabled, AllSupported disabled, Recording disabled and LastStatus is not SUCCESS. | ap-northeast-2 | 235678967
| arn:aws::ap-southeast-1:9123456789 | alarm  | ap-southeast-1 IncludeGlobalResourceTypes disabled, AllSupported disabled, Recording disabled and LastStatus is not SUCCESS. | ap-southeast-1 | 235678967
| arn:aws::ap-southeast-2:9123456789 | alarm  | ap-southeast-2 IncludeGlobalResourceTypes disabled, AllSupported disabled, Recording disabled and LastStatus is not SUCCESS. | ap-southeast-2 | 235678967
| arn:aws::me-south-1:9123456789     | alarm  | me-south-1 IncludeGlobalResourceTypes disabled, AllSupported disabled, Recording disabled and LastStatus is not SUCCESS.     | me-south-1     | 235678967
| arn:aws::eu-west-3:9123456789      | alarm  | eu-west-3 IncludeGlobalResourceTypes disabled, AllSupported disabled, Recording disabled and LastStatus is not SUCCESS.      | eu-west-3      | 235678967
| arn:aws::ap-south-1:9123456789     | ok     | ap-south-1 IncludeGlobalResourceTypes enabled, AllSupported enabled, Recording enabled and LastStatus is SUCCESS.            | ap-south-1     | 235678967
| arn:aws::ca-central-1:9123456789   | alarm  | ca-central-1 IncludeGlobalResourceTypes disabled, AllSupported disabled, Recording disabled and LastStatus is not SUCCESS.   | ca-central-1   | 235678967
| arn:aws::ap-northeast-1:9123456789 | alarm  | ap-northeast-1 IncludeGlobalResourceTypes disabled, AllSupported disabled, Recording disabled and LastStatus is not SUCCESS. | ap-northeast-1 | 235678967
+--------------------------------------+--------+------------------------------------------------------------------------------------------------------------------------------+----------------+-----------

rajlearner17
rajlearner17 approved these changes Nov 26, 2021
View reviewed changes
Copy link
Contributor

@rajlearner17 rajlearner17 left a comment

Choose a reason for hiding this comment

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

Looks good to release

@rajlearner17 rajlearner17 merged commit b6e0313 into turbot:release/v0.22 Nov 26, 2021
@rajlearner17 rajlearner17 mentioned this pull request Dec 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rajlearner17 rajlearner17 rajlearner17 approved these changes

@khushboo9024 khushboo9024 Awaiting requested review from khushboo9024

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect report for CIS 3.5 (AWS Config)
4 participants
@ecktom @rajlearner17 @khushboo9024 @bigdatasourav