Add AWS hardware MFA matcher #2892

pwelch · 2018-03-28T15:53:04Z

Adding a hardware as well as a virtual MFA matcher for aws_iam_root_user
resource

Signed-off-by: Paul Welch [email protected]

Adding a hardware as well as a virtual MFA matcher for aws_iam_root_user resource Fixes #2750 Signed-off-by: Paul Welch <[email protected]>

clintoncwolfe

A great start! I suggest a couple of renames, and I found a logic error. And we need docs! Great unit tests, though - this is coming along nicely!

clintoncwolfe · 2018-03-28T16:38:22Z

test/unit/resources/aws_iam_root_user_test.rb

@@ -44,4 +44,52 @@ def test_has_mfa_enabled_returns_false_when_account_mfa_devices_is_zero

    assert_equal false, AwsIamRootUser.new(@mock_conn).has_mfa_enabled?
  end
-end
+
+  def test_has_virtual_mfa_enabled_returns_true_when_account_vmfa_devices_is_one


💯 for meaningful unit test names

clintoncwolfe · 2018-03-28T16:45:13Z

lib/resources/aws/aws_iam_root_user.rb

@@ -46,6 +46,20 @@ def has_mfa_enabled?
    summary_account['AccountMFAEnabled'] == 1
  end

+  def has_virtual_mfa_devices?


So, it turns out that even though the API returns an array, you can only have one MFA device total per root account or IAM user (see FAQ, 'Q. Can I have multiple authentication devices active for my AWS account?').

Based on that, I think we should rename this matcher to be has_virtual_mfa_enabled?.

clintoncwolfe · 2018-03-28T16:46:06Z

lib/resources/aws/aws_iam_root_user.rb

+    false
+  end
+
+  def has_hardware_mfa_devices?


Likewise, this should be renamed has_hardware_mfa_enabled?.

clintoncwolfe · 2018-03-28T16:57:15Z

lib/resources/aws/aws_iam_root_user.rb

@@ -46,6 +46,20 @@ def has_mfa_enabled?
    summary_account['AccountMFAEnabled'] == 1
  end

+  def has_virtual_mfa_devices?
+    virtual_mfa_devices.each do |device|
+      if %r{arn:aws:iam::\d{12}:mfa\/root-account-mfa-device} =~


Might want to add a comment here - if the root account has a VMFA, it will have this special serial number, ending in 'root-account-mfa-device'.

clintoncwolfe · 2018-03-28T16:57:47Z

lib/resources/aws/aws_iam_root_user.rb

@@ -57,4 +71,8 @@ def summary_account
      @summary_account ||= @client.get_account_summary.summary_map
    end
  end
+
+  def virtual_mfa_devices
+    @__virtual_devices ||= @client.list_virtual_mfa_devices.virtual_mfa_devices


Need to wrap this in a catch_aws_errors block.

clintoncwolfe · 2018-03-28T17:04:40Z

lib/resources/aws/aws_iam_root_user.rb

+  end
+
+  def has_hardware_mfa_devices?
+    @__hardware_devices ||= [email protected]_mfa_devices.mfa_devices.empty?


Alas, this isn't the right logic. list_mfa_devices will list mfa devices for a particular user, or the currently authenticated user - which would be the test harness user, not the root user. You can't pass the username 'root', either:

[cwolfe@lodi inspec-review]$ aws --profile inspec-aws-test-default iam list-mfa-devices --user-name root An error occurred (NoSuchEntity) when calling the ListMFADevices operation: The user with name root cannot be found.

So that's no fun. However, the CIS PDF tells us that if the account has MFA enabled (which we know from get_account_summary, and we have a predicate for it in has_mfa_enabled?) BUT has no virtual MFA device (which you have added a predicate for), then it must have a hardware MFA. So I think this can simply be:

has_mfa_enabled? && !has_virtual_mfa_enabled?

clintoncwolfe · 2018-03-28T17:06:33Z

test/unit/resources/aws_iam_root_user_test.rb

+
+    assert_equal false, AwsIamRootUser.new(@mock_conn).has_hardware_mfa_devices?
+  end
+end


One missing piece: you need to add the two new matchers to the docs, at docs/resources/aws_iam_root_account.md.erb. Thanks!

pwelch · 2018-03-28T21:28:41Z

