Re: [HACKERS] Log operating system user connecting via unix socket
Hi again.
About the privileges, our support can create roles / databases, drop
existing databases, dump /restore, change other users passwords. It's not
feasible right now create a 1:1 map of system users and postgres users.
Maybe in the future.
I wrote 2 possible patches, both issuing a detail message only if
log_connections is enabled.
The first one using the Stephen Frost suggestion, inside the Port struct (I
guess that this is the one, I coudn't find the Peer struct)
The second one following the same approach of cf commit 5e0b5dcab, as
pointed by Tom Lane.
Again, feel free to comment and criticize.
On Sun, Jan 17, 2016 at 3:07 PM, Stephen Frost wrote:
> Tom,
>
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
> > Stephen Frost writes:
> > > What I think we really want here is logging of the general 'system
> > > user' for all auth methods instead of only for the 'peer' method.
> >
> > Well, we don't really know that except in a small subset of auth
> > methods. I agree that when we do know it, it's useful info to log.
>
> Right.
>
> > My big beef with the proposed patch is that the log message is emitted
> > unconditionally. There are lots and lots of users who feel that during
> > normal operation, *zero* log messages should get emitted. Those
> villagers
> > would be on our doorsteps with pitchforks if we shipped this patch as-is.
>
> Agreed.
>
> > I would propose that this information should be emitted only when
> > log_connections is enabled, and indeed that it should be part of the
> > log_connections message not a separate message. So this leads to
> > thinking that somehow, the code for individual auth methods should
> > be able to return an "additional info" field for inclusion in
> > log_connections. We already have such a concept for auth failures,
> > cf commit 5e0b5dcab.
>
> Apologies if it wasn't clear, but that's exactly what I was suggesting
> by saying to add it to PerformAuthentication, which is where we emit
> the connection info when log_connections is enabled.
>
> > > ... and also make it available in pg_stat_activity.
> >
> > That's moving the goalposts quite a bit, and I'm not sure it's necessary
> > or even desirable. Let's just get this added to log_connections output,
> > and then see if there's field demand for more.
>
> This was in context of peer_cn, which is just a specific "system user"
> value and which we're already showing in pg_stat_* info tables. I'd
> love to have the Kerberos principal available, but I don't think it'd
> make sense to have a 'pg_stat_kerberos' just for that.
>
> I agree that it's moving the goalposts for this patch and could be an
> independent patch, but I don't see it as any different, from a
> desirability and requirements perspective, than what we're doing for SSL
> connections.
>
> Thanks!
>
> Stephen
>
--
José Arthur Benetasso Villanova
commit 76594784c50bca1b09f687e58f17ff27230076be
Author: Jose Arthur Benetasso Villanova
Date: Tue Jan 19 11:50:22 2016 -0200
Log message
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 57c2f48..ac1c785 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -991,6 +991,7 @@ pg_GSS_recvauth(Port *port)
return STATUS_ERROR;
}
+ port->system_user = pstrdup(gbuf.value);
ret = check_usermap(port->hba->usermap, port->user_name, gbuf.value,
pg_krb_caseins_users);
@@ -1291,6 +1292,7 @@ pg_SSPI_recvauth(Port *port)
int retval;
namebuf = psprintf("%s@%s", accountname, domainname);
+ port->system_user = pstrdup(namebuf);
retval = check_usermap(port->hba->usermap, port->user_name, namebuf, true);
pfree(namebuf);
return retval;
@@ -1561,8 +1563,11 @@ ident_inet_done:
pg_freeaddrinfo_all(local_addr.addr.ss_family, la);
if (ident_return)
+ {
/* Success! Check the usermap */
+ port->system_user = pstrdup(ident_user);
return check_usermap(port->hba->usermap, port->user_name, ident_user, false);
+ }
return STATUS_ERROR;
}
@@ -1609,6 +1614,8 @@ auth_peer(hbaPort *port)
}
strlcpy(ident_user, pw->pw_name, IDENT_USERNAME_MAX + 1);
+ port->system_user = pstrdup(ident_user);
+
return check_usermap(port->hba->usermap, port->user_name, ident_user, false);
}
@@ -2124,6 +2131,7 @@ CheckLDAPAuth(Port *port)
return STATUS_ERROR;
}
+ port->system_user = pstrdup(fulluser);
pfree(fulluser);
return STATUS_OK;
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index e22d4db..f425808 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/b
[HACKERS] Log operating system user connecting via unix socket
Greetings, gentlemen.
Here in my work, we have about 100 PostgreSQL machines and about 20 users
with superuser privileges.
This group of 20 people change constantly, so it's cumbersome create a role
for each. Instead, we map all of then in pg_ident.conf.
The problem is: with current postgres log, I just know that a postgres user
connect, but I don't know which one is in case that more than one user is
logged in the server.
This simple log line can create the relations needed for an audit.
Feel free to comment and criticize.
--
José Arthur Benetasso Villanova
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 57c2f48..db111e0 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -1610,6 +1610,9 @@ auth_peer(hbaPort *port)
strlcpy(ident_user, pw->pw_name, IDENT_USERNAME_MAX + 1);
+ ereport(LOG,
+ (errmsg("Received a unix socket connection from %s", ident_user)));
+
return check_usermap(port->hba->usermap, port->user_name, ident_user, false);
}
#endif /* HAVE_UNIX_SOCKETS */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [patch] Include detailed information about a row failing a CHECK constraint into the error message
> Hi,
> when I insert/update many rows at once using INSERT ... SELECT into a
> table which has plenty of CHECK constraints, the error message that
> Postgres returns has no indication of which row failed the constraint
> check. The attached patch tries to provide information in a similar way
> to how duplicate items in a UNIQUE constraint are handled.
> Originally, I tried to simply check the new row's t_ctid, but it was
> always (0,0) -- I guess that's expected, maybe it's still in memory at
> that time and maybe such nodes don't have a ctid assigned yet.
> Please let me know if this patch is suitable for inclusion. It's based
> on REL9_0_STABLE, because that's the version I'm running.
> I'd like to thank intgr on IRC for his feedback when I was wondering
> about the t_ctid.
> With kind regards,
> Jan
Hi Jan / all.
I'm looking for a simple patch to review and this one doesn't look too
complicate.
The patch seens to be useful, it adds a better feedback.
First, I couldn't apply it as in the email, even in REL9_0_STABLE: the
offset doesn't look right. Which commit are your repository in?
Anyway, I could copy / paste it at the correct place, using the
current master. I could compile it, put a postgres with it running and
it's working:
postgres=# create table test1(id serial primary key, value text);
NOTICE: CREATE TABLE will create implicit sequence "test1_id_seq" for
serial column "test1.id"
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index
"test1_pkey" for table "test1"
CREATE TABLE
postgres=# ALTER TABLE test1 ADD CONSTRAINT must_be_unique unique (value);
NOTICE: ALTER TABLE / ADD UNIQUE will create implicit index
"must_be_unique" for table "test1"
ALTER TABLE
postgres=# insert into test1 values (default, 'Hello World');
INSERT 0 1
postgres=# insert into test1 values (default, 'Hello World');
ERROR: duplicate key value violates unique constraint "must_be_unique"
DETAIL: Key (value)=(Hello World) already exists.
The patch I've used:
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index fd7a9ed..57894cf 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1574,10 +1574,32 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
const char *failed;
if ((failed = ExecRelCheck(resultRelInfo, slot, estate)) !=
NULL)
+ {
+ StringInfoData buf;
+ int natts = rel->rd_att->natts;
+ int i;
+ initStringInfo(&buf);
+ for (i = 0; i < natts; ++i)
+ {
+ char *val;
+ Oid foutoid;
+ bool typisvarlena;
+
getTypeOutputInfo(rel->rd_att->attrs[i]->atttypid, &foutoid,
&typisvarlena);
+ if (slot->tts_isnull[i])
+ val = "NULL";
+ else
+ val = OidOutputFunctionCall(foutoid,
slot->tts_values[i]);
+ if (i > 0)
+ appendStringInfoString(&buf, ", ");
+ appendStringInfoString(&buf, val);
+ }
ereport(ERROR,
(errcode(ERRCODE_CHECK_VIOLATION),
errmsg("new row for relation \"%s\"
violates check constraint \"%s\"",
-
RelationGetRelationName(rel), failed)));
+ RelationGetRelationName(rel),
failed),
+ errdetail("New row with data (%s)
violates check constraint \"%s\".",
+ buf.data, failed)));
+ }
}
}
--
José Arthur Benetasso Villanova
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] directory archive format for pg_dump
Hi Dimitri and Joachim. I've looked the patch too, and I want to share some thoughts too. I've used http://wiki.postgresql.org/wiki/Reviewing_a_Patch to guide my review. Submission review: I've apllied and compiled the patch successfully using the current master. Usability review: The dir format generated in my database 60 files, with different sizes, and it looks very confusing. Is it possible to use the same trick as pigz and pbzip2, creating a concatenated file of streams? Feature test: Just a partial review. I can dump / restore using lzf, but didnt stress it hard to check robustness. Performance review: Didnt test it hard too, but looks ok. Coding review: Just a shallow review here. >> I think I'd like to see a separate patch for the new compression >> support. Sorry about that, I realize that's extra work… Same feeling here, this is the 1st thing that I notice. The md5.c and kwlookup.c reuse using a link doesn't look nice either. This way you need to compile twice, among others things, but I think that its temporary, right? -- José Arthur Benetasso Villanova -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Patch: Extend NOT NULL representation to pg_constraint
Hi all. My name is Jose Arthur and I use PostgreSQL for a while, but never contributed to the main project, until now. Since nobody else take this patch to review in this commitfest, I'm going to try :-). The main problem can be found here: http://archives.postgresql.org/pgsql-hackers/2009-11/msg00146.php How to simulate: CREATE TABLE foo(id integer NOT NULL, val text NOT NULL); CREATE TABLE foo2(another_id integer NOT NULL) INHERITS(foo); ALTER TABLE foo2 ALTER COLUMN val DROP NOT NULL; insert into foo2 values (1, null, 1); Then pg_dump > dump.pgsql and psql -f dump.pgsql I've applied the patch submitted here: http://archives.postgresql.org/pgsql-hackers/2010-09/msg00763.php against current master (635de8365f0299cfa2db24c56abcfccb65d020f0) and compile is fine Using the patch, I can't drop NOT NULL from foo2, just from foo, and I think that makes sense: postgres=# ALTER TABLE foo2 ALTER COLUMN val DROP NOT NULL; ERROR: cannot drop inherited NOT NULL constraint "foo2_val_not_null", relation "foo2" One thing that I take notice is when you create a simple table like this one: select count(*) from pg_constraint ; 12 rows appears in pg_constraint, 10 to the sequence. Is that ok? Other thing: #define CONSTRAINT_NOTNULL 'n' in src/include/catalog/pg_constraint.h is using spaces instead of tabs :-) -- José Arthur Benetasso Villanova
