Peter, Daniel, The recent commit 8a3d9425 which has introduced SSL passphrase support has also added be-secure-common.c, which works similarly to fe-secure-common.c but for the backend.
I was just reading this code area, when I noticed that check_ssl_key_file_permissions is called by be-secure-openssl.c but the routine is defined in be-secure.c, causing some back-and-forth between the two files. It seems to me that this routine should be logically put into be-secure-common.c so as future SSL implementations can use it. This makes the code more consistent with the frontend refactoring that has happened in f75a959. I would not have bothered about this refactoring if be-secure-openssl.c did not exist yet, but as it does I think that we should bite the bullet, and do that for v11 so as a good base is in place for the future. A patch is attached. Thanks, -- Michael
From 907810a8b35362503f99876ad86599d66652582c Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Mon, 2 Apr 2018 15:36:46 +0900
Subject: [PATCH] Make be-secure-common.c more consistent for future SSL
implementations
Recent commit 8a3d9425 which introduced an implementation for SSL
passphrases has introduced at the same time be-secure-common.c which is
aimed at including backend-side APIs which can be used across any
OpenSSL implementations. The logic behind this file is similar to
fe-secure-openssl.c for the frontend-side APIs.
However, this has forgotten to include check_ssl_key_file_permissions
in the move, which causes a double dependency between be-secure.c and
be-secure-openssl.c.
Refactor the code in a more logic way. This allows puts into light an
API which is usable by future SSL implementations for permissions on SSL
key files.
---
src/backend/libpq/be-secure-common.c | 74 ++++++++++++++++++++++++++++++++++++
src/backend/libpq/be-secure.c | 69 ---------------------------------
src/include/libpq/libpq.h | 3 +-
3 files changed, 76 insertions(+), 70 deletions(-)
diff --git a/src/backend/libpq/be-secure-common.c b/src/backend/libpq/be-secure-common.c
index d1740967f1..46f0b5f4a3 100644
--- a/src/backend/libpq/be-secure-common.c
+++ b/src/backend/libpq/be-secure-common.c
@@ -19,6 +19,9 @@
#include "postgres.h"
+#include <sys/stat.h>
+#include <unistd.h>
+
#include "libpq/libpq.h"
#include "storage/fd.h"
@@ -118,3 +121,74 @@ error:
pfree(command.data);
return len;
}
+
+
+/*
+ * Check permissions for SSL key files.
+ */
+bool
+check_ssl_key_file_permissions(const char *ssl_key_file, bool isServerStart)
+{
+ int loglevel = isServerStart ? FATAL : LOG;
+ struct stat buf;
+
+ if (stat(ssl_key_file, &buf) != 0)
+ {
+ ereport(loglevel,
+ (errcode_for_file_access(),
+ errmsg("could not access private key file \"%s\": %m",
+ ssl_key_file)));
+ return false;
+ }
+
+ if (!S_ISREG(buf.st_mode))
+ {
+ ereport(loglevel,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("private key file \"%s\" is not a regular file",
+ ssl_key_file)));
+ return false;
+ }
+
+ /*
+ * Refuse to load key files owned by users other than us or root.
+ *
+ * XXX surely we can check this on Windows somehow, too.
+ */
+#if !defined(WIN32) && !defined(__CYGWIN__)
+ if (buf.st_uid != geteuid() && buf.st_uid != 0)
+ {
+ ereport(loglevel,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("private key file \"%s\" must be owned by the database user or root",
+ ssl_key_file)));
+ return false;
+ }
+#endif
+
+ /*
+ * Require no public access to key file. If the file is owned by us,
+ * require mode 0600 or less. If owned by root, require 0640 or less to
+ * allow read access through our gid, or a supplementary gid that allows
+ * to read system-wide certificates.
+ *
+ * XXX temporarily suppress check when on Windows, because there may not
+ * be proper support for Unix-y file permissions. Need to think of a
+ * reasonable check to apply on Windows. (See also the data directory
+ * permission check in postmaster.c)
+ */
+#if !defined(WIN32) && !defined(__CYGWIN__)
+ if ((buf.st_uid == geteuid() && buf.st_mode & (S_IRWXG | S_IRWXO)) ||
+ (buf.st_uid == 0 && buf.st_mode & (S_IWGRP | S_IXGRP | S_IRWXO)))
+ {
+ ereport(loglevel,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("private key file \"%s\" has group or world access",
+ ssl_key_file),
+ errdetail("File must have permissions u=rw (0600) or less if owned by the database user, or permissions u=rw,g=r (0640) or less if owned by root.")));
+ return false;
+ }
+#endif
+
+ return true;
+}
diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index fb1f6b5bbe..edfe2c0751 100644
--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -18,12 +18,10 @@
#include "postgres.h"
-#include <sys/stat.h>
#include <signal.h>
#include <fcntl.h>
#include <ctype.h>
#include <sys/socket.h>
-#include <unistd.h>
#include <netdb.h>
#include <netinet/in.h>
#ifdef HAVE_NETINET_TCP_H
@@ -320,70 +318,3 @@ secure_raw_write(Port *port, const void *ptr, size_t len)
return n;
}
-
-bool
-check_ssl_key_file_permissions(const char *ssl_key_file, bool isServerStart)
-{
- int loglevel = isServerStart ? FATAL : LOG;
- struct stat buf;
-
- if (stat(ssl_key_file, &buf) != 0)
- {
- ereport(loglevel,
- (errcode_for_file_access(),
- errmsg("could not access private key file \"%s\": %m",
- ssl_key_file)));
- return false;
- }
-
- if (!S_ISREG(buf.st_mode))
- {
- ereport(loglevel,
- (errcode(ERRCODE_CONFIG_FILE_ERROR),
- errmsg("private key file \"%s\" is not a regular file",
- ssl_key_file)));
- return false;
- }
-
- /*
- * Refuse to load key files owned by users other than us or root.
- *
- * XXX surely we can check this on Windows somehow, too.
- */
-#if !defined(WIN32) && !defined(__CYGWIN__)
- if (buf.st_uid != geteuid() && buf.st_uid != 0)
- {
- ereport(loglevel,
- (errcode(ERRCODE_CONFIG_FILE_ERROR),
- errmsg("private key file \"%s\" must be owned by the database user or root",
- ssl_key_file)));
- return false;
- }
-#endif
-
- /*
- * Require no public access to key file. If the file is owned by us,
- * require mode 0600 or less. If owned by root, require 0640 or less to
- * allow read access through our gid, or a supplementary gid that allows
- * to read system-wide certificates.
- *
- * XXX temporarily suppress check when on Windows, because there may not
- * be proper support for Unix-y file permissions. Need to think of a
- * reasonable check to apply on Windows. (See also the data directory
- * permission check in postmaster.c)
- */
-#if !defined(WIN32) && !defined(__CYGWIN__)
- if ((buf.st_uid == geteuid() && buf.st_mode & (S_IRWXG | S_IRWXO)) ||
- (buf.st_uid == 0 && buf.st_mode & (S_IWGRP | S_IXGRP | S_IRWXO)))
- {
- ereport(loglevel,
- (errcode(ERRCODE_CONFIG_FILE_ERROR),
- errmsg("private key file \"%s\" has group or world access",
- ssl_key_file),
- errdetail("File must have permissions u=rw (0600) or less if owned by the database user, or permissions u=rw,g=r (0640) or less if owned by root.")));
- return false;
- }
-#endif
-
- return true;
-}
diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h
index 997947b091..a74ad521b5 100644
--- a/src/include/libpq/libpq.h
+++ b/src/include/libpq/libpq.h
@@ -92,7 +92,6 @@ extern ssize_t secure_read(Port *port, void *ptr, size_t len);
extern ssize_t secure_write(Port *port, void *ptr, size_t len);
extern ssize_t secure_raw_read(Port *port, void *ptr, size_t len);
extern ssize_t secure_raw_write(Port *port, const void *ptr, size_t len);
-extern bool check_ssl_key_file_permissions(const char *ssl_key_file, bool isServerStart);
extern bool ssl_loaded_verify_locations;
@@ -108,5 +107,7 @@ extern bool SSLPreferServerCiphers;
*/
extern int run_ssl_passphrase_command(const char *prompt, bool is_server_start,
char *buf, int size);
+extern bool check_ssl_key_file_permissions(const char *ssl_key_file,
+ bool isServerStart);
#endif /* LIBPQ_H */
--
2.16.3
signature.asc
Description: PGP signature
