fix(secretsmanager): Secret.fromSecretName doesn't work with ECS

[njlynch]

The ability to import and reference a Secret purely by the secret name was
introduced in #10309. One of the original requests was modelled after the
integration with CodeBuild, where either the secret name or the full ARN
-- including the SecretsManager-provided suffix -- were accepted, but not a
"partial" ARN without the suffix. To ease integrations with other services
in this case, the secretArn was defined as returning the secretName for
these secrets imported by name.

However, other services -- like ECS -- require that an ARN format is provided,
even as a partial ARN. This introduces a dual behavior where sometimes the
secretName works and partial ARN fails, and other times the partial ARN works
and the secretName fails.

This change deprecates fromSecretName and introduces a new, better-behaved
fromSecretNameV2 that sets the ARN to a "partial" ARN without the Secrets
Manager suffix. It also introduces a secretFullArn that is an optional version
of secretArn that will be undefined for secrets imported by name.

Related changes

• I improved the suffix-strippiung logic of parseSecretName to only strip a
  suffix if it's exactly 6 characters long, as all SecretsManager
  suffixes are 6 characters. This prevents accidentally stripping the last word
  off of a hyphenated secret name like 'github-token'.
• Updated the CodeBuild integration and added CodeBuild tests.

fixes #10519

---

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

[gitpod-io]

[skinny85]

Looks good! A few comments / questions.

[skinny85]

I would leave this as-is, look better indented IMO.

[skinny85]

I wonder - should this be -*?

[skinny85]

Actually... should this be -???????

[skinny85]

Can you re-write this just slightly?

Suggested change

	if (lastHyphenIndex !== -1 && resourceName.substr(lastHyphenIndex + 1).length === 6) {
	return resourceName.substr(0, lastHyphenIndex);
	}
	return resourceName;
	const hasHyphenAndSixCharSuffix = lastHyphenIndex !== -1 && resourceName.substr(lastHyphenIndex + 1).length === 6;
	return hasHyphenAndSixCharSuffix ? resourceName.substr(0, lastHyphenIndex) : resourceName;

[nija-at]

Don't like this quirk. It's so non-trivial and easy to miss.
arn should return ARN, not name.

How about instead introduce three things in ISecret - secretName, secretPartialArn and secretArn?
And then have the combination of import APIs as needed.

However, if a customer imports a secret with only the name or the partial ARN and their integration (i.e., ECS) requests a fullArn, the API will fail.

[njlynch]

However, if a customer imports a secret with only the name or the partial ARN and their integration (i.e., ECS) requests a fullArn, the API will fail.

This means customers who currently import the secret by name and make some reference to secretArn somewhere will go from having a working synth/deploy to an error.

The only way to make the above work -- I think -- would be to deprecate fromSecretName and introduce a new fromSecretNameV2 or fromSecretNameWithProperPartialArnHandling (or fromSecretNameButNotDeprecated) or something. Any suggestions?

---

This also doesn't quite solve how the integration knows what forms are available. Requesting the secretArn and catching an error doesn't fit. The only sane thing I can think of is that integrations must always just use the "weakest" form they can accept. For example, CodeBuild would always just use the secretName, even if the full ARN was available.

This has some implications, especially given that today users can provide either a partial or full ARN to fromSecretArn, and the code that determines the secretName from the ARN can't deterministically know which was provided. An ARN of arn:aws:secretsmanager:eu-west-1:111111111111:secret:github-tokens will be treated like a full ARN with a SecretsManager suffix of 'tokens', rather than a partial ARN.

const secret = secretsmanager.Secret.fromSecretArn(this, 'secret', 'arn:aws:secretsmanager:eu-west-1:111111111111:secret:github-tokens');
secret.secretName; // 'github'

There are likely scenarios where an integrating service will break due to the above, but be fine with the secretArn (or even partialArn).

In lieu of error checking, I suppose we could introduce new fields to track what is available (e.g., hasFullArn), but that feels gross. Alternatively, some meta field like secretId that returns the strictest form available might solve some problems (and introduce others).

What do you think?

[skinny85]

I think it's worth it to look from the perspective of the consumers of the SecretsManager library. How should using it look like?

For example, we have code in CodeBuild that uses ISecret:

aws-cdk/packages/@aws-cdk/aws-codebuild/lib/project.ts

Line 966 in 0537598

credential: this.buildImage.secretsManagerCredentials.secretArn,

Is there any way we can sensibly make a decision in that CodeBuild Project code to use either secretName or secretArn, considering CodeBuild is one of the services that supports both?

