Skip to content

Conversation

@pahud
Copy link
Contributor

@pahud pahud commented Jun 9, 2025

Description

This PR fixes the issue where OpenSearch domains were not getting a default TLS security policy, causing inconsistent behavior between the CDK construct and the actual AWS service default.

Previous behavior:

  • CDK construct: No tlsSecurityPolicy specified → TLSSecurityPolicy.TLS_1_0
  • AWS service: Defaults to TLS 1.2 when TLSSecurityPolicy is not specified in CloudFormation

New behavior:

  • CDK construct: No tlsSecurityPolicy specified → CDK explicitly sets TLSSecurityPolicy: Policy-Min-TLS-1-2-2019-07
  • Result: Consistent TLS 1.2 behavior between CDK construct and AWS service

Changes Made

  1. Updated domain.ts: Modified the DomainEndpointOptions configuration to use TLSSecurityPolicy.TLS_1_2 as the default when props.tlsSecurityPolicy is undefined:
  tlsSecurityPolicy: props.tlsSecurityPolicy ?? TLSSecurityPolicy.TLS_1_2,
  1. Added comprehensive tests: Created a new test suite covering:
    - Default TLS 1.2 behavior when no policy is specified
    - Explicit TLS policy values (1.0, 1.2, 1.2 PFS)
    - Interaction with enforceHttps setting
    - Backward compatibility scenarios
  2. Add a new integ test that checks DomainEndpointOptions to include the expected TLSSecurityPolicy assertion, ensuring they reflect the new default behavior.

Approach Rationale

This approach is simpler and more predictable:

  • CDK-controlled defaults: We simply change the implicit default from TLS 1.0 to TLS 1.2, ensuring the default value is fully controlled by CDK even when undefined
  • Breaking change: This changes the default TLS security policy behavior and should be called out in release notes
  • Matches integration test expectations: Aligns with existing integration test assertions that expect TLSSecurityPolicy: Policy-Min-TLS-1-2-2019-07
  • Follows AWS best practices: TLS 1.2 is the recommended minimum security standard

Testing

  • ✅ All existing OpenSearch domain tests pass (1,616 tests)
  • ✅ New comprehensive TLS security policy test suite
  • ✅ Integration test integ.opensearch.https.ts continues to pass
  • ✅ No linting issues

Related Issues

Closes #34658

Breaking Changes

OpenSearch Domain TLS Security Policy Default Changed

  • The default TLS security policy for OpenSearch domains has changed from TLS 1.0 to TLS 1.2
  • Impact: Domains created without an explicit tlsSecurityPolicy will now use TLS 1.2 instead of TLS 1.0
  • Migration: If you require TLS 1.0 for backward compatibility, explicitly set tlsSecurityPolicy: TLSSecurityPolicy.TLS_1_0

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

-  no TLS security policy set, allowing the service to use its default policy
@github-actions github-actions bot added the bug This issue is a bug. label Jun 9, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team June 9, 2025 16:44
@github-actions github-actions bot added effort/small Small work item – less than a day of effort p1 labels Jun 9, 2025
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jun 9, 2025
@pahud
Copy link
Contributor Author

pahud commented Jun 9, 2025

updating integ tests snapshots

Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This review is outdated)

@pahud pahud changed the title keep tlsSecurityPolicy optional fix(opensearchservice): aws-opensearchservice: Remove default TLS policy Jun 9, 2025
@pahud pahud changed the title fix(opensearchservice): aws-opensearchservice: Remove default TLS policy fix(opensearchservice): aws-opensearchservice: remove default TLS policy Jun 9, 2025
@aws-cdk-automation aws-cdk-automation dismissed their stale review June 9, 2025 18:35

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@pahud pahud marked this pull request as ready for review June 10, 2025 11:03
@vishaalmehrishi vishaalmehrishi self-assigned this Jun 10, 2025
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 10, 2025
domainEndpointOptions: {
enforceHttps,
tlsSecurityPolicy: props.tlsSecurityPolicy ?? TLSSecurityPolicy.TLS_1_0,
tlsSecurityPolicy: props.tlsSecurityPolicy,
Copy link
Contributor

@vishaalmehrishi vishaalmehrishi Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a risk of breaking existing customers who intentionally did not provide a security policy with the assumption that it would use Policy-Min-TLS-1-0-2019-07? This change will implicitly update the TLS security policy (by using the OpenSearch default) without customers knowing, which could break their existing setups which rely on the specific version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it will. To avoid this, I guess the only option for us is to introduce a new FF. Should we do that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the update in the User Impact section (and the testing done as well - thank you!):

Existing users with tlsSecurityPolicy undefined won't be impact, changing tlsSecurityPolicy from a cdk defined TLS 1.0 to undefined won't update the existing deployment. See Description of how you validated changes below.
After this PR, for all new deployments, the default will be TLS 1.2 if undefined. Users have to explicitly opt in TLSSecurityPolicy.TLS_1_0 if they need to stick to this version.

Do you think there is value in publishing a notice/informing customers somehow about this change? It seems safe to me, based on your testing, but it is possible that we are not thinking about a particular customer setup which can break.

@vishaalmehrishi
Copy link
Contributor

Comment on the commit message:

fix(opensearchservice): aws-opensearchservice: remove default TLS policy

Might be better to say:

fix(opensearchservice): aws-opensearchservice: use OpenSearch default TLS policy instead of CDK-specified default TLS policy

@mrgrain
Copy link
Contributor

mrgrain commented Jun 11, 2025

chore(opensearchservice): use OpenSearch default TLS policy instead of CDK-specified default TLS policy

Definitely not a chore either. Only changes that have no bearing on our customers should chores. Changing the default is always going to be something customer facing.

fix: describe the bug (not the solution)

Yes, but just because the issue has a certain title doesn't mean that title should be used. The bug isn't really "remove default TLS policy" but would be something like "cannot use OpenSearch default TLS policy because of outdated CDK-specified default TLS policy"

IMO this is not a bug. The current code falls back to an older TLS version, but that was intended behaviour which now needs updating, which is different from a bug (unintended behaviour).

FWIW I'd probably classify it as a feature.

@pahud pahud changed the title chore(opensearchservice): use OpenSearch default TLS policy instead of CDK-specified default TLS policy fix(opensearch): set default TLS security policy to TLS 1.2 Jun 11, 2025
@pahud pahud changed the title fix(opensearch): set default TLS security policy to TLS 1.2 fix(opensearch): default TLS security policy should be TLS 1.2 Jun 11, 2025
@pahud pahud changed the title fix(opensearch): default TLS security policy should be TLS 1.2 feat(opensearch): TLS 1.2 as the default TLS security policy for OpenSearch domains Jun 11, 2025
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This review is outdated)

@pahud pahud changed the title feat(opensearch): TLS 1.2 as the default TLS security policy for OpenSearch domains feat(opensearch): update default TLS security policy to TLS 1.2 for OpenSearch domains Jun 11, 2025
@pahud
Copy link
Contributor Author

pahud commented Jun 11, 2025

Exemption Request for the README update

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Jun 11, 2025
@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 11, 2025
@vishaalmehrishi vishaalmehrishi added the pr-linter/exempt-readme The PR linter will not require README changes label Jun 12, 2025
@aws-cdk-automation aws-cdk-automation dismissed their stale review June 12, 2025 08:38

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 3ede678
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Jun 12, 2025

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

@mergify mergify bot merged commit 62130c9 into aws:main Jun 12, 2025
21 of 22 checks passed
@github-actions
Copy link
Contributor

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 12, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/small Small work item – less than a day of effort p1 pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

aws-opensearchservice: Remove default TLS policy

4 participants