RLS related docs

Started by Joe Conwayover 9 years ago9 messages

Joe Conway

mail@joeconway.com

over 9 years ago

2 attachment(s)

Please see attached two proposed patches for the docs related to RLS:

1) Correction to pg_restore
2) Additional mentions that "COPY FROM" does not allow RLS to be enabled

Comments?

Attachments:

2016.05.24.00-pg_restore.sgml.difftext/x-diff; name=2016.05.24.00-pg_restore.sgml.diffDownload

diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml
index 66d09f4..e951182 100644
*** a/doc/src/sgml/ref/pg_restore.sgml
--- b/doc/src/sgml/ref/pg_restore.sgml
***************
*** 537,543 ****
  
         <para>
          Note that this option currently also requires the dump be in <command>INSERT</command>
!         format, as <command>COPY TO</command> does not support row security.
         </para>
        </listitem>
       </varlistentry>
--- 537,543 ----
  
         <para>
          Note that this option currently also requires the dump be in <command>INSERT</command>
!         format, as <command>COPY FROM</command> does not support row security.
         </para>
        </listitem>
       </varlistentry>

2016.05.24.00-RLS.docs.difftext/x-diff; name=2016.05.24.00-RLS.docs.diffDownload

diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 07e2f45..bb7662a 100644
*** a/doc/src/sgml/ref/copy.sgml
--- b/doc/src/sgml/ref/copy.sgml
*************** COPY <replaceable class="parameter">coun
*** 502,507 ****
--- 502,511 ----
      null strings to null values and unquoted null strings to empty strings.
     </para>
  
+    <para>
+     <command>COPY FROM</command> does not support row security. Use
+     <command>INSERT</command> instead.
+    </para>
   </refsect1>
  
   <refsect1>
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index bc28684..0995c6b 100644
*** a/doc/src/sgml/ref/pg_dump.sgml
--- b/doc/src/sgml/ref/pg_dump.sgml
*************** PostgreSQL documentation
*** 716,721 ****
--- 716,726 ----
          to dump the parts of the contents of the table that they have access to.
         </para>
  
+        <para>
+         Note that if you use this option currently, you probably also want
+         the dump be in <command>INSERT</command> format, as the
+         <command>COPY FROM</command> during restore does not support row security.
+        </para>
        </listitem>
       </varlistentry>

Dean Rasheed

dean.a.rasheed@gmail.com

over 9 years ago

In reply to: Joe Conway (#1)

Re: RLS related docs

On 25 May 2016 at 02:04, Joe Conway <mail@joeconway.com> wrote:

Please see attached two proposed patches for the docs related to RLS:

1) Correction to pg_restore
2) Additional mentions that "COPY FROM" does not allow RLS to be enabled

Comments?

The pg_restore change looks good -- that was clearly wrong.

Also, +1 for the new note in pg_dump.

For COPY, I think perhaps it would be more logical to put the new note
immediately after the third note which describes the privileges
required, since it's kind of related, and then we can talk about the
RLS policies required, e.g.:

If row-level security is enabled for the table, COPY table TO is
internally converted to COPY (SELECT * FROM table) TO, and the
relevant security policies are applied. Currently, COPY FROM is not
supported for tables with row-level security.

Related question: I believe

COPY tbl TO ...

is internally converted to

COPY (select * FROM tbl) TO ...

when RLS is involved. Do we want to document that?

I think so, yes, because that makes it clearer what policies will be applied.

Regards,
Dean

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

Joe Conway

mail@joeconway.com

over 9 years ago

In reply to: Dean Rasheed (#2)

Re: RLS related docs

On 05/26/2016 12:26 AM, Dean Rasheed wrote:

On 25 May 2016 at 02:04, Joe Conway <mail@joeconway.com> wrote:

Please see attached two proposed patches for the docs related to RLS:

1) Correction to pg_restore
2) Additional mentions that "COPY FROM" does not allow RLS to be enabled

Comments?

The pg_restore change looks good -- that was clearly wrong.

Also, +1 for the new note in pg_dump.

Great, thanks!

For COPY, I think perhaps it would be more logical to put the new note
immediately after the third note which describes the privileges
required, since it's kind of related, and then we can talk about the
RLS policies required, e.g.:

If row-level security is enabled for the table, COPY table TO is
internally converted to COPY (SELECT * FROM table) TO, and the
relevant security policies are applied. Currently, COPY FROM is not
supported for tables with row-level security.

This sounds better than what I had, so I will do it that way.

Thanks,

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Joe Conway

mail@joeconway.com

over 9 years ago

In reply to: Joe Conway (#3)

1 attachment(s)

Re: RLS related docs

On 05/30/2016 01:56 PM, Joe Conway wrote:

On 05/26/2016 12:26 AM, Dean Rasheed wrote:

On 25 May 2016 at 02:04, Joe Conway <mail@joeconway.com> wrote:

Please see attached two proposed patches for the docs related to RLS:

1) Correction to pg_restore
2) Additional mentions that "COPY FROM" does not allow RLS to be enabled

Comments?

The pg_restore change looks good -- that was clearly wrong.

Also, +1 for the new note in pg_dump.

Great, thanks!

For COPY, I think perhaps it would be more logical to put the new note
immediately after the third note which describes the privileges
required, since it's kind of related, and then we can talk about the
RLS policies required, e.g.:

If row-level security is enabled for the table, COPY table TO is
internally converted to COPY (SELECT * FROM table) TO, and the
relevant security policies are applied. Currently, COPY FROM is not
supported for tables with row-level security.

This sounds better than what I had, so I will do it that way.

Apologies for the delay, but new patch attached. Assuming no more
comments, will commit this, backpatched to 9.5, in a day or two.

Thanks,

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Attachments:

2016.08.28.00-RLS.docs.difftext/x-diff; name=2016.08.28.00-RLS.docs.diffDownload

diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 07e2f45..af15fd1 100644
*** a/doc/src/sgml/ref/copy.sgml
--- b/doc/src/sgml/ref/copy.sgml
*************** COPY <replaceable class="parameter">coun
*** 419,424 ****
--- 419,434 ----
     </para>
  
     <para>
+     If row-level security is enabled for the table, <literal>COPY
+     <replaceable class="parameter">table</> TO</literal> is
+     internally converted to <literal>COPY (SELECT * FROM
+     <replaceable class="parameter">table</>) TO ...</literal>,
+     and the relevant security policies are applied. Currently,
+     <command>COPY FROM</command> is not supported for tables with row-level
+     security. Use equivalent <command>INSERT</command> statements instead.
+    </para>
+ 
+    <para>
      Files named in a <command>COPY</command> command are read or written
      directly by the server, not by the client application. Therefore,
      they must reside on or be accessible to the database server machine,
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index be1b684..4fa925c 100644
*** a/doc/src/sgml/ref/pg_dump.sgml
--- b/doc/src/sgml/ref/pg_dump.sgml
*************** PostgreSQL documentation
*** 699,704 ****
--- 699,709 ----
          to dump the parts of the contents of the table that they have access to.
         </para>
  
+        <para>
+         Note that if you use this option currently, you probably also want
+         the dump be in <command>INSERT</command> format, as the
+         <command>COPY FROM</command> during restore does not support row security.
+        </para>
        </listitem>
       </varlistentry>
  
diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml
index c906919..ef5bab4 100644
*** a/doc/src/sgml/ref/pg_restore.sgml
--- b/doc/src/sgml/ref/pg_restore.sgml
***************
*** 527,533 ****
  
         <para>
          Note that this option currently also requires the dump be in <command>INSERT</command>
!         format, as <command>COPY TO</command> does not support row security.
         </para>
        </listitem>
       </varlistentry>
--- 527,533 ----
  
         <para>
          Note that this option currently also requires the dump be in <command>INSERT</command>
!         format, as <command>COPY FROM</command> does not support row security.
         </para>
        </listitem>
       </varlistentry>

Dean Rasheed

dean.a.rasheed@gmail.com

over 9 years ago

In reply to: Joe Conway (#4)

Re: RLS related docs

On 28 August 2016 at 21:23, Joe Conway <mail@joeconway.com> wrote:

Apologies for the delay, but new patch attached. Assuming no more
comments, will commit this, backpatched to 9.5, in a day or two.

Looking at this again, I think there is something fishy about these
dump/restore flags.

If you do pg_dump --enable-row-security, then row_security is turned
on during the dump and only the user-visible portions of the tables
are dumped. But why does such a dump emit "SET row_security = on;" as
part of the dump? There doesn't appear to be any reason for having
row_security turned on during the restore just because it was on
during the dump. The INSERT policies may well be different from the
SELECT policies, and so this may lead to a dump that cannot be
restored. ISTM that row_security should be off inside the dump, and
only enabled during restore if the user explicitly asks for it,
regardless of what setting was used to produce the dump.

Also, isn't it the case that --enable-row-security during pg_restore
is only relevant when performing a data-only restore (like
--disable-triggers). Otherwise, it looks to me as though the restore
will create the tables, restore the data, and then only at the end
restore the table policies and enable row level security on the
tables. So it looks like the flag would have no effect (and a
COPY-format dump would work fine) for a non-data-only dump.

I never really looked at the RLS dump/restore code. Am I missing something?

Regards,
Dean

--
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

over 9 years ago

In reply to: Dean Rasheed (#5)

Re: RLS related docs

On Tue, Aug 30, 2016 at 3:05 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

On 28 August 2016 at 21:23, Joe Conway <mail@joeconway.com> wrote:

Apologies for the delay, but new patch attached. Assuming no more
comments, will commit this, backpatched to 9.5, in a day or two.

Looking at this again, I think there is something fishy about these
dump/restore flags.

If you do pg_dump --enable-row-security, then row_security is turned
on during the dump and only the user-visible portions of the tables
are dumped. But why does such a dump emit "SET row_security = on;" as
part of the dump? There doesn't appear to be any reason for having
row_security turned on during the restore just because it was on
during the dump. The INSERT policies may well be different from the
SELECT policies, and so this may lead to a dump that cannot be
restored. ISTM that row_security should be off inside the dump, and
only enabled during restore if the user explicitly asks for it,
regardless of what setting was used to produce the dump.

I think you are right about this.

Also, isn't it the case that --enable-row-security during pg_restore
is only relevant when performing a data-only restore (like
--disable-triggers). Otherwise, it looks to me as though the restore
will create the tables, restore the data, and then only at the end
restore the table policies and enable row level security on the
tables. So it looks like the flag would have no effect (and a
COPY-format dump would work fine) for a non-data-only dump.

Hmm. That seems odd.

--
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

over 9 years ago

In reply to: Joe Conway (#4)

Re: RLS related docs

On Sun, Aug 28, 2016 at 4:23 PM, Joe Conway <mail@joeconway.com> wrote:

For COPY, I think perhaps it would be more logical to put the new note
immediately after the third note which describes the privileges
required, since it's kind of related, and then we can talk about the
RLS policies required, e.g.:

If row-level security is enabled for the table, COPY table TO is
internally converted to COPY (SELECT * FROM table) TO, and the
relevant security policies are applied. Currently, COPY FROM is not
supported for tables with row-level security.

This sounds better than what I had, so I will do it that way.

Apologies for the delay, but new patch attached. Assuming no more
comments, will commit this, backpatched to 9.5, in a day or two.

I don't think this was ever committed, but my comment is that it seems
to be exposing rather more of the implementation than is probably
wise. Can't we say that SELECT policies will apply rather than saying
that it is internally converted to a SELECT?

--
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

Joe Conway

mail@joeconway.com

over 9 years ago

In reply to: Robert Haas (#7)

Re: RLS related docs

On 09/15/2016 01:33 PM, Robert Haas wrote:

On Sun, Aug 28, 2016 at 4:23 PM, Joe Conway <mail@joeconway.com> wrote:

For COPY, I think perhaps it would be more logical to put the new note
immediately after the third note which describes the privileges
required, since it's kind of related, and then we can talk about the
RLS policies required, e.g.:

If row-level security is enabled for the table, COPY table TO is
internally converted to COPY (SELECT * FROM table) TO, and the
relevant security policies are applied. Currently, COPY FROM is not
supported for tables with row-level security.

This sounds better than what I had, so I will do it that way.

Apologies for the delay, but new patch attached. Assuming no more
comments, will commit this, backpatched to 9.5, in a day or two.

I don't think this was ever committed, but my comment is that it seems
to be exposing rather more of the implementation than is probably
wise. Can't we say that SELECT policies will apply rather than saying
that it is internally converted to a SELECT?

I've not committed it yet because I was going to look into the new info
Dean mentioned first. Seems like your wording is fine, so will make that
change.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Joe Conway

mail@joeconway.com

about 9 years ago

In reply to: Joe Conway (#8)

Re: RLS related docs

On 09/15/2016 02:34 PM, Joe Conway wrote:

On 09/15/2016 01:33 PM, Robert Haas wrote:

On Sun, Aug 28, 2016 at 4:23 PM, Joe Conway <mail@joeconway.com> wrote:

For COPY, I think perhaps it would be more logical to put the new note
immediately after the third note which describes the privileges
required, since it's kind of related, and then we can talk about the
RLS policies required, e.g.:

If row-level security is enabled for the table, COPY table TO is
internally converted to COPY (SELECT * FROM table) TO, and the
relevant security policies are applied. Currently, COPY FROM is not
supported for tables with row-level security.

This sounds better than what I had, so I will do it that way.

Apologies for the delay, but new patch attached. Assuming no more
comments, will commit this, backpatched to 9.5, in a day or two.

I don't think this was ever committed, but my comment is that it seems
to be exposing rather more of the implementation than is probably
wise. Can't we say that SELECT policies will apply rather than saying
that it is internally converted to a SELECT?

Committed that way, backpatched to 9.5.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development