Thanks for the updated version, it's really starting to take good shape. A few
small comments on v37 from a a first quick skim-through:
+ if (!strcmp(key, AUTH_KEY))
+ if (*expected_bearer && !strcmp(token, expected_bearer))
Nitpickery but these should be (strcmp(xxx) == 0) to match project style.
(ironically, the only !strcmp in the code was my mistake in ebc8b7d4416).
+ foreach(l, elemlist)
This one seems like a good candidate for a foreach_ptr construction.
+ *output = strdup(kvsep);
There is no check to ensure strdup worked AFAICT, and even though it's quite
unlikely to fail we definitely don't want to continue if it did.
fail_validator.c seems to have the #include list copied from validator.c and
pulls in unnecessarily many headers.
+ client's dummy reponse, and issues a FATAL error to end the exchange.
s/reponse/response/
In validate() I wonder if we should doublecheck that have a a proper set of
validator callbacks loaded just to make even more sure that we don't introduce
anything terrible in this codepath.
I will keep reviewing this version to try and provide more feedback.
--
Daniel Gustafsson
diff --git a/src/backend/libpq/auth-oauth.c b/src/backend/libpq/auth-oauth.c
index eea5032de8..2bb84b7bc8 100644
--- a/src/backend/libpq/auth-oauth.c
+++ b/src/backend/libpq/auth-oauth.c
@@ -424,7 +424,7 @@ parse_kvpairs_for_auth(char **input)
value = sep + 1;
validate_kvpair(key, value);
- if (!strcmp(key, AUTH_KEY))
+ if (strcmp(key, AUTH_KEY) == 0)
{
if (auth)
ereport(ERROR,
@@ -619,6 +619,15 @@ validate(Port *port, const char *auth)
if (!(token = validate_token_format(auth)))
return false;
+ /*
+ * Ensure that we have a validation library loaded, this should always
be
+ * the case and an error here is indicative of a bug.
+ */
+ if (!ValidatorCallbacks || !ValidatorCallbacks->validate_cb)
+ ereport(FATAL,
+ errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg("validation of OAuth token requested
without a validator loaded"));
+
/* Call the validation function from the validator module */
ret = ValidatorCallbacks->validate_cb(validator_module_state,
token, port->user_name);
@@ -708,6 +717,7 @@ load_validator_library(const char *libname)
"OAuth validator", libname,
"_PG_oauth_validator_module_init"));
ValidatorCallbacks = (*validator_init) ();
+ Assert(ValidatorCallbacks);
/* Allocate memory for validator library private state data */
validator_module_state = (ValidatorModuleState *)
palloc0(sizeof(ValidatorModuleState));
@@ -738,7 +748,6 @@ check_oauth_validator(HbaLine *hbaline, int elevel, char
**err_msg)
char *file_name = hbaline->sourcefile;
char *rawstring;
List *elemlist = NIL;
- ListCell *l;
*err_msg = NULL;
@@ -787,10 +796,8 @@ check_oauth_validator(HbaLine *hbaline, int elevel, char
**err_msg)
goto done;
}
- foreach(l, elemlist)
+ foreach_ptr(char, allowed, elemlist)
{
- char *allowed = lfirst(l);
-
if (strcmp(allowed, hbaline->oauth_validator) == 0)
goto done;
}
diff --git a/src/interfaces/libpq/fe-auth-oauth.c
b/src/interfaces/libpq/fe-auth-oauth.c
index 0d4185194d..5400e6df7a 100644
--- a/src/interfaces/libpq/fe-auth-oauth.c
+++ b/src/interfaces/libpq/fe-auth-oauth.c
@@ -841,6 +841,11 @@ oauth_exchange(void *opaq, bool final,
* Respond with the required dummy message (RFC 7628,
sec. 3.2.3).
*/
*output = strdup(kvsep);
+ if (unlikely(!*output))
+ {
+ libpq_append_conn_error(conn, "out of memory");
+ return SASL_FAILED;
+ }
*outputlen = strlen(*output); /* == 1 */
state->state = FE_OAUTH_SERVER_ERROR;
diff --git a/src/test/modules/oauth_validator/fail_validator.c
b/src/test/modules/oauth_validator/fail_validator.c
index b0fcc07c2a..c438ed4d17 100644
--- a/src/test/modules/oauth_validator/fail_validator.c
+++ b/src/test/modules/oauth_validator/fail_validator.c
@@ -16,9 +16,6 @@
#include "fmgr.h"
#include "libpq/oauth.h"
-#include "miscadmin.h"
-#include "utils/guc.h"
-#include "utils/memutils.h"
PG_MODULE_MAGIC;
diff --git a/src/test/python/client/test_oauth.py
b/src/test/python/client/test_oauth.py
index 60e57dba86..ce8e0f12c9 100644
--- a/src/test/python/client/test_oauth.py
+++ b/src/test/python/client/test_oauth.py
@@ -108,7 +108,7 @@ def get_auth_value(initial):
def fail_oauth_handshake(conn, sasl_resp, *, errmsg="doesn't matter"):
"""
Sends a failure response via the OAUTHBEARER mechanism, consumes the
- client's dummy reponse, and issues a FATAL error to end the exchange.
+ client's dummy response, and issues a FATAL error to end the exchange.
sasl_resp is a dictionary which will be serialized as the OAUTHBEARER JSON
response. If provided, errmsg is used in the FATAL ErrorResponse.
diff --git a/src/test/python/server/oauthtest.c
b/src/test/python/server/oauthtest.c
index ee39c2a14e..415748b9a6 100644
--- a/src/test/python/server/oauthtest.c
+++ b/src/test/python/server/oauthtest.c
@@ -108,7 +108,7 @@ test_validate(ValidatorModuleState *state, const char
*token, const char *role)
}
else
{
- if (*expected_bearer && !strcmp(token, expected_bearer))
+ if (*expected_bearer && strcmp(token, expected_bearer) == 0)
res->authorized = true;
if (set_authn_id)
res->authn_id = authn_id;