[njlynch]

@nija-at / @skinny85 - How much do you both hate something like this?

  /**
   * Returns an identifier for the secret that can be used to reference the secret in other constructs.
   * For owned secrets, this will always be the secret ARN. For imported secrets,this identifier may be either
   * the full secret ARN (with SecretsManager-suffix),
   * partial secret ARN (without the suffix),
   * or just the secret name, depending on what values are available.
   * Returns the "strongest" identifier available (full > partial > secret name), unless `acceptPartialArns` is false.
   */
  public secretId(acceptPartialArns: boolean = true): string {
    return (!this._isPartialArn || acceptPartialArns) ? this.secretArn : this.secretName;
  }

_isPartialArn would only be true for secrets imported by name; a new fromPartialSecretArn could also be created as well to set the flag but otherwise behave like fromSecretArn.

This moves the responsibility from the end-user to construct library author.

The CodeBuild code would be updated to read like this:

credential: this.buildImage.secretsManagerCredentials.secretId(false), 

And the ECS code could be updated as well:

aws-cdk/packages/@aws-cdk/aws-ecs/lib/container-definition.ts

Line 39 in a4a41b5

arn: field ? `${secret.secretArn}:${field}::` : secret.secretArn,

arn: field ? `${secret.secretId()}:${field}::` : secret.secretId(),

---

The above could be done in conjunction with -- or independently from -- creating a fromSecretNameV2 (better name suggestions welcome).

[skinny85]

Oh man. This is such a mess.

I actually think the simplest solution would be to have secretName return string | undefined. I actually thought that was already the case, because of #10914. So then, in CodeBuild, you could do:

credential: this.buildImage.secretsManagerCredentials.secretName ?? this.buildImage.secretsManagerCredentials.secretArn,

(I assume a new Secret without a secretName set in SecretProps would return an undefined secret.secretName)

While on ECS you would always use secretArn.

Is it too late to go this route?

[eladb]

Definitely feels wrong to put the burden on the user. This is precisely stuff we would want to completely encapsulate within the library. But I am not sure I completely understand all the implications.

@njlynch can you please summarize what would be the ideal API, if we had designed it from the ground up? Then, can you describe which aspects of this API break the existing behavior.

[nija-at]

I feel like the trick lies somewhere in deprecating a few APIs (probably one of the import APIs and secretArn itself) and introducing new APIs like secretPartialArn and secretFullArn.

Just like @eladb, I'm finding it hard to understand all of the constraints to provide a concrete suggestion.
I could respond to individual questions being asked here, but I'm not sure it's going to be useful without understanding the larger picture.

Summarizing the full picture could help, or happy to jump on a call and make this a synchronous conversation + pair coding.

[njlynch]

Background:

