feat(elasticsearch): Decouple setting access policies from domain constructor

[TikiTDO]

Currently when creating an elasticsearch domain the access policies must be set in the constructor. This makes it impossible to create access policies that reference the domain.

Use Cases

See: https://aws.amazon.com/premiumsupport/knowledge-center/kinesis-firehose-cross-account-streaming/

Proposed Solution

This PR extracts the access policy setting to a helper method which can be used if the accessPolicies and useUnsignedBasicAuth props are not set. The helper will error if access policies are already set to prevent creating duplicate custom resources.

---

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

[gitpod-io]

[TikiTDO]

@BenChaimberg Do you need any additional info to approve this PR? I would really like to set up a cross-region firehose config, and I'm blocked by this issue. I'd be happy to provide anything I can to speed this one up.

[BenChaimberg]

Unless I'm confused, this change should not be necessary. If you are setting a resource policy (which this effectively is), then you do not need to specify the full resource ARN; it's assumed to be the ARN of the resource you are attaching the policy to. That means you should be able to use a wildcard for the resource name like so: arn:aws:es:us-west-2:123456789012:domain/*/_all/_settings.

If this is not the case, I will continue to review this PR

[TikiTDO]

@BenChaimberg I'm on vacation this week, but I will try it next week to check if that does what I need.

[TikiTDO]

@BenChaimberg I took a look, and while your approach will satisfy my immediate use case, it opens up a very nasty potential security issue. Because the domain and path are part of the resource configuration using a wildcard for the domain name may result in you giving more permission than you intended if you are not careful.

Consider the following example:

{    
  "Effect": "Allow",
  "Principal": {
    "AWS": "arn:aws:iam::111222333444:user/HealthcheckTester"
  },
  "Action": "es:ESHttpGet",
  "Resource": [
    "arn:aws:es:us-west-2:123456789012:domain/*/"
  ]
}

The expectation there would be that I want to let a HealthcheckTester user in some account be able to query to server status page at the root of the ES server to validate that it is up. However, in practice that policy is going to let this user query any data on the server, as long as the request ends with /.

This is true for any URL that can be made to end with a valid post-wildcard segment.

It wouldn't affect my usecase directly, since I don't expect firehose will ever query anything it doesn't need, but it could happen in more advanced cases.

[BenChaimberg]

Understood the potential use-case, will add this to my queue!

Pull request has been modified.

[TikiTDO]

@BenChaimberg Most of the PR comments should be fixed

Pull request has been modified.

[TikiTDO]

@BenChaimberg Updated as per your previous review

Pull request has been modified.

[BenChaimberg]

see comment above about missing test

Pull request has been modified.

[TikiTDO]

@BenChaimberg The domain.addAccessPolicies currently takes an array, so that change in the test removing the wrapping array will not work. I could expand it to support both.

[TikiTDO]

@kaizen3031593 Sure, I can poke at it today

[TikiTDO]

@kaizen3031593 Can you clarify a bit about the direction of opensearch vs elsasticsearch packages? Is the intent to have feature parity between the two? The getting this feature into both is simple enough, I just want to make sure I'm approaching this correctly.

[TikiTDO]

@kaizen3031593 This PR should be ready for review

[kaizencc]

@TikiTDO as long as both opensearch and elasticsearch are stable, we're going to have to support feature parity between the two. However, the plan is to deprecate elasticsearch soon and once that happens, I think we'll only support p0 bugs in elasticsearch.

[kaizencc]

Thanks @TikiTDO! And sorry it took forever to get this in. I just went in and changed a few more elasticsearches into opensearches.

Pull request has been modified.

[TikiTDO]

Looks like the tests segfaulted on an unrelated module.

[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).

[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: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: f58966e
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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