Add getters for some fields #7216

lpy4105 · 2023-03-07T07:05:53Z

Description

Partially Fix: #6840

The getters are added for the convenience of application usage.

Gatekeeper checklist

changelog provided
backport not required
tests provided

yanesca · 2023-04-17T12:52:36Z

@lpy4105 according to the new process only planned work that is scheduled for the current quarter should have 'high-priority' label. PRs with 'high-priority' label should be added to the unified board. This PR is labelled 'high-priority' but is not on the unified board. Does this PR belong to some stream of work we planned for this quarter?

gilles-peskine-arm · 2023-04-17T13:28:14Z

@yanesca This is from last quarter and I don't see any reason to abandon it.

yanesca · 2023-04-17T14:17:38Z

@gilles-peskine-arm thank you for confirming. I am not trying to challenge the necessity of this work or proposing to abandon it. There is an inconsistency between the number of items on the unified board and the items labelled as 'priority-high' and would like to figure out what is going on.

daverodgman

Please add tests & changelog

Signed-off-by: Pengyu Lv <[email protected]>

It would be convenient for users to query the endpoint type directly from a ssl context: ``` mbedtls_ssl_conf_get_endpoint( mbedtls_ssl_context_get_config(&ssl)) ``` Signed-off-by: Pengyu Lv <[email protected]>

Signed-off-by: Pengyu Lv <[email protected]>

lpy4105 · 2023-07-10T04:22:21Z

Adding tests results a conflict with development due to some changes in tests/suites/test_suite_ssl.function. So I rebased the PR to the latest development.

Changes:

The parameter of mbedtls_ssl_conf_get_endpoint is changed to const mbedtls_ssl_config *
Add tests for mbedtls_ssl_conf_get_endpoint, mbedtls_ssl_get_hostname and mbedtls_ssl_cache_get_timeout.
Add changelog.

Signed-off-by: Pengyu Lv <[email protected]>

daverodgman · 2023-07-10T08:10:39Z

Please merge (e.g. git merge origin/development) rather than rebase, as it makes it easier for reviewers to see what's changed since their last review

daverodgman · 2023-07-10T08:13:22Z

And please don't forget to update labels (@tom-daubney-arm remember to remove needs-reviewer when adding yourself as second reviewer)

tom-daubney-arm

Overall LGTM. Testing looks sufficient.

A couple of minor points to address if you agree that they are beneficial changes but nothing that should hold up review. Thanks!

tom-daubney-arm · 2023-07-10T13:23:39Z

include/mbedtls/ssl.h

