Add accessor to get der from mbedtls_pem_context #5504

gstrauss · 2022-02-04T15:41:15Z

Description

Add accessor to get der from mbedtls_pem_context
closes #5414 Allow access to result of PEM decoding

Status

READY

Requires Backporting

NO

Migrations

NO

Todos

Tests
Documentation
Changelog updated

mpg

LGTM. I left a note about naming, I'm having doubts but I think the name is good enough, so I'm approving as is for now, and will wait to see that the second reviewer thinks.

include/mbedtls/pem.h

mpg · 2022-02-07T09:38:56Z

MSVC does not seem to like the new function:

C:\builds\workspace\mbed-tls-pr-head_PR-5504-head\src\include\mbedtls/pem.h(116): error C2143: syntax error : missing '{' before 'const' [C:\builds\workspace\mbed-tls-pr-head_PR-5504-head\src\library\mbedcrypto.vcxproj]

I think it's actually about the inline keyword, and you probably want to copy-paste this block near the beginning of pem.h. Hopefully that's what the error was about, and it will make the CI pass.

gstrauss · 2022-02-07T12:22:26Z

Thank you for fixing up the testing (#5508). If we change the function name to mbedtls_pem_get_raw(), that will need to be adjusted, too.

gilles-peskine-arm

Ok for the code, but I'd like some documentation improvements.

include/mbedtls/pem.h

ChangeLog.d/mbedtls_pem_get_der.txt

include/mbedtls/pem.h

tests/suites/test_suite_pem.function

include/mbedtls/pem.h

tests/suites/test_suite_pem.function

gstrauss · 2022-02-07T15:54:18Z

I changed the name of the inline function to mbedtls_pem_get_buffer() and have incorporated the other changes requested by reviewers. Thanks!

gstrauss · 2022-02-07T15:58:43Z

@mpg: would you like me to cherry-pick your changes in #5508 to be part of this PR?

mpg · 2022-02-08T10:45:40Z

@mpg: would you like me to cherry-pick your changes in #5508 to be part of this PR?

Yes, if you don't mind, that would be nice, thanks!

Co-authored-by: Gilles Peskine <gilles.peskine@arm.com> Signed-off-by: Glenn Strauss <gstrauss@gluelogic.com>

Currently all cases were negative, so the block that exercised mbedtls_pem_get_der() would never be reached. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

gstrauss · 2022-02-08T19:59:15Z

@mpg: would you like me to cherry-pick your changes in #5508 to be part of this PR?

Yes, if you don't mind, that would be nice, thanks!

Tests from #5508 have been cherry-picked into this PR. Thanks!

mpg

LGTM, thanks for cherry-picking the test extensions.

gstrauss · 2022-02-10T18:40:20Z

minor nit: I see the use of both \c param buflen and \c param buf_len in pem.h and other include/mbedtls/ headers. Which spelling is preferred?

gilles-peskine-arm · 2022-02-10T22:41:48Z

For future reference, please don't mark other people's review comments as resolved. It's up to the reviewer to decide whether they've been resolved satisfactorily. We use the convention that the “rocket” reaction belongs to the submitter and can mean “I've handled this in my working copy” or “I've commited a fix” or “I've pushed a fix” as you prefer.

gilles-peskine-arm

The test code could be made a little more robust, but I'm approving anyway in the interest of moving on.

gilles-peskine-arm · 2022-02-10T22:50:39Z

tests/suites/test_suite_pem.function

+    TEST_EQUAL( use_len, out->len );
+    TEST_ASSERT( memcmp( out->x, buf, out->len ) == 0 );


We have a macro for this.

Suggested change

TEST_EQUAL( use_len, out->len );

TEST_ASSERT( memcmp( out->x, buf, out->len ) == 0 );

ASSERT_COMPARE( out->x, out->len, buf, use_len );

gilles-peskine-arm · 2022-02-10T22:56:13Z

With my approval, Manuel's approval here and Andrzej's approval of #5508, we have two trusted approvers.

gstrauss · 2022-02-11T00:09:21Z

For future reference, please don't mark other people's review comments as resolved. It's up to the reviewer to decide whether they've been resolved satisfactorily. We use the convention that the “rocket” reaction belongs to the submitter and can mean “I've handled this in my working copy” or “I've commited a fix” or “I've pushed a fix” as you prefer.

Sorry. I was not aware of this convention here. Thanks for letting me know.

(meta: it would be nice if github had a way for reviewer and reviewee to independently indicate when each thinks a thread is resolved.)

gstrauss mentioned this pull request Feb 4, 2022

Allow access to result of PEM decoding #5414

Closed

mpg linked an issue Feb 7, 2022 that may be closed by this pull request

Allow access to result of PEM decoding #5414

Closed

mpg added Community 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 Feb 7, 2022

mpg previously approved these changes Feb 7, 2022

View reviewed changes

include/mbedtls/pem.h Outdated Show resolved Hide resolved

mpg mentioned this pull request Feb 7, 2022

Improve pem testing #5508

Closed

gstrauss dismissed mpg’s stale review via fa66ba8 February 7, 2022 12:16

gstrauss force-pushed the mbedtls_pem_get_der branch from a4afa2f to fa66ba8 Compare February 7, 2022 12:16

gilles-peskine-arm self-requested a review February 7, 2022 12:26

gilles-peskine-arm requested changes Feb 7, 2022

View reviewed changes

gilles-peskine-arm added needs-work component-crypto Crypto primitives and low-level interfaces 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 Feb 7, 2022

gstrauss force-pushed the mbedtls_pem_get_der branch from 6b65826 to 7f53da7 Compare February 7, 2022 15:52

gstrauss force-pushed the mbedtls_pem_get_der branch from 7f53da7 to 15d9d44 Compare February 7, 2022 15:55

gstrauss requested review from gilles-peskine-arm and mpg February 7, 2022 15:59

gstrauss force-pushed the mbedtls_pem_get_der branch from 15d9d44 to 7acf564 Compare February 7, 2022 16:13

gstrauss and others added 3 commits February 8, 2022 14:53

Add accessor to get buf from mbedtls_pem_context

72bd4e4

Co-authored-by: Gilles Peskine <gilles.peskine@arm.com> Signed-off-by: Glenn Strauss <gstrauss@gluelogic.com>

Add success case for pem_read testing

33ab075

Currently all cases were negative, so the block that exercised mbedtls_pem_get_der() would never be reached. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Check the result of PEM decoding

1df23b9

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

gstrauss force-pushed the mbedtls_pem_get_der branch from 7acf564 to 1df23b9 Compare February 8, 2022 19:54

mpg approved these changes Feb 10, 2022

View reviewed changes

mpg added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Feb 10, 2022

gilles-peskine-arm mentioned this pull request Feb 10, 2022

Support parsing non-nul-terminated PEM strings #5411

Closed

gilles-peskine-arm approved these changes Feb 10, 2022

View reviewed changes

gilles-peskine-arm merged commit bebeae9 into Mbed-TLS:development Feb 10, 2022

gilles-peskine-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, labels Feb 10, 2022

gstrauss deleted the mbedtls_pem_get_der branch February 11, 2022 00:09

daverodgman mentioned this pull request Feb 28, 2022

Add missing accessor for mbedtls_x509_crt.ext_types for uacme #5585

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add accessor to get der from mbedtls_pem_context #5504

Add accessor to get der from mbedtls_pem_context #5504

gstrauss commented Feb 4, 2022 •

edited by paul-elliott-arm

Loading

mpg left a comment

mpg commented Feb 7, 2022

gstrauss commented Feb 7, 2022

gilles-peskine-arm left a comment

gstrauss commented Feb 7, 2022

gstrauss commented Feb 7, 2022

mpg commented Feb 8, 2022

gstrauss commented Feb 8, 2022

mpg left a comment

gstrauss commented Feb 10, 2022

gilles-peskine-arm commented Feb 10, 2022

gilles-peskine-arm left a comment

gilles-peskine-arm Feb 10, 2022

gstrauss Feb 11, 2022

gilles-peskine-arm commented Feb 10, 2022 •

edited

Loading

gstrauss commented Feb 11, 2022

		TEST_EQUAL( use_len, out->len );
		TEST_ASSERT( memcmp( out->x, buf, out->len ) == 0 );

	TEST_EQUAL( use_len, out->len );
	TEST_ASSERT( memcmp( out->x, buf, out->len ) == 0 );
	ASSERT_COMPARE( out->x, out->len, buf, use_len );

Add accessor to get der from mbedtls_pem_context #5504

Add accessor to get der from mbedtls_pem_context #5504

Conversation

gstrauss commented Feb 4, 2022 • edited by paul-elliott-arm Loading

Description

Status

Requires Backporting

Migrations

Todos

mpg left a comment

Choose a reason for hiding this comment

mpg commented Feb 7, 2022

gstrauss commented Feb 7, 2022

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gstrauss commented Feb 7, 2022

gstrauss commented Feb 7, 2022

mpg commented Feb 8, 2022

gstrauss commented Feb 8, 2022

mpg left a comment

Choose a reason for hiding this comment

gstrauss commented Feb 10, 2022

gilles-peskine-arm commented Feb 10, 2022

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Feb 10, 2022

Choose a reason for hiding this comment

gstrauss Feb 11, 2022

Choose a reason for hiding this comment

gilles-peskine-arm commented Feb 10, 2022 • edited Loading

gstrauss commented Feb 11, 2022

gstrauss commented Feb 4, 2022 •

edited by paul-elliott-arm

Loading

gilles-peskine-arm commented Feb 10, 2022 •

edited

Loading