From c50e543e9d08e9012f8614097d066286193a7147 Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Thu, 6 May 2021 12:04:29 -0500 Subject: [PATCH] gtlscertificate: make private key properties readable WebKit wants these private key properties to be readable in order to implement a deserialization function. Currently they are read-only because at the time GTlsCertificate was originally designed, the plan was to support PKCS#11-backed private keys: private keys that are stored on a smartcard, where the private key is completely unreadable. The design goal was to support both memory-backed and smartcard-backed private keys with the same GTlsCertificate API, abstracting away the implementation differences such that code using GTlsCertificate doesn't need to know the difference. The original PKCS#11 implementation was never fully baked and at some point in the past I deleted it all. It has since been replaced with a new implementation, including a GTlsCertificate:private-key-pkcs11-uri property, which is readable. So our current API already exposes the differences between normal private keys and PKCS#11-backed private keys. The point of making the private-key and private-key-pem properties write-only was to avoid exposing this difference. Do we have to make this API function readable? No, because WebKit could be just as well served if we were to expose serialize and deserialize functions instead. But WebKit needs to support serializing and deserializing the non-private portion of GTlsCertificate with older versions of GLib anyway, so we can do whatever is nicest for GLib. And I think making this property readable is nicest, since the original design reason for it to not be readable is now obsolete. The disadvantage to this approach is that it's now possible for an application to read the private-key or private-key-pem property, receive NULL, and think "this certificate must not have a private key," which would be incorrect if the private-key-pkcs11-uri property is set. That seems like a minor risk, but it should be documented. --- gio/gtlscertificate.c | 56 ++++++++++++++++++++----------- gio/tests/gtesttlsbackend.c | 6 ---- gio/tests/gtesttlsbackend.h | 3 -- gio/tests/tls-certificate.c | 67 +++++++++++++++++-------------------- 4 files changed, 67 insertions(+), 65 deletions(-) diff --git a/gio/gtlscertificate.c b/gio/gtlscertificate.c index aadc562a6..cd5fd6403 100644 --- a/gio/gtlscertificate.c +++ b/gio/gtlscertificate.c @@ -84,6 +84,8 @@ g_tls_certificate_get_property (GObject *object, { switch (prop_id) { + case PROP_PRIVATE_KEY: + case PROP_PRIVATE_KEY_PEM: case PROP_PKCS11_URI: case PROP_PRIVATE_KEY_PKCS11_URI: /* Subclasses must override this property but this allows older backends to not fatally error */ @@ -154,17 +156,25 @@ g_tls_certificate_class_init (GTlsCertificateClass *class) G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS)); /** - * GTlsCertificate:private-key: + * GTlsCertificate:private-key: (nullable) * * The DER (binary) encoded representation of the certificate's - * private key, in either PKCS#1 format or unencrypted PKCS#8 - * format. This property (or the #GTlsCertificate:private-key-pem - * property) can be set when constructing a key (eg, from a file), - * but cannot be read. + * private key, in either [PKCS \#1 format](https://datatracker.ietf.org/doc/html/rfc8017) + * or unencrypted [PKCS \#8 format.](https://datatracker.ietf.org/doc/html/rfc5208) + * PKCS \#8 format is supported since 2.32; earlier releases only + * support PKCS \#1. You can use the `openssl rsa` tool to convert + * PKCS \#8 keys to PKCS \#1. * - * PKCS#8 format is supported since 2.32; earlier releases only - * support PKCS#1. You can use the `openssl rsa` - * tool to convert PKCS#8 keys to PKCS#1. + * This property (or the #GTlsCertificate:private-key-pem property) + * can be set when constructing a key (for example, from a file). + * Since GLib 2.70, it is now also readable; however, be aware that if + * the private key is backed by a PKCS \#11 URI – for example, if it + * is stored on a smartcard – then this property will be %NULL. If so, + * the private key must be referenced via its PKCS \#11 URI, + * #GTlsCertificate:private-key-pkcs11-uri. You must check both + * properties to see if the certificate really has a private key. + * When this property is read, the output format will be unencrypted + * PKCS \#8. * * Since: 2.28 */ @@ -173,22 +183,30 @@ g_tls_certificate_class_init (GTlsCertificateClass *class) P_("Private key"), P_("The DER representation of the certificate’s private key"), G_TYPE_BYTE_ARRAY, - G_PARAM_WRITABLE | + G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS)); /** - * GTlsCertificate:private-key-pem: + * GTlsCertificate:private-key-pem: (nullable) * * The PEM (ASCII) encoded representation of the certificate's - * private key in either PKCS#1 format ("`BEGIN RSA PRIVATE - * KEY`") or unencrypted PKCS#8 format ("`BEGIN - * PRIVATE KEY`"). This property (or the - * #GTlsCertificate:private-key property) can be set when - * constructing a key (eg, from a file), but cannot be read. + * private key in either [PKCS \#1 format](https://datatracker.ietf.org/doc/html/rfc8017) + * ("`BEGIN RSA PRIVATE KEY`") or unencrypted + * [PKCS \#8 format](https://datatracker.ietf.org/doc/html/rfc5208) + * ("`BEGIN PRIVATE KEY`"). PKCS \#8 format is supported since 2.32; + * earlier releases only support PKCS \#1. You can use the `openssl rsa` + * tool to convert PKCS \#8 keys to PKCS \#1. * - * PKCS#8 format is supported since 2.32; earlier releases only - * support PKCS#1. You can use the `openssl rsa` - * tool to convert PKCS#8 keys to PKCS#1. + * This property (or the #GTlsCertificate:private-key property) + * can be set when constructing a key (for example, from a file). + * Since GLib 2.70, it is now also readable; however, be aware that if + * the private key is backed by a PKCS \#11 URI - for example, if it + * is stored on a smartcard - then this property will be %NULL. If so, + * the private key must be referenced via its PKCS \#11 URI, + * #GTlsCertificate:private-key-pkcs11-uri. You must check both + * properties to see if the certificate really has a private key. + * When this property is read, the output format will be unencrypted + * PKCS \#8. * * Since: 2.28 */ @@ -197,7 +215,7 @@ g_tls_certificate_class_init (GTlsCertificateClass *class) P_("Private key (PEM)"), P_("The PEM representation of the certificate’s private key"), NULL, - G_PARAM_WRITABLE | + G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS)); /** diff --git a/gio/tests/gtesttlsbackend.c b/gio/tests/gtesttlsbackend.c index 1e07c1e5e..6c48c2394 100644 --- a/gio/tests/gtesttlsbackend.c +++ b/gio/tests/gtesttlsbackend.c @@ -407,12 +407,6 @@ g_test_tls_connection_initable_iface_init (GInitableIface *iface) iface->init = g_test_tls_connection_initable_init; } -const gchar * -g_test_tls_connection_get_private_key_pem (GTlsCertificate *cert) -{ - return ((GTestTlsCertificate *)cert)->key_pem; -} - /* Test database type */ typedef struct _GTestTlsDatabase GTestTlsDatabase; diff --git a/gio/tests/gtesttlsbackend.h b/gio/tests/gtesttlsbackend.h index 11a8bf149..07948fddc 100644 --- a/gio/tests/gtesttlsbackend.h +++ b/gio/tests/gtesttlsbackend.h @@ -39,9 +39,6 @@ struct _GTestTlsBackendClass { GType _g_test_tls_backend_get_type (void); -const gchar *g_test_tls_connection_get_private_key_pem (GTlsCertificate *cert); - - G_END_DECLS #endif /* __G_TEST_TLS_BACKEND_H__ */ diff --git a/gio/tests/tls-certificate.c b/gio/tests/tls-certificate.c index d0a908530..2995b1028 100644 --- a/gio/tests/tls-certificate.c +++ b/gio/tests/tls-certificate.c @@ -40,7 +40,7 @@ pem_parser (const Reference *ref) gchar *pem; gsize pem_len = 0; gchar *parsed_cert_pem = NULL; - const gchar *parsed_key_pem = NULL; + gchar *parsed_key_pem = NULL; GError *error = NULL; /* Check PEM parsing in certificate, private key order. */ @@ -55,13 +55,12 @@ pem_parser (const Reference *ref) g_object_get (cert, "certificate-pem", &parsed_cert_pem, + "private-key-pem", &parsed_key_pem, NULL); - parsed_key_pem = g_test_tls_connection_get_private_key_pem (cert); g_assert_cmpstr (parsed_cert_pem, ==, ref->cert_pems[0]); - g_free (parsed_cert_pem); - parsed_cert_pem = NULL; + g_clear_pointer (&parsed_cert_pem, g_free); g_assert_cmpstr (parsed_key_pem, ==, ref->key_pem); - parsed_key_pem = NULL; + g_clear_pointer (&parsed_key_pem, g_free); g_object_unref (cert); @@ -89,13 +88,12 @@ pem_parser (const Reference *ref) g_object_get (cert, "certificate-pem", &parsed_cert_pem, + "private-key-pem", &parsed_key_pem, NULL); - parsed_key_pem = g_test_tls_connection_get_private_key_pem (cert); g_assert_cmpstr (parsed_cert_pem, ==, ref->cert_pems[0]); - g_free (parsed_cert_pem); - parsed_cert_pem = NULL; + g_clear_pointer (&parsed_cert_pem, g_free); g_assert_cmpstr (parsed_key_pem, ==, ref->key_pem); - parsed_key_pem = NULL; + g_clear_pointer (&parsed_key_pem, g_free); g_free (pem); g_object_unref (cert); @@ -111,11 +109,10 @@ pem_parser (const Reference *ref) g_object_get (cert, "certificate-pem", &parsed_cert_pem, + "private-key-pem", &parsed_key_pem, NULL); - parsed_key_pem = g_test_tls_connection_get_private_key_pem (cert); g_assert_cmpstr (parsed_cert_pem, ==, ref->cert_pems[0]); - g_free (parsed_cert_pem); - parsed_cert_pem = NULL; + g_clear_pointer (&parsed_cert_pem, g_free); g_assert_null (parsed_key_pem); g_free (pem); @@ -141,7 +138,7 @@ pem_parser_handles_chain (const Reference *ref) GTlsCertificate *original_cert; gchar *pem; gchar *parsed_cert_pem = NULL; - const gchar *parsed_key_pem = NULL; + gchar *parsed_key_pem = NULL; GError *error = NULL; /* Check that a chain with exactly three certificates is returned */ @@ -156,14 +153,14 @@ pem_parser_handles_chain (const Reference *ref) g_object_get (cert, "certificate-pem", &parsed_cert_pem, + "private-key-pem", &parsed_key_pem, NULL); g_assert_cmpstr (parsed_cert_pem, ==, ref->cert_pems[0]); g_clear_pointer (&parsed_cert_pem, g_free); /* Make sure the private key was parsed */ - parsed_key_pem = g_test_tls_connection_get_private_key_pem (cert); g_assert_cmpstr (parsed_key_pem, ==, ref->key_pem); - parsed_key_pem = NULL; + g_clear_pointer (&parsed_key_pem, g_free); /* Now test the second cert */ issuer = g_tls_certificate_get_issuer (cert); @@ -175,12 +172,12 @@ pem_parser_handles_chain (const Reference *ref) g_object_get (cert, "certificate-pem", &parsed_cert_pem, + "private-key-pem", &parsed_key_pem, NULL); g_assert_cmpstr (parsed_cert_pem, ==, ref->cert_pems[1]); g_clear_pointer (&parsed_cert_pem, g_free); /* Only the first cert should have a private key */ - parsed_key_pem = g_test_tls_connection_get_private_key_pem (cert); g_assert_null (parsed_key_pem); /* Now test the final cert */ @@ -190,11 +187,11 @@ pem_parser_handles_chain (const Reference *ref) g_object_get (cert, "certificate-pem", &parsed_cert_pem, + "private-key-pem", &parsed_key_pem, NULL); g_assert_cmpstr (parsed_cert_pem, ==, ref->cert_pems[2]); g_clear_pointer (&parsed_cert_pem, g_free); - parsed_key_pem = g_test_tls_connection_get_private_key_pem (cert); g_assert_null (parsed_key_pem); g_object_unref (original_cert); @@ -237,7 +234,7 @@ from_file (const Reference *ref) { GTlsCertificate *cert; gchar *parsed_cert_pem = NULL; - const gchar *parsed_key_pem = NULL; + gchar *parsed_key_pem = NULL; GError *error = NULL; cert = g_tls_certificate_new_from_file (g_test_get_filename (G_TEST_DIST, "cert-tests", "key-cert.pem", NULL), @@ -247,13 +244,12 @@ from_file (const Reference *ref) g_object_get (cert, "certificate-pem", &parsed_cert_pem, + "private-key-pem", &parsed_key_pem, NULL); - parsed_key_pem = g_test_tls_connection_get_private_key_pem (cert); g_assert_cmpstr (parsed_cert_pem, ==, ref->cert_pems[0]); - g_free (parsed_cert_pem); - parsed_cert_pem = NULL; + g_clear_pointer (&parsed_cert_pem, g_free); g_assert_cmpstr (parsed_key_pem, ==, ref->key_pem); - parsed_key_pem = NULL; + g_clear_pointer (&parsed_key_pem, g_free); g_object_unref (cert); } @@ -263,7 +259,7 @@ from_files (const Reference *ref) { GTlsCertificate *cert; gchar *parsed_cert_pem = NULL; - const gchar *parsed_key_pem = NULL; + gchar *parsed_key_pem = NULL; GError *error = NULL; cert = g_tls_certificate_new_from_files (g_test_get_filename (G_TEST_DIST, "cert-tests", "cert1.pem", NULL), @@ -274,13 +270,12 @@ from_files (const Reference *ref) g_object_get (cert, "certificate-pem", &parsed_cert_pem, + "private-key-pem", &parsed_key_pem, NULL); - parsed_key_pem = g_test_tls_connection_get_private_key_pem (cert); g_assert_cmpstr (parsed_cert_pem, ==, ref->cert_pems[0]); - g_free (parsed_cert_pem); - parsed_cert_pem = NULL; + g_clear_pointer (&parsed_cert_pem, g_free); g_assert_cmpstr (parsed_key_pem, ==, ref->key_pem); - parsed_key_pem = NULL; + g_clear_pointer (&parsed_key_pem, g_free); g_object_unref (cert); @@ -332,7 +327,7 @@ from_files_crlf (const Reference *ref) { GTlsCertificate *cert; gchar *parsed_cert_pem = NULL; - const gchar *parsed_key_pem = NULL; + gchar *parsed_key_pem = NULL; GError *error = NULL; cert = g_tls_certificate_new_from_files (g_test_get_filename (G_TEST_DIST, "cert-tests", "cert-crlf.pem", NULL), @@ -343,13 +338,12 @@ from_files_crlf (const Reference *ref) g_object_get (cert, "certificate-pem", &parsed_cert_pem, + "private-key-pem", &parsed_key_pem, NULL); - parsed_key_pem = g_test_tls_connection_get_private_key_pem (cert); g_assert_cmpstr (parsed_cert_pem, ==, ref->cert_crlf_pem); - g_free (parsed_cert_pem); - parsed_cert_pem = NULL; + g_clear_pointer (&parsed_cert_pem, g_free); g_assert_cmpstr (parsed_key_pem, ==, ref->key_crlf_pem); - parsed_key_pem = NULL; + g_clear_pointer (&parsed_key_pem, g_free); g_object_unref (cert); } @@ -359,7 +353,7 @@ from_files_pkcs8 (const Reference *ref) { GTlsCertificate *cert; gchar *parsed_cert_pem = NULL; - const gchar *parsed_key_pem = NULL; + gchar *parsed_key_pem = NULL; GError *error = NULL; cert = g_tls_certificate_new_from_files (g_test_get_filename (G_TEST_DIST, "cert-tests", "cert1.pem", NULL), @@ -370,13 +364,12 @@ from_files_pkcs8 (const Reference *ref) g_object_get (cert, "certificate-pem", &parsed_cert_pem, + "private-key-pem", &parsed_key_pem, NULL); - parsed_key_pem = g_test_tls_connection_get_private_key_pem (cert); g_assert_cmpstr (parsed_cert_pem, ==, ref->cert_pems[0]); - g_free (parsed_cert_pem); - parsed_cert_pem = NULL; + g_clear_pointer (&parsed_cert_pem, g_free); g_assert_cmpstr (parsed_key_pem, ==, ref->key8_pem); - parsed_key_pem = NULL; + g_clear_pointer (&parsed_key_pem, g_free); g_object_unref (cert); }