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

added ECH API design doc #20408

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

added ECH API design doc #20408

wants to merge 9 commits into from

Conversation

sftcd
Copy link
Contributor

@sftcd sftcd commented Mar 1, 2023

Checklist
  • documentation is added or updated
  • tests are added or updated

@sftcd
Copy link
Contributor Author

sftcd commented Mar 1, 2023

Someone suggested it may be timely to create a PR that just contains an API design document for ECH, so this is that.
These APIs are implemented in this fork.
Comments welcome!

@richsalz
Copy link
Contributor

richsalz commented Mar 1, 2023

Do you want to copy the remaining relevant parts of the email chain? Or not bother.

@sftcd
Copy link
Contributor Author

sftcd commented Mar 2, 2023 via email

@nhorman nhorman added issue: feature request The issue was opened to request a feature triaged: design The issue/pr deals with a design document and removed issue: feature request The issue was opened to request a feature labels Mar 19, 2024
@nhorman nhorman added this to the 3.4.0 milestone Apr 19, 2024
Copy link

@vdukhovni vdukhovni left a comment

Choose a reason for hiding this comment

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

More left to read. These comments/questions are preliminary...

doc/designs/ech-api.md Show resolved Hide resolved
external key management process (likely managed via a cronjob). The last
allows the application to load keys from a buffer (that should contain the same
content as a file) and was added for haproxy which prefers not to do disk reads
after initial startup (for resilience reasons apparently).

Choose a reason for hiding this comment

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

Is the buffer function a building block for the other two (they fill the buffer from an external source, and tail call the buffer function)?

Are multiple configurations loaded expected to differ only the keys (to support key overlap during rollover), or might they also differ in the front-end SNI name (support multiple logical front-end names in the same process)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the buffer function a building block for the other two (they fill the buffer from an external source, and tail call the buffer function)?

Not really, they each mainly use an internal API.

Are multiple configurations loaded expected to differ only the keys (to support key overlap during rollover), or might they also differ in the front-end SNI name (support multiple logical front-end names in the same process)?

The code supports the latter.

[draft-farrell-tls-pemesni](https://datatracker.ietf.org/doc/draft-farrell-tls-pemesni/).

There are also functions to allow a server to see how many keys are currently
loaded, and one to flush keys that are older than ``age`` seconds.

Choose a reason for hiding this comment

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

Is the key lifetime explicit in the loaded file, or implicit based on time at which the key was loaded? If there are older and newer files in a directory, how is the age of the key determined?

Also, what sort of locking is involved here? How are threads handling incoming connections accessing the configuration concurrently with potential periodic updates by a "housekeeping" thread?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the key lifetime explicit in the loaded file, or implicit based on time at which the key was loaded? If there are older and newer files in a directory, how is the age of the key determined?

The key lifetime is not explicit in the loaded file. The age of the key is the time since it was loaded. If the caller uses SSL_CTX_ech_server_enable_dir() then all "good" matching files will be loaded, so the assumption is that the key management code/scheme in the environment has ensured the right set of files are present.

Also, what sort of locking is involved here? How are threads handling incoming connections accessing the configuration concurrently with potential periodic updates by a "housekeeping" thread?

Ah - good point. I need to check, but have no specific file locking code for this. I've added a TODO for that.

Choose a reason for hiding this comment

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

I did not mean file locking, rather appropriate locks on the SSL or SSL_CTX handles, or underlying hash tables of available ECH configs (keyed by SNI???) Can there be multiple configs for the same SNI, or does the most recent loaded replace all prior configs for the same logical (SNI) hostname?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I'll get back on multi-threaded stuff generally. (I've not seen any issues with web servers that do spawn multiple threads, but like I said I've yet to consider the corner cases for that.)

doc/designs/ech-api.md Show resolved Hide resolved
doc/designs/ech-api.md Outdated Show resolved Hide resolved
doc/designs/ech-api.md Show resolved Hide resolved
doc/designs/ech-api.md Show resolved Hide resolved
doc/designs/ech-api.md Show resolved Hide resolved
doc/designs/ech-api.md Show resolved Hide resolved
doc/designs/ech-api.md Show resolved Hide resolved
@sftcd
Copy link
Contributor Author

sftcd commented May 10, 2024 via email

@sftcd
Copy link
Contributor Author

sftcd commented May 16, 2024

More left to read. These comments/questions are preliminary...

Hi Viktor, sorry for the slow reply, was on a few days off. Will process these comments now though.

@sftcd
Copy link
Contributor Author

sftcd commented May 16, 2024

Last point - I might push changes resulting from the above to the ECH-PR which was created after this but also has all the ECH implementation code, and then close this documentation PR.

Would continuing the discussion be better there rather than here? (Just to keep things in one place.)

If keeping the 2 PRs open is better, happy to do that and will push the same changes here too afterwards.

@vdukhovni
Copy link

Last point - I might push changes resulting from the above to the ECH-PR which was created after this but also has all the ECH implementation code, and then close this documentation PR.

Would continuing the discussion be better there rather than here? (Just to keep things in one place.)

If keeping the 2 PRs open is better, happy to do that and will push the same changes here too afterwards.

I am generally a fan of developing and reviewing documentation in advance of deep dives into code. The documentation might subsequently evolve with the code, but the basics would ideally be settled. In that light, I'd keep going here, but given that the code is already written, and if the design docs are also part of that PR, it may be easier for you to evolve that, in which case I am willing to switch, once we resolve the few outstanding subthreads here. I will then not open any new topics here, and cut over once we're done with the questions so far.

@sftcd
Copy link
Contributor Author

sftcd commented May 17, 2024

once we resolve the few outstanding subthreads here. I will then not open any new topics here, and cut over once we're done with the questions so far.

Great. I'll post updated stuff later today (also handling some comments on the other PR first).

@sftcd sftcd mentioned this pull request May 17, 2024
2 tasks
@sftcd
Copy link
Contributor Author

sftcd commented May 17, 2024

I pushed the changes to ech-api.md to this PR. I also included those changes and others discussed about in the ECH-PR so we can maybe close this one soonish.

doc/designs/ech-api.md Outdated Show resolved Hide resolved
@sftcd
Copy link
Contributor Author

sftcd commented May 30, 2024

@vdukhovni I added back the SSL_ech_set_outer_alpn_protos() API as you suggested. That's documented here, and the code (for the moment) is in sftcd@006c140 (also for the moment: that's in the ECH-draft13c branch, I'll push it to the ECH-PR branch later).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.4.0 Planned eng view triaged: design The issue/pr deals with a design document
Projects
Status: In progress
Status: In progress
Development

Successfully merging this pull request may close these issues.

None yet

4 participants