Skip to content
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

Fix a bug where a tailing dot is not stripped for SNI #6046

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix a bug where a tailing dot is not stripped for SNI #6046

wants to merge 1 commit into from

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Dec 23, 2024 •
edited
Loading

Motivation:

A trailing dot specified in a hostname can be used to make DNS queries to avoid using search domains. However, the trailing dot should be removed for SNI.

https://datatracker.ietf.org/doc/html/rfc6066#section-3

The hostname is represented as a byte string using ASCII encoding
without a trailing dot.

Modifications:

  • Add Endpoint.withTrailingDot() to remove a trailing dot if present.
  • Use an Endpoint without a trailing dot to create a remote address which is used for SNI.

Result:

@ikhoon
Fix a bug where a tailing dot is not stripped for SNI
21c26dc 
Motivation:

A trailing dot specified in an hostname can be used to make DNS queries
to avoid using search domains. However, the trailing dot should be
removed for SNI.

https://datatracker.ietf.org/doc/html/rfc6066#section-3
> The hostname is represented as a byte string using ASCII encoding
> without a trailing dot.

Modifications:

- Add `Endpoint.withTrailingDot()` to remove a trailing dot if exists.
- Use an `Endpoint` without a trailing dot create an remote address
  which is used for SNI.

Result:

- Fixed a bug where a trailing dot was included in the hostname used by SNI.
- Closes line#6044
@ikhoon ikhoon added the defect label Dec 23, 2024
@ikhoon ikhoon added this to the 1.32.0 milestone Dec 23, 2024
@github-actions GitHub Actions
Copy link
Contributor

🔍 Build Scan® (commit: 21c26dc)

Job name Status Build Scan®
build-ubicloud-standard-8-jdk-8 ✅ https://ge.armeria.dev/s/o2prevayx5dh6
build-ubicloud-standard-8-jdk-21-snapshot-blockhound ✅ https://ge.armeria.dev/s/np7nw7ltjrkrs
build-ubicloud-standard-8-jdk-17-min-java-17-coverage ✅ https://ge.armeria.dev/s/essebydikizcg
build-ubicloud-standard-8-jdk-17-min-java-11 ✅ https://ge.armeria.dev/s/zlaoi77yltjby
build-ubicloud-standard-8-jdk-17-leak ❌ (failure) https://ge.armeria.dev/s/yamk55vvqt5c6
build-ubicloud-standard-8-jdk-11 ✅ https://ge.armeria.dev/s/bv3dexjvhbqvm
build-macos-latest-jdk-21 ✅ https://ge.armeria.dev/s/s3c4pnecgxonk

minwoox
minwoox approved these changes Dec 23, 2024
View reviewed changes
Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Add Endpoint.withTrailingDot() to remove a trailing dot if it exists.

Need to change the description. 😉

jrhee17
jrhee17 approved these changes Dec 30, 2024
View reviewed changes
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

👍 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@minwoox minwoox minwoox approved these changes

@jrhee17 jrhee17 jrhee17 approved these changes

@trustin trustin Awaiting requested review from trustin trustin is a code owner

Assignees
No one assigned
Labels
defect
Projects
None yet
Milestone
1.32.0
Development

Successfully merging this pull request may close these issues.

SNI Server Name Missing in ClientHello for Domain with Trailing Dot
3 participants
@ikhoon @jrhee17 @minwoox