@clintoncwolfe Thanks for the feedback! Pushed up changes.

- Add documentation for new root MFA matchers - Fix logic for checking MFA devices from feedback on PR Signed-off-by: Paul Welch <[email protected]>

TrevorBramble · 2018-03-29T17:21:19Z

lib/resources/aws/aws_iam_root_user.rb

+      end
+    end
+    false
+  end


How about extracting the pattern to an explaining variable and using Enumerable#any?() instead?

# if the root account has a Virtual MFA device then it will have a special # serial number ending in 'root-account-mfa-device' def has_virtual_mfa_enabled? mfa_device_pattern = %r{arn:aws:iam::\d{12}:mfa\/root-account-mfa-device} virtual_mfa_devices.any? { |device| mfa_device_pattern =~ device['serial_number'] } end

TrevorBramble · 2018-03-29T17:22:43Z

test/unit/resources/aws_iam_root_user_test.rb

+        serial_number: "arn:aws:iam::123456789011:mfa/root-account-mfa-device",
+        user: Aws::IAM::Types::User.new(
+          user_id: "123456789011",
+          arn:     "arn:aws:iam::123456789011:root",


I think we're aligning single quotes in this project?

clintoncwolfe · 2018-03-29T18:47:49Z

I'm happy with the docs and the code changes. One last thing, could you addd some integration tests? Thanks!

- Add integration tests for virtual and hardware MFA matchers - Clean up logic for has_virtual_mfa_enabled? method Signed-off-by: Paul Welch <[email protected]>

pwelch · 2018-03-29T21:54:37Z

@clintoncwolfe integration tests added.

jquick

Thanks @pwelch !

Add AWS hardware MFA matcher

38347e2

Adding a hardware as well as a virtual MFA matcher for aws_iam_root_user resource Fixes #2750 Signed-off-by: Paul Welch <[email protected]>

pwelch requested a review from clintoncwolfe March 28, 2018 15:53

pwelch requested a review from a team as a code owner March 28, 2018 15:53

clintoncwolfe requested changes Mar 28, 2018

View reviewed changes

Add New AWS Root Matcher Docs

ac6119b

- Add documentation for new root MFA matchers - Fix logic for checking MFA devices from feedback on PR Signed-off-by: Paul Welch <[email protected]>

pwelch force-pushed the pw/add-hardware-mfa-matcher branch from 5f18c54 to ac6119b Compare March 29, 2018 14:55

TrevorBramble suggested changes Mar 29, 2018

View reviewed changes

Add Integration tests for MFA matchers

0d2870e

- Add integration tests for virtual and hardware MFA matchers - Clean up logic for has_virtual_mfa_enabled? method Signed-off-by: Paul Welch <[email protected]>

TrevorBramble approved these changes Mar 29, 2018

View reviewed changes

clintoncwolfe approved these changes Apr 2, 2018

View reviewed changes

jquick approved these changes Apr 3, 2018

View reviewed changes

jquick added the Type: Enhancement Improves an existing feature label Apr 3, 2018

jquick merged commit 2720311 into master Apr 3, 2018

jquick deleted the pw/add-hardware-mfa-matcher branch April 3, 2018 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AWS hardware MFA matcher #2892

Add AWS hardware MFA matcher #2892

pwelch commented Mar 28, 2018

clintoncwolfe left a comment

clintoncwolfe Mar 28, 2018

clintoncwolfe Mar 28, 2018

pwelch Mar 28, 2018

clintoncwolfe Mar 28, 2018

clintoncwolfe Mar 28, 2018

clintoncwolfe Mar 28, 2018

pwelch Mar 28, 2018

clintoncwolfe Mar 28, 2018

clintoncwolfe Mar 28, 2018

pwelch commented Mar 28, 2018

TrevorBramble Mar 29, 2018

TrevorBramble Mar 29, 2018

clintoncwolfe commented Mar 29, 2018

pwelch commented Mar 29, 2018

jquick left a comment

Add AWS hardware MFA matcher #2892

Add AWS hardware MFA matcher #2892

Conversation

pwelch commented Mar 28, 2018

clintoncwolfe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwelch commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintoncwolfe commented Mar 29, 2018

pwelch commented Mar 29, 2018

jquick left a comment

Choose a reason for hiding this comment