The frontend-side routine in charge of building a SCRAM verifier
mentioned that the restrictions applying to SASLprep on the password
with the encoding are described at the top of fe-auth-scram.c, but this
information is in auth-scram.c.
This is wrong since 8f8b9be, so backpatch all the way down as this is an
important documentation bit.
Spotted while reviewing a different patch.
Backpatch-through: 11
When a query whose results were requested in single-row mode is the last
in the queue by the time those results are being read, the single-row
flag was not being reset, because we were returning early from
pqPipelineProcessQueue. Move that stanza up so that the flag is always
reset at the end of sending that query's results.
Add a test for the situation.
Backpatch to 14.
Author: Denis Laxalde <denis.laxalde@dalibo.com>
Discussion: https://postgr.es/m/01af18c5-dacc-a8c8-07ee-aecc7650c3e8@dalibo.com
Commit ebc8b7d44 intended to change the behavior of
PQsslAttribute(NULL, "library"), but accidentally also changed
what happens with a non-NULL conn pointer. Undo that so that
only the intended behavior change happens. Clarify some
associated documentation.
Per bug #17625 from Heath Lord. Back-patch to v15.
Discussion: https://postgr.es/m/17625-fc47c78b7d71b534@postgresql.org
The extended query protocol implementation I added in commit
acb7e4eb6b1c has bugs when used in pipeline mode. Rather than spend
more time trying to fix it, remove that code and make the function rely
on simple query protocol only, meaning it can no longer be used in
pipeline mode.
Users can easily change their applications to use PQsendQueryParams
instead. We leave PQsendQuery in place for Postgres 14, just in case
somebody is using it and has not hit the mentioned bugs; but we should
recommend that it not be used.
Backpatch to 15.
Per bug report from Gabriele Varrazzo.
Discussion: https://postgr.es/m/CA+mi_8ZGSQNmW6-mk_iSR4JZB_LJ4ww3suOF+1vGNs3MrLsv4g@mail.gmail.com
There's a convention that externally-visible libpq functions should
check for a NULL PGconn pointer, and fail gracefully instead of
crashing. PQflush() and PQisnonblocking() didn't get that memo
though. Also add a similar check to PQdefaultSSLKeyPassHook_OpenSSL;
while it's not clear that ordinary usage could reach that with a
null conn pointer, it's cheap enough to check, so let's be consistent.
Daniele Varrazzo and Tom Lane
Discussion: https://postgr.es/m/CA+mi_8Zm_mVVyW1iNFgyMd9Oh0Nv8-F+7Y3-BqwMgTMHuo_h2Q@mail.gmail.com
The common recipe when TAP tests are disabled doesn't work, because the
libpq-specific recipe wants to define the PATH environment variable, so
the starting '@' is misinterpreted as part of the command instead of
silencing said command.
Fix by setting the environment variable in a way that doesn't interfere
with the recipe.
Reported-by: Justin Pryzby <pryzby@telsasoft.com>
Discussion: https://postgr.es/m/20220720172321.GL12702@telsasoft.com
Commit 054325c5eeb3 created a memory leak in PQsendQueryInternal in case
an error occurs while sending the message. Repair.
Backpatch to 14, like that commit. Reported by Coverity.
We were going into IDLE state too soon when executing queries via
PQsendQuery in pipeline mode, causing several scenarios to misbehave in
different ways -- most notably, as reported by Daniele Varrazzo, that a
warning message is produced by libpq:
message type 0x33 arrived from server while idle
But it is also possible, if queries are sent and results consumed not in
lockstep, for the expected mediating NULL result values from PQgetResult
to be lost (a problem which has not been reported, but which is more
serious).
Fix this by introducing two new concepts: one is a command queue element
PGQUERY_CLOSE to tell libpq to wait for the CloseComplete server
response to the Close message that is sent by PQsendQuery. Because the
application is not expecting any PGresult from this, the mechanism to
consume it is a bit hackish.
The other concept, authored by Horiguchi-san, is a PGASYNC_PIPELINE_IDLE
state for libpq's state machine to differentiate "really idle" from
merely "the idle state that occurs in between reading results from the
server for elements in the pipeline". This makes libpq not go fully
IDLE when the libpq command queue contains entries; in normal cases, we
only go IDLE once at the end of the pipeline, when the server response
to the final SYNC message is received. (However, there are corner cases
it doesn't fix, such as terminating the query sequence by
PQsendFlushRequest instead of PQpipelineSync; this sort of scenario is
what requires PGQUERY_CLOSE bit above.)
This last bit helps make the libpq state machine clearer; in particular
we can get rid of an ugly hack in pqParseInput3 to avoid considering
IDLE as such when the command queue contains entries.
A new test mode is added to libpq_pipeline.c to tickle some related
problematic cases.
Reported-by: Daniele Varrazzo <daniele.varrazzo@gmail.com>
Co-authored-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://postgr.es/m/CA+mi_8bvD0_CW3sumgwPvWdNzXY32itoG_16tDYRu_1S2gV2iw@mail.gmail.com
Commits a59c79564 et al. tried to sync libpq's SSL key file
permissions checks with what we've used for years in the backend.
We did not intend to create any new failure cases, but it turns out
we did: restricting the key file's ownership breaks cases where the
client is allowed to read a key file despite not having the identical
UID. In particular a client running as root used to be able to read
someone else's key file; and having seen that I suspect that there are
other, less-dubious use cases that this restriction breaks on some
platforms.
We don't really need an ownership check, since if we can read the key
file despite its having restricted permissions, it must have the right
ownership --- under normal conditions anyway, and the point of this
patch is that any additional corner cases where that works should be
deemed allowable, as they have been historically. Hence, just drop
the ownership check, and rearrange the permissions check to get rid
of its faulty assumption that geteuid() can't be zero. (Note that the
comparable backend-side code doesn't have to cater for geteuid() == 0,
since the server rejects that very early on.)
This does have the end result that the permissions safety check used
for a root user's private key file is weaker than that used for
anyone else's. While odd, root really ought to know what she's doing
with file permissions, so I think this is acceptable.
Per report from Yogendra Suralkar. Like the previous patch,
back-patch to all supported branches.
Discussion: https://postgr.es/m/MW3PR15MB3931DF96896DC36D21AFD47CA3D39@MW3PR15MB3931.namprd15.prod.outlook.com
The original coding did this in pqDropServerData(), which seems
fairly backwards. Pending commands are more like already-queued
output data, which is dropped in pqDropConnection(). Moving the
operation means that we clear the command queue immediately upon
detecting connection drop, which improves the sanity of subsequent
behavior. In particular this eliminates duplicated error message
text as a consequence of code added in b15f25446, which supposed
that a nonempty command queue must mean the prior operation is
still active.
There might be an argument for backpatching this to v14; but as with
b15f25446, I'm unsure about interactions with 618c16707. For now,
given the lack of complaints about v14's behavior, leave it alone.
Per report from Peter Eisentraut.
Discussion: https://postgr.es/m/de57761c-b99b-3435-b0a6-474c72b1149a@enterprisedb.com
The testclient and uri-regress programs in the libpq test suite had
quite generic names which didn't convey much meaning. Since they are
installed as part of the MSVC test runs, ensure that their purpose
is a little bit clearer by renaming with a libpq_ prefix. While at
it rename uri-regress to uri_regress to avoid mixing dash and under-
score in the same filename.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20220501080706.GA1542365@rfd.leadboat.com
Commit 618c16707 invented an "error_result" flag in PGconn, which
intends to represent the state that we have an error condition and
need to build a PGRES_FATAL_ERROR PGresult from the message text in
conn->errorMessage, but have not yet done so. (Postponing construction
of the error object simplifies dealing with out-of-memory conditions
and with concatenation of messages for multiple errors.) For nearly all
purposes, this "virtual" PGresult object should act the same as if it
were already materialized. But a couple of places in fe-protocol3.c
didn't get that memo, and were only testing conn->result as they used
to, without also checking conn->error_result.
In hopes of reducing the probability of similar mistakes in future,
I invented a pgHavePendingResult() macro that includes both tests.
Per report from Peter Eisentraut.
Discussion: https://postgr.es/m/b52277b9-fa66-b027-4a37-fb8989c73ff8@enterprisedb.com
The current implementation supports exactly one IP address in a server
certificate's Common Name, which is brittle (the strings must match
exactly). This patch adds support for IPv4 and IPv6 addresses in a
server's Subject Alternative Names.
Per discussion on-list:
- If the client's expected host is an IP address, we allow fallback to
the Subject Common Name if an iPAddress SAN is not present, even if
a dNSName is present. This matches the behavior of NSS, in
violation of the relevant RFCs.
- We also, counter-intuitively, match IP addresses embedded in dNSName
SANs. From inspection this appears to have been the behavior since
the SAN matching feature was introduced in acd08d76.
- Unlike NSS, we don't map IPv4 to IPv6 addresses, or vice-versa.
Author: Jacob Champion <pchampion@vmware.com>
Co-authored-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Co-authored-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://www.postgresql.org/message-id/flat/9f5f20974cd3a4091a788cf7f00ab663d5fcdffe.camel@vmware.com
Currently, libpq client code must have a connection handle
before it can query the "library" SSL attribute. This poses
problems if the client needs to know what SSL library is in
use before constructing a connection string.
Allow PQsslAttribute(NULL, "library") to return the library
in use -- currently, just "OpenSSL" or NULL. The new behavior
is announced with the LIBPQ_HAS_SSL_LIBRARY_DETECTION feature
macro, allowing clients to differentiate between a libpq that
was compiled without SSL support and a libpq that's just too
old to tell.
Author: Jacob Champion <pchampion@vmware.com>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/4c8b76ef434a96627170a31c3acd33cbfd6e41f1.camel@vmware.com
MERGE performs actions that modify rows in the target table using a
source table or query. MERGE provides a single SQL statement that can
conditionally INSERT/UPDATE/DELETE rows -- a task that would otherwise
require multiple PL statements. For example,
MERGE INTO target AS t
USING source AS s
ON t.tid = s.sid
WHEN MATCHED AND t.balance > s.delta THEN
UPDATE SET balance = t.balance - s.delta
WHEN MATCHED THEN
DELETE
WHEN NOT MATCHED AND s.delta > 0 THEN
INSERT VALUES (s.sid, s.delta)
WHEN NOT MATCHED THEN
DO NOTHING;
MERGE works with regular tables, partitioned tables and inheritance
hierarchies, including column and row security enforcement, as well as
support for row and statement triggers and transition tables therein.
MERGE is optimized for OLTP and is parameterizable, though also useful
for large scale ETL/ELT. MERGE is not intended to be used in preference
to existing single SQL commands for INSERT, UPDATE or DELETE since there
is some overhead. MERGE can be used from PL/pgSQL.
MERGE does not support targetting updatable views or foreign tables, and
RETURNING clauses are not allowed either. These limitations are likely
fixable with sufficient effort. Rewrite rules are also not supported,
but it's not clear that we'd want to support them.
Author: Pavan Deolasee <pavan.deolasee@gmail.com>
Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
Author: Amit Langote <amitlangote09@gmail.com>
Author: Simon Riggs <simon.riggs@enterprisedb.com>
Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Reviewed-by: Andres Freund <andres@anarazel.de> (earlier versions)
Reviewed-by: Peter Geoghegan <pg@bowt.ie> (earlier versions)
Reviewed-by: Robert Haas <robertmhaas@gmail.com> (earlier versions)
Reviewed-by: Japin Li <japinli@hotmail.com>
Reviewed-by: Justin Pryzby <pryzby@telsasoft.com>
Reviewed-by: Tomas Vondra <tomas.vondra@enterprisedb.com>
Reviewed-by: Zhihong Yu <zyu@yugabyte.com>
Discussion: https://postgr.es/m/CANP8+jKitBSrB7oTgT9CY2i1ObfOt36z0XMraQc+Xrz8QB0nXA@mail.gmail.com
Discussion: https://postgr.es/m/CAH2-WzkJdBuxj9PO=2QaO9-3h3xGbQPZ34kJH=HukRekwM-GZg@mail.gmail.com
Discussion: https://postgr.es/m/20201231134736.GA25392@alvherre.pgsql
pg_regress reported "Unix socket" as the default location whenever
HAVE_UNIX_SOCKETS is defined. However, that's not been accurate
on Windows since 8f3ec75de. Update this logic to match what libpq
actually does now.
This is just cosmetic, but still it's potentially misleading.
Back-patch to v13 where 8f3ec75de came in.
Discussion: https://postgr.es/m/3894060.1646415641@sss.pgh.pa.us
This change makes libpq apply the same private-key-file ownership
and permissions checks that we have used in the backend since commit
9a83564c5. Namely, that the private key can be owned by either the
current user or root (with different file permissions allowed in the
two cases). This allows system-wide management of key files, which
is just as sensible on the client side as the server, particularly
when the client is itself some application daemon.
Sync the comments about this between libpq and the backend, too.
David Steele
Discussion: https://postgr.es/m/f4b7bc55-97ac-9e69-7398-335e212f7743@pgmasters.net
Since commit ffa2e4670, libpq resets conn->errorMessage only when
starting a new query. However, the later introduction of pipelining
requires a further refinement: the "start of query" isn't necessarily
when it's submitted to PQsendQueryStart. If we clear at that point
then we risk dropping text for an error that the application has not
noticed yet. Instead, when queuing a query while a previous query is
still in flight, leave errorMessage alone; reset it when we begin
to process the next query in pqPipelineProcessQueue.
Perhaps this should be back-patched to v14 where ffa2e4670 came in.
However I'm uncertain about whether it interacts with 618c16707.
In the absence of user complaints, leave v14 alone.
Discussion: https://postgr.es/m/1421785.1645723238@sss.pgh.pa.us
The old form of the test needed a bunch of custom infrastructure. These days
tap tests provide the necessary infrastructure to do better.
We discussed whether to move this test to src/test/modules, alongside
libpq_pipeline, but concluded that the opposite direction would be
better. libpq_pipeline will be moved at a later date, once the buildfarm and
msvc build infrastructure is ready for it.
The invocation of the tap test will be added in the next commit. It involves
just enough buildsystem changes to be worth commiting separately. Can't happen
the other way round because prove errors out when invoked without tests.
Discussion: https://postgr.es/m/20220223203031.ezrd73ohvjgfksow@alap3.anarazel.de
Oversight in commit 618c16707. This is mainly neatnik-ism, since
if PQrequestCancel is used per its API contract, we should perform
pqClearConnErrorState before reaching any place that would consult
errorReported. But still, it seems like a bad idea to potentially
leave errorReported pointing past errorMessage.len.
Since commit ffa2e4670, libpq accumulates text in conn->errorMessage
across a whole query cycle. In some situations, we may report more
than one error event within a cycle: the easiest case to reach is
where we report a FATAL error message from the server, and then a
bit later we detect loss of connection. Since, historically, each
error PGresult bears the entire content of conn->errorMessage,
this results in duplication of the FATAL message in any output that
concatenates the contents of the PGresults.
Accumulation in errorMessage still seems like a good idea, especially
in view of the number of places that did ad-hoc error concatenation
before ffa2e4670. So to fix this, let's track how much of
conn->errorMessage has been read out into error PGresults, and only
include new text in later PGresults. The tricky part of that is
to be sure that we never discard an error PGresult once made (else
we'd risk dropping some text, a problem much worse than duplication).
While libpq formerly did that in some code paths, a little bit of
rearrangement lets us postpone making an error PGresult at all until
we are about to return it.
A side benefit of that postponement is that it now becomes practical
to return a dummy static PGresult in cases where we hit out-of-memory
while trying to manufacture an error PGresult. This eliminates the
admittedly-very-rare case where we'd return NULL from PQgetResult,
indicating successful query completion, even though what actually
happened was an OOM failure.
Discussion: https://postgr.es/m/ab4288f8-be5c-57fb-2400-e3e857f53e46@enterprisedb.com
As currently implemented, failure of a PGEVT_CONNRESET callback
forces the PGconn into the CONNECTION_BAD state (without closing
the socket, which is inconsistent with other failure paths), and
prevents later callbacks from being called. This seems highly
questionable, and indeed is questioned by comments in the source.
Instead, let's just ignore the result value of PGEVT_CONNRESET
calls. Like the preceding commit, this converts event callbacks
into "pure observers" that cannot affect libpq's processing logic.
Discussion: https://postgr.es/m/3185105.1644960083@sss.pgh.pa.us
As currently implemented, failure of a PGEVT_RESULTCREATE callback
causes the PGresult to be converted to an error result. This is
intellectually inconsistent (shouldn't a failing callback likewise
prevent creation of the error result? what about side-effects on the
behavior seen by other event procs? why does PQfireResultCreateEvents
act differently from PQgetResult?), but more importantly it destroys
any promises we might wish to make about the behavior of libpq in
nontrivial operating modes, such as pipeline mode. For example,
it's not possible to promise that PGRES_PIPELINE_SYNC results will
be returned if an event callback fails on those. With this
definition, expecting applications to behave sanely in the face of
possibly-failing callbacks seems like a very big lift.
Hence, redefine the result of a callback failure as being simply
that that event procedure won't be called any more for this PGresult
(which was true already). Event procedures can still signal failure
back to the application through out-of-band mechanisms, for example
via their passthrough arguments.
Similarly, don't let failure of a PGEVT_RESULTCOPY callback prevent
PQcopyResult from succeeding. That definition allowed a misbehaving
event proc to break single-row mode (our sole internal use of
PQcopyResult), and it probably had equally deleterious effects for
outside uses.
Discussion: https://postgr.es/m/3185105.1644960083@sss.pgh.pa.us
The AF_UNIX macro was being used unprotected by HAVE_UNIX_SOCKETS,
apparently since 2008. So the redirection through IS_AF_UNIX() is
apparently no longer necessary. (More generally, all supported
platforms are now HAVE_UNIX_SOCKETS, but even if there were a new
platform in the future, it seems plausible that it would define the
AF_UNIX symbol even without kernel support.) So remove the
IS_AF_UNIX() macro and make the code a bit more consistent.
Discussion: https://www.postgresql.org/message-id/flat/f2d26815-9832-e333-d52d-72fbc0ade896%40enterprisedb.com
Commit 1f39a1c06 implemented write-failure postponement in pqSendSome,
which is above SSL/GSS processing. However, we've now seen failures
indicating that (some versions of?) OpenSSL have a tendency to report
write failures prematurely too. Hence, move the primary responsibility
for postponing write failures down to pqsecure_raw_write(), below
SSL/GSS processing. pqSendSome now sets write_failed only in corner
cases where we'd lost the connection already.
A side-effect of this change is that errors detected in the SSL/GSS
layer itself will be reported immediately (as if they were read
errors) rather than being postponed like write errors. That's
reverting an effect of 1f39a1c06, and I think it's fine: if there's
not a socket-level error, it's hard to be sure whether an OpenSSL
error ought to be considered a read or write failure anyway.
Another important point is that write-failure postponement is now
effective during connection setup. OpenSSL's misbehavior of this
sort occurs during SSL_connect(), so that's a change we want.
Per bug #17391 from Nazir Bilal Yavuz. Possibly this should be
back-patched, but I think it prudent to let it age awhile in HEAD
first.
Discussion: https://postgr.es/m/17391-304f81bcf724b58b@postgresql.org
In commit 1f39a1c06 I made PQisBusy consider conn->write_failed, but
that is now looking like complete brain fade. In the first place, the
logic is quite wrong: it ought to be like "and not" rather than "or".
This meant that once we'd gotten into a write_failed state, PQisBusy
would always return true, probably causing the calling application to
iterate its loop until PQconsumeInput returns a hard failure thanks
to connection loss. That's not what we want: the intended behavior
is to return an error PGresult, which the application probably has
much cleaner support for.
But in the second place, checking write_failed here seems like the
wrong thing anyway. The idea of the write_failed mechanism is to
postpone handling of a write failure until we've read all we can from
the server; so that flag should not interfere with input-processing
behavior. (Compare 7247e243a.) What we *should* check for is
status = CONNECTION_BAD, ie, socket already closed. (Most places that
close the socket don't touch asyncStatus, but they do reset status.)
This primarily ensures that if PQisBusy() returns true then there is
an open socket, which is assumed by several call sites in our own
code, and probably other applications too.
While at it, fix a nearby thinko in libpq's my_sock_write: we should
only consult errno for res < 0, not res == 0. This is harmless since
pqsecure_raw_write would force errno to zero in such a case, but it
still could confuse readers.
Noted by Andres Freund. Backpatch to v12 where 1f39a1c06 came in.
Discussion: https://postgr.es/m/20220211011025.ek7exh6owpzjyudn@alap3.anarazel.de
In libpq and ecpglib, multiple threads can concurrently enter the
initialization logic for message localization. Since we set the
its-done flag before actually doing the work, it'd be possible
for some threads to reach gettext() before anyone has called
bindtextdomain(). Barring bugs in libintl itself, this would not
result in anything worse than failure to localize some early
messages. Nonetheless, it's a bug, and an easy one to fix.
Noted while investigating bug #17299 from Clemens Zeidler
(much thanks to Liam Bowen for followup investigation on that).
It currently appears that that actually *is* a bug in libintl itself,
but that doesn't let us off the hook for this bit.
Back-patch to all supported versions.
Discussion: https://postgr.es/m/17299-7270741958c0b1ab@postgresql.org
Discussion: https://postgr.es/m/CAE7q7Eit4Eq2=bxce=Fm8HAStECjaXUE=WBQc-sDDcgJQ7s7eg@mail.gmail.com
If connectivity to the server has been lost or become flaky, the
user might well try to send a query cancel. It's highly annoying
if PQcancel hangs up in such a case, but that's exactly what's likely
to happen. To ameliorate this problem, apply the PGconn's
tcp_user_timeout and keepalives settings to the TCP connection used
to send the cancel. This should be safe on Unix machines, since POSIX
specifies that setsockopt() is async-signal-safe. We are guessing
that WSAIoctl(SIO_KEEPALIVE_VALS) is similarly safe on Windows.
(Note that at least in psql and our other frontend programs, there's
no safety issue involved anyway, since we run PQcancel in its own
thread rather than in a signal handler.)
Most of the value here comes from the expectation that tcp_user_timeout
will be applied as a connection timeout. That appears to happen on
Linux, even though its tcp(7) man page claims differently. The
keepalive options probably won't help much, but as long as we can
apply them for not much code, we might as well.
Jelte Fennema, reviewed by Fujii Masao and myself
Discussion: https://postgr.es/m/AM5PR83MB017870DE81FC84D5E21E9D1EF7AA9@AM5PR83MB0178.EURPRD83.prod.outlook.com
PQcancel() is supposed to be safe to call from a signal handler,
and indeed psql uses it that way. All of the library functions
it uses are specified to be async-signal-safe by POSIX ...
except for strerror. Neither plain strerror nor strerror_r
are considered safe. When this code was written, back in the
dark ages, we probably figured "oh, strerror will just index
into a constant array of strings" ... but in any locale except C,
that's unlikely to be true. Probably the reason we've not heard
complaints is that (a) this error-handling code is unlikely to be
reached in normal use, and (b) in many scenarios, localized error
strings would already have been loaded, after which maybe it's
safe to call strerror here. Still, this is clearly unacceptable.
The best we can do without relying on strerror is to print the
decimal value of errno, so make it do that instead. (This is
probably not much loss of user-friendliness, given that it is
hard to get a failure here.)
Back-patch to all supported branches.
Discussion: https://postgr.es/m/2937814.1641960929@sss.pgh.pa.us
This is similar to b69aba7, except that this completes the work for
HMAC with a new routine called pg_hmac_error() that would provide more
context about the type of error that happened during a HMAC computation:
- The fallback HMAC implementation in hmac.c relies on cryptohashes, so
in some code paths it is necessary to return back the error generated by
cryptohashes.
- For the OpenSSL implementation (hmac_openssl.c), the logic is very
similar to cryptohash_openssl.c, where the error context comes from
OpenSSL if one of its internal routines failed, with different error
codes if something internal to hmac_openssl.c failed or was incorrect.
Any in-core code paths that use the centralized HMAC interface are
related to SCRAM, for errors that are unlikely going to happen, with
only SHA-256. It would be possible to see errors when computing some
HMACs with MD5 for example and OpenSSL FIPS enabled, and this commit
would help in reporting the correct errors but nothing in core uses
that. So, at the end, no backpatch to v14 is done, at least for now.
Errors in SCRAM related to the computation of the server key, stored
key, etc. need to pass down the potential error context string across
more layers of their respective call stacks for the frontend and the
backend, so each surrounding routine is adapted for this purpose.
Reviewed-by: Sergey Shinderuk
Discussion: https://postgr.es/m/Yd0N9tSAIIkFd+qi@paquier.xyz
The point of this patch is to reduce inclusion spam by not needing
to #include <netdb.h> or <pwd.h> in port.h (which is read by every
compile in our tree). To do that, we must remove port.h's
declarations of pqGetpwuid and pqGethostbyname.
pqGethostbyname is only used, and is only ever likely to be used,
in src/port/getaddrinfo.c --- which isn't even built on most
platforms, making pqGethostbyname dead code for most people.
Hence, deal with that by just moving it into getaddrinfo.c.
To clean up pqGetpwuid, invent a couple of simple wrapper
functions with less-messy APIs. This allows removing some
duplicate error-handling code, too.
In passing, remove thread.c from the MSVC build, since it
contains nothing we use on Windows.
Noted while working on 376ce3e40.
Discussion: https://postgr.es/m/1634252654444.90107@mit.edu
Experimenting with FIPS mode enabled, I saw
regression=# \password joe
Enter new password for user "joe":
Enter it again:
could not encrypt password: disabled for FIPS
out of memory
because PQencryptPasswordConn was still of the opinion that "out of
memory" is always appropriate to print.
Minor oversight in b69aba745. Like that one, back-patch to v14.
The existing cryptohash facility was causing problems in some code paths
related to MD5 (frontend and backend) that relied on the fact that the
only type of error that could happen would be an OOM, as the MD5
implementation used in PostgreSQL ~13 (the in-core implementation is
used when compiling with or without OpenSSL in those older versions),
could fail only under this circumstance.
The new cryptohash facilities can fail for reasons other than OOMs, like
attempting MD5 when FIPS is enabled (upstream OpenSSL allows that up to
1.0.2, Fedora and Photon patch OpenSSL 1.1.1 to allow that), so this
would cause incorrect reports to show up.
This commit extends the cryptohash APIs so as callers of those routines
can fetch more context when an error happens, by using a new routine
called pg_cryptohash_error(). The error states are stored within each
implementation's internal context data, so as it is possible to extend
the logic depending on what's suited for an implementation. The default
implementation requires few error states, but OpenSSL could report
various issues depending on its internal state so more is needed in
cryptohash_openssl.c, and the code is shaped so as we are always able to
grab the necessary information.
The core code is changed to adapt to the new error routine, painting
more "const" across the call stack where the static errors are stored,
particularly in authentication code paths on variables that provide
log details. This way, any future changes would warn if attempting to
free these strings. The MD5 authentication code was also a bit blurry
about the handling of "logdetail" (LOG sent to the postmaster), so
improve the comments related that, while on it.
The origin of the problem is 87ae969, that introduced the centralized
cryptohash facility. Extra changes are done for pgcrypto in v14 for the
non-OpenSSL code path to cope with the improvements done by this
commit.
Reported-by: Michael Mühlbeyer
Author: Michael Paquier
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/89B7F072-5BBE-4C92-903E-D83E865D9367@trivadis.com
Backpatch-through: 14
