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

feat: Allow multiple MFA devices and users to manage MFA devices #313

Merged
merged 8 commits into from
Jan 19, 2023
Merged

feat: Allow multiple MFA devices and users to manage MFA devices #313

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ed42873
fix: MFA resource name in SelfManagement policy
Nov 22, 2022
778684c
Merge branch 'master' into master
rballan Dec 7, 2022
0b4acb0
Merge branch 'terraform-aws-modules:master' into master
rballan Dec 7, 2022
9d2e1f0
fix: revert changes and add statements for new MFA resource name
Dec 7, 2022
787fe02
Merge branch 'master' into master
bryantbiggs Jan 7, 2023
a191cb2
fix: rework iam self management policy
Jan 11, 2023
f96dae8
fix: typo
Jan 13, 2023
7088992
chore: terraform fmt
Jan 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 107 additions & 44 deletions modules/iam-group-with-policies/policies.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,83 +9,146 @@ locals {
partition = data.aws_partition.current.partition
}

# Allows MFA-authenticated IAM users to manage their own credentials on the My security credentials page
# https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_examples_aws_my-sec-creds-self-manage.html
data "aws_iam_policy_document" "iam_self_management" {
statement {
sid = "AllowSelfManagement"
sid = "AllowViewAccountInfo"

effect = "Allow"

actions = [
"iam:GetAccountPasswordPolicy",
"iam:ListVirtualMFADevices"
]

resources = ["*"]
}

statement {
sid = "AllowManageOwnPasswords"

effect = "Allow"

actions = [
"iam:ChangePassword",
"iam:GetUser"
]

resources = ["arn:${local.partition}:iam::${local.aws_account_id}:user/$${aws:username}"]
}

statement {
sid = "AllowManageOwnAccessKeys"

effect = "Allow"

actions = [
"iam:CreateAccessKey",
"iam:CreateLoginProfile",
"iam:CreateVirtualMFADevice",
"iam:DeleteAccessKey",
"iam:DeleteLoginProfile",
"iam:DeleteVirtualMFADevice",
"iam:EnableMFADevice",
"iam:GenerateCredentialReport",
"iam:GenerateServiceLastAccessedDetails",
"iam:Get*",
"iam:List*",
"iam:ResyncMFADevice",
"iam:UpdateAccessKey",
"iam:UpdateLoginProfile",
"iam:UpdateUser",
"iam:UploadSigningCertificate",
"iam:UploadSSHPublicKey",
"iam:TagUser",
"iam:ListUserTags",
"iam:UntagUser",
"iam:ListAccessKeys",
"iam:UpdateAccessKey"
]

# Allow for both users with "path" and without it
resources = [
"arn:${local.partition}:iam::${local.aws_account_id}:user/*/$${aws:username}",
"arn:${local.partition}:iam::${local.aws_account_id}:user/$${aws:username}",
"arn:${local.partition}:iam::${local.aws_account_id}:mfa/$${aws:username}",
Copy link
Member

Choose a reason for hiding this comment

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

I think we should make this a variable where the default is the current behavior, but users can override with a wildcard for multiple MFA devices

Copy link
Contributor Author

@rballan rballan Nov 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I will take another look to confirm (create a new IAM user and try to add a virtual MFA device with current IAMSelfManagement policy).
But it seems to me that since this new feature was released, the very first MFA device resource ARN take the name defined by user during his creation : arn:aws:iam::<aws_account_id>:mfa/<virtual_mfa_device_name>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for not getting back to you sooner, but after a new test, I can confirm the issue.

I have tested the following scenarios from a brand-new IAM user deployed with the module :

  • Add virtual MFA device : 👎
  • Remove existing virtual MFA device : 👎

After modifying the IAMSelfManagement policy with arn:aws:iam::<aws_account_id>:mfa/* :

  • Add virtual MFA device : 👍
  • Remove existing virtual MFA device when logged in without MFA : not working, that's normal 👍
  • Remove virtual MFA device : 👍

The day after the PR, I noticed that the AWS documentation was updated : see AllowManageOwnVirtualMFADevice statement.
But this is not updated everywhere yet.

Copy link
Member

@bryantbiggs bryantbiggs Dec 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

awesome, thank you for investigating!

  1. I see, you are absolutely correct on the wildcard
  2. However, we need to really update our policy. Could you update to match what they have provided in that doc which is more scoped now when we introduce wildcards. Note: I didn't fully check/compare so please do that - but we should look to split out to multiple statements based on the resource being specified
{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "AllowViewAccountInfo",
            "Effect": "Allow",
            "Action": "iam:ListVirtualMFADevices",
            "Resource": "*"
        },
        {
            "Sid": "AllowManageOwnVirtualMFADevice",
            "Effect": "Allow",
            "Action": [
                "iam:CreateVirtualMFADevice"
            ],
            "Resource": "arn:aws:iam::*:mfa/*"
        },
        {
            "Sid": "AllowManageOwnUserMFA",
            "Effect": "Allow",
            "Action": [
                "iam:DeactivateMFADevice",
                "iam:EnableMFADevice",
                "iam:GetUser",
                "iam:ListMFADevices",
                "iam:ResyncMFADevice"
            ],
            "Resource": "arn:aws:iam::*:user/${aws:username}"
        },
        {
            "Sid": "DenyAllExceptListedIfNoMFA",
            "Effect": "Deny",
            "NotAction": [
                "iam:CreateVirtualMFADevice",
                "iam:EnableMFADevice",
                "iam:GetUser",
                "iam:ListMFADevices",
                "iam:ListVirtualMFADevices",
                "iam:ResyncMFADevice",
                "sts:GetSessionToken"
            ],
            "Resource": "*",
            "Condition": {
                "BoolIfExists": {"aws:MultiFactorAuthPresent": "false"}
            }
        }
    ]
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To have a more scoped policy with no impacts, I made few changes :

  • Rollback previous changes (AllowSelfManagement & AllowDeactivateMFADevice statements)
  • Add 2 more explicit statements :
    • AllowManageOwnVirtualMFADevice => to allow action iam:CreateVirtualMFADevice with the correct resource name
    • AllowDeleteVirtualMFADevice => to allow the action with the correct resource name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello,

I made few changes here, according to this AWS documentation .

The AWS policy allows IAM users that are authenticated using multi-factor authentication (MFA) to manage their own credentials on the My security credentials page. It also requires the user to set up and authenticate using MFA before performing any other operations in AWS !

In real terms, that enforce MFA utilization. Because the IAM user cannot do anything before his MFA is configured, and he is authenticated with.

This is a great security improvement but this is a big change too. Is it what we want ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bryantbiggs Are you OK with that ?

If you're not, I can try to add a new variable to allow the user to choose between the standard policy (which do not force MFA utilization) or the more secure (which force MFA).

Copy link
Member

Choose a reason for hiding this comment

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

yes, I think this looks great and a good improvement; thank you

resources = ["arn:${local.partition}:iam::${local.aws_account_id}:user/$${aws:username}"]
}

statement {
sid = "AllowManageOwnSigningCertificates"

effect = "Allow"

actions = [
"iam:DeleteSigningCertificate",
"iam:ListSigningCertificates",
"iam:UpdateSigningCertificate",
"iam:UploadSigningCertificate"
]

resources = ["arn:${local.partition}:iam::${local.aws_account_id}:user/$${aws:username}"]
}

statement {
sid = "AllowIAMReadOnly"
sid = "AllowManageOwnSSHPublicKeys"

effect = "Allow"

actions = [
"iam:Get*",
"iam:List*",
"iam:DeleteSSHPublicKey",
"iam:GetSSHPublicKey",
"iam:ListSSHPublicKeys",
"iam:UpdateSSHPublicKey",
"iam:UploadSSHPublicKey"
]

resources = ["*"]
effect = "Allow"
resources = ["arn:${local.partition}:iam::${local.aws_account_id}:user/$${aws:username}"]
}

# Allow to deactivate MFA only when logging in with MFA
statement {
sid = "AllowDeactivateMFADevice"
sid = "AllowManageOwnGitCredentials"

effect = "Allow"

actions = [
"iam:CreateServiceSpecificCredential",
"iam:DeleteServiceSpecificCredential",
"iam:ListServiceSpecificCredentials",
"iam:ResetServiceSpecificCredential",
"iam:UpdateServiceSpecificCredential"
]

resources = ["arn:${local.partition}:iam::${local.aws_account_id}:user/$${aws:username}"]
}

statement {
sid = "AllowManageOwnVirtualMFADevice"

effect = "Allow"

actions = [
"iam:CreateVirtualMFADevice"
]

resources = ["arn:${local.partition}:iam::${local.aws_account_id}:mfa/*"]
}

statement {
sid = "AllowManageOwnUserMFA"

effect = "Allow"

actions = [
"iam:DeactivateMFADevice",
"iam:EnableMFADevice",
"iam:ListMFADevices",
"iam:ResyncMFADevice"
]

# Allow for both users with "path" and without it
resources = [
"arn:${local.partition}:iam::${local.aws_account_id}:user/*/$${aws:username}",
"arn:${local.partition}:iam::${local.aws_account_id}:user/$${aws:username}",
"arn:${local.partition}:iam::${local.aws_account_id}:mfa/$${aws:username}",
resources = ["arn:${local.partition}:iam::${local.aws_account_id}:user/$${aws:username}"]

}

statement {
sid = "DenyAllExceptListedIfNoMFA"

effect = "Deny"

not_actions = [
"iam:CreateVirtualMFADevice",
"iam:EnableMFADevice",
"iam:GetUser",
"iam:ListMFADevices",
"iam:ListVirtualMFADevices",
"iam:ResyncMFADevice",
"sts:GetSessionToken"
]

condition {
test = "Bool"
variable = "aws:MultiFactorAuthPresent"
values = ["true"]
}
resources = ["*"]

condition {
test = "NumericLessThan"
variable = "aws:MultiFactorAuthAge"
values = ["3600"]
test = "BoolIfExists"
variable = "aws:MultiFactorAuthPresent"
values = ["false"]
}
}
}