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

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

This comment has been minimized.

Copy link
@bagder

bagder Jun 11, 2019

Member

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

This comment has been minimized.

Copy link
@niallor

niallor Jun 11, 2019

Author

Yes, indeed. Sorry.

This comment has been minimized.

Copy link
@niallor

niallor Jun 12, 2019

Author

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

This comment has been minimized.

Copy link
@niallor

niallor Jun 13, 2019

Author

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

This comment has been minimized.

Copy link

commented Jun 11, 2019

@bagder

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

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

@niallor

This comment has been minimized.

Copy link
Author

commented Jun 11, 2019

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

@niallor

This comment has been minimized.

Copy link
Author

commented Jun 11, 2019

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

@niallor niallor force-pushed the niallor:ESNI-build branch 2 times, most recently from e1f6ecc to bd88ea6 Jun 11, 2019
@niallor

This comment has been minimized.

Copy link
Author

commented Jun 11, 2019

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

@bagder

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

no-consecutive-blank-lines

I think you can safely ignore that.

@niallor

This comment has been minimized.

Copy link
Author

commented Jun 12, 2019

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

@bagder

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

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

@niallor niallor force-pushed the niallor:ESNI-build branch 7 times, most recently from 36a4c90 to 1ae4aba Jun 12, 2019
@niallor

This comment has been minimized.

Copy link
Author

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.

@niallor niallor force-pushed the niallor:ESNI-build branch from 1ae4aba to 67fb9ef Jul 24, 2019
@bagder

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

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

@sftcd

This comment has been minimized.

Copy link

commented Aug 16, 2019

@niallor

This comment has been minimized.

Copy link
Author

commented Aug 19, 2019

@bagder

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Author

commented Aug 19, 2019

Thanks for the helpful feedback.

@niallor

This comment has been minimized.

Copy link
Author

commented Aug 19, 2019

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

@bagder

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

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

@niallor

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

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

@bagder bagder referenced this pull request Sep 12, 2019
@niallor niallor force-pushed the niallor:ESNI-build branch from 67fb9ef to 5265d94 Sep 30, 2019
@niallor

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Author

commented Oct 1, 2019

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

@niallor niallor force-pushed the niallor:ESNI-build branch from f63b0c6 to a400d06 Oct 1, 2019
Niall added 2 commits Oct 1, 2019
@bagder

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

Yes, the appveyor failure is unrelated.

@bagder bagder closed this in 0f48055 Oct 2, 2019
@bagder

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

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

@niallor

This comment has been minimized.

Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.