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.

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

Add IDN for Apple and iOS via System #5371

Closed
wants to merge 5 commits into from
Closed

Add IDN for Apple and iOS via System #5371

wants to merge 5 commits into from

Conversation

MonkeybreadSoftware
Copy link
Contributor

@MonkeybreadSoftware MonkeybreadSoftware commented May 10, 2020

This is a proposal for a better way to implement #5330 with using threaded resolver.
We ask MacOS and iOS to provide us the canonical name for IDN domains by setting AI_CANONNAME and then using the canon name as new host name for the HTTP request.

Set AI_CANONNAME flag for MacOS
Let IDN domain go through for Apple
Set AI_CANONNAME flag for MacOS
Set AI_CANONNAME flag for MacOS
When we get a ai_canonname, use it as new host name.
@@ -756,6 +756,14 @@ Curl_addrinfo *Curl_resolver_getaddrinfo(struct connectdata *conn,
hints.ai_socktype = (conn->transport == TRNSPRT_TCP)?
SOCK_STREAM : SOCK_DGRAM;

#ifdef USE_LIBIDN2
/* done earlier */
#elif defined(__APPLE__)
Copy link
Member

@nickzman nickzman May 10, 2020

Choose a reason for hiding this comment

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

This should probably be made a configure option, shouldn't it?

Copy link
Contributor Author

@MonkeybreadSoftware MonkeybreadSoftware May 10, 2020

Choose a reason for hiding this comment

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

Well, if you configure to use LibIDN, we assume you want to use LibIDN instead of some OS behavior.

Copy link
Member

@nickzman nickzman May 10, 2020

Choose a reason for hiding this comment

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

I have two concerns about always enabling this on Apple operating systems: (1) some people might want to turn IDNs off (usually for security reasons), and (2) it's also a configure option on Windows, so making it an option on Apple operating systems would maintain consistency.

Copy link
Contributor Author

@MonkeybreadSoftware MonkeybreadSoftware May 10, 2020

Choose a reason for hiding this comment

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

Feel free to make the changes for another configure option if you like.

@bagder
Copy link
Member

bagder commented May 10, 2020

  1. Should AI_CANONICAL perhaps only be set if the name is an IDN one? It will save memory if nothing else.
  2. The CURL_VERSION_IDN should also be set for builds that support IDN this way.
  3. Can we tell if there are older macOS versions where people might build curl that don't support IDN this way?
  4. I second @nickzman that it makes sense to allow users to switch this off. I volunteer to write the configure logic for that.

@MonkeybreadSoftware
Copy link
Contributor Author

MonkeybreadSoftware commented May 11, 2020

After some search I found the WWDC video from 2016:
https://developer.apple.com/videos/play/wwdc2016/714/

Starting now in iOS 10 and macOS Sierra ... with the same UTF-8 input, we will now translate that automatically to Punycode...

So MacOS 10.12 and iOS 10 will do it.

@bagder
Copy link
Member

bagder commented May 11, 2020

A basic --disable-macidn for configure could work like this: 9168fc8

@bagder
Copy link
Member

bagder commented May 14, 2020

(2) of my bullet points might be the trickiest...

@bagder bagder added name lookup DNS and related tech needs-info labels Jun 28, 2020
@bagder
Copy link
Member

bagder commented Aug 12, 2020

I propose we make this a TODO item and close this PR.

@bagder bagder closed this in 15f5d59 Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
name lookup DNS and related tech needs-info
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants