replace TLSv1.2_2019 with TLSv1.2_2021 as default policy #294
Conversation
hans-d
added
wip
|
/terratest |
|
Thanks @jamerply for creating this pull request! A maintainer will review your changes shortly. Please don't be discouraged if it takes a while. While you wait, make sure to review our contributor guidelines. Tip Need help or want to ask for a PR review to be expedited?Join us on Slack in the |
There was a problem hiding this comment.
The problem is, this is a potentially breaking change that will manifest as some old clients failing to connect all of a sudden, a problem which will be difficult to trace back to updating this component.
I think instead we should just recommend that users explicitly set this to the newer version.
If users have pinned their modules to a particular version per CloudPosse's own recommendation, wouldn't that prevent the breaking change unless they knowingly updated to the newer version? |
|
@Nuru thoughts on just making this a major revision change? I agree that we should upgrade this for future consumers of this module, otherwise we're encouraging an old and outdated TLS. The other route is to remove the default altogether, but either way we'll want to do a major version rev. Let me know your thoughts and I can work with @jamerply to push this forward. |
There was a problem hiding this comment.
Just came across this, think this would be good to have as default as it's officially recommended both on the docs (as the PR noted) and on the AWS console. I think a major bump would make sense in terms of @Nuru's concern about the potentially breaking change and backwards compatbility
|
💥 This pull request now has conflicts. Could you fix it @jamerply? 🙏 |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe changes update the default TLS protocol version for CloudFront in both documentation and Terraform configuration files. The default for Changes
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
variables.tf (1)
13-22: Add validation to restrict protocol version values.Consider adding a
validationblock to enforce thatvar.minimum_protocol_versioncan only be"","TLSv1", or one of the supportedTLSv1.2_*versions to prevent typos or unsupported values.variable "minimum_protocol_version" { type = string description = <<-EOT Cloudfront TLS minimum protocol version. If `var.acm_certificate_arn` is unset, only "TLSv1" can be specified. See: ... Defaults to "TLSv1.2_2021" unless `var.acm_certificate_arn` is unset, in which case it defaults to `TLSv1` EOT default = "" + validation { + condition = var.minimum_protocol_version == "" || + contains(["TLSv1","TLSv1.2_2019","TLSv1.2_2021"], var.minimum_protocol_version) + error_message = "minimum_protocol_version must be one of \"TLSv1\", \"TLSv1.2_2019\", or \"TLSv1.2_2021\"" + } }main.tf (1)
55-57: Verify default TLS logic and handle breaking change.The logic now defaults to
"TLSv1.2_2021"when a custom ACM certificate is provided, and"TLSv1"otherwise, aligning with AWS recommendations. Since this may break compatibility for older clients, please ensure you communicate this change clearly (e.g., via a major version bump and changelog entry).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
README.mdis excluded by!README.md
📒 Files selected for processing (3)
docs/terraform.md(1 hunks)main.tf(1 hunks)variables.tf(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (2)
variables.tf (1)
19-20: Update variable description for TLS default version.The description now correctly states the new default of
TLSv1.2_2021when an ACM certificate ARN is provided, andTLSv1otherwise. This aligns with the change in CloudFront’s recommended security policy.docs/terraform.md (1)
128-129: Update documentation to reflect new default TLS protocol.This entry now correctly documents the default as
"TLSv1.2_2021"when an ACM certificate is provided and"TLSv1"otherwise, matching the code changes in the module.🧰 Tools
🪛 LanguageTool
[typographical] ~129-~129: Consider adding a comma after ‘Usually’ for more clarity.
Context: ... name | ID element. Usually the component or solution name, e.g. 'a...(RB_LY_COMMA)
[uncategorized] ~129-~129: Possible missing preposition found.
Context: ...ID element not also included as atag.
The "name" tag is set to the fullids...(AI_EN_LECTOR_MISSING_PREPOSITION)
|
/terratest |
Gowiem
added
enhancement
|
/terratest |
|
@oycyc agreed. I'm taking this internally to the Cloud Posse folks and asking for another review so that we move this forward. Thanks for reviving this PR with your comment 👍 Sadly, we also have a test failure here: I'll ask if we've seen that before. |
|
@Gowiem Regarding the Golang tests, see golang/go#59305 (comment) @oycyc The problem with upgrades is that people are using automated tools to upgrade to new versions. This module has not matured to v1.0 semantic versioning, and v0 versioning works differently. That makes this upgrade a bigger deal, and IMHO the gain is not worth it. This is not the end of the discussion, but my leaning at the moment is toward just having a documentation update recommending people set the TLS version to the latest version explicitly. We can update the default when we get to v2 (which may not be for a long time). |
what
This PR updates the
mimimum_protocol_versionvariable so that it defaults toTLSv1.2_2021(the current recommended security policy recommended by AWS) instead ofTLSv1.2_2019.why
The most current security policy is no longer
TLSv1.2_2019but isTLSv1.2_2021.references
See the "Security Policy" heading under the "Distribution Setting" section of the AWS CloudFront Documentation for further information.