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

aws-cdk-lib/aws-kms: No available option to get the a Alias ARN #28105

Closed
rafaelrcamargo opened this issue Nov 22, 2023 · 6 comments · Fixed by #28197
Closed

aws-cdk-lib/aws-kms: No available option to get the a Alias ARN #28105

rafaelrcamargo opened this issue Nov 22, 2023 · 6 comments · Fixed by #28197
Labels
@aws-cdk/aws-kms Related to AWS Key Management bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@rafaelrcamargo
Copy link
Contributor

Describe the bug

Currently, there is no available option to retrieve an Alias ARN within the AWS KMS module of the AWS CDK.

Expected Behavior

I expected to find a method or property within the AWS KMS construct that allows retrieving the Alias ARN associated with a specific key.

Current Behavior

There isn't a direct method or property available to retrieve the Alias ARN associated with a KMS key in the AWS CDK AWS KMS module.

Reproduction Steps

const key = new Key(this, "app_key")
const alias = new Alias(this, "app_key_alias", {
    aliasName: "app/key",
    targetKey: key
})

Now there should be a way to get the ARN like: alias.aliasArn

Possible Solution

One potential fix for this issue could be to introduce a method or property within the KMS module that exposes the Alias ARN associated with a key. Like there is for the kms.Key

Additional Information/Context

Understanding the Alias ARN is crucial for various operational and management tasks involving KMS keys. Having a direct way to retrieve this information within the CDK AWS KMS module would streamline many use cases.

CDK CLI Version

2.110.0 (build c6471f2)

Framework Version

No response

Node.js Version

v20.9.0

OS

Ubuntu 22.04.3 LTS

Language

TypeScript

Language Version

Typescript (3.9.10)

Other information

image

The ARN is accessible on the AWS Dashboard.

@rafaelrcamargo rafaelrcamargo added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 22, 2023
@github-actions github-actions bot added the @aws-cdk/aws-kms Related to AWS Key Management label Nov 22, 2023
@pahud
Copy link
Contributor

pahud commented Nov 22, 2023

Looks like the only return attribute is the alias name:
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-kms-alias.html

Yes it would be great to return the ARN from L2 construct.

@pahud pahud added p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Nov 22, 2023
@daschaa
Copy link
Contributor

daschaa commented Nov 26, 2023

Isn't the keyArn method already returning the correct ARN? It looks like it's creating an ARN with the alias name, which should be the desired format. (I've checked the documentation here: https://docs.aws.amazon.com/kms/latest/developerguide/find-cmk-alias.html#find-alias-console)

public get keyArn(): string {
return Stack.of(this).formatArn({
service: 'kms',
// aliasName already contains the '/'
resource: this.aliasName,
});
}

@rafaelrcamargo
Copy link
Contributor Author

rafaelrcamargo commented Nov 28, 2023

Thanks for the quick feedback @daschaa. I noticed that using keyArn gives the Key ARN for a key, but for an alias, it returns the Alias ARN. It's a bit puzzling since the same name refers to different things. This solves the issue, but it might confuse users.

I noticed too that this alias.aliasTargetKey.keyArn also retrieves the Key ARN. Which makes this a confusing situation:

const key = new Key(this, "app_key") // My KMS Key
const alias = new Alias(this, "app_key_alias", { targetKey: key })

key.keyArn // My Key ARN
alias.keyArn // My Alias ARN
alias.aliasTargetKey.keyArn // My Key ARN

As we are in the same kms context this can be confusing.

Is there a reason behind this naming variation?

@daschaa
Copy link
Contributor

daschaa commented Nov 28, 2023

I agree, this is indeed very confusing for users. And I don't think there is a purpose behind the naming.

How could we improve this here? Renaming the attribute is not possible, because we have to be backwards compatible. Do you think it would be sufficient to change the documentation?

@rafaelrcamargo
Copy link
Contributor Author

May creating a new aliasArn that just mirrors the value from keyArn solve this. Of course, this can be bad to maintain due to the duplication of code, but would for sure make things simpler.

Other than that updating the docs would for sure help too, was looking into it and I think this one may be straight up wrong about this: class Alias (construct).

Let me know what you think about those points and if I can help further.

@mergify mergify bot closed this as completed in #28197 Dec 5, 2023
mergify bot pushed a commit that referenced this issue Dec 5, 2023
…lias (#28197)

**Motivation:**

The current implementation of `keyArn` within the AWS CDK AWS KMS module returns the Key ARN for a key and an alias, which causes confusion for users expecting the Alias ARN. This PR aims to alleviate this confusion by providing clearer access to the Alias ARN.

**Changes:**

Introducing a new attribute `aliasArn` that mirrors the value from `keyArn` specifically for aliases to explicitly retrieve the Alias ARN. 

```typescript
/**
 * The ARN of the alias.
 *
 * @Attribute
 * @deprecated use `aliasArn` instead
 */
public get keyArn(): string {
  return Stack.of(this).formatArn({
    service: 'kms',
    // aliasName already contains the '/'
    resource: this.aliasName,
  });
}

/**
 * The ARN of the alias.
 *
 * @Attribute
 */
public get aliasArn(): string {
  return this.keyArn;
}
```

**Query:**

Should we deprecate the existing `keyArn` and mirror it in `aliasArn` or change the logic within `keyArn` to `aliasArn` and use the `keyArn` as the mirror?

> Your feedback on the preferred approach would be greatly appreciated!

Closes #28105.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Copy link

github-actions bot commented Dec 5, 2023

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

chenjane-dev pushed a commit to chenjane-dev/aws-cdk that referenced this issue Dec 5, 2023
…lias (aws#28197)

**Motivation:**

The current implementation of `keyArn` within the AWS CDK AWS KMS module returns the Key ARN for a key and an alias, which causes confusion for users expecting the Alias ARN. This PR aims to alleviate this confusion by providing clearer access to the Alias ARN.

**Changes:**

Introducing a new attribute `aliasArn` that mirrors the value from `keyArn` specifically for aliases to explicitly retrieve the Alias ARN. 

```typescript
/**
 * The ARN of the alias.
 *
 * @Attribute
 * @deprecated use `aliasArn` instead
 */
public get keyArn(): string {
  return Stack.of(this).formatArn({
    service: 'kms',
    // aliasName already contains the '/'
    resource: this.aliasName,
  });
}

/**
 * The ARN of the alias.
 *
 * @Attribute
 */
public get aliasArn(): string {
  return this.keyArn;
}
```

**Query:**

Should we deprecate the existing `keyArn` and mirror it in `aliasArn` or change the logic within `keyArn` to `aliasArn` and use the `keyArn` as the mirror?

> Your feedback on the preferred approach would be greatly appreciated!

Closes aws#28105.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-kms Related to AWS Key Management bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants