pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #15932: Module passwordcheck doesn't reference previous hooks
7+ messages / 3 participants
[nested] [flat]
* BUG #15932: Module passwordcheck doesn't reference previous hooks
@ 2019-07-29 15:56 PG Bug reporting form <noreply@postgresql.org>
2019-07-30 02:24 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 7+ messages in thread
From: PG Bug reporting form @ 2019-07-29 15:56 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: rafaelthca@gmail.com
The following bug has been logged on the website:
Bug reference: 15932
Logged by: Rafael Castro
Email address: rafaelthca@gmail.com
PostgreSQL version: 11.4
Operating system: Any
Description:
Module passwordcheck doesn't reference (thus, doesn't call) previous
hooks:
https://github.com/postgres/postgres/blob/master/contrib/passwordcheck/passwordcheck.c#L133
As a result, if passwordcheck is used hook check_password_hook becomes
useless.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: BUG #15932: Module passwordcheck doesn't reference previous hooks
2019-07-29 15:56 BUG #15932: Module passwordcheck doesn't reference previous hooks PG Bug reporting form <noreply@postgresql.org>
@ 2019-07-30 02:24 ` Michael Paquier <michael@paquier.xyz>
2019-07-31 07:15 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 7+ messages in thread
From: Michael Paquier @ 2019-07-30 02:24 UTC (permalink / raw)
To: rafaelthca@gmail.com; pgsql-bugs@lists.postgresql.org
On Mon, Jul 29, 2019 at 03:56:35PM +0000, PG Bug reporting form wrote:
> Module passwordcheck doesn't reference (thus, doesn't call) previous
> hooks:
>
> https://github.com/postgres/postgres/blob/master/contrib/passwordcheck/passwordcheck.c#L133
>
> As a result, if passwordcheck is used hook check_password_hook becomes
> useless.
So, it would erase the trace of previous hooks if loaded last.
passwordcheck holds value as an example of use for some hooks, and
examples should be a good students, so I agree to do better and fix
that. The attached patch does the work, what do you think about it?
--
Michael
Attachments:
[text/x-diff] passwordcheck-prev-hook-v1.patch (1.1K, ../../20190730022420.GC1742@paquier.xyz/2-passwordcheck-prev-hook-v1.patch)
download | inline diff:
diff --git a/contrib/passwordcheck/passwordcheck.c b/contrib/passwordcheck/passwordcheck.c
index b161e0f146..0cc51d1489 100644
--- a/contrib/passwordcheck/passwordcheck.c
+++ b/contrib/passwordcheck/passwordcheck.c
@@ -26,10 +26,14 @@
PG_MODULE_MAGIC;
+/* Saved hook value in case of unload */
+static check_password_hook_type prev_check_password_hook = NULL;
+
/* passwords shorter than this will be rejected */
#define MIN_PWD_LENGTH 8
extern void _PG_init(void);
+extern void _PG_fini(void);
/*
* check_password
@@ -55,6 +59,11 @@ check_password(const char *username,
Datum validuntil_time,
bool validuntil_null)
{
+ if (prev_check_password_hook)
+ prev_check_password_hook(username, shadow_pass,
+ password_type, validuntil_time,
+ validuntil_null);
+
if (password_type != PASSWORD_TYPE_PLAINTEXT)
{
/*
@@ -133,5 +142,15 @@ void
_PG_init(void)
{
/* activate password checks when the module is loaded */
+ prev_check_password_hook = check_password_hook;
check_password_hook = check_password;
}
+
+/*
+ * Module unload function
+ */
+void
+_PG_fini(void)
+{
+ check_password_hook = prev_check_password_hook;
+}
[application/pgp-signature] signature.asc (832B, ../../20190730022420.GC1742@paquier.xyz/3-signature.asc)
download
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: BUG #15932: Module passwordcheck doesn't reference previous hooks
2019-07-29 15:56 BUG #15932: Module passwordcheck doesn't reference previous hooks PG Bug reporting form <noreply@postgresql.org>
2019-07-30 02:24 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Michael Paquier <michael@paquier.xyz>
@ 2019-07-31 07:15 ` Michael Paquier <michael@paquier.xyz>
2019-07-31 07:23 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 7+ messages in thread
From: Michael Paquier @ 2019-07-31 07:15 UTC (permalink / raw)
To: rafaelthca@gmail.com; pgsql-bugs@lists.postgresql.org
On Tue, Jul 30, 2019 at 11:24:20AM +0900, Michael Paquier wrote:
> So, it would erase the trace of previous hooks if loaded last.
> passwordcheck holds value as an example of use for some hooks, and
> examples should be a good students, so I agree to do better and fix
> that. The attached patch does the work, what do you think about it?
Are there any objections about that stuff? I would not touch anything
else than HEAD for that issue.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20190731071503.GA29334@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: BUG #15932: Module passwordcheck doesn't reference previous hooks
2019-07-29 15:56 BUG #15932: Module passwordcheck doesn't reference previous hooks PG Bug reporting form <noreply@postgresql.org>
2019-07-30 02:24 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Michael Paquier <michael@paquier.xyz>
2019-07-31 07:15 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Michael Paquier <michael@paquier.xyz>
@ 2019-07-31 07:23 ` Daniel Gustafsson <daniel@yesql.se>
2019-07-31 07:34 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Gustafsson @ 2019-07-31 07:23 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: rafaelthca@gmail.com; pgsql-bugs@lists.postgresql.org
> On 31 Jul 2019, at 09:15, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Jul 30, 2019 at 11:24:20AM +0900, Michael Paquier wrote:
>> So, it would erase the trace of previous hooks if loaded last.
>> passwordcheck holds value as an example of use for some hooks, and
>> examples should be a good students, so I agree to do better and fix
>> that. The attached patch does the work, what do you think about it?
>
> Are there any objections about that stuff? I would not touch anything
> else than HEAD for that issue.
LGTM. A small nitpick is that the below comment isn’t really giving the full
picture, prev_check_password_hook is not just used for unload but for the hook
chaining.
+/* Saved hook value in case of unload */
cheers ./daniel
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: BUG #15932: Module passwordcheck doesn't reference previous hooks
2019-07-29 15:56 BUG #15932: Module passwordcheck doesn't reference previous hooks PG Bug reporting form <noreply@postgresql.org>
2019-07-30 02:24 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Michael Paquier <michael@paquier.xyz>
2019-07-31 07:15 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Michael Paquier <michael@paquier.xyz>
2019-07-31 07:23 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Daniel Gustafsson <daniel@yesql.se>
@ 2019-07-31 07:34 ` Michael Paquier <michael@paquier.xyz>
2019-07-31 07:54 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 7+ messages in thread
From: Michael Paquier @ 2019-07-31 07:34 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: rafaelthca@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Jul 31, 2019 at 09:23:54AM +0200, Daniel Gustafsson wrote:
> LGTM. A small nitpick is that the below comment isn’t really giving the full
> picture, prev_check_password_hook is not just used for unload but for the hook
> chaining.
My understanding is that it's the same meaning, and also:
$ git grep "in case of unload"
contrib/auto_explain/auto_explain.c:/* Saved hook values in case of
unload */
contrib/pg_stat_statements/pg_stat_statements.c:/* Saved hook values
in case of unload */
src/test/modules/test_rls_hooks/test_rls_hooks.c:/* Saved hook values
in case of unload */
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20190731073401.GB29334@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: BUG #15932: Module passwordcheck doesn't reference previous hooks
2019-07-29 15:56 BUG #15932: Module passwordcheck doesn't reference previous hooks PG Bug reporting form <noreply@postgresql.org>
2019-07-30 02:24 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Michael Paquier <michael@paquier.xyz>
2019-07-31 07:15 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Michael Paquier <michael@paquier.xyz>
2019-07-31 07:23 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Daniel Gustafsson <daniel@yesql.se>
2019-07-31 07:34 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Michael Paquier <michael@paquier.xyz>
@ 2019-07-31 07:54 ` Daniel Gustafsson <daniel@yesql.se>
2019-08-01 00:42 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Gustafsson @ 2019-07-31 07:54 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: rafaelthca@gmail.com; pgsql-bugs@lists.postgresql.org
> On 31 Jul 2019, at 09:34, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Jul 31, 2019 at 09:23:54AM +0200, Daniel Gustafsson wrote:
>> LGTM. A small nitpick is that the below comment isn’t really giving the full
>> picture, prev_check_password_hook is not just used for unload but for the hook
>> chaining.
>
> My understanding is that it's the same meaning, and also:
> $ git grep "in case of unload"
> contrib/auto_explain/auto_explain.c:/* Saved hook values in case of
> unload */
> contrib/pg_stat_statements/pg_stat_statements.c:/* Saved hook values
> in case of unload */
> src/test/modules/test_rls_hooks/test_rls_hooks.c:/* Saved hook values
> in case of unload */
Hmm, ok, IMO my comment applies to those as well, but it may very well be me
just reading it wrong. Either way, +1 for pushing your patch.
cheers ./daniel
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: BUG #15932: Module passwordcheck doesn't reference previous hooks
2019-07-29 15:56 BUG #15932: Module passwordcheck doesn't reference previous hooks PG Bug reporting form <noreply@postgresql.org>
2019-07-30 02:24 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Michael Paquier <michael@paquier.xyz>
2019-07-31 07:15 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Michael Paquier <michael@paquier.xyz>
2019-07-31 07:23 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Daniel Gustafsson <daniel@yesql.se>
2019-07-31 07:34 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Michael Paquier <michael@paquier.xyz>
2019-07-31 07:54 ` Re: BUG #15932: Module passwordcheck doesn't reference previous hooks Daniel Gustafsson <daniel@yesql.se>
@ 2019-08-01 00:42 ` Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 7+ messages in thread
From: Michael Paquier @ 2019-08-01 00:42 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: rafaelthca@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Jul 31, 2019 at 09:54:17AM +0200, Daniel Gustafsson wrote:
> Hmm, ok, IMO my comment applies to those as well, but it may very well be me
> just reading it wrong. Either way, +1 for pushing your patch.
I am not saying that this cannot be improved either, still I am not
sure how it could be reworded. And if we do that, it should be
applied to all places where we use it. I have fixed the issue and
back-patched down to 9.4. Perhaps that was not worth caring about,
but the versions for the back-branches were not really an issue.
Thanks for the report, Rafael. And thanks for the review, Daniel.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20190801004211.GD29334@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2019-08-01 00:42 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-07-29 15:56 BUG #15932: Module passwordcheck doesn't reference previous hooks PG Bug reporting form <noreply@postgresql.org>
2019-07-30 02:24 ` Michael Paquier <michael@paquier.xyz>
2019-07-31 07:15 ` Michael Paquier <michael@paquier.xyz>
2019-07-31 07:23 ` Daniel Gustafsson <daniel@yesql.se>
2019-07-31 07:34 ` Michael Paquier <michael@paquier.xyz>
2019-07-31 07:54 ` Daniel Gustafsson <daniel@yesql.se>
2019-08-01 00:42 ` Michael Paquier <michael@paquier.xyz>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox