SYSTEM_USER reserved word implementation
Hi hackers,
The SYSTEM_USER is a sql reserved word as mentioned in [1]https://www.postgresql.org/docs/current/sql-keywords-appendix.html and is
currently not implemented.
Please find attached a patch proposal to make use of the SYSTEM_USER so
that it returns the authenticated identity (if any) (aka authn_id in the
Port struct).
Indeed in some circumstances, the authenticated identity is not the
SESSION_USER and then the information is lost from the connection point
of view (it could still be retrieved thanks to commit 9afffcb833 and
log_connections set to on).
_Example 1, using the gss authentification._
Say we have this entry in pg_hba.conf:
host all all 0.0.0.0/0 gss map=mygssmap
and the related mapping in pg_ident.conf
mygssmap /^(.*@.*)\.LOCAL$ mary
Then, connecting with a valid Kerberos Ticket that contains
“bertrand@BDTFOREST.LOCAL” as the default principal that way: psql -U
mary -h myhostname -d postgres,
we will get:
postgres=> select current_user, session_user;
current_user | session_user
--------------+--------------
mary | mary
(1 row)
While the SYSTEM_USER would produce the Kerberos principal:
postgres=> select system_user;
system_user
--------------------------
bertrand@BDTFOREST.LOCAL
(1 row)
_Example 2, using the peer authentification._
Say we have this entry in pg_hba.conf:
local all john peer map=mypeermap
and the related mapping in pg_ident.conf
mypeermap postgres john
Then connected localy as the system user postgres and connecting to the
database that way: psql -U john -d postgres, we will get:
postgres=> select current_user, session_user;
current_user | session_user
--------------+--------------
john | john
(1 row)
While the SYSTEM_USER would produce the system user that requested the
connection:
postgres=> select system_user;
system_user
-------------
postgres
(1 row)
Thanks to those examples we have seen some situations where the
information related to the authenticated identity has been lost from the
connection point of view (means not visible in the current_session or in
the session_user).
The purpose of this patch is to make it visible through the SYSTEM_USER
sql reserved word.
_Remarks: _
- In case port->authn_id is NULL then the patch is returning the
SESSION_USER for the SYSTEM_USER. Perhaps it should return NULL instead.
- There is another thread [2]/messages/by-id/793d990837ae5c06a558d58d62de9378ab525d83.camel@vmware.com to expose port->authn_id to extensions and
triggers thanks to a new API. This thread [2]/messages/by-id/793d990837ae5c06a558d58d62de9378ab525d83.camel@vmware.com leads to discussions about
providing this information to the parallel workers too. While the new
MyClientConnectionInfo being discussed in [2]/messages/by-id/793d990837ae5c06a558d58d62de9378ab525d83.camel@vmware.com could be useful to hold
the client information that needs to be shared between the backend and
any parallel workers, it does not seem to be needed in the case
port->authn_id is exposed through SYSTEM_USER (like it is not for
CURRENT_USER and SESSION_USER).
I will add this patch to the next commitfest.
I look forward to your feedback.
Bertrand
[1]: https://www.postgresql.org/docs/current/sql-keywords-appendix.html
[2]: /messages/by-id/793d990837ae5c06a558d58d62de9378ab525d83.camel@vmware.com
/messages/by-id/793d990837ae5c06a558d58d62de9378ab525d83.camel@vmware.com
Attachments:
v1-0001-system_user-implementation.patchtext/plain; charset=UTF-8; name=v1-0001-system_user-implementation.patchDownload+87-4
"Drouvot, Bertrand" <bdrouvot@amazon.com> writes:
Please find attached a patch proposal to make use of the SYSTEM_USER so
that it returns the authenticated identity (if any) (aka authn_id in the
Port struct).
On what grounds do you argue that that's the appropriate meaning of
SYSTEM_USER?
regards, tom lane
On 6/22/22 09:49, Tom Lane wrote:
"Drouvot, Bertrand" <bdrouvot@amazon.com> writes:
Please find attached a patch proposal to make use of the SYSTEM_USER so
that it returns the authenticated identity (if any) (aka authn_id in the
Port struct).On what grounds do you argue that that's the appropriate meaning of
SYSTEM_USER?
What else do you imagine it might mean?
Here is SQL Server interpretation for example:
https://docs.microsoft.com/en-us/sql/t-sql/functions/system-user-transact-sql?view=sql-server-ver16
And Oracle:
http://luna-ext.di.fc.ul.pt/oracle11g/timesten.112/e13070/ttsql257.htm#i1120532
"SYSTEM_USER
Returns the name of the current data store user
as identified by the operating system."
Seems equivalent.
--
Joe Conway
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Joe Conway <mail@joeconway.com> writes:
On 6/22/22 09:49, Tom Lane wrote:
On what grounds do you argue that that's the appropriate meaning of
SYSTEM_USER?
What else do you imagine it might mean?
I was hoping for some citation of the SQL spec.
Here is SQL Server interpretation for example:
"SYSTEM_USER
Returns the name of the current data store user
as identified by the operating system."
Meh. That's as clear as mud. (a) A big part of the question here
is what is meant by "current" user, in the face of operations like
SET ROLE. (b) "as identified by the operating system" does more to
confuse me than anything else. The operating system only deals in
OS user names; does that wording mean that what you get back is an OS
user name rather than a SQL role name?
My immediate guess would be that the SQL committee only intends
to deal in SQL role names and therefore SYSTEM_USER is defined
to return one of those, but I've not gone looking in the spec
to be sure.
I'm also not that clear on what we expect authn_id to be, but
a quick troll in the code makes it look like it's not necessarily
a SQL role name, but might be some external identifier such as a
Kerberos principal. If that's the case I think it's going to be
inappropriate to use SQL-spec syntax to return it. I don't object
to inventing some PG-specific function for the purpose, though.
BTW, are there any security concerns about exposing such identifiers?
regards, tom lane
On 6/22/22 10:51, Tom Lane wrote:
My immediate guess would be that the SQL committee only intends
to deal in SQL role names and therefore SYSTEM_USER is defined
to return one of those, but I've not gone looking in the spec
to be sure.
I only have a draft copy, but in SQL 2016 I find relatively thin
documentation for what SYSTEM_USER is supposed to represent:
The value specified by SYSTEM_USER is equal to an
implementation-defined string that represents the
operating system user who executed the SQL-client
module that contains the externally-invoked procedure
whose execution caused the SYSTEM_USER <general value
specification> to be evaluated.
I'm also not that clear on what we expect authn_id to be, but
a quick troll in the code makes it look like it's not necessarily
a SQL role name, but might be some external identifier such as a
Kerberos principal. If that's the case I think it's going to be
inappropriate to use SQL-spec syntax to return it. I don't object
to inventing some PG-specific function for the purpose, though.
To me the Kerberos principal makes perfect sense given the definition above.
BTW, are there any security concerns about exposing such identifiers?
On the contrary, I would argue that not having the identifier for the
external "user" available is a security concern. Ideally you want to be
able to trace actions inside Postgres to the actual user that invoked them.
--
Joe Conway
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Joe Conway <mail@joeconway.com> writes:
On 6/22/22 10:51, Tom Lane wrote:
My immediate guess would be that the SQL committee only intends
to deal in SQL role names and therefore SYSTEM_USER is defined
to return one of those, but I've not gone looking in the spec
to be sure.
I only have a draft copy, but in SQL 2016 I find relatively thin
documentation for what SYSTEM_USER is supposed to represent:
The value specified by SYSTEM_USER is equal to an
implementation-defined string that represents the
operating system user who executed the SQL-client
module that contains the externally-invoked procedure
whose execution caused the SYSTEM_USER <general value
specification> to be evaluated.
Huh. Okay, if it's implementation-defined then we can define it
as "whatever auth.c put into authn_id". Objection withdrawn.
regards, tom lane
On Wed, Jun 22, 2022 at 8:10 AM Joe Conway <mail@joeconway.com> wrote:
On the contrary, I would argue that not having the identifier for the
external "user" available is a security concern. Ideally you want to be
able to trace actions inside Postgres to the actual user that invoked them.
If auditing is also the use case for SYSTEM_USER, you'll probably want
to review the arguments for making it available to parallel workers
that were made in the other thread [1]/messages/by-id/793d990837ae5c06a558d58d62de9378ab525d83.camel@vmware.com.
Initial comments on the patch:
In case port->authn_id is NULL then the patch is returning the SESSION_USER for the SYSTEM_USER. Perhaps it should return NULL instead.
If the spec says that SYSTEM_USER "represents the operating system
user", but we don't actually know who that user was (authn_id is
NULL), then I think SYSTEM_USER should also be NULL so as not to
mislead auditors.
--- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -473,6 +473,7 @@ static Oid AuthenticatedUserId = InvalidOid; static Oid SessionUserId = InvalidOid; static Oid OuterUserId = InvalidOid; static Oid CurrentUserId = InvalidOid; +static const char *SystemUser = NULL;/* We also have to remember the superuser state of some of these levels */
static bool AuthenticatedUserIsSuperuser = false;
What's the rationale for introducing a new global for this? A downside
is that now there are two sources of truth, for a security-critical
attribute of the connection.
--Jacob
[1]: /messages/by-id/793d990837ae5c06a558d58d62de9378ab525d83.camel@vmware.com
Jacob Champion <jchampion@timescale.com> writes:
On Wed, Jun 22, 2022 at 8:10 AM Joe Conway <mail@joeconway.com> wrote:
In case port->authn_id is NULL then the patch is returning the SESSION_USER for the SYSTEM_USER. Perhaps it should return NULL instead.
If the spec says that SYSTEM_USER "represents the operating system
user", but we don't actually know who that user was (authn_id is
NULL), then I think SYSTEM_USER should also be NULL so as not to
mislead auditors.
Yeah, that seems like a fundamental type mismatch. If we don't know
the OS user identifier, substituting a SQL role name is surely not
the right thing.
I think a case could be made for ONLY returning non-null when authn_id
represents some externally-verified identifier (OS user ID gotten via
peer identification, Kerberos principal, etc).
regards, tom lane
On 6/22/22 11:52, Tom Lane wrote:
Jacob Champion <jchampion@timescale.com> writes:
On Wed, Jun 22, 2022 at 8:10 AM Joe Conway <mail@joeconway.com> wrote:
In case port->authn_id is NULL then the patch is returning the SESSION_USER for the SYSTEM_USER. Perhaps it should return NULL instead.
If the spec says that SYSTEM_USER "represents the operating system
user", but we don't actually know who that user was (authn_id is
NULL), then I think SYSTEM_USER should also be NULL so as not to
mislead auditors.Yeah, that seems like a fundamental type mismatch. If we don't know
the OS user identifier, substituting a SQL role name is surely not
the right thing.
+1 agreed
I think a case could be made for ONLY returning non-null when authn_id
represents some externally-verified identifier (OS user ID gotten via
peer identification, Kerberos principal, etc).
But -1 on that.
I think any time we have a non-null authn_id we should expose it. Are
there examples of cases when we have authn_id but for some reason don't
trust the value of it?
--
Joe Conway
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On 6/22/22 11:35, Jacob Champion wrote:
On Wed, Jun 22, 2022 at 8:10 AM Joe Conway <mail@joeconway.com> wrote:
--- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -473,6 +473,7 @@ static Oid AuthenticatedUserId = InvalidOid; static Oid SessionUserId = InvalidOid; static Oid OuterUserId = InvalidOid; static Oid CurrentUserId = InvalidOid; +static const char *SystemUser = NULL;/* We also have to remember the superuser state of some of these levels */
static bool AuthenticatedUserIsSuperuser = false;What's the rationale for introducing a new global for this? A downside
is that now there are two sources of truth, for a security-critical
attribute of the connection.
Why would you want to do it differently than
SessionUserId/OuterUserId/CurrentUserId? It is analogous, no?
--
Joe Conway
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Joe Conway <mail@joeconway.com> writes:
On 6/22/22 11:52, Tom Lane wrote:
I think a case could be made for ONLY returning non-null when authn_id
represents some externally-verified identifier (OS user ID gotten via
peer identification, Kerberos principal, etc).
But -1 on that.
I think any time we have a non-null authn_id we should expose it. Are
there examples of cases when we have authn_id but for some reason don't
trust the value of it?
I'm more concerned about whether we have a consistent story about what
SYSTEM_USER means (another way of saying "what type is it"). If it's
just the same as SESSION_USER it doesn't seem like we've added much.
Maybe, instead of just being the raw user identifier, it should be
something like "auth_method:user_identifier" so that one can tell
what the identifier actually is and how it was verified.
regards, tom lane
On 6/22/22 12:28, Tom Lane wrote:
Joe Conway <mail@joeconway.com> writes:
On 6/22/22 11:52, Tom Lane wrote:
I think a case could be made for ONLY returning non-null when authn_id
represents some externally-verified identifier (OS user ID gotten via
peer identification, Kerberos principal, etc).But -1 on that.
I think any time we have a non-null authn_id we should expose it. Are
there examples of cases when we have authn_id but for some reason don't
trust the value of it?I'm more concerned about whether we have a consistent story about what
SYSTEM_USER means (another way of saying "what type is it"). If it's
just the same as SESSION_USER it doesn't seem like we've added much.Maybe, instead of just being the raw user identifier, it should be
something like "auth_method:user_identifier" so that one can tell
what the identifier actually is and how it was verified.
Oh, that's an interesting thought -- I like that.
--
Joe Conway
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Wed, Jun 22, 2022 at 9:26 AM Joe Conway <mail@joeconway.com> wrote:
On 6/22/22 11:35, Jacob Champion wrote:
On Wed, Jun 22, 2022 at 8:10 AM Joe Conway <mail@joeconway.com> wrote:
Why would you want to do it differently than
SessionUserId/OuterUserId/CurrentUserId? It is analogous, no?
Like I said, now there are two different sources of truth, and
additional code to sync the two, and two different APIs to set what
should be a single write-once attribute. But if SystemUser is instead
derived from authn_id, like what's just been proposed with
`method:authn_id`, I think there's a better argument for separating
the two.
--Jacob
On Wed, Jun 22, 2022 at 9:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Joe Conway <mail@joeconway.com> writes:
On 6/22/22 11:52, Tom Lane wrote:
I think a case could be made for ONLY returning non-null when authn_id
represents some externally-verified identifier (OS user ID gotten via
peer identification, Kerberos principal, etc).But -1 on that.
I think any time we have a non-null authn_id we should expose it. Are
there examples of cases when we have authn_id but for some reason don't
trust the value of it?I'm more concerned about whether we have a consistent story about what
SYSTEM_USER means (another way of saying "what type is it"). If it's
just the same as SESSION_USER it doesn't seem like we've added much.Maybe, instead of just being the raw user identifier, it should be
something like "auth_method:user_identifier" so that one can tell
what the identifier actually is and how it was verified.
I was thinking this was trying to make the following possible:
psql -U postgres
# set session authorization other_superuser;
# set role other_role;
# select system_user, session_user, current_user;
postgres | other_superuser | other_role
Though admittedly using "system" for that seems somehow wrong.
connection_user would make more sense. Then the system_user would be, if
applicable, an external identifier that got matched with the assigned
connection_user. I can definitely see having the external identifier be a
structured value.
David J.
Hi,
On 6/22/22 6:32 PM, Joe Conway wrote:
CAUTION: This email originated from outside of the organization. Do
not click links or open attachments unless you can confirm the sender
and know the content is safe.On 6/22/22 12:28, Tom Lane wrote:
Joe Conway <mail@joeconway.com> writes:
On 6/22/22 11:52, Tom Lane wrote:
I think a case could be made for ONLY returning non-null when authn_id
represents some externally-verified identifier (OS user ID gotten via
peer identification, Kerberos principal, etc).But -1 on that.
I think any time we have a non-null authn_id we should expose it. Are
there examples of cases when we have authn_id but for some reason don't
trust the value of it?I'm more concerned about whether we have a consistent story about what
SYSTEM_USER means (another way of saying "what type is it"). If it's
just the same as SESSION_USER it doesn't seem like we've added much.Maybe, instead of just being the raw user identifier, it should be
something like "auth_method:user_identifier" so that one can tell
what the identifier actually is and how it was verified.Oh, that's an interesting thought -- I like that.
Thanks Joe and Tom for your feedback.
I like this idea too and that's also more aligned with what
log_connections set to on would report (aka the auth method).
Baring any objections, I'll work on that idea.
Bertrand
Hi,
On 6/22/22 5:35 PM, Jacob Champion wrote:
On Wed, Jun 22, 2022 at 8:10 AM Joe Conway <mail@joeconway.com> wrote:
On the contrary, I would argue that not having the identifier for the
external "user" available is a security concern. Ideally you want to be
able to trace actions inside Postgres to the actual user that invoked them.If auditing is also the use case for SYSTEM_USER, you'll probably want
to review the arguments for making it available to parallel workers
that were made in the other thread [1].
Thanks Jacob for your feedback.
I did some testing initially around the parallel workers and did not see
any issues at that time.
I just had another look and I agree that the parallel workers case needs
to be addressed.
I'll have a closer look to what you have done in [1]/messages/by-id/793d990837ae5c06a558d58d62de9378ab525d83.camel@vmware.com.
Thanks
Bertrand
[1]: /messages/by-id/793d990837ae5c06a558d58d62de9378ab525d83.camel@vmware.com
Hi,
On 6/23/22 10:06 AM, Drouvot, Bertrand wrote:
Hi,
On 6/22/22 5:35 PM, Jacob Champion wrote:
On Wed, Jun 22, 2022 at 8:10 AM Joe Conway <mail@joeconway.com> wrote:
On the contrary, I would argue that not having the identifier for the
external "user" available is a security concern. Ideally you want to be
able to trace actions inside Postgres to the actual user that
invoked them.If auditing is also the use case for SYSTEM_USER, you'll probably want
to review the arguments for making it available to parallel workers
that were made in the other thread [1].Thanks Jacob for your feedback.
I did some testing initially around the parallel workers and did not
see any issues at that time.I just had another look and I agree that the parallel workers case
needs to be addressed.I'll have a closer look to what you have done in [1].
Thanks
Bertrand
Please find attached patch version 2.
It does contain:
- Tom's idea implementation (aka presenting the system_user as
auth_method:authn_id)
- A fix for the parallel workers issue mentioned by Jacob. The patch now
propagates the SYSTEM_USER to the parallel workers.
- Doc updates
- Tap tests (some of them are coming from [1]/messages/by-id/793d990837ae5c06a558d58d62de9378ab525d83.camel@vmware.com)
Looking forward to your feedback,
Thanks
Bertrand
[1]: /messages/by-id/793d990837ae5c06a558d58d62de9378ab525d83.camel@vmware.com
/messages/by-id/793d990837ae5c06a558d58d62de9378ab525d83.camel@vmware.com
Attachments:
v2-0001-system_user-implementation.patchtext/plain; charset=UTF-8; name=v2-0001-system_user-implementation.patchDownload+213-5
On 6/24/22 11:49 AM, Drouvot, Bertrand wrote:
Hi,
On 6/23/22 10:06 AM, Drouvot, Bertrand wrote:
Hi,
On 6/22/22 5:35 PM, Jacob Champion wrote:
On Wed, Jun 22, 2022 at 8:10 AM Joe Conway <mail@joeconway.com> wrote:
On the contrary, I would argue that not having the identifier for the
external "user" available is a security concern. Ideally you want
to be
able to trace actions inside Postgres to the actual user that
invoked them.If auditing is also the use case for SYSTEM_USER, you'll probably want
to review the arguments for making it available to parallel workers
that were made in the other thread [1].Thanks Jacob for your feedback.
I did some testing initially around the parallel workers and did not
see any issues at that time.I just had another look and I agree that the parallel workers case
needs to be addressed.I'll have a closer look to what you have done in [1].
Thanks
Bertrand
Please find attached patch version 2.
It does contain:
- Tom's idea implementation (aka presenting the system_user as
auth_method:authn_id)- A fix for the parallel workers issue mentioned by Jacob. The patch
now propagates the SYSTEM_USER to the parallel workers.- Doc updates
- Tap tests (some of them are coming from [1])
Looking forward to your feedback,
Thanks
Bertrand
FWIW here is a link to the commitfest entry:
https://commitfest.postgresql.org/38/3703/
Bertrand
Hi,
On 6/24/22 2:47 PM, Drouvot, Bertrand wrote:
On 6/24/22 11:49 AM, Drouvot, Bertrand wrote:
Hi,
On 6/23/22 10:06 AM, Drouvot, Bertrand wrote:
Hi,
On 6/22/22 5:35 PM, Jacob Champion wrote:
On Wed, Jun 22, 2022 at 8:10 AM Joe Conway <mail@joeconway.com> wrote:
On the contrary, I would argue that not having the identifier for the
external "user" available is a security concern. Ideally you want
to be
able to trace actions inside Postgres to the actual user that
invoked them.If auditing is also the use case for SYSTEM_USER, you'll probably want
to review the arguments for making it available to parallel workers
that were made in the other thread [1].Thanks Jacob for your feedback.
I did some testing initially around the parallel workers and did not
see any issues at that time.I just had another look and I agree that the parallel workers case
needs to be addressed.I'll have a closer look to what you have done in [1].
Thanks
Bertrand
Please find attached patch version 2.
It does contain:
- Tom's idea implementation (aka presenting the system_user as
auth_method:authn_id)- A fix for the parallel workers issue mentioned by Jacob. The patch
now propagates the SYSTEM_USER to the parallel workers.- Doc updates
- Tap tests (some of them are coming from [1])
Looking forward to your feedback,
Thanks
Bertrand
FWIW here is a link to the commitfest entry:
https://commitfest.postgresql.org/38/3703/Bertrand
Attached a tiny rebase to make the CF bot CompilerWarnings happy.
Bertrand
Attachments:
v2-0002-system_user-implementation.patchtext/plain; charset=UTF-8; name=v2-0002-system_user-implementation.patchDownload+213-5
On 2022-Jun-25, Drouvot, Bertrand wrote:
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 0af130fbc5..8d761512fd 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -364,6 +364,10 @@ extern void InitializeSessionUserIdStandalone(void); extern void SetSessionAuthorization(Oid userid, bool is_superuser); extern Oid GetCurrentRoleId(void); extern void SetCurrentRoleId(Oid roleid, bool is_superuser); +/* kluge to avoid including libpq/libpq-be.h here */ +typedef struct Port MyPort; +extern void InitializeSystemUser(const MyPort *port); +extern const char* GetSystemUser(void);
This typedef here looks quite suspicious. I think this should suffice:
+/* kluge to avoid including libpq/libpq-be.h here */
+struct Port;
+extern void InitializeSystemUser(struct Port *port);I suspect that having a typedef called MyPort is going to wreak serious
havoc for pgindent.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
