Conversation
|
Someone suggested it may be timely to create a PR that just contains an API design document for ECH, so this is that. |
|
Do you want to copy the remaining relevant parts of the email chain? Or not bother. |
|
Hi
On 01/03/2023 23:54, Rich Salz wrote:
Do you want to copy the remaining relevant parts of the email chain?
Ah - sorry, I had meant to get back to you (but
travel, then forgot - apologies;-). The thread
starts at [1].
Or not bother.
I'll totally bother:-)
I think the main outstanding thing from [1] (other
than the two names below) was the error handling
where I added a comment noting either the ``SSLfatal()``
or the ``ERR_raise()`` macros are called by the APIs
if there's an error. (I think that's actually the case
with my current implementation too.) That seemed
like the pattern of other APIs but I'm happy to
be corrected if I got that wrong. I think that does
kinda match what you suggested too though?
WRT naming, I'm happy to do whatever seems best.
I think there were two things there (so far):
- I'm not sure your suggested ``SSL_ech_find_match()``
is better than the probably-bad ``SSL_ech_reduce()``
so, maybe best to get more input on that? That API
allows a client to down-select from the set of
ECHConfigs that've been fed into the API (e.g.
from multi-valued HTTPS/SVCB RRs) to pick just
the one the picky client wants to use.
- WRT your suggested ``ECH_NOT_FOUND``: I guess
I still do prefer ``SSL_ECH_STATUS_NOT_CONFIGURED``
but again, happy to get more input on that. That
status value is returned when the client or a
server checks the ECH status for an SSL connection,
but hadn't ever provided an ECHConfig (or ECHConfig
+ private key for a server).
Cheers,
S.
[1] https://mta.openssl.org/pipermail/ech/2023-February/000008.html
|
vdukhovni
left a comment
There was a problem hiding this comment.
More left to read. These comments/questions are preliminary...
| 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). |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.)
|
Hi Viktor,
Thanks for the review. Just to let you know I'll be
travelling this w/e, so could be Tuesday before I
get to respond.
Cheers,
S.
|
Hi Viktor, sorry for the slow reply, was on a few days off. Will process these comments now though. |
|
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. |
Great. I'll post updated stuff later today (also handling some comments on the other PR first). |
|
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. |
|
@vdukhovni I added back the |
|
ECH API design doc was merged to the feature branch in #24738 Closing this. |
Checklist