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

SplunkLogDriverProps adds splunk-token to logging options instead of secretoptions #7264

Closed
lockan opened this issue Apr 8, 2020 · 2 comments · Fixed by #15408
Closed

SplunkLogDriverProps adds splunk-token to logging options instead of secretoptions #7264

lockan opened this issue Apr 8, 2020 · 2 comments · Fixed by #15408
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. effort/medium Medium work item – several days of effort good first issue Related to contributions. See CONTRIBUTING.md needs-reproduction This issue needs reproduction. p2

Comments

@lockan
Copy link

lockan commented Apr 8, 2020

Bug Description

When creating a Task Definition and adding a SplunkLogDriver, a token property can be provided. The token is a secret value. In our case, we're providing the token from SecretsManager as a SecretValue per the cdk API spec.

In Cloudformation this would be provided to the Task Definition through secretOptions rather than options, and the secret would be hidden from the user in the ECS Console UI.
(See docs: https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task_definition_parameters.html)

When doing the same through CDK, the token is added to the Options property, and so the secret is resolved from Secrets Manager as expected, but then is plainly displayed as a string in the ECS console UI. This can also be confirmed by examining the output cloudformation

Reproduction Steps

Sample CDK code to reproduce the issue:

const splunkSecretArn = "arn:aws:secretsmanager:{region}:{accountno}:secret:{secretsmanagerkey}"

...

this.splunkSecretParam = new cdk.CfnParameter(this,
    'SplunkSecretArn', {
    type: 'String',
    description: "Secret manager ARN for the splunk HEC token",
    default: splunkSecretArn
})

...

this.logDriverConfig = new ecs.SplunkLogDriver({
    url: this.parameters.splunkUrl.valueAsString,
    index: this.parameters.splunkIndex.valueAsString,
    token: cdk.SecretValue.secretsManager(this.splunkSecretParam.valueAsString),
 })

this.fargateContainer = this.fargateTaskDefinition.addContainer("fg-containerdef", {
      image: ecs.ContainerImage.fromEcrRepository(this.ecrRepository, "latest"),
      logging: this.logDriverConfig
})

Error Log

Excerpt from the output cloudformation template after doing cdk synthesize. Notice that the splunk-token appears under Options, not SecretOptions.

...
LogConfiguration:
            LogDriver: splunk
            Options:
              splunk-token:
                'Fn::Join':
                  - ''
                  - - '{{resolve:secretsmanager:'
                    - Ref: ExternalSplunkSecretArn
                    - ':SecretString:::}}'
...

Environment

  • CLI Version : aws-cli/2.0.0dev0 Python/3.7.4 Darwin/17.7.0 botocore/2.0.0dev0
  • Framework Version: 1.31.0 (build 8f3ac79)
  • OS : MacOS High Sierra 10.13.16
  • Language : javascript / typescript

Other


This is 🐛 Bug Report

@lockan lockan added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 8, 2020
@SomayaB SomayaB added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Apr 9, 2020
@SoManyHs SoManyHs added needs-reproduction This issue needs reproduction. p2 effort/medium Medium work item – several days of effort effort/small Small work item – less than a day of effort and removed effort/small Small work item – less than a day of effort needs-triage This issue or PR still needs to be triaged. labels Apr 20, 2020
@SoManyHs SoManyHs added the good first issue Related to contributions. See CONTRIBUTING.md label Apr 20, 2020
@tobli tobli mentioned this issue May 24, 2020
2 tasks
@tobli
Copy link

tobli commented May 24, 2020

I'm thinking that any LogDriver parameter should be able to be passed as a Secret. We have an existing (non-cdk) task-def that specifies both splunk-token and splunk-url using secretOptions, but it should extend to all options, regardless of driver.

Right now it seems like splunk-token is the only option typed as a SecretValue, but I don't think that's right, given that it's the arn of the secret that we want to reference, not the value of the secret.

I'd love to help address this, but I'm not sure what the best design would be. Especially not while preserving backwards compatibility. Allowing any option to be passed as a Secret would be nice, including granting the task execution role read access, just like secret environment variables are handled.

@mergify mergify bot closed this as completed in #15408 Jul 9, 2021
mergify bot pushed a commit that referenced this issue Jul 9, 2021
…plunkLogDriver (#15408)

----
This PR closes [#7264](#7264).

The `token` field of the Splunk log driver populates the `Options` property of the Log Configuration which leads to the secret being resolved to its value on deploying, and then the token is viewable in plain text in the console and may be stored in plain text elsewhere. Thus, we are deprecating the `token` field of the Splunk log driver and are introducing a new `secretToken` field. `secretToken` can be used to provide the Splunk token as a Secrets Manager Secret or a Systems Manager Parameter and will be populated in the `SecretOptions` property of the Log Configuration. 

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

github-actions bot commented Jul 9, 2021

⚠️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.

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Aug 3, 2021
…plunkLogDriver (aws#15408)

----
This PR closes [aws#7264](aws#7264).

The `token` field of the Splunk log driver populates the `Options` property of the Log Configuration which leads to the secret being resolved to its value on deploying, and then the token is viewable in plain text in the console and may be stored in plain text elsewhere. Thus, we are deprecating the `token` field of the Splunk log driver and are introducing a new `secretToken` field. `secretToken` can be used to provide the Splunk token as a Secrets Manager Secret or a Systems Manager Parameter and will be populated in the `SecretOptions` property of the Log Configuration. 

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Aug 26, 2021
…plunkLogDriver (aws#15408)

----
This PR closes [aws#7264](aws#7264).

The `token` field of the Splunk log driver populates the `Options` property of the Log Configuration which leads to the secret being resolved to its value on deploying, and then the token is viewable in plain text in the console and may be stored in plain text elsewhere. Thus, we are deprecating the `token` field of the Splunk log driver and are introducing a new `secretToken` field. `secretToken` can be used to provide the Splunk token as a Secrets Manager Secret or a Systems Manager Parameter and will be populated in the `SecretOptions` property of the Log Configuration. 

*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-ecs Related to Amazon Elastic Container bug This issue is a bug. effort/medium Medium work item – several days of effort good first issue Related to contributions. See CONTRIBUTING.md needs-reproduction This issue needs reproduction. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants