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

tests: support multiple HTTPS RRs for ECH configs #504

Merged
merged 2 commits into from
Dec 16, 2024

Conversation

cpu
Copy link
Member

@cpu cpu commented Dec 14, 2024

Similar to a change in the upstream Rustls ech-client.rs demo we want to be able to process multiple HTTPS records for a given domain, and look at each ECH config list from each record for a potential compatible config. Follow-up to #485.

Mechanically this means:

  1. Updating the test/ech_fetch.rs helper to support writing multiple .bin files when there are multiple HTTPS records w/ ECH configs. The tool now outputs to stdout a comma separated list of the files it writes to make it easier to use with the client.c example.

  2. Updating the tests/client.c example to treat the ECH_CONFIG_LIST env var as a comma separated list of ECH config lists. We now loop through each and only fail if all of the provided files are unable to be used to configure the client config with a compatible ECH config.

Doing string manipulation with C remains "a delight". For Windows compat we achieve tokenizing the string by the comma delim with a define to call either strtok_r with GCC/clang, or strtok_s with MSCV.

You can test this update with:

ECH_CONFIG_LISTS=$(cargo test --test ech_fetch -- curves1-ng.test.defo.ie /tmp/curves1-ng.test.defo.ie)
RUSTLS_PLATFORM_VERIFIER=1 ECH_CONFIG_LIST="$ECH_CONFIG_LISTS" ./cmake-build-debug/tests/client curves1-ng.test.defo.ie 443 /echstat.php?format=json

If you're unlucky and the first HTTPS record served is the one with invalid configs you should see output like the following showing the client skipping over the .1 config list and using the .2 one instead:

client[188911]: no compatible/valid ECH configs found in '/tmp/curves1-ng.test.defo.ie.1'
client[188911]: using ECH with config list from '/tmp/curves1-ng.test.defo.ie.2'

This logic will also need to be ported into #497. I'll do that shortly but expect we can land this PR first against the pre-existing client.c and then catch 497 up afterwards.

Resolves #503

Similar to a change in the upstream Rustls ech-client.rs demo we want to
be able to process _multiple_ HTTPS records for a given domain, and look
at each ECH config list from each record for a potential compatible
config.

Mechanically this means:

1. Updating the `test/ech_fetch.rs` helper to support writing multiple
   `.bin` files when there are multiple HTTPS records w/ ECH configs.
   The tool now outputs to stdout a comma separated list of the files it
   writes to make it easier to use with the `client.c` example.

2. Updating the `tests/client.c` example to treat the
   `RUSTLS_ECH_CONFIG_LIST` env var as a comma separated list of ECH
   config lists. We now loop through each and only fail if all of the
   provided files are unable to be used to configure the client config
   with a compatible ECH config.

Doing string manipulation with C remains "a delight". For Windows compat
we achieve tokenizing the string by the comma delim with a define to
call either `strtok_r` with GCC/clang, or `strtok_s` with MSCV.

You can test this update with:

```
ECH_CONFIG_LISTS=$(cargo test --test ech_fetch -- curves1-ng.test.defo.ie /tmp/curves1-ng.test.defo.ie)
RUSTLS_PLATFORM_VERIFIER=1 RUSTLS_ECH_CONFIG_LIST="$ECH_CONFIG_LISTS" ./cmake-build-debug/tests/client curves1-ng.test.defo.ie 443 /echstat.php?format=json
```

If you're unlucky and the first HTTPS record served is the one with
invalid configs you should see output like the following showing the
client skipping over the `.1` config list and using the `.2` one
instead:

```
client[188911]: no compatible/valid ECH configs found in '/tmp/curves1-ng.test.defo.ie.1'
client[188911]: using ECH with config list from '/tmp/curves1-ng.test.defo.ie.2'
```
@cpu cpu self-assigned this Dec 16, 2024
@cpu cpu mentioned this pull request Dec 16, 2024
@cpu cpu requested a review from ctz December 16, 2024 14:36
tests/client.c Show resolved Hide resolved
@cpu cpu force-pushed the cpu-ech-improvements branch from b9c7aef to 280762a Compare December 16, 2024 15:35
@cpu
Copy link
Member Author

cpu commented Dec 16, 2024

This feels incremental enough to merge w/o waiting for further review.

@cpu cpu merged commit 7ee38fb into rustls:main Dec 16, 2024
35 checks passed
@cpu cpu deleted the cpu-ech-improvements branch December 16, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

client.c test ECH support updates
2 participants