+/**
+ * \brief          Get the hostname that checked against the received
+ *                 server certificate. It is used to set the ServerName
+ *                 TLS extension, too, if that extension is enabled.


nit: remove the commas around the word "too". Not really needed here.

tom-daubney-arm · 2023-07-10T14:41:11Z

tests/suites/test_suite_ssl.function

    mbedtls_ssl_context ssl;

    mbedtls_ssl_init(&ssl);
    USE_PSA_INIT();

    TEST_ASSERT(mbedtls_ssl_set_hostname(&ssl, hostname0) == 0);
+    hostname = mbedtls_ssl_get_hostname(&ssl);


I don't really like the naming of the variables here. I know hostname0 and hostname1 existed before this PR but since we're here it might be nicer to rename them a bit for clarity. Maybe to distinguish between inputs and outputs so perhaps hostname0 --> input_hotstname0, similar for hostname1 and then perhaps hostname --> ouput_hostname.

This is just my preference and I accept others may not think this is needed so I won't request changes just for this, but consider changing them if you agree.

daverodgman · 2023-07-10T16:14:03Z

I think Tom's comments are sensible, but I don't think it's worth iterating the PR just for this.

lpy4105 mentioned this pull request Mar 7, 2023

Add getters for some fields #6840

Closed

lpy4105 removed the needs-design-approval label Apr 10, 2023

lpy4105 added priority-medium Medium priority - this can be reviewed as time permits and removed priority-high High priority - will be reviewed soon labels Apr 19, 2023

daverodgman added the historical-reviewing Currently reviewing (for legacy PR/issues) label Jul 7, 2023

daverodgman self-requested a review July 7, 2023 13:13

tom-daubney-arm requested review from tom-daubney-arm and removed request for daverodgman July 7, 2023 13:13

daverodgman approved these changes Jul 7, 2023

View reviewed changes

daverodgman requested changes Jul 7, 2023

View reviewed changes

daverodgman added the needs-work label Jul 7, 2023

lpy4105 added 6 commits July 10, 2023 11:33

ssl_cache: Add getter access to timeout field

af724dd

Signed-off-by: Pengyu Lv <[email protected]>

Add getter access to hostname field in mbedtls_ssl_context

918ebf3

Signed-off-by: Pengyu Lv <[email protected]>

Add getter access to endpoint field in mbedtls_ssl_config

accd53f

Signed-off-by: Pengyu Lv <[email protected]>

Make endpoint getter parameter a pointer to const

08daebb

It would be convenient for users to query the endpoint type directly from a ssl context: ``` mbedtls_ssl_conf_get_endpoint( mbedtls_ssl_context_get_config(&ssl)) ``` Signed-off-by: Pengyu Lv <[email protected]>

Add test for hostname getter

30e0870

Signed-off-by: Pengyu Lv <[email protected]>

Add test for endpoint getter

db61433

Signed-off-by: Pengyu Lv <[email protected]>

lpy4105 force-pushed the issue/6840/add-getters-for-some-fields branch from d326840 to 7c57b9d Compare July 10, 2023 04:13

Add test for cache timeout getter

5cbb93e

Signed-off-by: Pengyu Lv <[email protected]>

Add changelog entries

5a3f5f4

Signed-off-by: Pengyu Lv <[email protected]>

lpy4105 force-pushed the issue/6840/add-getters-for-some-fields branch from 7c57b9d to 5a3f5f4 Compare July 10, 2023 05:27

daverodgman approved these changes Jul 10, 2023

View reviewed changes

daverodgman removed needs-work needs-reviewer This PR needs someone to pick it up for review labels Jul 10, 2023

daverodgman added the needs-ci Needs to pass CI tests label Jul 10, 2023

lpy4105 requested a review from daverodgman July 10, 2023 09:01

tom-daubney-arm added historical-reviewed Reviewed & agreed to keep legacy PR/issue and removed needs-ci Needs to pass CI tests historical-reviewing Currently reviewing (for legacy PR/issues) labels Jul 10, 2023

tom-daubney-arm approved these changes Jul 10, 2023

View reviewed changes

daverodgman approved these changes Jul 10, 2023

View reviewed changes

daverodgman 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, labels Jul 10, 2023

daverodgman merged commit f3e488e into Mbed-TLS:development Jul 10, 2023

lpy4105 mentioned this pull request Jul 11, 2023

misc improvements #7905

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add getters for some fields #7216

Add getters for some fields #7216

lpy4105 commented Mar 7, 2023 •

edited by daverodgman

Loading

yanesca commented Apr 17, 2023

gilles-peskine-arm commented Apr 17, 2023

yanesca commented Apr 17, 2023

daverodgman left a comment

lpy4105 commented Jul 10, 2023

daverodgman commented Jul 10, 2023 •

edited

Loading

daverodgman commented Jul 10, 2023

tom-daubney-arm left a comment •

edited

Loading

tom-daubney-arm Jul 10, 2023

tom-daubney-arm Jul 10, 2023

daverodgman commented Jul 10, 2023

Add getters for some fields #7216

Add getters for some fields #7216

Conversation

lpy4105 commented Mar 7, 2023 • edited by daverodgman Loading

Description

Gatekeeper checklist

yanesca commented Apr 17, 2023

gilles-peskine-arm commented Apr 17, 2023

yanesca commented Apr 17, 2023

daverodgman left a comment

Choose a reason for hiding this comment

lpy4105 commented Jul 10, 2023

daverodgman commented Jul 10, 2023 • edited Loading

daverodgman commented Jul 10, 2023

tom-daubney-arm left a comment • edited Loading

Choose a reason for hiding this comment

tom-daubney-arm Jul 10, 2023

Choose a reason for hiding this comment

tom-daubney-arm Jul 10, 2023

Choose a reason for hiding this comment

daverodgman commented Jul 10, 2023

lpy4105 commented Mar 7, 2023 •

edited by daverodgman

Loading

daverodgman commented Jul 10, 2023 •

edited

Loading

tom-daubney-arm left a comment •

edited

Loading