Allow access to result of PEM decoding

[mpg]

The mbedtls_pem_read_buffer() function decodes PEM data and store the decoded data into the provided mbedtls_pem_context object. However, there's no way for an application to access that decoded data from the context except by accessing the fields of the context structure, which we made private in 3.0. As a result, mbedtls_pem_read_buffer() became unusable by applications in 3.0.

This should be fixed, either by making the buf and buflen fields public, or by adding a simple getter function (with appropriate documentation about ownership and lifetime).

[gstrauss]

How does this look? Should it be named mbedtls_pem_get_der (my preference), or should it be named mbedtls_pem_get_buf since it accesses the private buf member?

diff --git a/include/mbedtls/pem.h b/include/mbedtls/pem.h
--- a/include/mbedtls/pem.h
+++ b/include/mbedtls/pem.h
@@ -103,6 +103,18 @@ int mbedtls_pem_read_buffer( mbedtls_pem_context *ctx, const char *header, const
                      const unsigned char *pwd,
                      size_t pwdlen, size_t *use_len );
 
+/**
+ * \brief       Retrieve pointer to der data stored in \c mbedtls_pem_context.
+ *
+ * \param ctx       context to use
+ * \param derlen    destination for length of der data
+ *
+ * \return          const unsigned char * pointer to der data stored in
+ *                  \c mbedtls_pem_context.  Pointer is valid while
+ *                  \c mbedtls_pem_context is not further modified or freed.
+ */
+const unsigned char *mbedtls_pem_get_der( mbedtls_pem_context *ctx, size_t *derlen );
+
 /**
  * \brief       PEM context memory freeing
  *
diff --git a/library/pem.c b/library/pem.c
--- a/library/pem.c
+++ b/library/pem.c
@@ -416,6 +416,12 @@ int mbedtls_pem_read_buffer( mbedtls_pem_context *ctx, const char *header, const
     return( 0 );
 }
 
+const unsigned char *mbedtls_pem_get_der( mbedtls_pem_context *ctx, size_t *derlen )
+{
+    *derlen = ctx->buflen;
+    return( ctx->buf );
+}
+
 void mbedtls_pem_free( mbedtls_pem_context *ctx )
 {
     if ( ctx->buf != NULL )

[gstrauss]

Also, should the accessor be a static inline function in include/mbedtls/pem.h?

[gstrauss]

--- a/include/mbedtls/pem.h
+++ b/include/mbedtls/pem.h
@@ -103,6 +103,23 @@ int mbedtls_pem_read_buffer( mbedtls_pem_context *ctx, const char *header, const
                      const unsigned char *pwd,
                      size_t pwdlen, size_t *use_len );
 
+/**
+ * \brief       Retrieve pointer to der data stored in \c mbedtls_pem_context.
+ *
+ * \param ctx       context to use
+ * \param derlen    destination for length of der data
+ *
+ * \return          const unsigned char * pointer to der data stored in
+ *                  \c mbedtls_pem_context.  Pointer is valid while
+ *                  \c mbedtls_pem_context is not further modified or freed.
+ */
+static inline const unsigned char *mbedtls_pem_get_der( mbedtls_pem_context *ctx, size_t *derlen )
+{
+    *derlen = ctx->MBEDTLS_PRIVATE(buflen);
+    return( ctx->MBEDTLS_PRIVATE(buf) );
+}
+
+
 /**
  * \brief       PEM context memory freeing
  *

[mpg]

I also prefer get_der as it's more semantic. (We've made struct fields private because we don't want people to look at them and we want to be free to change them if needed, so I don't think the public API should try to reflect the names of the fields.)

I think a static inline function is good. It's short enough that there's no code size cost (might even compile to smaller code than a function call), and I think it's better than making the fields public: gives us more freedom if we ever want to change the implementation, and perhaps more importantly, provides a place to document pointer lifetime/ownership (thanks for including that).

Would you like to open a PR with the above patch (plus a ChangeLog entry)?

Aw, I looked at the state of our testing of PEM and it's not looking great. For pem_read we only have failing cases, and only with encryption; it would be good to add at least on case when decoding succeeds and one case of unencrypted data. Then we could check that the result of the decoding is as expected, which would exercise the new getter function.

Would you be OK with extending the testing in that direction? If not, I can look at doing it myself, it's not your fault that we have testing gaps in areas where you want to contribute.

(The bigger picture is there are other tests where PEM decoding succeeds, for example in test_suite_x509parse and so on, but it would be better to have some case directly in test_suite_pem, and more relevant to the discussion at hand, it would provide a good opportunity to exercise the new function.)

[gstrauss]

Would you be OK with extending the testing in that direction? If not, I can look at doing it myself, it's not your fault that we have testing gaps in areas where you want to contribute.

Writing tests is a good thing. "Testing" a struct member accessor function is not complicated, but is little more than code coverage, which has its own value. Given the desire for more than trivial code coverage, I think that you are in a better position to choose how much effort to invest in extending the pem tests, and that might be done in a separate issue, on which this issue can depend.

(To put it another way, I do not think I will have more time to spend in this area.)

[mpg]

Sure, I'll take care of improving testing.

[gstrauss]

I submitted #5504 with a trivial addition to tests to add code coverage for mbedtls_pem_get_der()

FYI: PR #5504 will not be useful to lighttpd until other more important PRs are addressed. In the meantime, lighttpd 1.4.64 disables ALPN "acme-tls/1" support in lighttpd mod_mbedtls when used with mbedtls 3.0.0.

#5413 Allow per-context SNI callback (state)
PR #5426 Add accessors to mbedtls_ssl_context: user data, version
#5430 Add an unconditonnal end-of-ClientHello callback
PR #5454 server certificate selection callback
#5421 Add access to some fields of mbedtls_ssl_ticket
PR #5501 Add mbedtls_ssl_ticket_rotate() for ticket rotation
#4383 Introduce SSL getter API for handshake state