Skip to content

Handle TLS 1.3 bad cert errors in proxy handler #2182

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Dec 7, 2020
Merged

Handle TLS 1.3 bad cert errors in proxy handler #2182

merged 2 commits into from
Dec 7, 2020

Conversation

zoewangg
Copy link
Contributor

@zoewangg zoewangg commented Dec 4, 2020 •
edited
Loading

Description

Fixed the issue where BAD_CERTIFICATE issue manifested as acquire connection timeout error when using TLS1.3 and proxy.

Motivation and Context

Requests w/ proxy

  • Before this change
TLS version BAD_CERTIFICATE error
TLS 1.2 java.io.IOException: Unable to send CONNECT request to proxy
TLS 1.3 Acquire operation took longer than the configured maximum time
  • After this change
TLS version BAD_CERTIFICATE error
TLS 1.2 java.io.IOException: Unable to send CONNECT request to proxy
TLS 1.3 java.io.IOException: Unable to send CONNECT request to proxy

Requests w/o proxy

  • Before this change
TLS version BAD_CERTIFICATE error
TLS 1.2 The channel was closed...
TLS 1.3 Server failed to send complete response
  • After this change
TLS version BAD_CERTIFICATE error
TLS 1.2 The channel was closed...
TLS 1.3 Server failed to send complete response. The channel was closed...

Testing

Added unit tests.

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed
  • A short description of the change has been added to the CHANGELOG
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@zoewangg zoewangg force-pushed the zoewang/fixNettyProxy-TLS1.3 branch 2 times, most recently from e088330 to 07322d2 Compare December 4, 2020 01:42
millems
millems reviewed Dec 4, 2020
View reviewed changes
...io-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyUtils.java Outdated
Comment on lines 45 to 46
"by the service (e.g. because there was a handshake error[use -Djavax"
+ ".net.debug=ssl to enable SSL logs], the request "
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way we can detect that it was a handshake error so that they don't have to enable SSL logs? I wouldn't want every customer having to enable SSL logs to see if it was a handshake error just a plain-ol'-closed-channel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, unfortunately, this kind of errors are bubbled up as ClosedChannelException with no details at all. When I was troubleshooting the failing test, it took me while to figure out it was related to handshake.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wish we could link to docs that help customers debug these errors. Maybe something we can talk to @Carey-AWS about. In the meantime, it's really hard to read this with the parens-in-parens for the "use -D...". Can we exclude that? The other potential causes we list don't say how to diagnose them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1. Yeah, a guide on how to troubleshoot closed channel exception would be super helpful!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for calling this out, @millems. I've added it to the backlog.

@zoewangg zoewangg force-pushed the zoewang/fixNettyProxy-TLS1.3 branch 2 times, most recently from 658a3df to babd601 Compare December 4, 2020 23:29
dagnir
dagnir approved these changes Dec 4, 2020
View reviewed changes
@zoewangg zoewangg force-pushed the zoewang/fixNettyProxy-TLS1.3 branch from babd601 to d102e8a Compare December 7, 2020 18:47
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 7, 2020

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

75.9% 75.9% Coverage
0.0% 0.0% Duplication

@zoewangg zoewangg merged commit a235d0a into master Dec 7, 2020
@zoewangg zoewangg deleted the zoewang/fixNettyProxy-TLS1.3 branch December 22, 2020 22:46
dagnir added a commit that referenced this pull request Oct 19, 2022
@dagnir
Don't generate duplicate client context params (#2182)
645f8a6 
For S3, the client context params are duplicated on on its ServiceConfiguration.
For these duplicates, just keep them in the ServiceConfiguration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@millems millems millems left review comments

@Carey-AWS Carey-AWS Carey-AWS left review comments

@dagnir dagnir dagnir approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zoewangg @dagnir @millems @Carey-AWS