Add TrustAllCertificates in CRT S3 Client options #3903
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joviegas
force-pushed
the
joviegas/trust_certificate
branch
from
e526312 to
5a66a9d
Compare
zoewangg
reviewed
Apr 11, 2023
| * @param shouldTrustAllCertificates True if SSL cert validation is disabled. | ||
| * @return The builder of the method chaining. | ||
| */ | ||
| Builder shouldTrustAllCertificates(Boolean shouldTrustAllCertificates); |
There was a problem hiding this comment.
Maybe trustAllCertificatesEnabled to be consistent with existing boolean options?
| * This turns off x.509 validation. | ||
| * By default, this option is off. | ||
| * Only enable this option for testing purposes. | ||
| * </p> |
There was a problem hiding this comment.
nit: </p> is not needed; a single <p> should be sufficient
|
|
||
| if (builder.httpConfiguration != null | ||
| && builder.httpConfiguration.shouldTrustAllCertificates() != null) { | ||
| clientTlsContextOptions.withVerifyPeer(!builder.httpConfiguration.shouldTrustAllCertificates()); |
There was a problem hiding this comment.
Should we log a warning message here if it's enabled?
SSL Certificate verification is disabled. This is not a safe setting and should only be used for testing.
There was a problem hiding this comment.
I had following concern
-> User had disabled the SSL certificate validation for testing but user doesnot want anyone to know about it. If we are loggin it then we are making this info available to everyone who is accessing the logs.
Because of this this reason I was not sure if we should log this , what do you say about this scenario ?
There was a problem hiding this comment.
We log a warning message in other HTTP clients, so it should be fine :)
https://github.com/aws/aws-sdk-java-v2/blob/master/http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/ApacheHttpClient.java#L698-L701
zoewangg
approved these changes
Apr 18, 2023
|
SonarCloud Quality Gate failed. |
aws-sdk-java-automation
added a commit
that referenced
this pull request
Apr 19, 2023
…#2467) * Update to next snapshot version: 2.20.49-SNAPSHOT * Fix flaky IMDS client test (#3912) This test should be less flaky since the mock client should never trigger the retry logic in the IMDS client by sometimes returning a 500. * Add TrustAllCertificates in CRT S3 Client options (#3903) * Add TrustAllCertificates in CRT S3 Client options * Handled PR comments * Amazon Relational Database Service Update: Adds support for the ImageId parameter of CreateCustomDBEngineVersion to RDS Custom for Oracle * AWS SecurityHub Update: Update that adds SDK code examples for Security Hub * Amazon Comprehend Update: This release supports native document models for custom classification, in addition to plain-text models. You train native document models using documents (PDF, Word, images) in their native format. * Amazon Simple Storage Service Update: Provides support for "Snow" Storage class. * Amazon EC2 Container Service Update: This release supports the Account Setting "TagResourceAuthorization" that allows for enhanced Tagging security controls. * AWS Secrets Manager Update: Documentation updates for Secrets Manager * AWS S3 Control Update: Provides support for overriding endpoint when region is "snow". This will enable bucket APIs for Amazon S3 Compatible storage on Snow Family devices. * AWS Resource Access Manager Update: This release adds support for customer managed permissions. Customer managed permissions enable customers to author and manage tailored permissions for resources shared using RAM. * Updated endpoints.json and partitions.json. * Release 2.20.49. Updated CHANGELOG.md, README.md and all pom.xml. --------- Co-authored-by: AWS <> Co-authored-by: Dongie Agnir <[email protected]> Co-authored-by: John Viegas <[email protected]>
millems
added a commit
that referenced
this pull request
Apr 19, 2023
* Pull request: release <- staging/31d9787e-9a7d-4544-bbf2-ffb44eac702a (#2467) * Update to next snapshot version: 2.20.49-SNAPSHOT * Fix flaky IMDS client test (#3912) This test should be less flaky since the mock client should never trigger the retry logic in the IMDS client by sometimes returning a 500. * Add TrustAllCertificates in CRT S3 Client options (#3903) * Add TrustAllCertificates in CRT S3 Client options * Handled PR comments * Amazon Relational Database Service Update: Adds support for the ImageId parameter of CreateCustomDBEngineVersion to RDS Custom for Oracle * AWS SecurityHub Update: Update that adds SDK code examples for Security Hub * Amazon Comprehend Update: This release supports native document models for custom classification, in addition to plain-text models. You train native document models using documents (PDF, Word, images) in their native format. * Amazon Simple Storage Service Update: Provides support for "Snow" Storage class. * Amazon EC2 Container Service Update: This release supports the Account Setting "TagResourceAuthorization" that allows for enhanced Tagging security controls. * AWS Secrets Manager Update: Documentation updates for Secrets Manager * AWS S3 Control Update: Provides support for overriding endpoint when region is "snow". This will enable bucket APIs for Amazon S3 Compatible storage on Snow Family devices. * AWS Resource Access Manager Update: This release adds support for customer managed permissions. Customer managed permissions enable customers to author and manage tailored permissions for resources shared using RAM. * Updated endpoints.json and partitions.json. * Release 2.20.49. Updated CHANGELOG.md, README.md and all pom.xml. --------- Co-authored-by: AWS <> Co-authored-by: Dongie Agnir <[email protected]> Co-authored-by: John Viegas <[email protected]> * Remove unnecessary ACL integration test for S3. (#2468) * Update to next snapshot version: 2.20.50-SNAPSHOT --------- Co-authored-by: aws-sdk-java-automation <[email protected]> Co-authored-by: Dongie Agnir <[email protected]> Co-authored-by: John Viegas <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
Modifications
Based on https://github.com/aws/aws-sdk-java-v2/blob/master/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/AwsCrtAsyncHttpClient.java#L103 added a option in S3CrtHttpConfiguration class to disable TrustAllCertificates options
Testing
Screenshots (if appropriate)
Types of changes
License