GNOME Bugzilla – Bug 794476
Add support for OCSP stapling to glib-openssl
Last modified: 2018-03-20 07:50:49 UTC
This patch enables support for OCSP stapling to glib-openssl if supported by the OpenSSL version in use. Disabled by default, enabled by setting the environment variable G_TLS_OPENSSL_OCSP_ENABLED to 1. When enabled, still enabled in a soft-fail mode where it will accept any server not giving a response to the OCSP stapling request.
Created attachment 369854 [details] [review] Patch to add OCSP stapling support
Review of attachment 369854 [details] [review]: First of all thanks for working on this. I did not yet review the method verify_ocsp_response but here some general notes: - variables should be with _ instead of camel case - indentation should be 2 spaces - no braces on single liners - a space before ( - asterisk should be attached to the variable name: const gchar *var instead of const gchar* var ::: tls/openssl/gtlsclientconnection-openssl.c @@ +468,2 @@ static gboolean +use_ocsp() functions without parameters should have (void) @@ +469,3 @@ +use_ocsp() +{ + const gchar* ocsp_enabled; 2 spaces indentation @@ +470,3 @@ +{ + const gchar* ocsp_enabled; + ocsp_enabled = g_getenv ("G_TLS_OPENSSL_OCSP_ENABLED"); I would say to just do: g_getenv ("G_TLS_OPENSSL_OCSP_ENABLED") != NULL -> TRUE @@ +567,3 @@ + !defined(OPENSSL_NO_OCSP) + if (use_ocsp()) + { no need for single line braces @@ +568,3 @@ + if (use_ocsp()) + { + SSL_set_tlsext_status_type(priv->ssl, TLSEXT_STATUSTYPE_ocsp); space before ( ::: tls/openssl/gtlsconnection-openssl.c @@ +286,3 @@ + OCSP_RESPONSE *resp = NULL; + long len = 0; + const unsigned char *p = NULL; leave empty line after variables declaration @@ +288,3 @@ + const unsigned char *p = NULL; + ssl = g_tls_connection_openssl_get_ssl (openssl); + len = SSL_get_tlsext_status_ocsp_resp(ssl, &p); space before ( @@ +289,3 @@ + ssl = g_tls_connection_openssl_get_ssl (openssl); + len = SSL_get_tlsext_status_ocsp_resp(ssl, &p); + // Soft fail in case of no response is the best we can do always use /* */ comments since they are c89 compatible @@ +293,3 @@ + return 0; + + resp = d2i_OCSP_RESPONSE(NULL, &p, len); space before ( @@ +295,3 @@ + resp = d2i_OCSP_RESPONSE(NULL, &p, len); + if (resp == NULL) + { no braces for single line @@ +353,3 @@ + if (is_client && (errors == 0)) + errors = verify_ocsp_response(openssl, database, peer_certificate); space before (
Created attachment 369856 [details] [review] Patch to add OCSP stapling support Updated patch which hopefully should have all formatting corrected.
Review of attachment 369856 [details] [review]: The patch is almost there, but since I have not much idea about this feature I would really ask you to provide an unit test as well. ::: tls/openssl/gtlsfiledatabase-openssl.c @@ +756,3 @@ + !defined(OPENSSL_NO_OCSP) + GTlsFileDatabaseOpenssl *file_database = NULL; + GTlsFileDatabaseOpensslPrivate *priv = NULL; no need to set to NULL @@ +779,3 @@ + file_database = G_TLS_FILE_DATABASE_OPENSSL (database); + priv = g_tls_file_database_openssl_get_instance_private (file_database); + store = X509_STORE_new(); space before ( @@ +792,3 @@ + for (int i = 0; i < sk_X509_num (priv->trusted); i++) + { + X509_STORE_add_cert(store, sk_X509_value (priv->trusted, i)); space before ( @@ +803,3 @@ + for (int i = 0; i < OCSP_resp_count (basic_resp); i++) + { + OCSP_SINGLERESP *singleResp = OCSP_resp_get0 (basic_resp, i); these variables should be lower case and separated with _
Created attachment 369863 [details] [review] Patch to add OCSP stapling support One more round of code style fixes.
Review of attachment 369863 [details] [review]: One more thing. ::: tls/openssl/gtlsfiledatabase-openssl.c @@ +807,3 @@ + ASN1_GENERALIZEDTIME *this_update_time = NULL; + ASN1_GENERALIZEDTIME *next_update_time = NULL; + int crlReason = 0; still these 2
Regarding unit tests I guess it should be possible to create tests of the g_tls_file_database_openssl_verify_ocsp_response function, however, I don't think I can do it without dynamically creating ocsp responses using openssl functions in the unit tests due to dependencies on current time for verifying the responses. From what I can see of other tests there is nothing directly using openssl functionality in the tests right now. So I would need to know if it is okay to do that or not.
Created attachment 369864 [details] [review] Patch to add OCSP stapling support
I guess what I would expect is having a cert on the server side with this functionality, connect with the viewer receive it and validate the result? Nothing super fancy, just something that give us as small test for this code.
The functionality isn't really part of the server certificate. How OCSP stapling supports in short is like this 1. The server will periodically be contacting the CA server for OCSP responses (which are signed by the CA) 2. When a client connects and sets the TLSEXT_STATUSTYPE_ocsp extension the server will, if it supports it, send back the OCSP response it has cached during the SSL handshake 3. The client can read of the response from the handshake data and first verify that the response is correct using it's set of trusted roots, and then if valid use the response to check the revocation status of the server certificate. So to test the code in practice I've been using this as part of a browser and pointing the browser at various live sites with different OCSP status.
OK... let's go without it then :)
Review of attachment 369864 [details] [review]: Fine for me.
Pushed to master. Thanks for the patch