From de87550d4e178d7d10158704d49ca778f56a8bd3 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Fri, 7 Jul 2023 16:06:31 -0700 Subject: [PATCH 1/6] Correctly handles `path` and `virtual_mfa_device_name` on import --- internal/service/iam/virtual_mfa_device.go | 26 ++++++ .../service/iam/virtual_mfa_device_test.go | 80 +++++++++++++++---- 2 files changed, 92 insertions(+), 14 deletions(-) diff --git a/internal/service/iam/virtual_mfa_device.go b/internal/service/iam/virtual_mfa_device.go index 203d1bc7b63b..83df8abff47d 100644 --- a/internal/service/iam/virtual_mfa_device.go +++ b/internal/service/iam/virtual_mfa_device.go @@ -5,10 +5,12 @@ package iam import ( "context" + "fmt" "log" "regexp" "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/arn" "github.com/aws/aws-sdk-go/service/iam" "github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" @@ -101,6 +103,7 @@ func resourceVirtualMFADeviceCreate(ctx context.Context, d *schema.ResourceData, vMFA := output.VirtualMFADevice d.SetId(aws.StringValue(vMFA.SerialNumber)) + // Base32StringSeed and QRCodePNG must be read here, because they are not available via ListVirtualMFADevices d.Set("base_32_string_seed", string(vMFA.Base32StringSeed)) d.Set("qr_code_png", string(vMFA.QRCodePNG)) @@ -139,6 +142,14 @@ func resourceVirtualMFADeviceRead(ctx context.Context, d *schema.ResourceData, m d.Set("arn", vMFA.SerialNumber) + path, name, err := parseVirtualMFADeviceARN(aws.StringValue(vMFA.SerialNumber)) + if err != nil { + return sdkdiag.AppendErrorf(diags, "reading IAM Virtual MFA Device (%s): %s", d.Id(), err) + } + + d.Set("path", path) + d.Set("virtual_mfa_device_name", name) + // The call above returns empty tags. output, err := conn.ListMFADeviceTagsWithContext(ctx, &iam.ListMFADeviceTagsInput{ SerialNumber: aws.String(d.Id()), @@ -222,3 +233,18 @@ func FindVirtualMFADeviceBySerialNumber(ctx context.Context, conn *iam.IAM, seri return output, nil } + +func parseVirtualMFADeviceARN(s string) (path, name string, err error) { + arn, err := arn.Parse(s) + if err != nil { + return "", "", err + } + + re := regexp.MustCompile(`^mfa(/|/[\x{0021}-\x{007E}]+/)([-A-Za-z0-9_+=,.@]+)$`) + matches := re.FindStringSubmatch(arn.Resource) + if len(matches) != 3 { + return "", "", fmt.Errorf("IAM Virtual MFA Device ARN: invalid resource section (%s)", arn.Resource) + } + + return matches[1], matches[2], nil +} diff --git a/internal/service/iam/virtual_mfa_device_test.go b/internal/service/iam/virtual_mfa_device_test.go index d3ee898768f9..d88d36a72aca 100644 --- a/internal/service/iam/virtual_mfa_device_test.go +++ b/internal/service/iam/virtual_mfa_device_test.go @@ -34,18 +34,58 @@ func TestAccIAMVirtualMFADevice_basic(t *testing.T) { Steps: []resource.TestStep{ { Config: testAccVirtualMFADeviceConfig_basic(rName), - Check: resource.ComposeTestCheckFunc( + Check: resource.ComposeAggregateTestCheckFunc( testAccCheckVirtualMFADeviceExists(ctx, resourceName, &conf), acctest.CheckResourceAttrGlobalARN(resourceName, "arn", "iam", fmt.Sprintf("mfa/%s", rName)), resource.TestCheckResourceAttrSet(resourceName, "base_32_string_seed"), + resource.TestCheckResourceAttr(resourceName, "path", "/"), resource.TestCheckResourceAttrSet(resourceName, "qr_code_png"), ), }, { - ResourceName: resourceName, - ImportState: true, - ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"path", "virtual_mfa_device_name", "base_32_string_seed", "qr_code_png"}, + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{ + "base_32_string_seed", + "qr_code_png", + }, + }, + }, + }) +} + +func TestAccIAMVirtualMFADevice_path(t *testing.T) { + ctx := acctest.Context(t) + var conf iam.VirtualMFADevice + resourceName := "aws_iam_virtual_mfa_device.test" + + path := "/path/" + + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(ctx, t) }, + ErrorCheck: acctest.ErrorCheck(t, iam.EndpointsID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckVirtualMFADeviceDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccVirtualMFADeviceConfig_path(rName, path), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckVirtualMFADeviceExists(ctx, resourceName, &conf), + acctest.CheckResourceAttrGlobalARN(resourceName, "arn", "iam", fmt.Sprintf("mfa%s%s", path, rName)), + resource.TestCheckResourceAttr(resourceName, "path", path), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{ + "base_32_string_seed", + "qr_code_png", + }, }, }, }) @@ -66,21 +106,24 @@ func TestAccIAMVirtualMFADevice_tags(t *testing.T) { Steps: []resource.TestStep{ { Config: testAccVirtualMFADeviceConfig_tags1(rName, "key1", "value1"), - Check: resource.ComposeTestCheckFunc( + Check: resource.ComposeAggregateTestCheckFunc( testAccCheckVirtualMFADeviceExists(ctx, resourceName, &conf), resource.TestCheckResourceAttr(resourceName, "tags.%", "1"), resource.TestCheckResourceAttr(resourceName, "tags.key1", "value1"), ), }, { - ResourceName: resourceName, - ImportState: true, - ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"path", "virtual_mfa_device_name", "base_32_string_seed", "qr_code_png"}, + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{ + "base_32_string_seed", + "qr_code_png", + }, }, { Config: testAccVirtualMFADeviceConfig_tags2(rName, "key1", "value1updated", "key2", "value2"), - Check: resource.ComposeTestCheckFunc( + Check: resource.ComposeAggregateTestCheckFunc( testAccCheckVirtualMFADeviceExists(ctx, resourceName, &conf), resource.TestCheckResourceAttr(resourceName, "tags.%", "2"), resource.TestCheckResourceAttr(resourceName, "tags.key1", "value1updated"), @@ -89,7 +132,7 @@ func TestAccIAMVirtualMFADevice_tags(t *testing.T) { }, { Config: testAccVirtualMFADeviceConfig_tags1(rName, "key2", "value2"), - Check: resource.ComposeTestCheckFunc( + Check: resource.ComposeAggregateTestCheckFunc( testAccCheckVirtualMFADeviceExists(ctx, resourceName, &conf), resource.TestCheckResourceAttr(resourceName, "tags.%", "1"), resource.TestCheckResourceAttr(resourceName, "tags.key2", "value2"), @@ -114,10 +157,9 @@ func TestAccIAMVirtualMFADevice_disappears(t *testing.T) { Steps: []resource.TestStep{ { Config: testAccVirtualMFADeviceConfig_basic(rName), - Check: resource.ComposeTestCheckFunc( + Check: resource.ComposeAggregateTestCheckFunc( testAccCheckVirtualMFADeviceExists(ctx, resourceName, &conf), acctest.CheckResourceDisappears(ctx, acctest.Provider, tfiam.ResourceVirtualMFADevice(), resourceName), - acctest.CheckResourceDisappears(ctx, acctest.Provider, tfiam.ResourceVirtualMFADevice(), resourceName), ), ExpectNonEmptyPlan: true, }, @@ -182,6 +224,16 @@ resource "aws_iam_virtual_mfa_device" "test" { `, rName) } +func testAccVirtualMFADeviceConfig_path(rName, path string) string { + return fmt.Sprintf(` +resource "aws_iam_virtual_mfa_device" "test" { + virtual_mfa_device_name = %[1]q + + path = %[2]q +} +`, rName, path) +} + func testAccVirtualMFADeviceConfig_tags1(rName, tagKey1, tagValue1 string) string { return fmt.Sprintf(` resource "aws_iam_virtual_mfa_device" "test" { From 9494126577f6feda763c1a61a532569d64acd884 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Fri, 7 Jul 2023 16:22:46 -0700 Subject: [PATCH 2/6] Adds `enable_date` and `user_name` attributes --- internal/service/iam/virtual_mfa_device.go | 17 +++++++++++++++++ internal/service/iam/virtual_mfa_device_test.go | 2 ++ .../docs/r/iam_virtual_mfa_device.html.markdown | 8 +++++++- 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/internal/service/iam/virtual_mfa_device.go b/internal/service/iam/virtual_mfa_device.go index 83df8abff47d..75270e897782 100644 --- a/internal/service/iam/virtual_mfa_device.go +++ b/internal/service/iam/virtual_mfa_device.go @@ -8,6 +8,7 @@ import ( "fmt" "log" "regexp" + "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/arn" @@ -48,6 +49,10 @@ func ResourceVirtualMFADevice() *schema.Resource { Type: schema.TypeString, Computed: true, }, + "enable_date": { + Type: schema.TypeString, + Computed: true, + }, "path": { Type: schema.TypeString, Optional: true, @@ -61,6 +66,10 @@ func ResourceVirtualMFADevice() *schema.Resource { }, names.AttrTags: tftags.TagsSchema(), names.AttrTagsAll: tftags.TagsSchemaComputed(), + "user_name": { + Type: schema.TypeString, + Computed: true, + }, "virtual_mfa_device_name": { Type: schema.TypeString, Required: true, @@ -150,6 +159,14 @@ func resourceVirtualMFADeviceRead(ctx context.Context, d *schema.ResourceData, m d.Set("path", path) d.Set("virtual_mfa_device_name", name) + if v := vMFA.EnableDate; v != nil { + d.Set("enable_date", aws.TimeValue(v).Format(time.RFC3339)) + } + + if u := vMFA.User; u != nil { + d.Set("user_name", u.UserName) + } + // The call above returns empty tags. output, err := conn.ListMFADeviceTagsWithContext(ctx, &iam.ListMFADeviceTagsInput{ SerialNumber: aws.String(d.Id()), diff --git a/internal/service/iam/virtual_mfa_device_test.go b/internal/service/iam/virtual_mfa_device_test.go index d88d36a72aca..ec6f4f208921 100644 --- a/internal/service/iam/virtual_mfa_device_test.go +++ b/internal/service/iam/virtual_mfa_device_test.go @@ -38,8 +38,10 @@ func TestAccIAMVirtualMFADevice_basic(t *testing.T) { testAccCheckVirtualMFADeviceExists(ctx, resourceName, &conf), acctest.CheckResourceAttrGlobalARN(resourceName, "arn", "iam", fmt.Sprintf("mfa/%s", rName)), resource.TestCheckResourceAttrSet(resourceName, "base_32_string_seed"), + resource.TestCheckNoResourceAttr(resourceName, "enable_date"), resource.TestCheckResourceAttr(resourceName, "path", "/"), resource.TestCheckResourceAttrSet(resourceName, "qr_code_png"), + resource.TestCheckNoResourceAttr(resourceName, "user_name"), ), }, { diff --git a/website/docs/r/iam_virtual_mfa_device.html.markdown b/website/docs/r/iam_virtual_mfa_device.html.markdown index c252a48de290..80f447216d0e 100644 --- a/website/docs/r/iam_virtual_mfa_device.html.markdown +++ b/website/docs/r/iam_virtual_mfa_device.html.markdown @@ -13,6 +13,10 @@ Provides an IAM Virtual MFA Device. ~> **Note:** All attributes will be stored in the raw state as plain-text. [Read more about sensitive data in state](https://www.terraform.io/docs/state/sensitive-data.html). +~> **Note:** A virtual MFA device cannot be directly associated with an IAM User from Terraform. + To associate the virtual MFA device with a user and enable it, use the code returned in either `base_32_string_seed` or `qr_code_png` to generate TOTP authentication codes. + The authentication codes can then be used with the AWS CLI command [`aws iam enable-mfa-device`](https://docs.aws.amazon.com/cli/latest/reference/iam/enable-mfa-device.html) or the AWS API call [`EnableMFADevice`](https://docs.aws.amazon.com/IAM/latest/APIReference/API_EnableMFADevice.html). + ## Example Usage **Using certs on file:** @@ -37,8 +41,10 @@ In addition to all arguments above, the following attributes are exported: * `arn` - The Amazon Resource Name (ARN) specifying the virtual mfa device. * `base_32_string_seed` - The base32 seed defined as specified in [RFC3548](https://tools.ietf.org/html/rfc3548.txt). The `base_32_string_seed` is base64-encoded. -* `qr_code_png` - A QR code PNG image that encodes `otpauth://totp/$virtualMFADeviceName@$AccountName?secret=$Base32String` where `$virtualMFADeviceName` is one of the create call arguments. AccountName is the user name if set (otherwise, the account ID otherwise), and Base32String is the seed in base32 format. +* `enable_date` - The date and time when the virtual MFA device was enabled. +* `qr_code_png` - A QR code PNG image that encodes `otpauth://totp/$virtualMFADeviceName@$AccountName?secret=$Base32String` where `$virtualMFADeviceName` is one of the create call arguments. AccountName is the user name if set (otherwise, the account ID), and Base32String is the seed in base32 format. * `tags_all` - A map of tags assigned to the resource, including those inherited from the provider [`default_tags` configuration block](https://registry.terraform.io/providers/hashicorp/aws/latest/docs#default_tags-configuration-block). +* `user_name` - The associated IAM User name if the virtual MFA device is enabled. ## Import From 3e3f26d109acd7e6dbe97c4f11e96730b60405f2 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Sat, 8 Jul 2023 11:42:46 -0700 Subject: [PATCH 3/6] Adds deactivation if virtual MFA device is associated with an IAM user --- internal/service/iam/virtual_mfa_device.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/internal/service/iam/virtual_mfa_device.go b/internal/service/iam/virtual_mfa_device.go index 75270e897782..729af064f4ce 100644 --- a/internal/service/iam/virtual_mfa_device.go +++ b/internal/service/iam/virtual_mfa_device.go @@ -205,6 +205,19 @@ func resourceVirtualMFADeviceDelete(ctx context.Context, d *schema.ResourceData, var diags diag.Diagnostics conn := meta.(*conns.AWSClient).IAMConn(ctx) + if v := d.Get("user_name"); v != "" { + _, err := conn.DeactivateMFADeviceWithContext(ctx, &iam.DeactivateMFADeviceInput{ + UserName: aws.String(v.(string)), + SerialNumber: aws.String(d.Id()), + }) + if tfawserr.ErrCodeEquals(err, iam.ErrCodeNoSuchEntityException) { + return diags + } + if err != nil { + return sdkdiag.AppendErrorf(diags, "deactivating IAM Virtual MFA Device (%s): %s", d.Id(), err) + } + } + log.Printf("[INFO] Deleting IAM Virtual MFA Device: %s", d.Id()) _, err := conn.DeleteVirtualMFADeviceWithContext(ctx, &iam.DeleteVirtualMFADeviceInput{ SerialNumber: aws.String(d.Id()), From 615acb63f639c767ca82a6d32bd85c4a98459e28 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Sat, 8 Jul 2023 11:46:47 -0700 Subject: [PATCH 4/6] Updates sweeper to populate resource before trying deletion. Ignores AccessDenied error --- internal/service/iam/sweep.go | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/internal/service/iam/sweep.go b/internal/service/iam/sweep.go index 5078afcf3cfe..85dff6e276b2 100644 --- a/internal/service/iam/sweep.go +++ b/internal/service/iam/sweep.go @@ -918,8 +918,10 @@ func sweepVirtualMFADevice(region string) error { } conn := client.IAMConn(ctx) var sweeperErrs *multierror.Error - input := &iam.ListVirtualMFADevicesInput{} + accessDenied := regexp.MustCompile(`AccessDenied: .+ with an explicit deny`) + + input := &iam.ListVirtualMFADevicesInput{} err = conn.ListVirtualMFADevicesPagesWithContext(ctx, input, func(page *iam.ListVirtualMFADevicesOutput, lastPage bool) bool { if len(page.VirtualMFADevices) == 0 { log.Printf("[INFO] No IAM Virtual MFA Devices to sweep") @@ -936,11 +938,19 @@ func sweepVirtualMFADevice(region string) error { r := ResourceVirtualMFADevice() d := r.Data(nil) d.SetId(serialNum) + + if err := sdk.ReadResource(ctx, r, d, client); err != nil { + sweeperErrs = multierror.Append(sweeperErrs, fmt.Errorf("reading IAM Virtual MFA Device (%s): %w", serialNum, err)) + continue + } + err := sdk.DeleteResource(ctx, r, d, client) if err != nil { - sweeperErr := fmt.Errorf("error deleting IAM Virtual MFA Device (%s): %w", device, err) - log.Printf("[ERROR] %s", sweeperErr) - sweeperErrs = multierror.Append(sweeperErrs, sweeperErr) + if accessDenied.MatchString(err.Error()) { + log.Printf("[DEBUG] Skipping IAM Virtual MFA Device (%s): %s", serialNum, err) + continue + } + sweeperErrs = multierror.Append(sweeperErrs, fmt.Errorf("deleting IAM Virtual MFA Device (%s): %w", serialNum, err)) continue } } From 90ac6b9dea206a2b445dcd8e6975a0f1e441f1d4 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Sat, 8 Jul 2023 11:47:11 -0700 Subject: [PATCH 5/6] Fixes panic in OpsWorks user sweeper --- internal/service/opsworks/sweep.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/service/opsworks/sweep.go b/internal/service/opsworks/sweep.go index ef351d08272e..47a35123d20c 100644 --- a/internal/service/opsworks/sweep.go +++ b/internal/service/opsworks/sweep.go @@ -377,7 +377,7 @@ func newUserProfileSweeper(resource *schema.Resource, d *schema.ResourceData, cl func (ups userProfileSweeper) Delete(ctx context.Context, timeout time.Duration, optFns ...tfresource.OptionsFunc) error { err := ups.sweepable.Delete(ctx, timeout, optFns...) - if strings.Contains(err.Error(), "Cannot delete self") { + if err != nil && strings.Contains(err.Error(), "Cannot delete self") { log.Printf("[WARN] Skipping OpsWorks User Profile (%s): %s", ups.d.Id(), err) return nil } From 9e74fb74875ca3fbeafb89280989b01150fe1544 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Tue, 11 Jul 2023 11:04:25 -0700 Subject: [PATCH 6/6] Adds CHANGELOG entry --- .changelog/32462.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/32462.txt diff --git a/.changelog/32462.txt b/.changelog/32462.txt new file mode 100644 index 000000000000..7ac8b9a05490 --- /dev/null +++ b/.changelog/32462.txt @@ -0,0 +1,3 @@ +```release-note:enhancement +resource/aws_iam_virtual_mfa_device: Add `enable_date` and `user_name` attributes +```