Changelog improvements before the 3.2 release #5905

gilles-peskine-arm · 2022-06-09T17:14:28Z

Miscellaneous improvements to changelog entries, including:

Consolidating changelog entries from SECLIB-667: Accelerate SHA-512 with A64 crypto extensions #5632 and SECLIB-667: Accelerate SHA-256 with A64 crypto extensions #5547. Also update the documentation to list SHA-224 and SHA-384.
Removing the bugfix entry added in Add missing key destruction calls in ssl_write_client_key_exchange #5576, because upon further analysis there wasn't actually a bug.
The clarification I requested in https://github.com/Mbed-TLS/mbedtls/pull/5837/files#r880337653.

I have only reviewed existing entries. I have not looked for missing entries.

Needs backport for a few entries.

Consolidate the entries for SHA512 and SHA256 since they are being released together. List the algorithms generically as SHA-2 since this also applies to SHA224 and SHA384. Clearly state that the instructions are only supported when building for Aarch64 (64-bit ARMv8+) and not for Aarch32 (32-bit ARMv8+). Signed-off-by: Gilles Peskine <[email protected]>

…figs The entry for “Fix several bugs…” already covers this. This is borderline an internal detail anyway. Signed-off-by: Gilles Peskine <[email protected]>

If the key agreement or the public key export in ssl_write_client_key_exchange() fails, the handshake enters a failed state. The only valid thing you can do in a failed handshake is to abort it, which calls mbedtls_ssl_handshake_free(), which destroys ecdh_psa_privey. While it's good hygiene to destroy the key in the function that creates it, it would have been cleaned up a little later in the normal course of things anyway, so there wasn't an actual bug. Signed-off-by: Gilles Peskine <[email protected]>

Signed-off-by: Gilles Peskine <[email protected]>

tom-cosgrove-arm · 2022-06-10T19:28:57Z

ChangeLog.d/add_final_delay_accessor

@@ -1,4 +1,4 @@
 Features
   * Add the function mbedtls_timing_get_final_delay() to access the private
     final delay field in an mbedtls_timing_delay_context, as requested in
-     #5183
+     #5183.


Rather than have to do this check manually, why not update assemble_changelog.py to add a trailing full stop if one is missing? It could also reflow the text if that is desired.

And if we're going to change this, how about removing the for consistency?

Add function mbedtls_timing_get_final_delay()...

We usually write changelog entries in proper English, not headlinese, thus “add a function” or (since it's named) “add the function”.

Regarding automation: automation has the cost of writing the code and of handling exceptions. Exceptions are why we aren't reflowing (sometimes we want forced breaks at specific points) and why we aren't doing spell checking. I'm not sure if there are exceptions where we really don't want a full stop at the end.

The ChangeLog is presented as a bulleted list. A bulleted list is not really fully prose, in the same way documentation is; and changelogs are typically read rapidly by users to identify "what might interest me in this new release". So writing for rapid parsing (i.e. consistency across multiple entries) and removal of filler would seem to be reasonable here.

Generally in text, where bulleted lists appear they will (depending on style) either (1) all but the last item end in ;, last item ends in . (this is common in documents where bulleted lists aren't the main content) or (2) none have punctuation at the end (which is common on slides).

However, this gets tricky when some bullet points might be multiple sentences, because each sentence within a bullet point needs its punctuation at the end, and so (for consistency) the last sentence in a multi-sentence bullet point should have its punctuation, and so (also for consistency) a bullet point of only one sentence should also have punctuation at the end.

One might consider not doing this for a very short phrase, such as Fixes #1234, but since we do put full stops at the end of those, I can't see content that wouldn't have punctuation at the end.

tom-cosgrove-arm · 2022-06-10T19:29:51Z

ChangeLog.d/mbedtls_ecp_export.txt

@@ -1,3 +1,3 @@
 Features
-   * Add mbedtls_ecp_export() function to export ECP
-     keypair parameters. Fixes #4838.
+   * Add a function mbedtls_ecp_export() to export ECP key pair parameters.


Actually, I think this is better without a:

Suggested change

* Add a function mbedtls_ecp_export() to export ECP key pair parameters.

* Add function mbedtls_ecp_export() to export ECP key pair parameters.

I agree with Tom that the version without "a" is better out of the two, but I'm not sure if "function" is even needed here, it's pretty self-evident. The addition of details at the end is OK.

Why is the version without “a” better? “A” is required for proper English grammar. It's only left out in headlinese.

I've changed the entries you objected to to “add function”.

Since this is not a preestablished usage, and I disagree with making it a standard, I have not gone through other entries to align them. If we're to establish a policy, my preference is for “the new X does this”, because it has the simplest sentence construction. I'm also fine with “Add a X that does this”. I don't like “Add X that does this” because it's headlinese, and we have a well-established tradition of writing changelog entries in proper English sentences.

tom-cosgrove-arm · 2022-06-10T19:35:06Z

ChangeLog.d/mbedtls_tlsver_enum.txt

+   * The new functions mbedtls_ssl_conf_max_tls_version() and
+     mbedtls_ssl_conf_min_tls_version() use a single value to specify
+     the protocol version.


Presumably the section contents are coalesced for the ChangeLog, so these two entries won't necessarily be adjacent. In which case it would be better to use the standard "Add function foo()"-style:

Suggested change

* The new functions mbedtls_ssl_conf_max_tls_version() and

mbedtls_ssl_conf_min_tls_version() use a single value to specify

the protocol version.

* Add functions mbedtls_ssl_conf_max_tls_version() and

mbedtls_ssl_conf_min_tls_version() that use single values to specify

protocol versions.

Why is “Add functions …” preferable to “The new functions …”? We've been using both.

I would say that "The new functions" does not imply that there was a change that added them. It might mean that these new functions now use a single value to specify [...]. I agree with Tom here.

I don't get this. “New” means it wasn't there before the change. It's a common way to word changelog entries (example: Emacs — mostly under “Lisp changes”).

I understand your approach, but even in the given example a lot of changes follow one of these rules:

The new function xxx has been added, The new xxx replaces the current [...]

headline: New function xxx, then, in the body which is, or just some description.

I would say that in fact the form proposed is also used, so I'm more inclined to accept it, however I still like other versions more.
Given that I'm not a native speaker, I'll just let others speak in this specific change request.

tom-cosgrove-arm · 2022-06-10T19:37:46Z

ChangeLog.d/md_info_from_ctx.txt

-   * Add a function to extract message digest information from a message
-     digest context.
+   * The new function mbedtls_md_info_from_ctx() recalls the message digest
+     information that was used to set up a message digest context.


Why change from the the "Add function foo()" style?

I added the function name. I didn't consider “add a function” vs “the new function” to be significant, I just thought my updated sentence flows better.

Same as above: "the new function" doesn't sound to me like the function was added, it rather states that this new function now recalls the message digest [...].
Subjective: Add _function_name()_ to extract > Add function _function_name()_ > The new function.

On reflection I personally prefer “the new function …”, but that's a matter of preference, so I've changed this back.

Re: "Add ..." vs "The new ..." is a case of active vs passive, us saying what we have done (in present tense, as is common for release notes) vs documenting what is done (it's quite common for documentation to be passive voice).

AndrzejKurek · 2022-06-24T11:10:34Z

ChangeLog.d/tls13-fix-finished-fetch.txt

-     preprocessing computations to avoid doing them multiple times, which was
-     causing the handshake to fail.
+   * Fix a TLS 1.3 handshake failure when the peer Finished message has not
+     been received yet when we first try to fetch it.


I understand that we don't have to give more detail, but is it harmful if the work is already done?

Having irrelevant information hides the relevant information, yes. So I'm not reverting this.

AndrzejKurek

Minor changes requested.

Signed-off-by: Gilles Peskine <[email protected]>

tom-cosgrove-arm

LGTM

gilles-peskine-arm added 10 commits June 9, 2022 18:32

Changelog: Remove redundant entry about USE_PSA_CRYPTO in reduced con…

3ea721e

…figs The entry for “Fix several bugs…” already covers this. This is borderline an internal detail anyway. Signed-off-by: Gilles Peskine <[email protected]>

Changelog: mention bug id in bugfix entry

fed024d

Signed-off-by: Gilles Peskine <[email protected]>

Changelog: clarify some cmake-related entries as being about cmake

8df3623

Signed-off-by: Gilles Peskine <[email protected]>

Changelog: list deprecations in the dedicated section

26a51cf

Signed-off-by: Gilles Peskine <[email protected]>

Changelog: make some long entries about TLS 1.3 more to the point

c23a6d4

Signed-off-by: Gilles Peskine <[email protected]>

Changelog: when adding a new function, state its name

e1efbf7

Signed-off-by: Gilles Peskine <[email protected]>

Changelog: minor copyediting

76c1c6b

Signed-off-by: Gilles Peskine <[email protected]>

Doc: the SHA256/SHA512 options also cover SHA224/SHA384

9b3278b

Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm added enhancement size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Jun 9, 2022

This was referenced Jun 9, 2022

Allow building as a subdir #5837

Merged

Add missing key destruction calls in ssl_write_client_key_exchange #5576

Merged

gilles-peskine-arm added the needs-backports Backports are missing or are pending review and approval. label Jun 9, 2022

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Jun 9, 2022

tom-cosgrove-arm reviewed Jun 10, 2022

View reviewed changes

AndrzejKurek reviewed Jun 24, 2022

View reviewed changes

AndrzejKurek suggested changes Jun 24, 2022

View reviewed changes

Use headlinese for added functions, per request

bf918b9

Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm requested review from AndrzejKurek and tom-cosgrove-arm June 27, 2022 21:40

gilles-peskine-arm mentioned this pull request Jun 27, 2022

Changelog improvements before the 2.28.1 release #5984

Merged

AndrzejKurek approved these changes Jun 27, 2022

View reviewed changes

tom-cosgrove-arm approved these changes Jun 28, 2022

View reviewed changes

tom-cosgrove-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Jun 28, 2022

gilles-peskine-arm merged commit 7d14c19 into Mbed-TLS:development Jun 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changelog improvements before the 3.2 release #5905

Changelog improvements before the 3.2 release #5905

gilles-peskine-arm commented Jun 9, 2022 •

edited

Loading

tom-cosgrove-arm Jun 10, 2022

tom-cosgrove-arm Jun 10, 2022

gilles-peskine-arm Jun 13, 2022

tom-cosgrove-arm Jun 28, 2022

tom-cosgrove-arm Jun 10, 2022

AndrzejKurek Jun 24, 2022

gilles-peskine-arm Jun 25, 2022

gilles-peskine-arm Jun 27, 2022

tom-cosgrove-arm Jun 10, 2022

gilles-peskine-arm Jun 13, 2022

AndrzejKurek Jun 24, 2022

gilles-peskine-arm Jun 25, 2022

AndrzejKurek Jun 27, 2022

tom-cosgrove-arm Jun 10, 2022

gilles-peskine-arm Jun 13, 2022

AndrzejKurek Jun 24, 2022 •

edited

Loading

gilles-peskine-arm Jun 27, 2022

tom-cosgrove-arm Jun 28, 2022

AndrzejKurek Jun 24, 2022

gilles-peskine-arm Jun 27, 2022

AndrzejKurek left a comment

tom-cosgrove-arm left a comment

	* Add a function mbedtls_ecp_export() to export ECP key pair parameters.
	* Add function mbedtls_ecp_export() to export ECP key pair parameters.

Changelog improvements before the 3.2 release #5905

Changelog improvements before the 3.2 release #5905

Conversation

gilles-peskine-arm commented Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrzejKurek Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrzejKurek left a comment

Choose a reason for hiding this comment

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Jun 9, 2022 •

edited

Loading

AndrzejKurek Jun 24, 2022 •

edited

Loading