Implement modified key export API for Mbed TLS 3.0 #4552

hanno-becker · 2021-05-24T10:17:26Z

This resolves #4363 and implements the API proposed therein. See #4363 and the documentation for more information.

hanno-becker · 2021-05-26T08:28:32Z

CI only failing because of the API break.

daverodgman

A couple of minor points, otherwise looks good

daverodgman · 2021-05-26T17:06:02Z

include/mbedtls/ssl.h

+ * \param client_random The client random bytes.
+ * \param server_random The server random bytes.
+ * \param tls_prf_type The identifier for the PRF used in the handshake
+ *                     to which the key belongs.


Missing docs for secret and secret_len

daverodgman · 2021-05-26T17:34:05Z

include/mbedtls/ssl.h

+typedef enum
+{
+   MBEDTLS_SSL_TLS_PRF_NONE,
+   MBEDTLS_SSL_TLS_PRF_TLS1,


Is MBEDTLS_SSL_TLS_PRF_TLS1 still needed after TLS 1.0 and 1.1 have been removed?

No, it's not. I've removed it.

hanno-becker · 2021-05-28T04:16:34Z

@daverodgman Thank you for your review. I believe to have addressed your comments. Could you please re-review?

hanno-becker · 2021-06-01T04:01:47Z

@ronald-cron-arm Why was this removed from the Mbed TLS 3.0 Optional EPIC?

ronald-cron-arm · 2021-06-01T06:35:51Z

@ronald-cron-arm Why was this removed from the Mbed TLS 3.0 Optional EPIC?

Because it is linked to the issue #4363 that is in the Mbed TLS 3.0 Optional EPIC and thus in the EPIC we can still see this PR through the issue. When a PR is linked to an issue that is an EPIC we prefer not to add the PR to the EPIC as well. EPICs are better organized that way.

hanno-becker · 2021-06-01T06:44:25Z

@ronald-cron-arm Got it!

daverodgman

Looks good, comments have been addressed - thanks!

gilles-peskine-arm

I've read the discussion in #2188 and #4363 and reviewed the proposal here.

I think the callback should have access to the SSL context. This was proposed in #2188 and I think this is both simpler and more future-proof than the proposed alternatives such as identifying a connection by its random. I don't see any rationale for the current choice of parameters for the callback.

ChangeLog.d/key-export.txt

include/mbedtls/ssl.h

gilles-peskine-arm · 2021-06-06T19:07:27Z

include/mbedtls/ssl.h

+ * \brief           Callback type: Export key alongside random values for
+ *                                 session identification, and PRF for
+ *                                 implementation of TLS key exporters.
+ *


(Doesn't necessarily need to be answered now, but it may matter to the design of the API.) With MBEDTLS_USE_PSA_CRYPTO, at some point, it will become possible to have session secrets that cannot be exported. Are we planning to just not call the callback?

If the answer might be "don't call it", should we document now that under some circumstances (that we don't need to define at this point), the callback may not be called?

include/mbedtls/ssl.h

gilles-peskine-arm · 2021-06-06T19:14:30Z

include/mbedtls/ssl.h

+ *                     to which the key belongs.
+ *
+ * \return          \c 0 if successful.
+ * \return          A negative error code on failure.


What happens if the callback fails? Right now the failure is ignored. Maybe the callback should return void? If not I think the effect of a failure should be a documented aspect of the API.

Hmm, good point. At first I thought we should change the return type to void, but perhaps it's a convenient for the user to be able to signal a fatal error to the stack when something goes wrong in the export. I'll implement and document the latter for now.

Let's wait for @mpg's opinion before changing anything.

I think returning void is the right thing to do for this kind of callback. If anything goes wrong with the export for some reason, it doesn't invalidate anything in the connection - it might be a problem for the application, and then the application will decide what to do (including closing the connection if desired).

I think I agree with @mpg - not sure what Mbed TLS could usefully do with the error.

Changing to void.

For the record, I agree with @mpg's analysis.

Signed-off-by: Hanno Becker <[email protected]>

Fixes Mbed-TLS#2188 Signed-off-by: Hanno Becker <[email protected]>

Signed-off-by: Hanno Becker <[email protected]>

This saves some code when compiling for Thumb, where access to fields with offset index > 127 requires intermediate address computations. Frequently used fields should therefore be located at the top of the structure, while less frequently used ones -- such as the export callback -- can be moved to the back. Signed-off-by: Hanno Becker <[email protected]>

daverodgman · 2021-06-18T17:41:34Z

Trivial rebase onto latest development to try re-running the CI

Signed-off-by: Hanno Becker <[email protected]>

hanno-becker · 2021-06-21T08:34:18Z

Yet another try...

daverodgman

LGTM

gilles-peskine-arm

LGTM except for one omission in the migration guide.

gilles-peskine-arm · 2021-06-21T14:53:12Z

docs/3.0-migration-guide.d/key-export.md

+Those APIs have been removed and replaced by the new API
+`mbedtls_ssl_set_export_keys_cb()`. This API differs from
+the previous key export API in the following ways:
+


Another way is that the new callback returns void, whereas the old callbacks returned an int value that was ignored. Since the old value was ignored, that's minor enough not to be mentioned in the changelog which is a high-level semantic overview, but I think it should be listed in the migration guide, because I'd expect the list here to be a complete list of steps to migrate existing code.

Fair point, I've discussed wording with @hanno-arm & pushed a commit with the addition to the migration guide.

Signed-off-by: Dave Rodgman <[email protected]>

daverodgman · 2021-06-22T12:49:44Z

Sorry, typo in my first attempt.

hanno-becker added needs-review Every commit must be reviewed by at least two team members, component-tls mbedtls-3 needs-reviewer This PR needs someone to pick it up for review labels May 24, 2021

hanno-becker mentioned this pull request May 24, 2021

Unify SSL key exporter API and prepare for TLS 1.3 #4363

Closed

hanno-becker added the size-s Estimated task size: small (~2d) label May 24, 2021

mpg self-requested a review May 25, 2021 09:41

daverodgman self-requested a review May 25, 2021 15:22

daverodgman requested changes May 26, 2021

View reviewed changes

daverodgman removed the needs-reviewer This PR needs someone to pick it up for review label May 27, 2021

hanno-becker requested a review from daverodgman May 28, 2021 04:16

hanno-becker mentioned this pull request Jun 1, 2021

Key export callback must contain session specific information #2188

Closed

hanno-becker force-pushed the mbedtls_3_0_key_export branch from d63ab59 to 6b7bb36 Compare June 1, 2021 07:44

daverodgman previously approved these changes Jun 1, 2021

View reviewed changes

gilles-peskine-arm requested changes Jun 7, 2021

View reviewed changes

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Jun 7, 2021

hanno-becker dismissed daverodgman’s stale review via 3b42e5b June 8, 2021 04:11

hanno-becker requested review from gilles-peskine-arm and daverodgman June 8, 2021 04:21

Hanno Becker added 7 commits June 18, 2021 18:40

Add missing documentation for key export callback parameters

ddc739c

Signed-off-by: Hanno Becker <[email protected]>

Remote key export identifier used for TLS < 1.2.

22b34f7

Signed-off-by: Hanno Becker <[email protected]>

Remove all occurrences of TLS < 1.2 PRF identifier

a7991f2

Signed-off-by: Hanno Becker <[email protected]>

Make key export callback and context connection-specific

7e6c178

Fixes Mbed-TLS#2188 Signed-off-by: Hanno Becker <[email protected]>

Remove return value from key export callback

e0dad72

Signed-off-by: Hanno Becker <[email protected]>

Improve ChangeLog wording for key export

1e1c23d

Signed-off-by: Hanno Becker <[email protected]>

daverodgman force-pushed the mbedtls_3_0_key_export branch from b62ad8f to 46b0217 Compare June 18, 2021 17:41

Fix return type of example key export callbacks

296fefe

Signed-off-by: Hanno Becker <[email protected]>

hanno-becker force-pushed the mbedtls_3_0_key_export branch from 46b0217 to 296fefe Compare June 21, 2021 08:33

daverodgman previously approved these changes Jun 21, 2021

View reviewed changes

gilles-peskine-arm requested changes Jun 21, 2021

View reviewed changes

mpg removed the needs-ci Needs to pass CI tests label Jun 22, 2021

daverodgman dismissed their stale review via 6d937ed June 22, 2021 12:45

gilles-peskine-arm previously approved these changes Jun 22, 2021

View reviewed changes

daverodgman added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Jun 22, 2021

Document the return type change in the migration guide

5ec5003

Signed-off-by: Dave Rodgman <[email protected]>

daverodgman dismissed gilles-peskine-arm’s stale review via 5ec5003 June 22, 2021 12:49

daverodgman force-pushed the mbedtls_3_0_key_export branch from 6d937ed to 5ec5003 Compare June 22, 2021 12:49

gilles-peskine-arm approved these changes Jun 22, 2021

View reviewed changes

daverodgman approved these changes Jun 22, 2021

View reviewed changes

gilles-peskine-arm removed the needs-review Every commit must be reviewed by at least two team members, label Jun 22, 2021

gilles-peskine-arm merged commit e9bc857 into Mbed-TLS:development Jun 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement modified key export API for Mbed TLS 3.0 #4552

Implement modified key export API for Mbed TLS 3.0 #4552

hanno-becker commented May 24, 2021

hanno-becker commented May 26, 2021

daverodgman left a comment

daverodgman May 26, 2021

hanno-becker May 28, 2021

daverodgman May 26, 2021

hanno-becker May 28, 2021 •

edited

Loading

hanno-becker commented May 28, 2021

hanno-becker commented Jun 1, 2021

ronald-cron-arm commented Jun 1, 2021 •

edited

Loading

hanno-becker commented Jun 1, 2021

daverodgman left a comment

gilles-peskine-arm left a comment

gilles-peskine-arm Jun 6, 2021

daverodgman Jun 11, 2021

gilles-peskine-arm Jun 6, 2021

hanno-becker Jun 8, 2021 •

edited

Loading

mpg Jun 9, 2021

daverodgman Jun 11, 2021

hanno-becker Jun 11, 2021

hanno-becker Jun 11, 2021

daverodgman commented Jun 18, 2021 •

edited

Loading

hanno-becker commented Jun 21, 2021

daverodgman left a comment

gilles-peskine-arm left a comment

gilles-peskine-arm Jun 21, 2021

daverodgman Jun 22, 2021

daverodgman commented Jun 22, 2021

Implement modified key export API for Mbed TLS 3.0 #4552

Implement modified key export API for Mbed TLS 3.0 #4552

Conversation

hanno-becker commented May 24, 2021

hanno-becker commented May 26, 2021

daverodgman left a 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

hanno-becker May 28, 2021 • edited Loading

Choose a reason for hiding this comment

hanno-becker commented May 28, 2021

hanno-becker commented Jun 1, 2021

ronald-cron-arm commented Jun 1, 2021 • edited Loading

hanno-becker commented Jun 1, 2021

daverodgman left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a 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

hanno-becker Jun 8, 2021 • 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

daverodgman commented Jun 18, 2021 • edited Loading

hanno-becker commented Jun 21, 2021

daverodgman left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daverodgman commented Jun 22, 2021

hanno-becker May 28, 2021 •

edited

Loading

ronald-cron-arm commented Jun 1, 2021 •

edited

Loading

hanno-becker Jun 8, 2021 •

edited

Loading

daverodgman commented Jun 18, 2021 •

edited

Loading