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

TLS: Provide ESNI support framework for curl and libcurl #4011

Closed
wants to merge 12 commits into from

Conversation

niallor
Copy link

@niallor niallor commented Jun 11, 2019

TLS: Provide ESNI support framework for curl and libcurl

The proposed change provides a framework to facilitate work to
implement ESNI support in curl and libcurl. It is not intended
either to provide ESNI functionality or to favour any particular
TLS-providing backend. Specifically, the change reserves a
feature bit for ESNI support (symbol CURL_VERSION_ESNI),
implements setting and reporting of this bit, includes dummy
book-keeping for the symbol, adds a build-time configuration
option (--enable-esni), provides an extensible check for
resources available to provide ESNI support, and defines a
compiler pre-processor symbol (USE_ESNI) accordingly.

Proposed-by: @niallor (Niall O'Reilly)
Encouraged-by: @sftcd (Stephen Farrell)
See-also: this message
Limitations:

  • Book-keeping (symbols-in-versions) needs real release number, not 'DUMMY'.
  • Framework is incomplete, as it covers autoconf, but not cmake.
  • Check for available resources, although extensible, refers only
    to specific work in progress (described here) to implement ESNI for OpenSSL,
    as this is the immediate motivation for the proposed change.

configure.ac Show resolved Hide resolved
if test "x$ESNI_ENABLED" = "x1"; then
SUPPORT_FEATURES="$SUPPORT_FEATURES ESNI"
fi

Copy link
Member

Choose a reason for hiding this comment

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

You could squash all your configure commits into a single one to reduce the number of commits and make the PR easier to review...

Copy link
Author

Choose a reason for hiding this comment

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

Yes, indeed. Sorry.

Copy link
Author

Choose a reason for hiding this comment

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

Do you need me to consolidate this, or is it tolerable as it is just this once?

Copy link
Author

Choose a reason for hiding this comment

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

Done. I began to feel that the jumble was hurting even my head. I felt it was useful to use two commits (rather than just one) in order to keep handling of the command-line token --enable-esni separate from applying the corresponding configuration.

@sftcd
Copy link

sftcd commented Jun 11, 2019 via email

@bagder
Copy link
Member

bagder commented Jun 11, 2019

(The appveyor build issue is due to #3770 and not your fault)

@niallor
Copy link
Author

niallor commented Jun 11, 2019

Appveyor: I was pretty sure I hadn't introduced anything to do with SMB. Thanks for confirming.

@niallor
Copy link
Author

niallor commented Jun 11, 2019

I'm waiting to see what Travis reports before making any changes.

@niallor niallor force-pushed the ESNI-build branch 2 times, most recently from e1f6ecc to bd88ea6 Compare June 11, 2019 23:19
@niallor
Copy link
Author

niallor commented Jun 11, 2019

Codacy is reporting a violation of the "no-consecutive-blank-lines" rule. I can't see why.

@bagder
Copy link
Member

bagder commented Jun 12, 2019

no-consecutive-blank-lines

I think you can safely ignore that.

@niallor
Copy link
Author

niallor commented Jun 12, 2019

Unless I've missed something, components flagged in coverage check seem to be outside scope of this PR.

@bagder
Copy link
Member

bagder commented Jun 12, 2019

yeah, ignore coveralls. It goes totally crazy at times with no explanation available.

@niallor niallor force-pushed the ESNI-build branch 7 times, most recently from 36a4c90 to 1ae4aba Compare June 17, 2019 15:34
@niallor
Copy link
Author

niallor commented Jul 23, 2019

After taking time to learn how to drive libcurl from the curl tool, and OpenSSL from libcurl, achieved apparently successful interaction with ESNI-aware server today. Much tidying-up remains to be done.

@bagder
Copy link
Member

bagder commented Aug 16, 2019

As a fan of ESNI I'm curious, how's this going?

@sftcd
Copy link

sftcd commented Aug 16, 2019 via email

@niallor
Copy link
Author

niallor commented Aug 19, 2019 via email

@bagder
Copy link
Member

bagder commented Aug 19, 2019

Cool, thanks for this write-up. (in my current poll for what users would like to see in our roadmap going forward, ESNI is one of the top rated choices)

I think I will get a better understanding of things once your HOWTO becomes real and we can play and see what it entails. Here's some initial comments in the mean time:

  1. I would encourage you to work hard on first minimizing the necessary OpenSSL patching and then to take your proposed changes to them sooner rather than later. If you're asking for API changes, those take time.
  2. Extracting the server name is easy. curl already parses the URL and keeps the host name around internally in an easily accessible struct.
  3. We need to discuss how to make curl pull the required data off DNS for ESNI. Currently, the only DNS-oriented code we have use c-ares or DoH, and I would probably suggest that a first shot at this is done with a build and transfer that use one of those. We can then discuss later on how we can do this for regular transfers with a non-c-ares-built curl.
  4. We should also check if this can be done in a QUIC friendly way too. I haven't checked how/if ESNI works the same way there.

@niallor
Copy link
Author

niallor commented Aug 19, 2019

Thanks for the helpful feedback.

@niallor
Copy link
Author

niallor commented Aug 19, 2019

I'm (currently) minded to explore DoH first, as this gives privacy in transit.

@bagder
Copy link
Member

bagder commented Aug 21, 2019

Makes perfect sense for a start, and for example Firefox still only supports ESNI when using DoH.

@niallor
Copy link
Author

niallor commented Sep 10, 2019

@bagder, as you put it, "We need to discuss how to make curl pull the required data off DNS for ESNI." Where is best: here, on curl-library list, offline in P2P email, or somewhere I haven't thought of?

@niallor
Copy link
Author

niallor commented Sep 10, 2019

@sftcd is following up on point 1 above. I've dealt with point 2, but will hold off on updating the branch that this PR depends on, likely at least until ESNI data is being fetched from DNS within libcurl.

@bagder
Copy link
Member

bagder commented Sep 10, 2019

The curl-library list is the best place for discussions, questions and brain storming.

@niallor
Copy link
Author

niallor commented Sep 30, 2019

This PR is just for reserving a code-point and implementing configuration- and build-time support for it. I don't expect to make further changes, unless to correct code-point collisions as in 5265d94.
If this PR were merged soon, other people minded to do ESNI work would have a code-point and configuration/build framework to work from. Is there more that I need to do so that this can happen?

@bagder
Copy link
Member

bagder commented Sep 30, 2019

  1. It's an usual procedure and ask. I generally dislike merging code that only adds building blocks for a potentially future work since that future work can take a long time or even never happen and then we've only merged ourselves a lot of work. I typically prefer to wait until we can actually merge something that can be used, at least to some extent by some eager users. Are you saying that merging this PR "early" will actually help you work on ESNI better/easier/faster?

  2. I propose you rename you docs file to plain and simple ESNI.md so that it can be the home of all ESNI related docs and in-progress notes.

  3. if this is actually aimed at merging the [WIP] prefix should probably be removed as that suggests something else...

@niallor
Copy link
Author

niallor commented Sep 30, 2019

  1. Yes (to final question). OTOH, if you prefer to have "something that can be used, at least to some extent by some eager users", then maybe I should close this and submit another PR which allows building something which can interoperate with existing servers, such as the targets which have been used in demo, Cloudflare and DEfO. It seems to me that you should call this according to whether you prefer multiple, compartmentalized, merges or rather a big one, kitchen-sink style.
  2. Fair enough: happy to do, but depends on your call (see 1).
  3. See 2.
    Thanks.

@bagder
Copy link
Member

bagder commented Sep 30, 2019

Thanks. Okay, I'm game. Let's work on getting this PR merged as a foundation to ease the ongoing work of getting the rest of the ESNI work going.

@bagder bagder changed the title [WIP] TLS: Provide ESNI support framework for curl and libcurl TLS: Provide ESNI support framework for curl and libcurl Sep 30, 2019
@niallor
Copy link
Author

niallor commented Sep 30, 2019

Great.

In docs/libcurl/symbols-in-versions, a release number is needed (instead of DUMMY) for when the symbol CURL_VERSION_ESNI will have been introduced. Should I update the PR with a real number, or is this something you would usually do afterwards? If the former, I'll need you to tell me the number.

You've dealt with item 3 (removing the [WIP] prefix.

I should look after item 2.

@bagder
Copy link
Member

bagder commented Sep 30, 2019

Go with 7.67.0 there and yes please update the PR accordingly. (We have 9 days before the window closes for that, but it seems that should be enough!)

@niallor
Copy link
Author

niallor commented Oct 1, 2019

AppVeyor failures don't look like they have to do with this PR. Trying rebase ...

@bagder
Copy link
Member

bagder commented Oct 2, 2019

Yes, the appveyor failure is unrelated.

@bagder bagder closed this in 0f48055 Oct 2, 2019
@bagder
Copy link
Member

bagder commented Oct 2, 2019

Thanks. You'll notice that I did some minor edits before merge.

@niallor
Copy link
Author

niallor commented Oct 2, 2019

Great. Thanks for your patience with all the learning I had to do to get this far; next PR should be easier as a result. It will have enough function for an enthusiast to demonstrate interoperability against an ESNI-aware server.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants