Directory/File Access Permissions for COPY and Generic File Access Functions

"Brightwell, Adam" <adam.brightwell@crunchydatasolutions.com> writes:

The attached patch for review implements a directory permission system that
allows for providing a directory read/write capability to directories for
COPY TO/FROM and Generic File Access Functions to non-superusers.

TBH, this sounds like it's adding a lot of mechanism and *significant*
risk of unforeseen security issues in order to solve a problem that we
do not need to solve. The field demand for such a feature is just about
indistinguishable from zero.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Oct 15, 2014 at 11:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Brightwell, Adam" <adam.brightwell@crunchydatasolutions.com> writes:

The attached patch for review implements a directory permission system that
allows for providing a directory read/write capability to directories for
COPY TO/FROM and Generic File Access Functions to non-superusers.

TBH, this sounds like it's adding a lot of mechanism and *significant*
risk of unforeseen security issues in order to solve a problem that we
do not need to solve. The field demand for such a feature is just about
indistinguishable from zero.

I am also not convinced that we need this. If we need to allow
non-superusers COPY permission at all, can we just exclude certain
"unsafe" directories (like the data directory, and tablespaces) and
let them access anything else? Or can we have a whitelist of
directories stored as a PGC_SUSER GUC? This seems awfully heavyweight
for what it is.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Robert Haas (robertmhaas@gmail.com) wrote:

On Wed, Oct 15, 2014 at 11:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Brightwell, Adam" <adam.brightwell@crunchydatasolutions.com> writes:

The attached patch for review implements a directory permission system that
allows for providing a directory read/write capability to directories for
COPY TO/FROM and Generic File Access Functions to non-superusers.

TBH, this sounds like it's adding a lot of mechanism and *significant*
risk of unforeseen security issues in order to solve a problem that we
do not need to solve. The field demand for such a feature is just about
indistinguishable from zero.

I am also not convinced that we need this. If we need to allow
non-superusers COPY permission at all, can we just exclude certain
"unsafe" directories (like the data directory, and tablespaces) and
let them access anything else?

Wow.. I'd say 'no' to this, certainly. Granularity is required here.
I want to give a non-superuser the ability to slurp data off a specific
NFS mount, not read /etc/passwd..

Or can we have a whitelist of
directories stored as a PGC_SUSER GUC? This seems awfully heavyweight
for what it is.

Hrm, perhaps this would work though..

Allow me to outline a few use-cases which I see for this though and
perhaps that'll help us make progress.

This started out as a request for a non-superuser to be able to review
the log files without needing access to the server. Now, things can
certainly be set up on the server to import *all* logs and then grant
access to a non-superuser, but generally it's "I need to review the log
from X to Y" and not *all* logs need to be stored or kept in PG.

In years past, I've wanted to be able to grant this ability out for
users to do loads without having to transfer the data through the user's
laptop or get them to log onto the Linux box from their Windows desktop
and pull the data in via psql (it's a bigger deal than some might
think..), and then there's the general ETL case where, without this, you
end up running something like Pentaho and having to pass all the data
through Java to get it into the database.

Building on that is the concept of *background* loads, with
pg_background. That's a killer capability, in my view. "Hey, PG, go
load all the files in this directory into this table, but don't make me
have to stick around and make sure my laptop is still connected for the
next 3 hours."

Next, the file_fdw could leverage this catalog to do its own checks and
allow non-superusers to use it, which would be fantastic and gets back
to the 'log file' use-case above.

And then there is the next-level item: CREATE TABLESPACE, which we
already see folks like RDS and others having to hack the codebase to
add as a non-superuser capability. It'd need to be an independently
grantable capability, of course.

Thanks!

Stephen

On Thu, Oct 16, 2014 at 12:01:28PM -0400, Stephen Frost wrote:

This started out as a request for a non-superuser to be able to review
the log files without needing access to the server. Now, things can
certainly be set up on the server to import *all* logs and then grant
access to a non-superuser, but generally it's "I need to review the log
from X to Y" and not *all* logs need to be stored or kept in PG.

Why is this patch showing up before being discussed? You are having to
back into the discusion because of this.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Bruce Momjian (bruce@momjian.us) wrote:

On Thu, Oct 16, 2014 at 12:01:28PM -0400, Stephen Frost wrote:

This started out as a request for a non-superuser to be able to review
the log files without needing access to the server. Now, things can
certainly be set up on the server to import *all* logs and then grant
access to a non-superuser, but generally it's "I need to review the log
from X to Y" and not *all* logs need to be stored or kept in PG.

Why is this patch showing up before being discussed? You are having to
back into the discusion because of this.

For my part, I didn't actually see it as being a questionable use-case
from the start.. That was obviously incorrect, though I didn't know
that previously. The general idea has been discussed a couple of times
before, at least as far back as 2005:

/messages/by-id/430F78E0.9020206@cs.concordia.ca

It's also a feature available in other databases (at least MySQL and
Oracle, but I'm pretty sure others also).

I can also recall chatting with folks about it a couple of times over
the years at various conferences. Still, perhaps it would have been
better to post about the idea before the patch, but hindsight is often
20/20.

Thanks!

Stephen

On 10/16/14 12:01 PM, Stephen Frost wrote:

This started out as a request for a non-superuser to be able to review
the log files without needing access to the server.

I think that can be done with a security-definer function.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

You patch is missing the files src/include/catalog/pg_diralias.h,
src/include/commands/diralias.h, and src/backend/commands/diralias.c.

(Hint: git add -N)

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter,

You patch is missing the files src/include/catalog/pg_diralias.h,

src/include/commands/diralias.h, and src/backend/commands/diralias.c.

(Hint: git add -N)

Yikes, sorry about that, not sure how that happened. Attached is an
updated patch.

-Adam

--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

* Peter Eisentraut (peter_e@gmx.net) wrote:

On 10/16/14 12:01 PM, Stephen Frost wrote:

This started out as a request for a non-superuser to be able to review
the log files without needing access to the server.

I think that can be done with a security-definer function.

Of course it can be. We could replace the entire authorization system
with security definer functions too. I don't view this as an argument
against this feature, particularly as we know other systems have it,
users have asked for multiple times, and large PG deployments have had
to hack around our lack of it.

Thanks,

Stephen

All,

Attached is a patch with minor updates/corrections.

-Adam

--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

I think the way this should work is that if you create a DIRALIAS, then
the COPY command should refer to it by logical name, e.g.,

CREATE DIRALIAS dumpster AS '/tmp/trash';
COPY mytable TO dumpster;

If you squint a bit, this is the same as a tablespace. Maybe those two
concepts could be combined.

On the other hand, we already have file_fdw, which does something very
similar.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 10/27/14 7:27 AM, Stephen Frost wrote:

* Peter Eisentraut (peter_e@gmx.net) wrote:

On 10/16/14 12:01 PM, Stephen Frost wrote:

This started out as a request for a non-superuser to be able to review
the log files without needing access to the server.

I think that can be done with a security-definer function.

Of course it can be. We could replace the entire authorization system
with security definer functions too.

I don't think that is correct.

It's easy to do something with security definer functions if it's single
purpose, with a single argument, like load this file into this table,
let these users do it.

It's not easy to do it with functions if you have many parameters, like
in a general SELECT statement.

So I would like to see at least three wildly different use cases for
this before believing that a security definer function isn't appropriate.

I don't view this as an argument
against this feature, particularly as we know other systems have it,
users have asked for multiple times, and large PG deployments have had
to hack around our lack of it.

What other systems have it? Do you have links to their documentation?

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Peter Eisentraut (peter_e@gmx.net) wrote:

On 10/27/14 7:27 AM, Stephen Frost wrote:

* Peter Eisentraut (peter_e@gmx.net) wrote:

On 10/16/14 12:01 PM, Stephen Frost wrote:

This started out as a request for a non-superuser to be able to review
the log files without needing access to the server.

I think that can be done with a security-definer function.

Of course it can be. We could replace the entire authorization system
with security definer functions too.

I don't think that is correct.

Of course it is- you simply have to move all the logic into the
function.

It's easy to do something with security definer functions if it's single
purpose, with a single argument, like load this file into this table,
let these users do it.

The files won't be consistently named and there may be cases to make
ad-hoc runs or test runs. No, it isn't as simple as always being a
single, specific filename and when consider that there needs to be
intelligence about the actual path being specified and making sure that
there can't be '..' and similar, it gets to be a pretty ugly situation
to make our users have to deal with.

It's not easy to do it with functions if you have many parameters, like
in a general SELECT statement.

You could define SRFs for every table.

So I would like to see at least three wildly different use cases for
this before believing that a security definer function isn't appropriate.

I'm not following this- there's probably 100s of use-cases for this, but
they're all variations n 'read and/or write data server-side instead of
through a front-end connection', which is what the purpose of the
feature is.. I do see this as being useful for COPY, Large Object, and
the file_fdw...

I don't view this as an argument
against this feature, particularly as we know other systems have it,
users have asked for multiple times, and large PG deployments have had
to hack around our lack of it.

What other systems have it? Do you have links to their documentation?

MySQL:
http://dev.mysql.com/doc/refman/5.1/en/privileges-provided.html#priv_file

(note they provide a way to limit access also, via secure_file_priv)

Oracle:
http://docs.oracle.com/cd/B19306_01/server.102/b14200/statements_5007.htm
http://docs.oracle.com/cd/B19306_01/server.102/b14200/statements_9013.htm#i2125999

SQL Server:
http://msdn.microsoft.com/en-us/library/ms175915.aspx
(Note: they can actually run as the user connected instead of the SQL DB
server, if Windows authentication is used, which is basically doing
Kerberos proxying unless I'm mistaken; it's unclear how the security is
maintained if it's a SQL server logon user..).

DB2:
http://www-01.ibm.com/support/knowledgecenter/SSEPGG_9.7.0/com.ibm.db2.luw.admin.dm.doc/doc/c0004589.html?cp=SSEPGG_9.7.0

etc...

Thanks,

Stephen

* Peter Eisentraut (peter_e@gmx.net) wrote:

I think the way this should work is that if you create a DIRALIAS, then
the COPY command should refer to it by logical name, e.g.,

CREATE DIRALIAS dumpster AS '/tmp/trash';
COPY mytable TO dumpster;

You'd have to be able to specify the filename also. I'm not against the
idea of using the 'diralias' alias name this way, just saying it isn't
quite as simple as the above.

If you squint a bit, this is the same as a tablespace. Maybe those two
concepts could be combined.

CREATE TABLESPACE is something else which could be supported with
diralias, though it'd have to be an independently grantable capability
and it'd be a bad idea to let a user create tablespaces in a directory
and then also be able to copy from/to files there (backend crashes,
etc). This exact capability is more-or-less what RDS has had to hack on
to PG for their environment, as I understand it, in case you're looking
for a use-case.

On the other hand, we already have file_fdw, which does something very
similar.

It's really not at all the same.. Perhaps we'll get there some day, but
we're a very long way away from file_fdw having the ability to replace
normal tablespaces...

Thanks!

Stephen

On Mon, Oct 27, 2014 at 5:59 PM, Adam Brightwell
<adam.brightwell@crunchydatasolutions.com> wrote:

Attached is a patch with minor updates/corrections.

Given that no fewer than four people - all committers - have expressed
doubts about the design of this patch, I wonder why you're bothering
to post a new version. It seems to me that you should be discussing
the fundamental design, not making minor updates to the code. I
really hope this is not moving in the direction of another "surprise
commit" like we had with RLS. There is absolutely NOT consensus on
this design or anything close to it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Robert Haas (robertmhaas@gmail.com) wrote:

There is absolutely NOT consensus on
this design or anything close to it.

There is no doubt that consensus on the desirability and design needs
to be reached before we can even consider committing it. I suspect
Adam posted it simply because he had identified issues himself and
wanted to make others aware that things had been fixed.

That said, it sounds like the primary concern has been if we want this
feature at all and there hasn't been much discussion of the design
itself. Comments about the technical design would be great. I
appreciate your thoughts about using a PGC_SUSER GUC, but I don't feel
like it really works as it's all-or-nothing and doesn't provide
read-vs-write, unless we extend it out to be multiple GUCs and then
there is still the question about per-role access..

I'm not sure that I see a way to allow the per-role granularity without
having a top-level catalog object on which the GRANT can be executed and
ACL information stored. Perhaps it's unfortunate that we don't have a
more generic way to address that but I'm not sure I really see another
catalog table as a big problem..

Thanks!

Stephen

On 2014-10-28 09:24:18 -0400, Stephen Frost wrote:

* Robert Haas (robertmhaas@gmail.com) wrote:

There is absolutely NOT consensus on
this design or anything close to it.

There is no doubt that consensus on the desirability and design needs
to be reached before we can even consider committing it. I suspect
Adam posted it simply because he had identified issues himself and
wanted to make others aware that things had been fixed.

That said, it sounds like the primary concern has been if we want this
feature at all and there hasn't been much discussion of the design
itself.

Well, why waste time on the technical details when we haven't agreed
that the feature is worthwile? Review bandwidth is a serious problem in
this community.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Andres Freund (andres@2ndquadrant.com) wrote:

On 2014-10-28 09:24:18 -0400, Stephen Frost wrote:

There is no doubt that consensus on the desirability and design needs
to be reached before we can even consider committing it. I suspect
Adam posted it simply because he had identified issues himself and
wanted to make others aware that things had been fixed.

That said, it sounds like the primary concern has been if we want this
feature at all and there hasn't been much discussion of the design
itself.

Well, why waste time on the technical details when we haven't agreed
that the feature is worthwile? Review bandwidth is a serious problem in
this community.

Fair enough, and I'm happy to discuss that (and have been..); I was
simply objecting to the implication that the desirability concerns
raised were design concerns- the only design concern raised was wrt
it being possibly too heavyweight and the PGC_SUSET GUC suggestion (at
least, based on my re-reading of the thread..).

Thanks!

Stephen

On Tue, Oct 28, 2014 at 9:24 AM, Stephen Frost <sfrost@snowman.net> wrote:

That said, it sounds like the primary concern has been if we want this
feature at all and there hasn't been much discussion of the design
itself. Comments about the technical design would be great. I
appreciate your thoughts about using a PGC_SUSER GUC, but I don't feel
like it really works as it's all-or-nothing and doesn't provide
read-vs-write, unless we extend it out to be multiple GUCs and then
there is still the question about per-role access..

It sounds to me like you've basically settled on the way that you want
to implement it - without prior discussion on the mailing list - and
you're not trying very hard to make any of the alternatives work.
It's not the community's job to come up with a design that satisfies
you; it's your job to come up with as design that satisfies the
community. That doesn't *necessarily* mean that you have to change
the design that you've come up with; convincing other people that your
design is the best one is also an option. But I don't see that you're
making any real attempt to do that.

Your previous comment on the idea of a PGC_SUSET GUC was "Hrm, perhaps
this would work though.." and then, with zero further on-list
discussion, you've arrived at "I don't feel like it really works as
it's all-or-nothing and doesn't provide read-vs-write". Those are
precisely the kinds of issues that you should be discussing here in
detail, not cogitating on in isolation and then expecting this group
of people to accept that your original design is really for the best
after all.

I also find your technical arguments - to the extent that you've
bothered to articulate them at all - to be without merit. The
"question about per-role access" is easily dealt with, so let's start
there: if you make it a GUC, ALTER USER .. SET can be used to set
different values for different users. No problem. Your other
criticism that it is "all-vs-nothing" seems to me to be totally
incomprehensible, since as far as I can see a GUC with a list of
pathnames is exactly the same functionality that you're proposing to
implement via a much more baroque syntax. It is no more or less
all-or-nothing than that. Finally, you mention "read-vs-write"
access. You haven't even attempted to argue that we need to make that
distinction - in fact, you don't seem to have convinced a
significantly majority of the people that we need this feature at all
- but if we do, the fact that it might require two GUCs instead of one
is not a fatal objection to that design. (I'd be prepared to concede
that if there are half a dozen different privileges on directories
that we might want to grant, then wedging it into a GUC might be a
stretch.)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers