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

principalOrgId condition for wildcard principal #47

Closed
pju-d2si opened this issue May 10, 2019 · 5 comments
Closed

principalOrgId condition for wildcard principal #47

pju-d2si opened this issue May 10, 2019 · 5 comments

Comments

@pju-d2si
Copy link

Hi!

I am using cfn_nag lastest version 0.3.84, we have a usecase with wildcard on policy
with an sns policy we would authorize any account in our aws organization to publish on it, on our policy we have an condition aws:principalOrgId.
https://aws.amazon.com/fr/blogs/security/control-access-to-aws-resources-by-using-the-aws-organization-of-iam-principals/

We have devellop a small and durty function to catch this condition, my question is

make sense to implement this on model/statements, for add a condition if orgid is specify and value equal with our orgid ?

thank's for your work on cfn_nag and your support

@ghost
Copy link

ghost commented May 10, 2019

Do you want to enforce that any sns topic policy must have "aws:PrincipalOrgID"==xxx?

@pju-d2si
Copy link
Author

pju-d2si commented May 10, 2019

Thank for your reply

Just add condition for check if wildcard for policy (sns policy or another policy)

If condition contain principalorgid
unless "aws:PrincipalOrgID"==xxx
Check wildcard in principal

@ghost
Copy link

ghost commented May 12, 2019

@pju-d2si so your best bet with the existing code base is to write a custom rule. given that XXX is something you'd like to parameterize... there's not really a great way to make that bit easy to change at runtime. I appreciate you reaching out with this use case and I will try to consider a more generic way to support parameterizing rules.

That said, the following code is probably a good bit of the ways toward what you want. To be honest I did not test this code - but if you send me an actual template I will spend the time to test it.

To use this, you would save the code in a file, update xxxxx to the proper value and then you could put the file in a directory and point the cli to use that custom rule directory.

require 'cfn-nag/violation'
require_relative 'base'

class SnsTopicPolicyWildcardPrincipalWithOrgConditionRule < BaseRule
  def rule_text
    'SNS topic policy should not allow * principal unless the PrincipalOrgID is set properly'
  end

  def rule_type
    Violation::FAILING_VIOLATION
  end

  def rule_id
    'F3333'
  end

  def audit_impl(cfn_model)
    org_condition_arr = {
      'StringEquals' => {
        'aws:PrincipalOrgID' => [
          legal_organization
        ]
      }
    }
    org_condition = {
      'StringEquals' => {
        'aws:PrincipalOrgID'=> legal_organization
      }
    }

    violating_topic_policies = cfn_model.resources_by_type('AWS::SNS::TopicPolicy').select do |topic_policy|
      # use the raw version instead of the objectified version which doesn't deal with Condition
      policy_condition = topic_policy.policyDocument['Condition']
      unless [org_condition_arr, org_condition].include?(policy_condition)
        !topic_policy.policy_document.wildcard_allowed_principals.empty?
      end
    end

    violating_topic_policies.map(&:logical_resource_id)
  end

  private

  def legal_organization
    'o-xxxxxx'
  end
end

@pju-d2si
Copy link
Author

thank you,
this requires to modify the standard rules that you have coded, we will continue to overwrite the standard rules pending a more generic integration

Thank you for your help

@ghost
Copy link

ghost commented May 13, 2019

You have a few options for suppressing F18 in favor of this new rule. The easiest path is to put the original rule F18 on a global blacklist, and then add this rule as a custom rule - this will effectively replace F18 with the new rule.

You could also use resource-level suppressions (via metadata) or use the profile concept to only include the rules you care about (and leave out F18).

@ghost ghost closed this as completed May 13, 2019
This issue was closed.
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

No branches or pull requests

1 participant