Support is out of scope from all the major vendors for these versions
(for example RHEL5 uses a version based on 0.9.8, and RHEL6 uses 1.0.1),
and it created some extra maintenance work. Upstream has stopped
support of 0.9.8 in December 2015 and of 1.0.0 in February 2016.
Since b1abfec, note that the default SSL protocol version set with
ssl_min_protocol_version is TLSv1.2, whose support was added in OpenSSL
1.0.1, so there is no point to enforce ssl_min_protocol_version to TLSv1
in the SSL tests.
Author: Michael Paquier
Reviewed-by: Daniel Gustafsson, Tom Lane
Discussion: https://postgr.es/m/20191205083252.GE5064@paquier.xyz
We realized years ago that it's better for libpq to accept all
connection parameters syntactically, even if some are ignored or
restricted due to lack of the feature in a particular build.
However, that lesson from the SSL support was for some reason never
applied to the GSSAPI support. This is causing various buildfarm
members to have problems with a test case added by commit 6136e94dc,
and it's just a bad idea from a user-experience standpoint anyway,
so fix it.
While at it, fix some places where parameter-related infrastructure
was added with the aid of a dartboard, or perhaps with the aid of
the anti-pattern "add new stuff at the end". It should be safe
to rearrange the contents of struct pg_conn even in released
branches, since that's private to libpq (and we'd have to move
some fields in some builds to fix this, anyway).
Back-patch to all supported branches.
Discussion: https://postgr.es/m/11297.1576868677@sss.pgh.pa.us
The getpeereid() uses have so far been protected by HAVE_UNIX_SOCKETS,
so they didn't ever care about Windows support. But in anticipation
of Unix-domain socket support on Windows, that needs to be handled
differently.
Windows doesn't support getpeereid() at this time, so we use the
existing not-supported code path. We let configure do its usual thing
of picking up the replacement from libpgport, instead of the custom
overrides that it was doing before.
But then Windows doesn't have struct passwd, so this patch sprinkles
some additional #ifdef WIN32 around to make it work. This is similar
to existing code that deals with this issue.
Reviewed-by: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
Discussion: https://www.postgresql.org/message-id/5974caea-1267-7708-40f2-6009a9d653b0@2ndquadrant.com
This partially reverts commit 4dc6355210.
The information returned by the function can be obtained by calling
PQconninfo(), so the function is redundant.
This function is supported down to OpenSSL 0.9.8, which is the oldest
version supported since 593d4e4 (from Postgres 10 onwards), and is used
since e3bdb2d (from 11 onwards). It is defined as a macro from OpenSSL
0.9.8 to 1.0.2, and as a function in 1.1.0 and newer versions. However,
the configure check present is only adapted for functions. So, even if
the code would be able to compile, configure fails to detect the macro,
causing it to be ignored when compiling the code with OpenSSL from 0.9.8
to 1.0.2.
The code needs a configure check as per a364dfa, which has fixed a
compilation issue with a past version of LibreSSL in NetBSD 5.1. On
HEAD, just remove the configure check as the last release of NetBSD 5 is
from 2014 (and we have no more buildfarm members for it). In 11 and 12,
improve the configure logic so as both macros and functions are
correctly detected. This makes NetBSD 5 still work on already-released
branches, but not for 13 onwards.
The patch for HEAD is from me, and Daniel has written the version to use
for the back-branches.
Author: Michael Paquier, Daniel Gustaffson
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/20191205083252.GE5064@paquier.xyz
Discussion: https://postgr.es/m/98F7F99E-1129-41D8-B86B-FE3B1E286881@yesql.se
Backpatch-through: 11
This patch providies for support for password protected SSL client
keys in libpq, and for DER format keys, both encrypted and unencrypted.
There is a new connection parameter sslpassword, which is supplied to
the OpenSSL libraries via a callback function. The callback function can
also be set by an application by calling PQgetSSLKeyPassHook(). There is
also a function to retreive the connection setting, PQsslpassword().
Craig Ringer and Andrew Dunstan
Reviewed by: Greg Nancarrow
Discussion: https://postgr.es/m/f7ee88ed-95c4-95c1-d4bf-7b415363ab62@2ndQuadrant.com
When using %b or %B patterns to format a date, the code was simply using
tm_mon as an index into array of month names. But that is wrong, because
tm_mon is 1-based, while array indexes are 0-based. The result is we
either use name of the next month, or a segfault (for December).
Fix by subtracting 1 from tm_mon for both patterns, and add a regression
test triggering the issue. Backpatch to all supported versions (the bug
is there far longer, since at least 2003).
Reported-by: Paul Spencer
Backpatch-through: 9.4
Discussion: https://postgr.es/m/16143-0d861eb8688d3fef%40postgresql.org
This completes the task begun in commit 1408d5d86, to synchronize
ECPG's exported definitions with the definition of bool used by
c.h (and, therefore, the one actually in use in the ECPG library).
On practically all modern platforms, ecpglib.h will now just
include <stdbool.h>, which should surprise nobody anymore.
That removes a header-inclusion-order hazard for ECPG clients,
who previously might get build failures or unexpected behavior
depending on whether they'd included <stdbool.h> themselves,
and if so, whether before or after ecpglib.h.
On platforms where sizeof(_Bool) is not 1 (only old PPC-based
Mac systems, as far as I know), things are still messy, as
inclusion of <stdbool.h> could still break ECPG client code.
There doesn't seem to be any clean fix for that, and given the
probably-negligible population of users who would care anymore,
it's not clear we should go far out of our way to cope with it.
This change at least fixes some header-inclusion-order hazards
for our own code, since c.h and ecpglib.h previously disagreed
on whether bool should be char or unsigned char.
To implement this with minimal invasion of ECPG client namespace,
move the choice of whether to rely on <stdbool.h> into configure,
and have it export a configuration symbol PG_USE_STDBOOL.
ecpglib.h no longer exports definitions for TRUE and FALSE,
only their lowercase brethren. We could undo that if we get
push-back about it.
Ideally we'd back-patch this as far as v11, which is where c.h
started to rely on <stdbool.h>. But the odds of creating problems
for formerly-working ECPG client code seem about as large as the
odds of fixing any non-working cases, so we'll just do this in HEAD.
Discussion: https://postgr.es/m/CAA4eK1LmaKO7Du9M9Lo=kxGU8sB6aL8fa3sF6z6d5yYYVe3BuQ@mail.gmail.com
Declaring this in the client-visible header ecpglib.h was a pretty
poor decision. It's not meant to be application-callable (and if
it was, putting it outside the extern "C" { ... } wrapper means
that C++ clients would fail to call it). And the declaration would
not even compile for a client, anyway, since it would not have the
macro pg_attribute_format_arg(). Fortunately, it seems that no
clients have tried to include this header with ENABLE_NLS defined,
or we'd have gotten complaints about that. But we have no business
putting such a restriction on client code.
Move the declaration to ecpglib_extern.h, since in fact nothing
outside src/interfaces/ecpg/ecpglib/ needs to call it.
The practical effect of this is just that clients can now safely
#include ecpglib.h while having ENABLE_NLS defined, but that seems
like enough of a reason to back-patch it.
Discussion: https://postgr.es/m/20590.1573069709@sss.pgh.pa.us
When maintaining or merging patches, one of the most common sources
for conflicts are the list of objects in makefiles. Especially when
the split across lines has been changed on both sides, which is
somewhat common due to attempting to stay below 80 columns, those
conflicts are unnecessarily laborious to resolve.
By splitting, and alphabetically sorting, OBJS style lines into one
object per line, conflicts should be less frequent, and easier to
resolve when they still occur.
Author: Andres Freund
Discussion: https://postgr.es/m/20191029200901.vww4idgcxv74cwes@alap3.anarazel.de
Under MinGW, when compiling the ecpg test files, you get compiler
warnings about the use of %lld in printf().
These files don't use our printf replacement or the c.h porting layer,
so determine the appropriate format conversion the hard way.
Reviewed-by: Michael Meskes <meskes@postgresql.org>
Discussion: https://www.postgresql.org/message-id/flat/760c9dd1-2d80-c223-3f90-609b615f7918%402ndquadrant.com
pgtypeslib_extern.h contained fallback definitions of "bool", "FALSE",
and "TRUE". The latter two are just plain unused, and have been for
awhile. The former came into play only if there wasn't a macro
definition of "bool", which is true only if we aren't using <stdbool.h>.
However, it then defined bool as "char"; since commit d26a810eb that
conflicts with c.h's desire to use "unsigned char". We'd missed seeing
any bad effects of that due to accidental header inclusion order choices,
but dddf4cdc3 exposed that it was problematic.
To fix, let's just get rid of these definitions. They should not be
needed because everyplace in Postgres should be relying on c.h to
provide a definition for type bool. (Note that despite its name,
pgtypeslib_extern.h isn't exposed to any outside code; we don't
install it.)
This doesn't fully resolve the issue, because ecpglib.h is doing
similar things, but that seems to require more thought to fix.
Back-patch to v12 where d26a810eb came in, to forestall any unpleasant
surprises from future back-patched bug fixes.
Discussion: https://postgr.es/m/CAA4eK1LmaKO7Du9M9Lo=kxGU8sB6aL8fa3sF6z6d5yYYVe3BuQ@mail.gmail.com
The commit dddf4cdc3 tries to ensure that the Postgres header file
inclusions are in order based on their ASCII value. However, in one of
the case there is a header file dependency due to which we can't maintain
such order.
Author: Amit Kapila
Discussion: https://postgr.es/m/E1iNpHW-000855-1u@gemulon.postgresql.org
A check was redundant. While on it, add an assertion to make sure that
the parsing routine is never called with a NULL input. All the code
paths currently calling the parsing routine are careful with NULL inputs
already, but future callers may forget that.
Reported-by: Peter Eisentraut, Lars Kanis
Discussion: https://postgr.es/m/ec64956b-4597-56b6-c3db-457d15250fe4@2ndquadrant.com
Backpatch-through: 12
The logic was correctly detecting a parsing failure, but the parsing
error did not get reported back to the client properly.
Reported-by: Ed Morley
Author: Lars Kanis
Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/a9b4cbd7-4ecb-06b2-ebd7-1739bbff3217@greiz-reinsdorf.de
Backpatch-through: 12
Commit e7a2217 has introduced stricter checks for integer values in
connection parameters for libpq. However this failed to correctly check
after trailing whitespaces, while leading whitespaces were discarded per
the use of strtol(3). This fixes and refactors the parsing logic to
handle both cases consistently. Note that trying to restrict the use of
trailing whitespaces can easily break connection strings like in ECPG
regression tests (these have allowed me to catch the parsing bug with
connect_timeout).
Author: Michael Paquier
Reviewed-by: Lars Kanis
Discussion: https://postgr.es/m/a9b4cbd7-4ecb-06b2-ebd7-1739bbff3217@greiz-reinsdorf.de
Backpatch-through: 12
There were some leftovers from ancient ad-hoc ways to build on
Windows, prior to the standardization on MSVC and MinGW. We don't
need to build a lib$(NAME)ddll.def (debug build, as opposed to
lib$(NAME)dll.def) for MinGW, since nothing uses that. We also don't
need to build the regular .def file during distprep, since the MinGW
build environment is perfectly capable of creating that normally at
build time.
Discussion: https://www.postgresql.org/message-id/flat/0f9db9f8-47b8-a48b-6ccc-15b22b412316%402ndquadrant.com
Within the context of SCRAM, "verifier" has a specific meaning in the
protocol, per RFCs. The existing code used "verifier" differently, to
mean whatever is or would be stored in pg_auth.rolpassword.
Fix this by using the term "secret" for this, following RFC 5803.
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://www.postgresql.org/message-id/flat/be397b06-6e4b-ba71-c7fb-54cae84a7e18%402ndquadrant.com
This reverts commit f7ab80285. Per discussion, we can't remove an
exported symbol without a SONAME bump, which we don't want to do.
In particular that breaks usage of current libpq.so with pre-9.3
versions of psql etc, which need libpq to export pqsignal().
As noted in that commit message, exporting the symbol from libpgport.a
won't work reliably; but actually we don't want to export src/port's
implementation anyway. Any pre-9.3 client is going to be expecting the
definition that pqsignal() had before 9.3, which was that it didn't
set SA_RESTART for SIGALRM. Hence, put back pqsignal() in a separate
source file in src/interfaces/libpq, and give it the old semantics.
Back-patch to v12.
Discussion: https://postgr.es/m/E1g5vmT-0003K1-6S@gemulon.postgresql.org
As of d9dd406fe2, we require MSVC 2013,
which means _MSC_VER >= 1800. This means that conditionals about
older versions of _MSC_VER can be removed or simplified.
Previous code was also in some cases handling MinGW, where _MSC_VER is
not defined at all, incorrectly, such as in pg_ctl.c and win32_port.h,
leading to some compiler warnings. This should now be handled better.
Reviewed-by: Michael Paquier <michael@paquier.xyz>
When using a client compiled without channel binding support (linking to
OpenSSL 1.0.1 or older) to connect to a server which supports channel
binding (linking to OpenSSL 1.0.2 or newer), libpq would generate a
confusing error message with channel_binding=require for an SSL
connection, where the server sends back SCRAM-SHA-256-PLUS:
"channel binding is required, but server did not offer an authentication
method that supports channel binding."
This is confusing because the server did send a SASL mechanism able to
support channel binding, but libpq was not able to detect that
properly.
The situation can be summarized as followed for the case described in
the previous paragraph for the SASL mechanisms used with the various
modes of channel_binding:
1) Client supports channel binding.
1-1) channel_binding = disable => OK, with SCRAM-SHA-256.
1-2) channel_binding = prefer => OK, with SCRAM-SHA-256-PLUS.
1-3) channel_binding = require => OK, with SCRAM-SHA-256-PLUS.
2) Client does not support channel binding.
2-1) channel_binding = disable => OK, with SCRAM-SHA-256.
2-2) channel_binding = prefer => OK, with SCRAM-SHA-256.
2-3) channel_binding = require => failure with new error message,
instead of the confusing one.
This commit updates case 2-3 to generate a better error message. Note
that the SSL TAP tests are not impacted as it is not possible to test
with mixed versions of OpenSSL for the backend and libpq.
Reported-by: Tom Lane
Author: Michael Paquier
Reviewed-by: Jeff Davis, Tom Lane
Discussion: https://postgr.es/m/24857.1569775891@sss.pgh.pa.us
Coverity pointed out that it's pretty silly to check for a null pointer
after we've already dereferenced the pointer. To fix, just swap the
order of the two error checks. Oversight in commit d6e612f83.
HEAD supports OpenSSL 0.9.8 and newer versions, and this code likely got
forgotten as its surrounding comments mention an incorrect version
number.
Author: Michael Paquier
Reviewed-by: Peter Eisentraut
Discussion: https://postgr.es/m/20190927032311.GB8485@paquier.xyz
This reverts commit bd7c95f0c1,
along with assorted follow-on fixes. There are some questions
about the definition and implementation of that statement, and
we don't have time to resolve them before v13 release. Rather
than ship the feature and then have backwards-compatibility
concerns constraining any redesign, let's remove it for now
and try again later.
Discussion: https://postgr.es/m/TY2PR01MB2443EC8286995378AEB7D9F8F5B10@TY2PR01MB2443.jpnprd01.prod.outlook.com
Make the error messages around GSSAPI encryption a bit clearer. Tweak
some messages to avoid plural problems.
Also make a code change for clarity. Using "conf" for "confidential"
is quite confusing. Using "conf_state" is perhaps not much better but
that's what the GSSAPI documentation uses, so there is at least some
hope of understanding it.
There was some duplicate code to run SHOW transaction_read_only to
determine whether the server is read-write or read-only. Reduce it by
adding another state to the state machine.
Author: Hari Babu Kommi
Reviewed-by: Takayuki Tsunakawa, Álvaro Herrera
Discussion: https://postgr.es/m/CAJrrPGe_qgdbbN+yBgEVpd+YLHXXjTruzk6RmTMhqrFig+32ag@mail.gmail.com
Use the explicit_bzero() function in places where it is important that
security information such as passwords is cleared from memory. There
might be other places where it could be useful; this is just an
initial collection.
For platforms that don't have explicit_bzero(), provide various
fallback implementations. (explicit_bzero() itself isn't standard,
but as Linux/glibc, FreeBSD, and OpenBSD have it, it's the most common
spelling, so it makes sense to make that the invocation point.)
Discussion: https://www.postgresql.org/message-id/flat/42d26bde-5d5b-c90d-87ae-6cab875f73be%402ndquadrant.com
The comment describing the string format was a lie. Make it agree with
reality, add/improve some other comments, fix coding style for loops with
empty bodies. Also add an Assert that we counted parameters correctly,
because the spread-out logic for that looks pretty fragile.
No actual bugs fixed here, so no need to back-patch.
Discussion: https://postgr.es/m/848B1649C8A6274AA527C4472CA11EDD5FC70CBE@G01JPEXMBYT02
Commit a4327296d taught pg_regress proper to do this, but
missed the opportunity to do likewise in the isolationtester
and ecpg variants of pg_regress. Seems like this might be
helpful for tracking down issues exposed by those tests.
Prefix inet_net_ntop and sibling routines with "pg_" to ensure that
they aren't mistaken for C-library functions. This fixes warnings
from cpluspluscheck on some platforms, and should help reduce reader
confusion everywhere, since our functions aren't exactly interchangeable
with the library versions (they may have different ideas about address
family codes).
This shouldn't be fixing any actual bugs, unless somebody's linker
is misbehaving, so no need to back-patch.
Discussion: https://postgr.es/m/20518.1559494394@sss.pgh.pa.us
This has to have <time.h>, or the references to "struct tm" don't
mean what they should.
We have some other recently-introduced issues of the same ilk,
but this one seems old. No backpatch though, as it's only a
latent problem for most purposes.
b654714 has reworked the way trailing CR/LF characters are removed from
strings. This commit introduces a new routine in common/string.c and
refactors the code so as the logic is in a single place, mostly.
Author: Michael Paquier
Reviewed-by: Bruce Momjian
Discussion: https://postgr.es/m/20190801031820.GF29334@paquier.xyz
libpq failed to ignore Windows-style newlines in connection service files.
This normally wasn't a problem on Windows itself, because fgets() would
convert \r\n to just \n. But if libpq were running inside a program that
changes the default fopen mode to binary, it would see the \r's and think
they were data. In any case, it's project policy to ignore \r in text
files unconditionally, because people sometimes try to use files with
DOS-style newlines on Unix machines, where the C library won't hide that
from us.
Hence, adjust parseServiceFile() to ignore \r as well as \n at the end of
the line. In HEAD, go a little further and make it ignore all trailing
whitespace, to match what it's always done with leading whitespace.
In HEAD, also run around and fix up everyplace where we have
newline-chomping code to make all those places look consistent and
uniformly drop \r. It is not clear whether any of those changes are
fixing live bugs. Most of the non-cosmetic changes are in places that
are reading popen output, and the jury is still out as to whether popen
on Windows can return \r\n. (The Windows-specific code in pipe_read_line
seems to think so, but our lack of support for this elsewhere suggests
maybe it's not a problem in practice.) Hence, I desisted from applying
those changes to back branches, except in run_ssl_passphrase_command()
which is new enough and little-tested enough that we'd probably not have
heard about any problems there.
Tom Lane and Michael Paquier, per bug #15827 from Jorge Gustavo Rocha.
Back-patch the parseServiceFile() change to all supported branches,
and the run_ssl_passphrase_command() change to v11 where that was added.
Discussion: https://postgr.es/m/15827-e6ba53a3a7ed543c@postgresql.org
This is numbered take 7, and addresses a set of issues around:
- Fixes for typos and incorrect reference names.
- Removal of unneeded comments.
- Removal of unreferenced functions and structures.
- Fixes regarding variable name consistency.
Author: Alexander Lakhin
Discussion: https://postgr.es/m/10bfd4ac-3e7c-40ab-2b2e-355ed15495e8@gmail.com
This addresses a couple of issues in the code:
- Typos and inconsistencies in comments and function declarations.
- Removal of unreferenced function declarations.
- Removal of unnecessary compile flags.
- A cleanup error in regressplans.sh.
Author: Alexander Lakhin
Discussion: https://postgr.es/m/0c991fdf-2670-1997-c027-772a420c4604@gmail.com
This is a follow-up refactoring after 09ec55b and b674211, which has
proved that the encoding and decoding routines used by SCRAM have a
poor interface when it comes to check after buffer overflows. This adds
an extra argument in the shape of the length of the result buffer for
each routine, which is used for overflow checks when encoding or
decoding an input string. The original idea comes from Tom Lane.
As a result of that, the encoding routine can now fail, so all its
callers are adjusted to generate proper error messages in case of
problems.
On failure, the result buffer gets zeroed.
Author: Michael Paquier
Reviewed-by: Daniel Gustafsson
Discussion: https://postgr.es/m/20190623132535.GB1628@paquier.xyz
This changes various places where appendPQExpBuffer was used in places
where it was possible to use appendPQExpBufferStr, and likewise for
appendStringInfo and appendStringInfoString. This is really just a
stylistic improvement, but there are also small performance gains to be
had from doing this.
Discussion: http://postgr.es/m/CAKJS1f9P=M-3ULmPvr8iCno8yvfDViHibJjpriHU8+SXUgeZ=w@mail.gmail.com
The date/time values 'current', 'invalid', and 'undefined' were
removed a long time ago, but the code still contains explicit error
handling for the transition. To simplify the code and avoid having to
handle these values everywhere, just remove the recognition of these
tokens altogether now.
Reviewed-by: Michael Paquier <michael@paquier.xyz>
When a client connects to a rogue server sending specifically-crafted
messages, this can suffice to execute arbitrary code as the operating
system account used by the client.
While on it, fix one error handling when decoding an incorrect salt
included in the first message received from server.
Author: Michael Paquier
Reviewed-by: Jonathan Katz, Heikki Linnakangas
Security: CVE-2019-10164
Backpatch-through: 10
Fixes some problems introduced by 6e5f8d489acc:
* When reusing conninfo data from the previous connection in \connect,
the host address should only be reused if it was specified as
hostaddr; if it wasn't, then 'host' is resolved afresh. We were
reusing the same IP address, which ignores a possible DNS change
as well as any other addresses that the name resolves to than the
one that was used in the original connection.
* PQhost, PQhostaddr: Don't present user-specified hostaddr when we have
an inet_net_ntop-produced equivalent address. The latter has been
put in canonical format, which is cleaner (so it produces "127.0.0.1"
when given "host=2130706433", for example).
* Document the hostaddr-reusing aspect of \connect.
* Fix some code comments
Author: Fabien Coelho
Reported-by: Noah Misch
Discussion: https://postgr.es/m/20190527203713.GA58392@gust.leadboat.com
Declaring a function "inline" still doesn't work with Windows compilers
(C99? what's that?), unless the macro provided by pg_config.h is
in-scope, which it is not in our ECPG test programs. So the workaround
I tried to use in commit 7640f9312 doesn't work for Windows. Revert
the change in printf_hack.h, and instead just blacklist that file
in cpluspluscheck --- since it's a not-installed test file, we don't
really need to verify its C++ cleanliness anyway.
We have a longstanding project convention that all .h files should
be includable with no prerequisites other than postgres.h. This is
tested/relied-on by cpluspluscheck. However, cpluspluscheck has not
historically been applied to most headers outside the src/include
tree, with the predictable consequence that some of them don't work.
Fix that, usually by adding missing #include dependencies.
The change in printf_hack.h might require some explanation: without
it, my C++ compiler whines that the function is unused. There's
not so many call sites that "inline" is going to cost much, and
besides all the callers are in test code that we really don't care
about the size of.
There's no actual bugs being fixed here, so I see no need to back-patch.
Discussion: https://postgr.es/m/b517ec3918d645eb950505eac8dd434e@gaz-is.ru
ecpg_build_params() failed to check for ecpg_alloc failure in one
newly-added code path, and leaked a temporary string in another path.
Errors in commit a1dc6ab46, spotted by Coverity.
ecpg_register_prepared_stmt() is pretty obviously checking the wrong
variable while trying to detect malloc failure. Error in commit
a1dc6ab46, spotted by Coverity.
This is still using the 2.0 version of pg_bsd_indent.
I thought it would be good to commit this separately,
so as to document the differences between 2.0 and 2.1 behavior.
Discussion: https://postgr.es/m/16296.1558103386@sss.pgh.pa.us
Besides implementing the new statement this change fix some issues with the
parsing of PREPARE and EXECUTE statements. The different forms of these
statements are now all handled in a ujnified way.
Author: Matsumura-san <matsumura.ryo@jp.fujitsu.com>
The original placement of this module in src/fe_utils/ is ill-considered,
because several src/common/ modules have dependencies on it, meaning that
libpgcommon and libpgfeutils now have mutual dependencies. That makes it
pointless to have distinct libraries at all. The intended design is that
libpgcommon is lower-level than libpgfeutils, so only dependencies from
the latter to the former are acceptable.
We already have the precedent that fe_memutils and a couple of other
modules in src/common/ are frontend-only, so it's not stretching anything
out of whack to treat logging.c as a frontend-only module in src/common/.
To the extent that such modules help provide a common frontend/backend
environment for the rest of common/ to use, it's a reasonable design.
(logging.c does not yet provide an ereport() emulation, but one can
dream.)
Hence, move these files over, and revert basically all of the build-system
changes made by commit cc8d41511. There are no places that need to grow
new dependencies on libpgcommon, further reinforcing the idea that this
is the right solution.
Discussion: https://postgr.es/m/a912ffff-f6e4-778a-c86a-cf5c47a12933@2ndquadrant.com
pgtls_read_pending is declared to return bool, but what the underlying
SSL_pending function returns is a count of available bytes.
This is actually somewhat harmless if we're using C99 bools, but in
the back branches it's a live bug: if the available-bytes count happened
to be a multiple of 256, it would get converted to a zero char value.
On machines where char is signed, counts of 128 and up could misbehave
as well. The net effect is that when using SSL, libpq might block
waiting for data even though some has already been received.
Broken by careless refactoring in commit 4e86f1b16, so back-patch
to 9.5 where that came in.
Per bug #15802 from David Binderman.
Discussion: https://postgr.es/m/15802-f0911a97f0346526@postgresql.org
The GSSAPI encryption patch neglected to update the protocol
documentation to describe how to set up a GSSAPI encrypted connection
from a client to the server, so fix that by adding the appropriate
documentation to protocol.sgml.
The tests added for encryption support were overly long and couldn't be
run in parallel due to race conditions; this was largely because each
test was setting up its own KDC to perform the tests. Instead, merge
the authentication tests and the encryption tests into the original
test, where we only create one KDC to run the tests with. Also, have
the tests check what the server's opinion is of the connection and if it
was GSS authenticated or encrypted using the pg_stat_gssapi view.
In passing, fix the libpq label for GSSENC-Mode to be consistent with
the "PGGSSENCMODE" environment variable.
Missing protocol documentation pointed out by Michael Paquier.
Issues with the tests pointed out by Tom Lane and Peter Eisentraut.
Refactored tests and added documentation by me.
Reviewed by Robbie Harwood (protocol documentation) and Michael Paquier
(rework of the tests).
I noted that some buildfarm members were complaining about %ld being
used to format values that are (probably) declared size_t. Use %zu
instead, and insert a cast just in case some versions of the GSSAPI
API declare the length field differently. While at it, clean up
gratuitous differences in wording of equivalent messages, show
the complained-of length in all relevant messages not just some,
include trailing newline where needed, adjust random deviations
from project-standard code layout and message style, etc.
Similarly to the set of parameters for keepalive, a connection parameter
for libpq is added as well as a backend GUC, called tcp_user_timeout.
Increasing the TCP user timeout is useful to allow a connection to
survive extended periods without end-to-end connection, and decreasing
it allows application to fail faster. By default, the parameter is 0,
which makes the connection use the system default, and follows a logic
close to the keepalive parameters in its handling. When connecting
through a Unix-socket domain, the parameters have no effect.
Author: Ryohei Nagaura
Reviewed-by: Fabien Coelho, Robert Haas, Kyotaro Horiguchi, Kirk
Jamison, Mikalai Keida, Takayuki Tsunakawa, Andrei Yahorau
Discussion: https://postgr.es/m/EDA4195584F5064680D8130B1CA91C45367328@G01JPEXMBYT04
There was some confusion over the format of the error message returned
from the server during GSSAPI startup; specifically, it was expected
that a length would be returned when, in reality, at this early stage in
the startup sequence, no length is returned from the server as part of
an error message.
Correct the client-side code for dealing with error messages sent by the
server during startup by simply reading what's available into our
buffer, after we've discovered it's an error message, and then reporting
back what was returned.
In passing, also add in documentation of the environment variable
PGGSSENCMODE which was missed previously, and adjust the code to look
for the PGGSSENCMODE variable (the environment variable change was
missed in the prior GSSMODE -> GSSENCMODE commit).
Error-handling issue discovered by Peter Eisentraut, the rest were items
discovered during testing of the error handling.
This is intended for use mostly in test scripts for external tools,
which could do without cross-PG-version variations in error message
wording. Of course, the SQLSTATE isn't guaranteed stable either, but
it should be more so than the error message text.
Note: there's a bit of an ABI change for libpq here, but it seems
OK because if somebody compiles against a newer version of libpq-fe.h,
and then tries to pass PQERRORS_SQLSTATE to PQsetErrorVerbosity()
of an older libpq library, it will be accepted and then act like
PQERRORS_DEFAULT, thanks to the way the tests in pqBuildErrorMessage3
have historically been phrased. That seems acceptable.
Didier Gautheron, reviewed by Dagfinn Ilmari Mannsåker
Discussion: https://postgr.es/m/CAJRYxuKyj4zA+JGVrtx8OWAuBfE-_wN4sUMK4H49EuPed=mOBw@mail.gmail.com
On both the frontend and backend, prepare for GSSAPI encryption
support by moving common code for error handling into a separate file.
Fix a TODO for handling multiple status messages in the process.
Eliminate the OIDs, which have not been needed for some time.
Add frontend and backend encryption support functions. Keep the
context initiation for authentication-only separate on both the
frontend and backend in order to avoid concerns about changing the
requested flags to include encryption support.
In postmaster, pull GSSAPI authorization checking into a shared
function. Also share the initiator name between the encryption and
non-encryption codepaths.
For HBA, add "hostgssenc" and "hostnogssenc" entries that behave
similarly to their SSL counterparts. "hostgssenc" requires either
"gss", "trust", or "reject" for its authentication.
Similarly, add a "gssencmode" parameter to libpq. Supported values are
"disable", "require", and "prefer". Notably, negotiation will only be
attempted if credentials can be acquired. Move credential acquisition
into its own function to support this behavior.
Add a simple pg_stat_gssapi view similar to pg_stat_ssl, for monitoring
if GSSAPI authentication was used, what principal was used, and if
encryption is being used on the connection.
Finally, add documentation for everything new, and update existing
documentation on connection security.
Thanks to Michael Paquier for the Windows fixes.
Author: Robbie Harwood, with changes to the read/write functions by me.
Reviewed in various forms and at different times by: Michael Paquier,
Andres Freund, David Steele.
Discussion: https://www.postgresql.org/message-id/flat/jlg1tgq1ktm.fsf@thriss.redhat.com
This unifies the various ad hoc logging (message printing, error
printing) systems used throughout the command-line programs.
Features:
- Program name is automatically prefixed.
- Message string does not end with newline. This removes a common
source of inconsistencies and omissions.
- Additionally, a final newline is automatically stripped, simplifying
use of PQerrorMessage() etc., another common source of mistakes.
- I converted error message strings to use %m where possible.
- As a result of the above several points, more translatable message
strings can be shared between different components and between
frontends and backend, without gratuitous punctuation or whitespace
differences.
- There is support for setting a "log level". This is not meant to be
user-facing, but can be used internally to implement debug or
verbose modes.
- Lazy argument evaluation, so no significant overhead if logging at
some level is disabled.
- Some color in the messages, similar to gcc and clang. Set
PG_COLOR=auto to try it out. Some colors are predefined, but can be
customized by setting PG_COLORS.
- Common files (common/, fe_utils/, etc.) can handle logging much more
simply by just using one API without worrying too much about the
context of the calling program, requiring callbacks, or having to
pass "progname" around everywhere.
- Some programs called setvbuf() to make sure that stderr is
unbuffered, even on Windows. But not all programs did that. This
is now done centrally.
Soft goals:
- Reduces vertical space use and visual complexity of error reporting
in the source code.
- Encourages more deliberate classification of messages. For example,
in some cases it wasn't clear without analyzing the surrounding code
whether a message was meant as an error or just an info.
- Concepts and terms are vaguely aligned with popular logging
frameworks such as log4j and Python logging.
This is all just about printing stuff out. Nothing affects program
flow (e.g., fatal exits). The uses are just too varied to do that.
Some existing code had wrappers that do some kind of print-and-exit,
and I adapted those.
I tried to keep the output mostly the same, but there is a lot of
historical baggage to unwind and special cases to consider, and I
might not always have succeeded. One significant change is that
pg_rewind used to write all error messages to stdout. That is now
changed to stderr.
Reviewed-by: Donald Dong <xdong@csumb.edu>
Reviewed-by: Arthur Zakirov <a.zakirov@postgrespro.ru>
Discussion: https://www.postgresql.org/message-id/flat/6a609b43-4f57-7348-6480-bd022f924310@2ndquadrant.com
Michael Paquier
Bruce Momjian
Tom Lane
Peter Eisentraut
Andrew Dunstan
Alvaro Herrera
Tomas Vondra
Amit Kapila
Andres Freund
Jeff Davis
David Rowley
Michael Meskes
Noah Misch
Thomas Munro
Stephen Frost