Secrets are given a name on creation, either by users or generated by CloudFormation. The name is then suffixed with a hyphen plus a six-character string to form the "full" name (e.g., MySecret becomes MySecret-abc123). The ARN is formatted as arn:aws:secretsmanager:$REGION:$ACCOUNT:secret:$SECRET_NAME. Depending on the scenario (which API or CloudFormation integration you're using), the secret name (with and without the suffix) or ARN (with and without the suffix) can be used. Additional complexity comes from the fact that secret names can contain hyphens themselves, so 'my-secret-abc123' could be a "partial" secret name or a "full" secret name, and there's no deterministic way to know. Secrets Manager discourages creating secrets of this format, but doesn't prevent it.

Given a secret name (without a 6-character suffix), a partial ARN may be created. Given an ARN, a partial ARN and secret name can be derived; however, this runs into issues when the secret name itself contains a 6-character suffix (which is valid).

Desired state:

Users want to be able to import existing secrets by both (partial|full) ARN and (partial|full) name. Importing by partial name is often the most convenient for multi-environment setups, as the partial name is the only identifier that may be the same across accounts and regions. Ideally, a user imports the secret given a (partial|full) (name|ARN) and then is able to use it anywhere an ISecret is accepted, without worrying about the underlying details.

Various constructs have different rules for which secret identifier they can accept. CodeBuild will not accept a partial ARN but can accept a secret name. ECS will only accept an ARN, either partial or full. The onus of determining which identifier(s) are available and which to use should fall on the construct library, not the user. Generally, the construct library should prefer to use the "strongest" (full ARN > partial ARN > secret name) identifier available, as it reduces ambiguity.

Ideal API:

interface ISecret {
  readonly encryptionKey?: kms.IKey;
  readonly secretArn: string;
  readonly secretName: string;
  readonly secretValue: SecretValue;
  // True iff the `secretArn` does not contain the SecretsManager-provided suffix.
  readonly secretArnIsPartial: boolean;
}

// Open to slightly different representations, see https://github.com/aws/aws-cdk/pull/10410#discussion_r490465209
interface SecretAttributes {
  readonly secretArn?: string;
  readonly encryptionKey?: kms.IKey;
  readonly secretName?: string;
}

// Imports the secret by name, sets `secretArnIsPartial = true`.
public static fromSecretName(scope: Construct, id: string, secretName: string)
// Imports the secret by ARN, automatically sets the secret name. `secretArnIsPartial = false`
public static fromSecretArn(scope: Construct, id: string, secretArn: string)
// Imports the secret by ARN or name.
// If secretName and secretArn are both provided, explicitly sets secretName,
// and sets `secretArnIsPartial` based on the comparison of the ARN and secretName.
// Use case is for handling the worst-case secret names with 6-character suffixes.
public static fromSecretAttributes(scope: Construct, id: string, attrs: SecretAttributes)

constructor(scope: Construct, id: string, props: SecretProps) {
 // secretArnIsPartial = false;
}

Consumers of the ISecret use the secretArnIsPartial flag to make a determination on how to handle the secret. For example, CodeBuild's logic would be:

credential: secret.secretArnIsPartial ? secret.secretName : secret.secretArn;

ECS would simply continue using secretArn as today.

Why can't we do this today?:

Today, fromSecretName sets secretArn === secretName. This breaks several existing integrations, but can't be unset because it also works for some integrations.

The easiest way around that is deprecating fromSecretName and introducing fromSecretNameV2 which conforms to the above behavior.

---

Thoughts? Counter proposals?

[nija-at]

(discussed offline and here's where we landed)

The API is going to look like this -

export interface ISecret {
  readonly secretName: string;
  readonly secretPartialArn: string;

  // throws when used via fromSecretName() and fromSecretPartialArn()
  readonly secretFullArn: string;

  // return false when used via fromSecretName() and fromSecretPartialArn()
  public trySecretFullArn(): boolean;

  // returns `trySecretFullArn() ?? secretPartialArn`;
  // so that 15 uses of `secretArn` across the CDK still works
  // and according to @njlynch either works just fine in most cases.
  readonly secretArn: string; 
}

Following this the fromSecretName() API will be deprecated and another fromSecretNameV2() introduced that does the correct thing. Also introduce, fromSecretPartialArn() and fromSecretFullArn().

[njlynch]

I am [not yet] aware of any use case for secretPartialArn, and I don't believe creating it for owned secrets is possible (there's no equivalent of lastIndexOf as a CloudFormation Fn). So that means either having secretPartialArn throw for owned secrets, having a trySecretPartialArn, or just removing it entirely. Of the three options, the latter seems cleanest.

So this is @nija-at's (modified) ISecret.

export interface ISecret {
  readonly encryptionKey?: kms.IKey;
  readonly secretName: string;
  readonly secretValue: SecretValue;
  // Behaves exactly as today. Will return a partial ARN from `fromSecretNameV2`.
  readonly secretArn: string; 
  // throws when used via fromSecretName() and fromSecretNameV2(),
  // returns secretArn otherwise.
  readonly secretFullArn: string;
  // return undefined when used via fromSecretName() and fromSecretNameV2(),
  // returns secretArn otherwise.
  readonly trySecretFullArn?: string;
}

And this is my proposal for comparison ISecret:

export interface ISecret {
  readonly encryptionKey?: kms.IKey;
  readonly secretName: string;
  readonly secretValue: SecretValue;
  // Behaves exactly as today. Will return a partial ARN from `fromSecretNameV2`.
  readonly secretArn: string;
  // True when used via `fromSecretNameV2`.
  readonly secretArnIsPartial: boolean;
}

Alternatively, this would also work:

export interface ISecret {
  readonly encryptionKey?: kms.IKey;
  readonly secretName: string;
  readonly secretValue: SecretValue;
  // Behaves exactly as today. Will return a partial ARN from `fromSecretNameV2`.
  readonly secretArn: string;
  // Equivalent to `secretArn`, except will return undefined for
  // `fromSecretName` and `fromSecretNameV2`.
  readonly secretFullArn?: string;
}

[eladb]

Added do not merge

[skinny85]

Looks great! 2 minor suggestions.

Changes addressed.

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: 6c0a826
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).