feat(cloudfront): warn users that minimumProtocolVersion defaults to TLSv1 without a custom certificate#35416
feat(cloudfront): warn users that minimumProtocolVersion defaults to TLSv1 without a custom certificate#35416
Conversation
Add validation to prevent setting minimumProtocolVersion and sslSupportMethod without providing a custom certificate, which would cause CloudFormation deployment errors. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Add integration test to verify that minimumProtocolVersion and sslSupportMethod validation works correctly with and without custom certificates. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
@badmintoncryer |
Add CloudFormation template snapshots for the certificate validation integration test. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Fix trailing whitespace errors in CloudFront distribution tests. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
| // Validate that minimumProtocolVersion and sslSupportMethod are only specified with a certificate | ||
| if (!props.certificate) { | ||
| if (props.minimumProtocolVersion !== undefined) { | ||
| throw new ValidationError('minimumProtocolVersion can only be specified when using a custom certificate. Use the \'certificate\' property to provide a certificate.', this); |
There was a problem hiding this comment.
Throwing an error could potentially be a breaking change for existing users. Could we show a warning instead? We want to let the users know that CloudFront will default to TLSv1. The error message would change accordingly
There was a problem hiding this comment.
Thank you for the feedback! I've updated the implementation to use Annotations.addWarningV2() instead of throwing a ValidationError.
| new IntegTest(app, 'cloudfront-distribution-certificate-validation-test', { | ||
| testCases: [stack], | ||
| }); | ||
|
|
There was a problem hiding this comment.
You can consider leaving an assertion to verify that the test succeeds. Alternatively, try mentioning it in the PR why an assertion was not needed. The Integration Test guide has some helpful examples!
https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md
There was a problem hiding this comment.
Thank you for the feedback. I've added assertions to the integration test to verify the CloudFront behavior.
| throw new ValidationError('sslSupportMethod can only be specified when using a custom certificate. Use the \'certificate\' property to provide a certificate.', this); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Likewise for sslSuportMethod
Based on PR feedback: - Change ValidationError to warning when minimumProtocolVersion or sslSupportMethod are specified without a certificate - Update unit tests to verify warnings instead of errors - Add assertions to integration test to verify CloudFront behavior This maintains backward compatibility while informing users that CloudFront will use defaults (TLSv1 for protocol, SNI for method) when no custom certificate is provided. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
@dgandhi62 Thanks for the review! I've updated the PR based on your feedback:
Ready for re-review when you have time. Thank you 🙇 |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing To prevent automatic closure:
This PR will automatically close in 14 days if no action is taken. |
|
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #35404
Reason for this change
Currently, AWS CDK allows setting minimumProtocolVersion on a CloudFront Distribution without requiring a custom SSL/TLS certificate. However, CloudFront silently ignores this setting when using its default certificate, defaulting to TLSv1 regardless of the configured value. This creates a security vulnerability where developers believe their distribution enforces TLS 1.2+ when it actually accepts TLS 1.0 and 1.1.
Description of changes
Added validation in the Distribution constructor to throw an error when minimumProtocolVersion is specified without a custom certificate
Added validation for sslSupportMethod as well, as it has the same dependency on custom certificates
The validation ensures that developers are explicitly aware that these security settings require a custom certificate to take effect
Added comprehensive unit tests to verify the validation logic
The validation logic checks if either minimumProtocolVersion or sslSupportMethod are set without a certificate, and throws a descriptive error message guiding users to provide a custom certificate.
Alternative considered: Adding a warning instead of an error for backward compatibility. However, given the security implications of this misconfiguration, a breaking change with clear error messaging was deemed more appropriate to prevent security vulnerabilities.
Describe any new or updated permissions being added
No new or updated IAM permissions are required for this change.
Description of how you validated changes
Added unit tests to verify that an error is thrown when minimumProtocolVersion is set without a certificate
Added unit tests to verify that an error is thrown when sslSupportMethod is set without a certificate
Added unit tests to confirm that no error is thrown when these properties are set with a valid certificate
Verified that existing tests pass with the new validation
Manually tested the changes by attempting to deploy a Distribution with the problematic configuration and confirming the error is raised during synthesis
Checklist
My code adheres to the [CONTRIBUTING GUIDE](https:/ /github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and
DESIGN GUIDELINES
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license