commit e31c57deb18f703fdae8cb0fd5c7b02e6674cada Author: Gurjeet Singh Date: Sun May 22 04:05:59 2022 -0700 Readability improvements to startup-packet and SSL processing functions diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c index 3d0168a369..9303d4be67 100644 --- a/src/backend/libpq/be-secure-openssl.c +++ b/src/backend/libpq/be-secure-openssl.c @@ -85,12 +85,28 @@ static const char *ssl_protocol_version_to_string(int v); /* Public interface */ /* ------------------------------------------------------------ */ +/* + * Try to (re)initialize SSL configuration. + * + * Per the convention established by secure_initialize(), if we're initilizing + * SSL during server startup, we emit a FATAL message on error, otherwise any + * error we encounter is emitted as a LOG message. This way, if configured to + * use SSL, the server will refuse to start on any SSL errors, but will continue + * to operate normally when the SSL configuration is changed and reloaded, even + * if it encounters an error during the reload/reinitilization. + * + * In case of an error during reinitilization, we do not destroy the previously + * initialized, global SSL_context, if any. If there's an already established + * SSL_context, it will continue to serve our needs until next time we try + * reinitilizing the SSL configuration. + */ int be_tls_init(bool isServerStart) { SSL_CTX *context; int ssl_ver_min = -1; int ssl_ver_max = -1; + int logLevel = isServerStart ? FATAL : LOG; /* This stuff need be done only once. */ if (!SSL_initialized) @@ -107,8 +123,7 @@ be_tls_init(bool isServerStart) /* * Create a new SSL context into which we'll load all the configuration - * settings. If we fail partway through, we can avoid memory leakage by - * freeing this context; we don't install it as active until the end. + * settings. * * We use SSLv23_method() because it can negotiate use of the highest * mutually supported protocol version, while alternatives like @@ -118,7 +133,7 @@ be_tls_init(bool isServerStart) context = SSL_CTX_new(SSLv23_method()); if (!context) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errmsg("could not create SSL context: %s", SSLerrmessage(ERR_get_error())))); goto error; @@ -143,7 +158,7 @@ be_tls_init(bool isServerStart) */ if (SSL_CTX_use_certificate_chain_file(context, ssl_cert_file) != 1) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("could not load server certificate file \"%s\": %s", ssl_cert_file, SSLerrmessage(ERR_get_error())))); @@ -163,12 +178,12 @@ be_tls_init(bool isServerStart) SSL_FILETYPE_PEM) != 1) { if (dummy_ssl_passwd_cb_called) - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("private key file \"%s\" cannot be reloaded because it requires a passphrase", ssl_key_file))); else - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("could not load private key file \"%s\": %s", ssl_key_file, SSLerrmessage(ERR_get_error())))); @@ -177,7 +192,7 @@ be_tls_init(bool isServerStart) if (SSL_CTX_check_private_key(context) != 1) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("check of private key failed: %s", SSLerrmessage(ERR_get_error())))); @@ -190,7 +205,7 @@ be_tls_init(bool isServerStart) if (ssl_ver_min == -1) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, /*- translator: first %s is a GUC option name, second %s is its value */ (errmsg("\"%s\" setting \"%s\" not supported by this build", "ssl_min_protocol_version", @@ -201,7 +216,7 @@ be_tls_init(bool isServerStart) if (!SSL_CTX_set_min_proto_version(context, ssl_ver_min)) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errmsg("could not set minimum SSL protocol version"))); goto error; } @@ -213,7 +228,7 @@ be_tls_init(bool isServerStart) if (ssl_ver_max == -1) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, /*- translator: first %s is a GUC option name, second %s is its value */ (errmsg("\"%s\" setting \"%s\" not supported by this build", "ssl_max_protocol_version", @@ -224,7 +239,7 @@ be_tls_init(bool isServerStart) if (!SSL_CTX_set_max_proto_version(context, ssl_ver_max)) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errmsg("could not set maximum SSL protocol version"))); goto error; } @@ -240,7 +255,7 @@ be_tls_init(bool isServerStart) */ if (ssl_ver_min > ssl_ver_max) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errmsg("could not set SSL protocol version range"), errdetail("\"%s\" cannot be higher than \"%s\"", "ssl_min_protocol_version", @@ -277,7 +292,7 @@ be_tls_init(bool isServerStart) /* set up the allowed cipher list */ if (SSL_CTX_set_cipher_list(context, SSLCipherSuites) != 1) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("could not set the cipher list (no valid ciphers available)"))); goto error; @@ -297,7 +312,7 @@ be_tls_init(bool isServerStart) if (SSL_CTX_load_verify_locations(context, ssl_ca_file, NULL) != 1 || (root_cert_list = SSL_load_client_CA_file(ssl_ca_file)) == NULL) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("could not load root certificate file \"%s\": %s", ssl_ca_file, SSLerrmessage(ERR_get_error())))); @@ -346,7 +361,7 @@ be_tls_init(bool isServerStart) } else if (ssl_crl_dir[0] == 0) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("could not load SSL certificate revocation list file \"%s\": %s", ssl_crl_file, SSLerrmessage(ERR_get_error())))); @@ -354,7 +369,7 @@ be_tls_init(bool isServerStart) } else if (ssl_crl_file[0] == 0) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("could not load SSL certificate revocation list directory \"%s\": %s", ssl_crl_dir, SSLerrmessage(ERR_get_error())))); @@ -362,7 +377,7 @@ be_tls_init(bool isServerStart) } else { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("could not load SSL certificate revocation list file \"%s\" or directory \"%s\": %s", ssl_crl_file, ssl_crl_dir, @@ -394,6 +409,7 @@ be_tls_init(bool isServerStart) error: if (context) SSL_CTX_free(context); + return -1; } @@ -962,11 +978,12 @@ load_dh_file(char *filename, bool isServerStart) FILE *fp; DH *dh = NULL; int codes; + int logLevel = isServerStart ? FATAL : LOG; /* attempt to open file. It's not an error if it doesn't exist. */ if ((fp = AllocateFile(filename, "r")) == NULL) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errcode_for_file_access(), errmsg("could not open DH parameters file \"%s\": %m", filename))); @@ -978,7 +995,7 @@ load_dh_file(char *filename, bool isServerStart) if (dh == NULL) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("could not load DH parameters file: %s", SSLerrmessage(ERR_get_error())))); @@ -988,7 +1005,7 @@ load_dh_file(char *filename, bool isServerStart) /* make sure the DH parameters are usable */ if (DH_check(dh, &codes) == 0) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("invalid DH parameters: %s", SSLerrmessage(ERR_get_error())))); @@ -997,7 +1014,7 @@ load_dh_file(char *filename, bool isServerStart) } if (codes & DH_CHECK_P_NOT_PRIME) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("invalid DH parameters: p is not prime"))); DH_free(dh); @@ -1006,7 +1023,7 @@ load_dh_file(char *filename, bool isServerStart) if ((codes & DH_NOT_SUITABLE_GENERATOR) && (codes & DH_CHECK_P_NOT_SAFE_PRIME)) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("invalid DH parameters: neither suitable generator or safe prime"))); DH_free(dh); @@ -1158,6 +1175,7 @@ static bool initialize_dh(SSL_CTX *context, bool isServerStart) { DH *dh = NULL; + int logLevel = isServerStart ? FATAL : LOG; SSL_CTX_set_options(context, SSL_OP_SINGLE_DH_USE); @@ -1167,7 +1185,7 @@ initialize_dh(SSL_CTX *context, bool isServerStart) dh = load_dh_buffer(FILE_DH2048, sizeof(FILE_DH2048)); if (!dh) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("DH: could not load DH parameters"))); return false; @@ -1175,7 +1193,7 @@ initialize_dh(SSL_CTX *context, bool isServerStart) if (SSL_CTX_set_tmp_dh(context, dh) != 1) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("DH: could not set DH parameters: %s", SSLerrmessage(ERR_get_error())))); @@ -1198,11 +1216,12 @@ initialize_ecdh(SSL_CTX *context, bool isServerStart) #ifndef OPENSSL_NO_ECDH EC_KEY *ecdh; int nid; + int logLevel = isServerStart ? FATAL : LOG; nid = OBJ_sn2nid(SSLECDHCurve); if (!nid) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("ECDH: unrecognized curve name: %s", SSLECDHCurve))); return false; @@ -1211,7 +1230,7 @@ initialize_ecdh(SSL_CTX *context, bool isServerStart) ecdh = EC_KEY_new_by_curve_name(nid); if (!ecdh) { - ereport(isServerStart ? FATAL : LOG, + ereport(logLevel, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("ECDH: could not create key"))); return false; diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 3b73e26956..d73ac59565 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -1031,7 +1031,9 @@ PostmasterMain(int argc, char *argv[]) #ifdef USE_SSL if (EnableSSL) { + /* Any failure during SSL initialization here will result in FATAL error. */ (void) secure_initialize(true); + /* ... so here we know for sure that SSL was successfully initialized. */ LoadedSSL = true; } #endif @@ -2094,11 +2096,10 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done) SSLok = 'N'; /* No support for SSL */ #endif -retry1: - if (send(port->sock, &SSLok, 1, 0) != 1) + while (send(port->sock, &SSLok, 1, 0) != 1) { if (errno == EINTR) - goto retry1; /* if interrupted, just retry */ + continue; /* if interrupted, just retry */ ereport(COMMERROR, (errcode_for_socket_access(), errmsg("failed to send SSL negotiation response: %m")));