Fix EOM description in certificate validator helpers docs#47
Fix EOM description in certificate validator helpers docs#47rousskov wants to merge 4 commits intosquid-cache:mainfrom
Conversation
* SslServerCertValidator and the corresponding AddonHelpers section incorrectly claimed that "Input received from Squid" is "delimited by a 0x01 byte instead of the standard \n byte". The opposite is true. * SslServerCertValidator and the corresponding AddonHelpers section incorrectly implied that "Result line sent back to Squid" is delimited by new lines typical to other helpers. In reality, these helper responses are delimited by ASCII SOH (0x01) bytes. * SslServerCertValidator syntax descriptions poorly duplicated the corresponding AddonHelpers sections. SslServerCertValidator "Input line received from Squid" section contained a malformed table. * AddonHelpers section for "SSL certificate generation" had malformed "warning" markup and malformed title row in the key=value table. Duplicated SslServerCertValidator sections are now removed. Message delimiter (a.k.a. EOM character) documentation in the corresponding AddonHelpers sections are now fixed, along with tje identified markup problems. Also improved related message syntax descriptions to improve phrasing and to be more explicit about odd spacing format that, for example, has a dangling SP character after zero body size. TODO: Fix similar problems in certificate _generator_ sections.
* The `request-ID` field was not documented; `cert_validate` request example used an invalid request-ID value of zero, while the response example did not use a `request-ID` field at all. We now document `request-ID` fields in requests and responses. The two examples use matching request-ID values. N.B. Syntax descriptions for other helpers use misleading "channel-ID" phrasing that implies that Squid may send multiple requests (or can handle multiple responses) sent over the same communication "channel". In reality, concurrency is implemented using unique request IDs; there is no reusable "channel" in this context. Most request IDs exceed `concurrency=N` parameter. These differences can have a significant impact on helper implementations. I believe there were questions about this on Squid forums. TODO: Use `request-ID` terminology in all relevant descriptions.
|
I am posting this as a Draft PR because I do not know how to test its markup changes. @kinkie, IIRC, we have discussed this before, but I failed to find your recommendation in 2024-2026 discussions of this repository PRs -- is there a no-heroic-efforts way to deploy these changes in a sandbox to check rendering and such? P.S. It looks like this PR partially duplicates #30 changes. This PR was prompted by fresh admin complaints and was developed independently -- I only noticed/recalled #30 existence afterwords, when looking for answers to the above question... I believe this PR is an overall better solution and should supersede dormant #30, for several reasons that I can detail if needed. |
If this PR is merged promptly, I will follow up with the above changes (that would largely mimic certificate validation section changes in the merged PR). |
yadij
left a comment
There was a problem hiding this comment.
This PR proposes changing a lot of text that is "terms of art". A change to such terms should be done as a dedicated change, updating the definitions of the terms and all uses consistently.
PS. Do you want to push PR #30 forward (first)?, the holdup there was just disagreement from you on how I match the existing wiki style using bold instead of verbatim-code quoting for em-phasis.
| This interface is similar to the SSL certificate generation interface. | ||
|
|
||
| Input *line* received from Squid: | ||
| Request messages sent by Squid have the following syntax: |
There was a problem hiding this comment.
"Input line received from Squid" is text shared by all helper API documentation. Please revert.
| Request messages sent by Squid have the following syntax: | ||
|
|
||
| request size [kv-pairs] | ||
| [request-ID SP] action SP size SP body LF |
There was a problem hiding this comment.
Multiple issues:
-
Field representing "concurrency channel" is already defined for other helpers as
channel-ID.
a) Please use that name for consistency.
b) AFAIK, concurrency channel is not an optional field for this helper. -
This part of the documentation is a fieldset semantic definition, not binary syntax definition. As such the delimiter (
SP) and EOL (LF) characters should not be listed. (Updating the entire page to define the binary grammar is topic for a different PR scope). -
bodyis a vague term (used only by Factory, and only for this helper API). The existing text already provides a more-specific termkv-pairswhich clearly documents that Squid sends a set of "key=value" fields. This term and usage is shared with all other helper APIs.
Please match the definition from PR #30:
| [request-ID SP] action SP size SP body LF | |
| channel-ID action size [kv-pairs] |
| this helper stdin connection). Squid sends this field when (and only when) | ||
| the `concurrency=N` helper configuration option is used with `N` values | ||
| grater than 1. When this field is present, the corresponding helper response | ||
| must start with a matching `request-ID` field. |
There was a problem hiding this comment.
IIRC these certificate helpers were where we chose to make concurrency the default-on and a mandatory field. Which would mean all this text about special values is wrong/irrelevant, at least for this specific helper API.
Please use the text already agreed in PR #30:
| this helper stdin connection). Squid sends this field when (and only when) | |
| the `concurrency=N` helper configuration option is used with `N` values | |
| grater than 1. When this field is present, the corresponding helper response | |
| must start with a matching `request-ID` field. | |
| - channel-ID | |
| : A numeric identifier for the concurrency channel correlating this message and its response. | |
| Helper should typically treat this as an opaque value that must be returned as the start of the response to Squid. | |
| Multi-threaded helpers must ensure that each message is written back to Squid as an "atomic" sequence of bytes, | |
| this parameter permits responses to be returned out-of-order to squid. |
| - action: A string with the name of the helper operation being requested. | ||
| Squid currently only sends `cert_validate` requests. |
There was a problem hiding this comment.
If you are going to change the field semantic from "request" to "action". Then please do so consistently.
| - action: A string with the name of the helper operation being requested. | |
| Squid currently only sends `cert_validate` requests. | |
| - action | |
| : The type of action being requested. Presently the code | |
| `cert_validate` is the only action sent. |
|
|
||
|
|
||
| Example response message: | ||
| Example response message (with the terminating SOH character shown as `^A`): |
There was a problem hiding this comment.
Abbreviations for ASCII character are standardized. SOH and EOL are specific characters with specific well-known semantic meanings.
- The semantic meaning Squid is sending does not match
SOHdefinition, and - The character(s) Squid is sending does not match the intended EOL definition.
The correct "term of art" to be using here is EOM ("END of MESSAGE").
| Example response message (with the terminating SOH character shown as `^A`): | |
| Example response message (with the terminating EOM character sequence shown as `^A`): |
| - body: A possibly empty list of the following key=value pairs separated by | ||
| ASCII new line characters (\\n). |
There was a problem hiding this comment.
Please do not add the new term "body". It is far too easily conflated with the entire message being a "body". This area of the message is an optional sequence of kv-pair, as already documented.
| Helper communication protocol will be similar to the one used by the x509 | ||
| certificate generator helper, as documented | ||
| [elsewhere](Features/AddonHelpers). |
There was a problem hiding this comment.
Better just to link to the actual definition(s).
| Helper communication protocol will be similar to the one used by the x509 | |
| certificate generator helper, as documented | |
| [elsewhere](Features/AddonHelpers). | |
| ## Helper communication protocol | |
| See [Add-On Helper](Features/AddonHelpers) documentation, | |
| and this helpers [specific API definition](Features/AddonHelpers#ssl-server-certificate-validator). |
Fix EOM description in certificate validator helpers docs
... and addressed a few other problems in related text:
SslServerCertValidator and the corresponding AddonHelpers section
incorrectly claimed that "Input received from Squid" is "delimited by
a 0x01 byte instead of the standard \n byte". The opposite is true.
SslServerCertValidator and the corresponding AddonHelpers section
incorrectly implied that "Result line sent back to Squid" is delimited
by new lines typical to other helpers. In reality, these helper
responses are delimited by ASCII SOH (0x01) bytes.
SslServerCertValidator syntax descriptions poorly duplicated the
corresponding AddonHelpers sections. SslServerCertValidator "Input
line received from Squid" section contained a malformed table.
The
request-IDfield was not documented;cert_validaterequestexample used an invalid request-ID value of zero, while the response
example did not use a
request-IDfield at all.AddonHelpers section for "SSL certificate generation" had malformed
"warning" markup and malformed title row in the key=value table.
Duplicated SslServerCertValidator sections are now removed.
Message delimiter (a.k.a. EOM character) documentation in the
corresponding AddonHelpers sections are now fixed.
We now document
request-IDfields in requests and responses1. Thetwo examples use matching request-ID values.
Identified markup problems are now fixed.
Also improved related message syntax descriptions to improve phrasing
and to be more explicit about odd spacing format that, for example, has
a dangling SP character after zero body size.
TODO: Fix similar problems in certificate generator sections.
Footnotes
Syntax descriptions for other helpers use misleading "channel-ID"
phrasing that implies that Squid may send multiple requests (or can
handle multiple responses) sent over the same communication "channel".
In reality, concurrency is implemented using unique request IDs; there
is no reusable "channel" in this context; see reqId in helperDispatch().
Most request IDs exceed
concurrency=Nparameter. These differences canhave a significant impact on helper implementations. I believe there
were questions about this on Squid forums. TODO: Use
request-IDterminology in all relevant descriptions. ↩