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

ECH: Supplying an ECHConfigList on the command line does not work #16006

Closed
sutaner opened this issue Jan 15, 2025 · 11 comments
Closed

ECH: Supplying an ECHConfigList on the command line does not work #16006

sutaner opened this issue Jan 15, 2025 · 11 comments

Comments

@sutaner
Copy link

sutaner commented Jan 15, 2025

I did this

I am playing around with the ECH support in curl. I did the build steps described in https://github.com/curl/curl/blob/master/docs/ECH.md using a Docker container:

Dockerfile:

FROM alpine:latest

RUN apk add --no-cache \
    git \
    perl \
    build-base \
    linux-headers \
    autoconf \
    automake \
    libtool \
    libpsl \
    libpsl-dev

ENV HOME=/root
WORKDIR $HOME/code

RUN git clone https://github.com/defo-project/openssl \
    && cd openssl \
    && ./config --libdir=lib --prefix=$HOME/code/openssl-local-inst \
    && make -j8 \
    && make install_sw

RUN git clone https://github.com/curl/curl \
    && cd curl \
    && autoreconf -fi \
    && LDFLAGS="-Wl,-rpath,$HOME/code/openssl-local-inst/lib/" ./configure --with-ssl=$HOME/code/openssl-local-inst --enable-ech \
    && make

Doing a curl ECH request using DoH works as expected:

LD_LIBRARY_PATH=$HOME/code/openssl ./src/curl --ech true --doh-url https://one.one.one.one/dns-query https://defo.ie/ech-check.php

Result

...
SSL_ECH_STATUS: success <img src="greentick-small.png" alt="good" /> <br/>
...

But supplying an ECH config via command line argument does NOT work:

LD_LIBRARY_PATH=$HOME/code/openssl ./src/curl --ech ecl:AED+DQA8VgAgACBoEIuIZJ77bgesiZ/k3tarHlAKzNSlmPosivmPykpwBgAEAAEAAQANY292ZXIuZGVmby5pZQAA https
://defo.ie/ech-check.php

Result

...
SSL_ECH_STATUS: not attempted <img src="redx-small.png" alt="bummer" /> <br/>
...

I expected the following

SSL_ECH_STATUS: success when using --ech ecl:...

curl/libcurl version

curl 8.12.0-DEV (aarch64-unknown-linux-musl) libcurl/8.12.0-DEV OpenSSL/3.5.0 libidn2/2.3.7 libpsl/0.21.5
Release-Date: [unreleased]
Protocols: dict file ftp ftps gopher gophers http https imap imaps ipfs ipns mqtt pop3 pop3s rtsp smb smbs smtp smtps telnet tftp ws wss
Features: alt-svc AsynchDNS ECH HSTS HTTPS-proxy IDN IPv6 Largefile NTLM PSL SSL threadsafe TLS-SRP UnixSockets

operating system

Alpine Linux in Docker container (running on macOS 15.3):
Linux 118d1cdf363b 6.10.14-linuxkit #1 SMP Fri Nov 29 17:22:03 UTC 2024 aarch64 Linux

@bagder
Copy link
Member

bagder commented Jan 15, 2025

/cc @sftcd

@bagder bagder added the TLS label Jan 15, 2025
@sftcd
Copy link
Contributor

sftcd commented Jan 15, 2025

will have a look in a sec - probably my fault:-)

@sftcd
Copy link
Contributor

sftcd commented Jan 15, 2025

Ah, the ech command line parsing for "ecl:" and "pn:" is being checked-for and skipped-over twice now. Looks like there was a tidy-up fix in Dec (6bb76d9) that (modified or) put that check into parse_ech whereas I guess before and still it's also being done in setopt_cptr (here) - result is that using "ecl:ecl:...." works now (and presumably "pn:pn:..."). I'll play about with seeing if the obvious fix (not skipping over in setpopt_cptr) has any bad side-effects and if not, make a PR...

@bagder
Copy link
Member

bagder commented Jan 15, 2025

Ah, sorry. My fault for poking on code without properly testing the results...

@sftcd
Copy link
Contributor

sftcd commented Jan 15, 2025

Actually it looks like leaving the skipping-over in setopt_cptr and removing it from parse_ech is better - setopt_cptr doesn't otherwise know if the value is an ECHConfigList or a public_name.

Was there some other reason to do the skipping-over in parse_ech ?

@sftcd
Copy link
Contributor

sftcd commented Jan 15, 2025

Ah, sorry. My fault for poking on code without properly testing the results...

Speaking of testing - it's on my list to add some tests for ECH but I've been holding back as I'm not sure how to add a test for running the ECH protocol without the test harness somehow including an ECH-enabled TLS server, which is a bit daunting. Any ideas there welcome. (I'll send a mail on that though, off-topic for this issue.)

@bagder
Copy link
Member

bagder commented Jan 15, 2025

Was there some other reason to do the skipping-over in parse_ech ?

Just me thinking the prefixes are sent in to the setopt option but they don't need to be stored in the data->set.str[] strings.

setopt_cptr doesn't otherwise know if the value is an ECHConfigList or a public_name.

Does it need to? It is a provided config set by the application.

@sftcd
Copy link
Contributor

sftcd commented Jan 15, 2025

Does it need to? It is a provided config set by the application.

Currently, yes. setopt_cptr just knows the value is something to do with ECH but has to parse whether the value is e.g. hard or ecl:... or pn:...

I forget why exactly, but I think if the config was provided by a libcurl caller, then maybe parse_ech wasn't called or something?

@bagder
Copy link
Member

bagder commented Jan 15, 2025

I'm not sure I'm following. The setopt_cptr() function does check for the prefixes now. parse_ech is the command line parser, it is not in library so that sounds sane.

@sftcd
Copy link
Contributor

sftcd commented Jan 15, 2025

I'm not sure I'm following. The setopt_cptr() function does check for the prefixes now. parse_ech is the command line parser, it is not in library so that sounds sane.

Right, probably just me explaining badly:-) parse_ech checks the command line stuff, and is able to read the ECHConfigList from a file or stdin, but setopt_cptr has to also do the checks in case the caller is a libcurl application rather than the command line tool. (But setopt_cptr doesn't support reading from a file or stdin.)

@bagder
Copy link
Member

bagder commented Jan 15, 2025

setopt_cptr has to also do the checks in case the caller is a libcurl application rather than the command line tool

Why? What would be the difference? the command line tool is just a libcurl application.

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

No branches or pull requests

3 participants