Обсуждение: Marking some contrib modules as trusted extensions

Now that we're just about there on the patch to invent trusted
extensions [1], I'd like to start a discussion about whether to mark
anything besides the trusted PLs as trusted.  I think generally
we ought to mark contrib modules as trusted if it's sane to do so;
there's not much point in handing people plperl (even sandboxed)
but not, say, hstore.  I trawled through what's in contrib today
and broke things down like this:

Certainly NO, as these allow external or low-level access:

adminpack
dblink
file_fdw
postgres_fdw
pageinspect
pg_buffercache
pg_freespacemap
pg_visibility
pgstattuple

Probably NO, if only because you'd need additional privileges
to use these anyway:

amcheck
dict_xsyn
hstore_plperlu
hstore_plpython2u
hstore_plpython3u
hstore_plpythonu
jsonb_plperlu
jsonb_plpython2u
jsonb_plpython3u
jsonb_plpythonu
ltree_plpython2u
ltree_plpython3u
ltree_plpythonu
pg_prewarm
pg_stat_statements

Definitely candidates to mark trusted:

citext
cube
dict_int        (unlike dict_xsyn, this needs no external file)
earthdistance        (marginal usefulness though)
fuzzystrmatch
hstore
hstore_plperl
intagg            (marginal usefulness though)
intarray
isn
jsonb_plperl
lo
ltree
pg_trgm
pgcrypto
seg
tablefunc
tcn
tsm_system_rows
tsm_system_time
unaccent        (needs external file, but the default one is useful)
uuid-ossp

Not sure what I think about these:

bloom            (are these useful in production?)
btree_gin
btree_gist
pgrowlocks        (seems safe, but are there security issues?)
spi/autoinc        (I doubt that these four are production grade)
spi/insert_username
spi/moddatetime
spi/refint
sslinfo            (seems safe, but are there security issues?)
xml2            (nominally safe, but deprecated, and libxml2
             has been a fertile source of security issues)

Any opinions about these, particularly the on-the-edge cases?

Also, how should we document this, if we do it?  Add a boilerplate
sentence to each module's description about whether it is trusted
or not?  Put a table up at the front of Appendxix F?  Both?

I'm happy to go make this happen, once we have consensus on what
should happen.

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/5889.1566415762%40sss.pgh.pa.us

On 2020-Jan-29, Tom Lane wrote:

> Not sure what I think about these:
> 
> bloom            (are these useful in production?)
> btree_gin
> btree_gist
> pgrowlocks        (seems safe, but are there security issues?)
> spi/autoinc        (I doubt that these four are production grade)
> spi/insert_username
> spi/moddatetime
> spi/refint
> sslinfo            (seems safe, but are there security issues?)
> xml2            (nominally safe, but deprecated, and libxml2
>              has been a fertile source of security issues)

Of these, btree_gist is definitely useful from a user perspective,
because it enables creation of certain exclusion constraints.

I've never heard of anyone using bloom indexes in production.  I'd
argue that if the feature is useful, then we should turn it into a
core-included index AM with regular WAL logging for improved
performance, and add a stripped-down version to src/test/modules to
cover the WAL-log testing needs.  Maybe exposing it more, as promoting
it as a trusted extension would do, would help find more use cases for
it.

> Also, how should we document this, if we do it?  Add a boilerplate
> sentence to each module's description about whether it is trusted
> or not?  Put a table up at the front of Appendxix F?  Both?

If it were possible to do both from a single source of truth, that would
be great.  Failing that, I'd just list it in each module's section.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

=?UTF-8?Q?Darafei_=22Kom=D1=8Fpa=22_Praliaskouski?= <me@komzpa.net> writes:
>> btree_gin
>> btree_gist

> I would even ask btree_gin and btree_gist to be moved to core.

That's not in scope here.  Our past experience with trying to move
extensions into core is that it creates a pretty painful upgrade
experience for users, so that's not something I'm interested in doing
... especially for relatively marginal cases like these.

There's also a more generic question of why we should want to move
anything to core anymore.  The trusted-extension mechanism removes
one of the biggest remaining gripes about extensions, namely the
pain level for installing them.  (But please, let's not have that
debate on this thread.)

            regards, tom lane

On Wed, Jan 29, 2020 at 9:46 PM Darafei "Komяpa" Praliaskouski
<me@komzpa.net> wrote:
>
> Hello,
>
>>
>> btree_gin
>> btree_gist
>
>
> I would even ask btree_gin and btree_gist to be moved to core.

Without going that far, I also agree that I relied on those extension
quite often, so +1 for marking them as trusted.

>> Probably NO, if only because you'd need additional privileges
>> to use these anyway:
>> pg_stat_statements

But the additional privileges are global, so assuming the extension
has been properly setup, wouldn't it be sensible to ease the
per-database installation?  If not properly setup, there's no harm in
creating the extension anyway.

Julien Rouhaud <rjuju123@gmail.com> writes:
>>> Probably NO, if only because you'd need additional privileges
>>> to use these anyway:
>>> pg_stat_statements

> But the additional privileges are global, so assuming the extension
> has been properly setup, wouldn't it be sensible to ease the
> per-database installation?  If not properly setup, there's no harm in
> creating the extension anyway.

Mmm, I'm not convinced --- the ability to see what statements are being
executed in other sessions (even other databases) is something that
paranoid installations might not be so happy about.  Our previous
discussions about what privilege level is needed to look at
pg_stat_statements info were all made against a background assumption
that you needed some extra privilege to set up the view in the first
place.  I think that would need another look or two before being
comfortable that we're not shifting the goal posts too far.

The bigger picture here is that I don't want to get push-back that
we've broken somebody's security posture by marking too many extensions
trusted.  So for anything where there's any question about security
implications, we should err in the conservative direction of leaving
it untrusted.

            regards, tom lane

On Wed, 29 Jan 2020 at 21:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> >>> pg_stat_statements
>
> Mmm, I'm not convinced --- the ability to see what statements are being
> executed in other sessions (even other databases) is something that
> paranoid installations might not be so happy about.  Our previous
> discussions about what privilege level is needed to look at
> pg_stat_statements info were all made against a background assumption
> that you needed some extra privilege to set up the view in the first
> place.  I think that would need another look or two before being
> comfortable that we're not shifting the goal posts too far.
>
> The bigger picture here is that I don't want to get push-back that
> we've broken somebody's security posture by marking too many extensions
> trusted.  So for anything where there's any question about security
> implications, we should err in the conservative direction of leaving
> it untrusted.
>

+1

I wonder if the same could be said about pgrowlocks.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On Wed, 29 Jan 2020 at 21:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The bigger picture here is that I don't want to get push-back that
>> we've broken somebody's security posture by marking too many extensions
>> trusted.  So for anything where there's any question about security
>> implications, we should err in the conservative direction of leaving
>> it untrusted.

> I wonder if the same could be said about pgrowlocks.

Good point.  I had figured it was probably OK given that it's
analogous to the pg_locks view (which is unrestricted AFAIR),
and that it already has some restrictions on what you can see.
I'd have no hesitation about dropping it off this list though,
since it's probably not used that much and it could also be seen
as exposing internals.

            regards, tom lane

After looking more closely at these modules, I'm kind of inclined
*not* to put the trusted marker on intagg.  That module is just
a backwards-compatibility wrapper around functionality that
exists in the core code nowadays.  So I think what we ought to be
doing with it is deprecating and eventually removing it, not
encouraging people to keep using it.

Given that and the other discussion in this thread, I think the
initial list of modules to trust is:

btree_gin
btree_gist
citext
cube
dict_int
earthdistance
fuzzystrmatch
hstore
hstore_plperl
intarray
isn
jsonb_plperl
lo
ltree
pg_trgm
pgcrypto
seg
tablefunc
tcn
tsm_system_rows
tsm_system_time
unaccent
uuid-ossp

So attached is a patch to do that.  The code changes are trivial; just
add "trusted = true" to each control file.  We don't need to bump the
module version numbers, since this doesn't change the contents of any
extension, just who can install it.  I do not think any regression
test changes are needed either.  (Note that commit 50fc694e4 already
added a test that trusted extensions behave as expected, see
src/pl/plperl/sql/plperl_setup.sql.)  So it seems like the only thing
that needs much discussion is the documentation changes.  I adjusted
contrib.sgml's discussion of how to install these modules in general,
and then labeled the individual modules if they are trusted.

            regards, tom lane

diff --git a/contrib/btree_gin/btree_gin.control b/contrib/btree_gin/btree_gin.control
index d576da7..67d0c99 100644
--- a/contrib/btree_gin/btree_gin.control
+++ b/contrib/btree_gin/btree_gin.control
@@ -3,3 +3,4 @@ comment = 'support for indexing common datatypes in GIN'
 default_version = '1.3'
 module_pathname = '$libdir/btree_gin'
 relocatable = true
+trusted = true
diff --git a/contrib/btree_gist/btree_gist.control b/contrib/btree_gist/btree_gist.control
index 81c8509..cd2d7eb 100644
--- a/contrib/btree_gist/btree_gist.control
+++ b/contrib/btree_gist/btree_gist.control
@@ -3,3 +3,4 @@ comment = 'support for indexing common datatypes in GiST'
 default_version = '1.5'
 module_pathname = '$libdir/btree_gist'
 relocatable = true
+trusted = true
diff --git a/contrib/citext/citext.control b/contrib/citext/citext.control
index a872a3f..ccf4454 100644
--- a/contrib/citext/citext.control
+++ b/contrib/citext/citext.control
@@ -3,3 +3,4 @@ comment = 'data type for case-insensitive character strings'
 default_version = '1.6'
 module_pathname = '$libdir/citext'
 relocatable = true
+trusted = true
diff --git a/contrib/cube/cube.control b/contrib/cube/cube.control
index f39a838..3e238fc 100644
--- a/contrib/cube/cube.control
+++ b/contrib/cube/cube.control
@@ -3,3 +3,4 @@ comment = 'data type for multidimensional cubes'
 default_version = '1.4'
 module_pathname = '$libdir/cube'
 relocatable = true
+trusted = true
diff --git a/contrib/dict_int/dict_int.control b/contrib/dict_int/dict_int.control
index 6e2d2b3..ec04cce 100644
--- a/contrib/dict_int/dict_int.control
+++ b/contrib/dict_int/dict_int.control
@@ -3,3 +3,4 @@ comment = 'text search dictionary template for integers'
 default_version = '1.0'
 module_pathname = '$libdir/dict_int'
 relocatable = true
+trusted = true
diff --git a/contrib/earthdistance/earthdistance.control b/contrib/earthdistance/earthdistance.control
index 5816d22..3df666d 100644
--- a/contrib/earthdistance/earthdistance.control
+++ b/contrib/earthdistance/earthdistance.control
@@ -3,4 +3,5 @@ comment = 'calculate great-circle distances on the surface of the Earth'
 default_version = '1.1'
 module_pathname = '$libdir/earthdistance'
 relocatable = true
+trusted = true
 requires = 'cube'
diff --git a/contrib/fuzzystrmatch/fuzzystrmatch.control b/contrib/fuzzystrmatch/fuzzystrmatch.control
index 6b2832a..3cd6660 100644
--- a/contrib/fuzzystrmatch/fuzzystrmatch.control
+++ b/contrib/fuzzystrmatch/fuzzystrmatch.control
@@ -3,3 +3,4 @@ comment = 'determine similarities and distance between strings'
 default_version = '1.1'
 module_pathname = '$libdir/fuzzystrmatch'
 relocatable = true
+trusted = true
diff --git a/contrib/hstore/hstore.control b/contrib/hstore/hstore.control
index 93688cd..e0fbb8b 100644
--- a/contrib/hstore/hstore.control
+++ b/contrib/hstore/hstore.control
@@ -3,3 +3,4 @@ comment = 'data type for storing sets of (key, value) pairs'
 default_version = '1.6'
 module_pathname = '$libdir/hstore'
 relocatable = true
+trusted = true
diff --git a/contrib/hstore_plperl/hstore_plperl.control b/contrib/hstore_plperl/hstore_plperl.control
index 16277f6..4b9fd13 100644
--- a/contrib/hstore_plperl/hstore_plperl.control
+++ b/contrib/hstore_plperl/hstore_plperl.control
@@ -3,4 +3,5 @@ comment = 'transform between hstore and plperl'
 default_version = '1.0'
 module_pathname = '$libdir/hstore_plperl'
 relocatable = true
+trusted = true
 requires = 'hstore,plperl'
diff --git a/contrib/intarray/intarray.control b/contrib/intarray/intarray.control
index 7e50cc3..bf28804 100644
--- a/contrib/intarray/intarray.control
+++ b/contrib/intarray/intarray.control
@@ -3,3 +3,4 @@ comment = 'functions, operators, and index support for 1-D arrays of integers'
 default_version = '1.2'
 module_pathname = '$libdir/_int'
 relocatable = true
+trusted = true
diff --git a/contrib/isn/isn.control b/contrib/isn/isn.control
index 765dce0..1cb5e2b 100644
--- a/contrib/isn/isn.control
+++ b/contrib/isn/isn.control
@@ -3,3 +3,4 @@ comment = 'data types for international product numbering standards'
 default_version = '1.2'
 module_pathname = '$libdir/isn'
 relocatable = true
+trusted = true
diff --git a/contrib/jsonb_plperl/jsonb_plperl.control b/contrib/jsonb_plperl/jsonb_plperl.control
index 26c86a7..4acee93 100644
--- a/contrib/jsonb_plperl/jsonb_plperl.control
+++ b/contrib/jsonb_plperl/jsonb_plperl.control
@@ -3,4 +3,5 @@ comment = 'transform between jsonb and plperl'
 default_version = '1.0'
 module_pathname = '$libdir/jsonb_plperl'
 relocatable = true
+trusted = true
 requires = 'plperl'
diff --git a/contrib/lo/lo.control b/contrib/lo/lo.control
index 820326c..f73f8b5 100644
--- a/contrib/lo/lo.control
+++ b/contrib/lo/lo.control
@@ -3,3 +3,4 @@ comment = 'Large Object maintenance'
 default_version = '1.1'
 module_pathname = '$libdir/lo'
 relocatable = true
+trusted = true
diff --git a/contrib/ltree/ltree.control b/contrib/ltree/ltree.control
index 03c3fb1..3118df6 100644
--- a/contrib/ltree/ltree.control
+++ b/contrib/ltree/ltree.control
@@ -3,3 +3,4 @@ comment = 'data type for hierarchical tree-like structures'
 default_version = '1.1'
 module_pathname = '$libdir/ltree'
 relocatable = true
+trusted = true
diff --git a/contrib/pg_trgm/pg_trgm.control b/contrib/pg_trgm/pg_trgm.control
index 3e325dd..831ba23 100644
--- a/contrib/pg_trgm/pg_trgm.control
+++ b/contrib/pg_trgm/pg_trgm.control
@@ -3,3 +3,4 @@ comment = 'text similarity measurement and index searching based on trigrams'
 default_version = '1.4'
 module_pathname = '$libdir/pg_trgm'
 relocatable = true
+trusted = true
diff --git a/contrib/pgcrypto/pgcrypto.control b/contrib/pgcrypto/pgcrypto.control
index 5839832..d2151d3 100644
--- a/contrib/pgcrypto/pgcrypto.control
+++ b/contrib/pgcrypto/pgcrypto.control
@@ -3,3 +3,4 @@ comment = 'cryptographic functions'
 default_version = '1.3'
 module_pathname = '$libdir/pgcrypto'
 relocatable = true
+trusted = true
diff --git a/contrib/seg/seg.control b/contrib/seg/seg.control
index d697cd6..9ac3080 100644
--- a/contrib/seg/seg.control
+++ b/contrib/seg/seg.control
@@ -3,3 +3,4 @@ comment = 'data type for representing line segments or floating-point intervals'
 default_version = '1.3'
 module_pathname = '$libdir/seg'
 relocatable = true
+trusted = true
diff --git a/contrib/tablefunc/tablefunc.control b/contrib/tablefunc/tablefunc.control
index 248b0a7..7b25d16 100644
--- a/contrib/tablefunc/tablefunc.control
+++ b/contrib/tablefunc/tablefunc.control
@@ -3,3 +3,4 @@ comment = 'functions that manipulate whole tables, including crosstab'
 default_version = '1.0'
 module_pathname = '$libdir/tablefunc'
 relocatable = true
+trusted = true
diff --git a/contrib/tcn/tcn.control b/contrib/tcn/tcn.control
index 8abfd19..6972e11 100644
--- a/contrib/tcn/tcn.control
+++ b/contrib/tcn/tcn.control
@@ -3,3 +3,4 @@ comment = 'Triggered change notifications'
 default_version = '1.0'
 module_pathname = '$libdir/tcn'
 relocatable = true
+trusted = true
diff --git a/contrib/tsm_system_rows/tsm_system_rows.control b/contrib/tsm_system_rows/tsm_system_rows.control
index 4bd0232..b495fb1 100644
--- a/contrib/tsm_system_rows/tsm_system_rows.control
+++ b/contrib/tsm_system_rows/tsm_system_rows.control
@@ -3,3 +3,4 @@ comment = 'TABLESAMPLE method which accepts number of rows as a limit'
 default_version = '1.0'
 module_pathname = '$libdir/tsm_system_rows'
 relocatable = true
+trusted = true
diff --git a/contrib/tsm_system_time/tsm_system_time.control b/contrib/tsm_system_time/tsm_system_time.control
index c247987..b1b9789 100644
--- a/contrib/tsm_system_time/tsm_system_time.control
+++ b/contrib/tsm_system_time/tsm_system_time.control
@@ -3,3 +3,4 @@ comment = 'TABLESAMPLE method which accepts time in milliseconds as a limit'
 default_version = '1.0'
 module_pathname = '$libdir/tsm_system_time'
 relocatable = true
+trusted = true
diff --git a/contrib/unaccent/unaccent.control b/contrib/unaccent/unaccent.control
index a77a65f..649cf68 100644
--- a/contrib/unaccent/unaccent.control
+++ b/contrib/unaccent/unaccent.control
@@ -3,3 +3,4 @@ comment = 'text search dictionary that removes accents'
 default_version = '1.1'
 module_pathname = '$libdir/unaccent'
 relocatable = true
+trusted = true
diff --git a/contrib/uuid-ossp/uuid-ossp.control b/contrib/uuid-ossp/uuid-ossp.control
index 657476c..142a99e 100644
--- a/contrib/uuid-ossp/uuid-ossp.control
+++ b/contrib/uuid-ossp/uuid-ossp.control
@@ -3,3 +3,4 @@ comment = 'generate universally unique identifiers (UUIDs)'
 default_version = '1.1'
 module_pathname = '$libdir/uuid-ossp'
 relocatable = true
+trusted = true
diff --git a/doc/src/sgml/btree-gin.sgml b/doc/src/sgml/btree-gin.sgml
index 314e001..5bc5a05 100644
--- a/doc/src/sgml/btree-gin.sgml
+++ b/doc/src/sgml/btree-gin.sgml
@@ -32,6 +32,12 @@
   two separate indexes that would have to be combined via bitmap ANDing.
  </para>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title>Example Usage</title>

diff --git a/doc/src/sgml/btree-gist.sgml b/doc/src/sgml/btree-gist.sgml
index 774442f..3b61d27 100644
--- a/doc/src/sgml/btree-gist.sgml
+++ b/doc/src/sgml/btree-gist.sgml
@@ -52,6 +52,12 @@
   <type>oid</type>, and <type>money</type>.
  </para>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title>Example Usage</title>

diff --git a/doc/src/sgml/citext.sgml b/doc/src/sgml/citext.sgml
index 85aa339..667824f 100644
--- a/doc/src/sgml/citext.sgml
+++ b/doc/src/sgml/citext.sgml
@@ -24,6 +24,12 @@
   </para>
  </tip>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title>Rationale</title>

diff --git a/doc/src/sgml/contrib.sgml b/doc/src/sgml/contrib.sgml
index b626a34..08bb110 100644
--- a/doc/src/sgml/contrib.sgml
+++ b/doc/src/sgml/contrib.sgml
@@ -54,7 +54,7 @@
   Many modules supply new user-defined functions, operators, or types.
   To make use of one of these modules, after you have installed the code
   you need to register the new SQL objects in the database system.
-  In <productname>PostgreSQL</productname> 9.1 and later, this is done by executing
+  This is done by executing
   a <xref linkend="sql-createextension"/> command.  In a fresh database,
   you can simply do

@@ -62,15 +62,24 @@
 CREATE EXTENSION <replaceable>module_name</replaceable>;
 </programlisting>

-  This command must be run by a database superuser.  This registers the
-  new SQL objects in the current database only, so you need to run this
-  command in each database that you want
+  This command registers the new SQL objects in the current database only,
+  so you need to run it in each database that you want
   the module's facilities to be available in.  Alternatively, run it in
   database <literal>template1</literal> so that the extension will be copied into
   subsequently-created databases by default.
  </para>

  <para>
+  For all these modules, <command>CREATE EXTENSION</command> must be run
+  by a database superuser, unless the module is
+  considered <quote>trusted</quote>, in which case it can be run by any
+  user who has <literal>CREATE</literal> privilege on the current
+  database.  Modules that are trusted are identified as such in the
+  sections that follow.  Generally, trusted modules are ones that cannot
+  provide access to outside-the-database functionality.
+ </para>
+
+ <para>
   Many modules allow you to install their objects in a schema of your
   choice.  To do that, add <literal>SCHEMA
   <replaceable>schema_name</replaceable></literal> to the <command>CREATE EXTENSION</command>
diff --git a/doc/src/sgml/cube.sgml b/doc/src/sgml/cube.sgml
index c6e5862..71772d7 100644
--- a/doc/src/sgml/cube.sgml
+++ b/doc/src/sgml/cube.sgml
@@ -12,6 +12,12 @@
   representing multidimensional cubes.
  </para>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title>Syntax</title>

diff --git a/doc/src/sgml/dict-int.sgml b/doc/src/sgml/dict-int.sgml
index c15cbd0..b556f1b 100644
--- a/doc/src/sgml/dict-int.sgml
+++ b/doc/src/sgml/dict-int.sgml
@@ -15,6 +15,12 @@
   unique words, which greatly affects the performance of searching.
  </para>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title>Configuration</title>

diff --git a/doc/src/sgml/earthdistance.sgml b/doc/src/sgml/earthdistance.sgml
index 670fc99..7ca2c40 100644
--- a/doc/src/sgml/earthdistance.sgml
+++ b/doc/src/sgml/earthdistance.sgml
@@ -23,6 +23,12 @@
   project.)
  </para>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title>Cube-Based Earth Distances</title>

diff --git a/doc/src/sgml/fuzzystrmatch.sgml b/doc/src/sgml/fuzzystrmatch.sgml
index 373ac48..382e54b 100644
--- a/doc/src/sgml/fuzzystrmatch.sgml
+++ b/doc/src/sgml/fuzzystrmatch.sgml
@@ -20,6 +20,12 @@
   </para>
  </caution>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title>Soundex</title>

diff --git a/doc/src/sgml/hstore.sgml b/doc/src/sgml/hstore.sgml
index 94ccd12..64c2477 100644
--- a/doc/src/sgml/hstore.sgml
+++ b/doc/src/sgml/hstore.sgml
@@ -15,6 +15,12 @@
   simply text strings.
  </para>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title><type>hstore</type> External Representation</title>

@@ -633,6 +639,11 @@ ALTER TABLE tablename ALTER hstorecol TYPE hstore USING hstorecol || '';
    convention).  If you use them, <type>hstore</type> values are mapped to
    Python dictionaries.
   </para>
+
+  <para>
+   Of these additional extensions, <literal>hstore_plperl</literal> is
+   considered trusted; the rest are not.
+  </para>
  </sect2>

  <sect2>
diff --git a/doc/src/sgml/intarray.sgml b/doc/src/sgml/intarray.sgml
index b633cf3..025cbca 100644
--- a/doc/src/sgml/intarray.sgml
+++ b/doc/src/sgml/intarray.sgml
@@ -24,6 +24,12 @@
   treated as though it were a linear array in storage order.
  </para>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title><filename>intarray</filename> Functions and Operators</title>

diff --git a/doc/src/sgml/isn.sgml b/doc/src/sgml/isn.sgml
index 2117454..6c61f14 100644
--- a/doc/src/sgml/isn.sgml
+++ b/doc/src/sgml/isn.sgml
@@ -21,6 +21,12 @@
   dropped from a future version of this module.
  </para>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title>Data Types</title>

diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml
index 6ff8751..1b6aaf0 100644
--- a/doc/src/sgml/json.sgml
+++ b/doc/src/sgml/json.sgml
@@ -622,6 +622,13 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu
    use them, <type>jsonb</type> values are mapped to Python dictionaries,
    lists, and scalars, as appropriate.
   </para>
+
+  <para>
+   Of these extensions, <literal>jsonb_plperl</literal> is
+   considered <quote>trusted</quote>, that is, it can be installed by
+   non-superusers who have <literal>CREATE</literal> privilege on the
+   current database.  The rest require superuser privilege to install.
+  </para>
  </sect2>

  <sect2 id="datatype-jsonpath">
diff --git a/doc/src/sgml/lo.sgml b/doc/src/sgml/lo.sgml
index cce3793..0a4f2e4 100644
--- a/doc/src/sgml/lo.sgml
+++ b/doc/src/sgml/lo.sgml
@@ -13,6 +13,12 @@
   and a trigger <function>lo_manage</function>.
  </para>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title>Rationale</title>

diff --git a/doc/src/sgml/ltree.sgml b/doc/src/sgml/ltree.sgml
index 3ddd335..b4e07f6 100644
--- a/doc/src/sgml/ltree.sgml
+++ b/doc/src/sgml/ltree.sgml
@@ -13,6 +13,12 @@
   Extensive facilities for searching through label trees are provided.
  </para>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title>Definitions</title>

diff --git a/doc/src/sgml/pgcrypto.sgml b/doc/src/sgml/pgcrypto.sgml
index 0acd11e..cc916ff 100644
--- a/doc/src/sgml/pgcrypto.sgml
+++ b/doc/src/sgml/pgcrypto.sgml
@@ -17,6 +17,12 @@
   <productname>PostgreSQL</productname>.
  </para>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title>General Hashing Functions</title>

diff --git a/doc/src/sgml/pgtrgm.sgml b/doc/src/sgml/pgtrgm.sgml
index 3e6fd73..049f496 100644
--- a/doc/src/sgml/pgtrgm.sgml
+++ b/doc/src/sgml/pgtrgm.sgml
@@ -15,6 +15,12 @@
   strings.
  </para>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title>Trigram (or Trigraph) Concepts</title>

diff --git a/doc/src/sgml/seg.sgml b/doc/src/sgml/seg.sgml
index d07329f..2492de9 100644
--- a/doc/src/sgml/seg.sgml
+++ b/doc/src/sgml/seg.sgml
@@ -14,6 +14,12 @@
   making it especially useful for representing laboratory measurements.
  </para>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title>Rationale</title>

diff --git a/doc/src/sgml/tablefunc.sgml b/doc/src/sgml/tablefunc.sgml
index 007e9c6..ad435d6 100644
--- a/doc/src/sgml/tablefunc.sgml
+++ b/doc/src/sgml/tablefunc.sgml
@@ -14,6 +14,12 @@
   multiple rows.
  </para>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title>Functions Provided</title>

diff --git a/doc/src/sgml/tcn.sgml b/doc/src/sgml/tcn.sgml
index aa2fe4f..82afe9a 100644
--- a/doc/src/sgml/tcn.sgml
+++ b/doc/src/sgml/tcn.sgml
@@ -18,6 +18,12 @@
  </para>

  <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
+ <para>
   Only one parameter may be supplied to the function in a
   <literal>CREATE TRIGGER</literal> statement, and that is optional.  If supplied
   it will be used for the channel name for the notifications.  If omitted
diff --git a/doc/src/sgml/tsm-system-rows.sgml b/doc/src/sgml/tsm-system-rows.sgml
index 3dcd948..071ff30 100644
--- a/doc/src/sgml/tsm-system-rows.sgml
+++ b/doc/src/sgml/tsm-system-rows.sgml
@@ -33,6 +33,12 @@
   the <literal>REPEATABLE</literal> clause.
  </para>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title>Examples</title>

diff --git a/doc/src/sgml/tsm-system-time.sgml b/doc/src/sgml/tsm-system-time.sgml
index fd8e999..cd07492 100644
--- a/doc/src/sgml/tsm-system-time.sgml
+++ b/doc/src/sgml/tsm-system-time.sgml
@@ -35,6 +35,12 @@
   the <literal>REPEATABLE</literal> clause.
  </para>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title>Examples</title>

diff --git a/doc/src/sgml/unaccent.sgml b/doc/src/sgml/unaccent.sgml
index 547ac54..5cd716a 100644
--- a/doc/src/sgml/unaccent.sgml
+++ b/doc/src/sgml/unaccent.sgml
@@ -21,6 +21,12 @@
   normalizing dictionary for the <filename>thesaurus</filename> dictionary.
  </para>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title>Configuration</title>

diff --git a/doc/src/sgml/uuid-ossp.sgml b/doc/src/sgml/uuid-ossp.sgml
index 0fbabbf..54d7813 100644
--- a/doc/src/sgml/uuid-ossp.sgml
+++ b/doc/src/sgml/uuid-ossp.sgml
@@ -16,6 +16,12 @@
   linkend="functions-uuid"/> for built-in ways to generate UUIDs.
  </para>

+ <para>
+  This module is considered <quote>trusted</quote>, that is, it can be
+  installed by non-superusers who have <literal>CREATE</literal> privilege
+  on the current database.
+ </para>
+
  <sect2>
   <title><literal>uuid-ossp</literal> Functions</title>

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Julien Rouhaud <rjuju123@gmail.com> writes:
> >>> Probably NO, if only because you'd need additional privileges
> >>> to use these anyway:
> >>> pg_stat_statements
>
> > But the additional privileges are global, so assuming the extension
> > has been properly setup, wouldn't it be sensible to ease the
> > per-database installation?  If not properly setup, there's no harm in
> > creating the extension anyway.
>
> Mmm, I'm not convinced --- the ability to see what statements are being
> executed in other sessions (even other databases) is something that
> paranoid installations might not be so happy about.

Of course, but that's why we have a default role which allows
installations to control access to that kind of information- and it's
already being checked in the pg_stat_statements case and in the
pg_stat_activity case:

/* Superusers or members of pg_read_all_stats members are allowed */
is_allowed_role = is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_STATS);

> Our previous
> discussions about what privilege level is needed to look at
> pg_stat_statements info were all made against a background assumption
> that you needed some extra privilege to set up the view in the first
> place.  I think that would need another look or two before being
> comfortable that we're not shifting the goal posts too far.

While you could maybe argue that's true for pg_stat_statements, it's
certainly not true for pg_stat_activity, so I don't buy it for either.
This looks like revisionist history to justify paranoia.  I understand
the general concern, but if we were really depending on the mere
installation of the extension to provide security then we wouldn't have
bothered putting in checks like the one above, and, worse, I think our
users would be routinely complaining that our extensions don't follow
our security model and how they can't install them.

Lots of people want to use pg_stat_statements, even in environments
where not everyone on the database server, or even in the database you
want pg_stat_statements in, is trusted, and therefore we have to have
these additional checks inside the extension itself.

The same goes for just about everything else (I sure hope, at least) in
our extensions set- none of the core extensions should be allowing
access to things which break our security model, even if they're
installed, unless some additional privileges are granted out.  The act
of installing a core extension should not create a security risk for our
users- if it did, it'd be a security issue and CVE-worthy.

As such, I really don't agree with this entire line of thinking when it
comes to our core extensions.  I view the 'trusted extension' model as
really for things where the extension author doesn't care about, and
doesn't wish to care about, dealing with our security model and making
sure that it's followed.  We do care, and we do maintain, the security
model that we have throughout the core extensions.

What I expect and hope will happen is that people will realize that, now
that they can have non-superusers installing these extensions and
therefore they don't have to give out superuser-level rights as much,
there will be asks for more default roles to allow granting out of
access to formerly superuser-only capabilities.  There's a bit of a
complication there since there might be privileges that only make sense
for a specific extension, but an extension can't really install a new
default role (and, even if it did, the role would have to be only
available to the superuser initially anyway), so we might have to try
and come up with some more generic and reusable default role for that
case.  Still, we can try to deal with that when it happens.

Consider that you may wish to have a system that, once installed, a
superuser will virtually never access again, but one where you want
users to be able to install and use extensions like postgis and
pg_stat_statements.  That can be done with these changes, and that's
fantastic progress- you just install PG, create a non-superuser role,
make them the DB owner, and then GRANT things like pg_read_all_stats to
their role with admin rights, and boom, they're good to go and you
didn't have to hack up the PG source code at all.

> The bigger picture here is that I don't want to get push-back that
> we've broken somebody's security posture by marking too many extensions
> trusted.  So for anything where there's any question about security
> implications, we should err in the conservative direction of leaving
> it untrusted.

This is just going to a) cause our users to complain about not being
able to install extensions that they've routinely installed in the past,
and b) make our users wonder what it is about these extensions that
we've decided can't be trusted to even just be installed and if they're
at risk today because they've installed them.

While it might not seem obvious, the discussion over on the thread about
DEFAULT PRIVILEGES and pg_init_privs is actually a lot more relevant
here- there's extensions we have that expect certain functions, once
installed, to be owned by a superuser (which will still be the case
here, thanks to how you've addressed that), but then to not have EXECUTE
rights GRANT'd to anyone (thanks to the REVERT FROM PUBLIC in the
installation), but that falls apart when someone's decided to set
up DEFAULT PRIVILEGES for the superuser.  While no one seems to want to
discuss that with me, unfortunately, it's becoming more and more clear
that we need to skip DEFAULT PRIVILEGES from being applied during
extension creation.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Our previous
>> discussions about what privilege level is needed to look at
>> pg_stat_statements info were all made against a background assumption
>> that you needed some extra privilege to set up the view in the first
>> place.  I think that would need another look or two before being
>> comfortable that we're not shifting the goal posts too far.

> While you could maybe argue that's true for pg_stat_statements, it's
> certainly not true for pg_stat_activity, so I don't buy it for either.

The analogy of pg_stat_activity certainly suggests that there shouldn't be
a reason *in principle* why pg_stat_statements couldn't be made trusted.
There's a difference between that statement and saying that *in practice*
pg_stat_statements is ready to be trusted right now with no further
changes.  I haven't done the analysis needed to conclude that, and don't
care to do so as part of this patch proposal.

> The same goes for just about everything else (I sure hope, at least) in
> our extensions set- none of the core extensions should be allowing
> access to things which break our security model, even if they're
> installed, unless some additional privileges are granted out.

Maybe not, but the principle of defense-in-depth still says that admins
could reasonably want to not let dangerous tools get installed in the
first place.

> As such, I really don't agree with this entire line of thinking when it
> comes to our core extensions.  I view the 'trusted extension' model as
> really for things where the extension author doesn't care about, and
> doesn't wish to care about, dealing with our security model and making
> sure that it's followed.  We do care, and we do maintain, the security
> model that we have throughout the core extensions.  

I am confused as to what "entire line of thinking" you are objecting
to.  Are you now saying that we should forget the trusted-extension
model?  Or maybe that we can just mark *everything* we ship as trusted?
I'm not going to agree with either.

>> The bigger picture here is that I don't want to get push-back that
>> we've broken somebody's security posture by marking too many extensions
>> trusted.  So for anything where there's any question about security
>> implications, we should err in the conservative direction of leaving
>> it untrusted.

> This is just going to a) cause our users to complain about not being
> able to install extensions that they've routinely installed in the past,

That's utter nonsense.  Nothing here is taking away privileges that
existed before; if you could install $whatever as superuser before,
you still can.  OTOH, we *would* have a problem of that sort if we
marked $whatever as trusted and then later had to undo it.  So I
think there's plenty of reason to be conservative about the first
wave of what-to-mark-as-trusted.  Once we've got more experience
with this mechanism under our belts, we might decide we can be more
liberal about it.

> and b) make our users wonder what it is about these extensions that
> we've decided can't be trusted to even just be installed and if they're
> at risk today because they've installed them.

Yep, you're right, this patch does make value judgements of that
sort, and I'll stand behind them.  Giving people the impression that,
say, postgres_fdw isn't any more dangerous than cube isn't helpful.

> While it might not seem obvious, the discussion over on the thread about
> DEFAULT PRIVILEGES and pg_init_privs is actually a lot more relevant
> here- there's extensions we have that expect certain functions, once
> installed, to be owned by a superuser (which will still be the case
> here, thanks to how you've addressed that), but then to not have EXECUTE
> rights GRANT'd to anyone (thanks to the REVERT FROM PUBLIC in the
> installation), but that falls apart when someone's decided to set
> up DEFAULT PRIVILEGES for the superuser.  While no one seems to want to
> discuss that with me, unfortunately, it's becoming more and more clear
> that we need to skip DEFAULT PRIVILEGES from being applied during
> extension creation.

Or that we can't let people apply default privileges to superuser-created
objects at all.  But I agree that that's a different discussion.

            regards, tom lane

Hi,

On 2020-01-29 14:41:16 -0500, Tom Lane wrote:
> pgcrypto

FWIW, given the code quality, I'm doubtful about putting itq into the trusted
section.


Have you audited how safe the create/upgrade scripts are against being
used to elevate privileges?

Especially with FROM UNPACKAGED it seems like it'd be fairly easy to get
an extension script to do dangerous things (as superuser). One could
just create pre-existing objects that have *not* been created by a
previous version, and some upgrade scripts would do pretty weird
stuff. There's several that do things like updating catalogs directly
etc.  It seems to me that FROM UNPACKAGED shouldn't support trusted.

Regards,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2020-01-29 14:41:16 -0500, Tom Lane wrote:
>> pgcrypto

> FWIW, given the code quality, I'm doubtful about putting itq into the trusted
> section.

I don't particularly have an opinion about that --- is it really that
awful?  If there is anything broken in it, wouldn't we consider that
a security problem anyhow?

> Especially with FROM UNPACKAGED it seems like it'd be fairly easy to get
> an extension script to do dangerous things (as superuser). One could
> just create pre-existing objects that have *not* been created by a
> previous version, and some upgrade scripts would do pretty weird
> stuff. There's several that do things like updating catalogs directly
> etc.  It seems to me that FROM UNPACKAGED shouldn't support trusted.

Hmm, seems like a reasonable idea, but I'm not quite sure how to mechanize
it given that "unpackaged" isn't magic in any way so far as extension.c
is concerned.  Maybe we could decide that the time for supporting easy
updates from pre-9.1 is past, and just remove all the unpackaged-to-XXX
scripts?  Maybe even remove the "FROM version" option altogether.

            regards, tom lane

I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> It seems to me that FROM UNPACKAGED shouldn't support trusted.

> Hmm, seems like a reasonable idea, but I'm not quite sure how to mechanize
> it given that "unpackaged" isn't magic in any way so far as extension.c
> is concerned.  Maybe we could decide that the time for supporting easy
> updates from pre-9.1 is past, and just remove all the unpackaged-to-XXX
> scripts?  Maybe even remove the "FROM version" option altogether.

[ thinks some more... ]  A less invasive idea would be to insist that
you be superuser to use the FROM option.  But I'm thinking that the
unpackaged-to-XXX scripts are pretty much dead letters anymore.  Has
anyone even tested them in years?  How much longer do we want to be
on the hook to fix them?

            regards, tom lane

Hi,

On 2020-02-13 18:57:10 -0500, Tom Lane wrote:
> Maybe we could decide that the time for supporting easy updates from
> pre-9.1 is past, and just remove all the unpackaged-to-XXX scripts?
> Maybe even remove the "FROM version" option altogether.

Yea, that strikes me as a reasonable thing to do. These days that just
seems to be dangerous, without much advantage.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2020-02-13 18:57:10 -0500, Tom Lane wrote:
>> Maybe we could decide that the time for supporting easy updates from
>> pre-9.1 is past, and just remove all the unpackaged-to-XXX scripts?
>> Maybe even remove the "FROM version" option altogether.

> Yea, that strikes me as a reasonable thing to do. These days that just
> seems to be dangerous, without much advantage.

Here's a patch to remove the core-code support and documentation for
that.  I have not included the actual deletion of the contrib modules'
'unpackaged' scripts, as that seems both long and boring.

            regards, tom lane

diff --git a/doc/src/sgml/contrib.sgml b/doc/src/sgml/contrib.sgml
index 08bb110..261a559 100644
--- a/doc/src/sgml/contrib.sgml
+++ b/doc/src/sgml/contrib.sgml
@@ -88,22 +88,6 @@ CREATE EXTENSION <replaceable>module_name</replaceable>;
  </para>

  <para>
-  If your database was brought forward by dump and reload from a pre-9.1
-  version of <productname>PostgreSQL</productname>, and you had been using the pre-9.1
-  version of the module in it, you should instead do
-
-<programlisting>
-CREATE EXTENSION <replaceable>module_name</replaceable> FROM unpackaged;
-</programlisting>
-
-  This will update the pre-9.1 objects of the module into a proper
-  <firstterm>extension</firstterm> object.  Future updates to the module will be
-  managed by <xref linkend="sql-alterextension"/>.
-  For more information about extension updates, see
-  <xref linkend="extend-extensions"/>.
- </para>
-
- <para>
   Note, however, that some of these modules are not <quote>extensions</quote>
   in this sense, but are loaded into the server in some other way, for instance
   by way of
diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml
index ffe068b..9ec1af7 100644
--- a/doc/src/sgml/extend.sgml
+++ b/doc/src/sgml/extend.sgml
@@ -917,33 +917,6 @@ SELECT pg_catalog.pg_extension_config_dump('my_config', 'WHERE NOT standard_entr
     </para>

     <para>
-     The update mechanism can be used to solve an important special case:
-     converting a <quote>loose</quote> collection of objects into an extension.
-     Before the extension mechanism was added to
-     <productname>PostgreSQL</productname> (in 9.1), many people wrote
-     extension modules that simply created assorted unpackaged objects.
-     Given an existing database containing such objects, how can we convert
-     the objects into a properly packaged extension?  Dropping them and then
-     doing a plain <command>CREATE EXTENSION</command> is one way, but it's not
-     desirable if the objects have dependencies (for example, if there are
-     table columns of a data type created by the extension).  The way to fix
-     this situation is to create an empty extension, then use <command>ALTER
-     EXTENSION ADD</command> to attach each pre-existing object to the extension,
-     then finally create any new objects that are in the current extension
-     version but were not in the unpackaged release.  <command>CREATE
-     EXTENSION</command> supports this case with its <literal>FROM</literal> <replaceable
-     class="parameter">old_version</replaceable> option, which causes it to not run the
-     normal installation script for the target version, but instead the update
-     script named
-
<literal><replaceable>extension</replaceable>--<replaceable>old_version</replaceable>--<replaceable>target_version</replaceable>.sql</literal>.
-     The choice of the dummy version name to use as <replaceable
-     class="parameter">old_version</replaceable> is up to the extension author, though
-     <literal>unpackaged</literal> is a common convention.  If you have multiple
-     prior versions you need to be able to update into extension style, use
-     multiple dummy version names to identify them.
-    </para>
-
-    <para>
      <command>ALTER EXTENSION</command> is able to execute sequences of update
      script files to achieve a requested update.  For example, if only
      <literal>foo--1.0--1.1.sql</literal> and <literal>foo--1.1--2.0.sql</literal> are
diff --git a/doc/src/sgml/ref/create_extension.sgml b/doc/src/sgml/ref/create_extension.sgml
index d76ac3e..6a21bff 100644
--- a/doc/src/sgml/ref/create_extension.sgml
+++ b/doc/src/sgml/ref/create_extension.sgml
@@ -24,7 +24,6 @@ PostgreSQL documentation
 CREATE EXTENSION [ IF NOT EXISTS ] <replaceable class="parameter">extension_name</replaceable>
     [ WITH ] [ SCHEMA <replaceable class="parameter">schema_name</replaceable> ]
              [ VERSION <replaceable class="parameter">version</replaceable> ]
-             [ FROM <replaceable class="parameter">old_version</replaceable> ]
              [ CASCADE ]
 </synopsis>
  </refsynopsisdiv>
@@ -48,8 +47,9 @@ CREATE EXTENSION [ IF NOT EXISTS ] <replaceable class="parameter">extension_name

   <para>
    The user who runs <command>CREATE EXTENSION</command> becomes the
-   owner of the extension for purposes of later privilege checks, as well
-   as the owner of any objects created by the extension's script.
+   owner of the extension for purposes of later privilege checks, and
+   normally also becomes the owner of any objects created by the
+   extension's script.
   </para>

   <para>
@@ -142,33 +142,6 @@ CREATE EXTENSION [ IF NOT EXISTS ] <replaceable class="parameter">extension_name
      </varlistentry>

      <varlistentry>
-      <term><replaceable class="parameter">old_version</replaceable></term>
-      <listitem>
-       <para>
-        <literal>FROM</literal> <replaceable class="parameter">old_version</replaceable>
-        must be specified when, and only when, you are attempting to install
-        an extension that replaces an <quote>old style</quote> module that is just
-        a collection of objects not packaged into an extension.  This option
-        causes <command>CREATE EXTENSION</command> to run an alternative installation
-        script that absorbs the existing objects into the extension, instead
-        of creating new objects.  Be careful that <literal>SCHEMA</literal> specifies
-        the schema containing these pre-existing objects.
-       </para>
-
-       <para>
-        The value to use for <replaceable
-        class="parameter">old_version</replaceable> is determined by the
-        extension's author, and might vary if there is more than one version
-        of the old-style module that can be upgraded into an extension.
-        For the standard additional modules supplied with pre-9.1
-        <productname>PostgreSQL</productname>, use <literal>unpackaged</literal>
-        for <replaceable class="parameter">old_version</replaceable> when
-        updating a module to extension style.
-       </para>
-      </listitem>
-     </varlistentry>
-
-     <varlistentry>
       <term><literal>CASCADE</literal></term>
       <listitem>
        <para>
@@ -220,16 +193,6 @@ CREATE EXTENSION [ IF NOT EXISTS ] <replaceable class="parameter">extension_name
 CREATE EXTENSION hstore;
 </programlisting>
   </para>
-
-  <para>
-   Update a pre-9.1 installation of <literal>hstore</literal> into
-   extension style:
-<programlisting>
-CREATE EXTENSION hstore SCHEMA public FROM unpackaged;
-</programlisting>
-   Be careful to specify the schema in which you installed the existing
-   <literal>hstore</literal> objects.
-  </para>
  </refsect1>

  <refsect1>
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index ddd46f4..a0db7db 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -1357,7 +1357,6 @@ static ObjectAddress
 CreateExtensionInternal(char *extensionName,
                         char *schemaName,
                         const char *versionName,
-                        const char *oldVersionName,
                         bool cascade,
                         List *parents,
                         bool is_create)
@@ -1367,6 +1366,8 @@ CreateExtensionInternal(char *extensionName,
     Oid            extowner = GetUserId();
     ExtensionControlFile *pcontrol;
     ExtensionControlFile *control;
+    char       *filename;
+    struct stat fst;
     List       *updateVersions;
     List       *requiredExtensions;
     List       *requiredSchemas;
@@ -1401,56 +1402,6 @@ CreateExtensionInternal(char *extensionName,
      * does what is needed, we try to find a sequence of update scripts that
      * will get us there.
      */
-    if (oldVersionName)
-    {
-        /*
-         * "FROM old_version" was specified, indicating that we're trying to
-         * update from some unpackaged version of the extension.  Locate a
-         * series of update scripts that will do it.
-         */
-        check_valid_version_name(oldVersionName);
-
-        if (strcmp(oldVersionName, versionName) == 0)
-            ereport(ERROR,
-                    (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                     errmsg("FROM version must be different from installation target version \"%s\"",
-                            versionName)));
-
-        updateVersions = identify_update_path(pcontrol,
-                                              oldVersionName,
-                                              versionName);
-
-        if (list_length(updateVersions) == 1)
-        {
-            /*
-             * Simple case where there's just one update script to run. We
-             * will not need any follow-on update steps.
-             */
-            Assert(strcmp((char *) linitial(updateVersions), versionName) == 0);
-            updateVersions = NIL;
-        }
-        else
-        {
-            /*
-             * Multi-step sequence.  We treat this as installing the version
-             * that is the target of the first script, followed by successive
-             * updates to the later versions.
-             */
-            versionName = (char *) linitial(updateVersions);
-            updateVersions = list_delete_first(updateVersions);
-        }
-    }
-    else
-    {
-        /*
-         * No FROM, so we're installing from scratch.  If there is an install
-         * script for the desired version, we only need to run that one.
-         */
-        char       *filename;
-        struct stat fst;
-
-        oldVersionName = NULL;
-
         filename = get_extension_script_filename(pcontrol, NULL, versionName);
         if (stat(filename, &fst) == 0)
         {
@@ -1484,7 +1435,6 @@ CreateExtensionInternal(char *extensionName,
             /* Otherwise, install best starting point and then upgrade */
             versionName = evi_start->name;
         }
-    }

     /*
      * Fetch control parameters for installation target version
@@ -1624,7 +1574,7 @@ CreateExtensionInternal(char *extensionName,
      * Execute the installation script file
      */
     execute_extension_script(extensionOid, control,
-                             oldVersionName, versionName,
+                             NULL, versionName,
                              requiredSchemas,
                              schemaName, schemaOid);

@@ -1691,7 +1641,6 @@ get_required_extension(char *reqExtensionName,
             addr = CreateExtensionInternal(reqExtensionName,
                                            origSchemaName,
                                            NULL,
-                                           NULL,
                                            cascade,
                                            cascade_parents,
                                            is_create);
@@ -1719,11 +1668,9 @@ CreateExtension(ParseState *pstate, CreateExtensionStmt *stmt)
 {
     DefElem    *d_schema = NULL;
     DefElem    *d_new_version = NULL;
-    DefElem    *d_old_version = NULL;
     DefElem    *d_cascade = NULL;
     char       *schemaName = NULL;
     char       *versionName = NULL;
-    char       *oldVersionName = NULL;
     bool        cascade = false;
     ListCell   *lc;

@@ -1787,16 +1734,6 @@ CreateExtension(ParseState *pstate, CreateExtensionStmt *stmt)
             d_new_version = defel;
             versionName = defGetString(d_new_version);
         }
-        else if (strcmp(defel->defname, "old_version") == 0)
-        {
-            if (d_old_version)
-                ereport(ERROR,
-                        (errcode(ERRCODE_SYNTAX_ERROR),
-                         errmsg("conflicting or redundant options"),
-                         parser_errposition(pstate, defel->location)));
-            d_old_version = defel;
-            oldVersionName = defGetString(d_old_version);
-        }
         else if (strcmp(defel->defname, "cascade") == 0)
         {
             if (d_cascade)
@@ -1815,7 +1752,6 @@ CreateExtension(ParseState *pstate, CreateExtensionStmt *stmt)
     return CreateExtensionInternal(stmt->extname,
                                    schemaName,
                                    versionName,
-                                   oldVersionName,
                                    cascade,
                                    NIL,
                                    true);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 1b0edf5..96e7fdb 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4460,7 +4460,7 @@ DropTableSpaceStmt: DROP TABLESPACE name
  *
  *        QUERY:
  *             CREATE EXTENSION extension
- *             [ WITH ] [ SCHEMA schema ] [ VERSION version ] [ FROM oldversion ]
+ *             [ WITH ] [ SCHEMA schema ] [ VERSION version ]
  *
  *****************************************************************************/

@@ -4500,7 +4500,10 @@ create_extension_opt_item:
                 }
             | FROM NonReservedWord_or_Sconst
                 {
-                    $$ = makeDefElem("old_version", (Node *)makeString($2), @1);
+                    ereport(ERROR,
+                            (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                             errmsg("CREATE EXTENSION ... FROM is no longer supported"),
+                             parser_errposition(@1)));
                 }
             | CASCADE
                 {

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2020-01-29 14:41:16 -0500, Tom Lane wrote:
> >> pgcrypto
>
> > FWIW, given the code quality, I'm doubtful about putting itq into the trusted
> > section.
>
> I don't particularly have an opinion about that --- is it really that
> awful?  If there is anything broken in it, wouldn't we consider that
> a security problem anyhow?

I would certainly hope so- and I would expect that to go for any of the
other extensions which are included in core.  If we aren't going to
maintain them and deal with security issues in them, then we should drop
them.

Which goes back to my earlier complaint that having extensions in core
which aren't or can't be marked as trusted is not a position we should
put our users in.  Either they're maintained and have been vetted
through our commit process, or they aren't and should be removed.

> > Especially with FROM UNPACKAGED it seems like it'd be fairly easy to get
> > an extension script to do dangerous things (as superuser). One could
> > just create pre-existing objects that have *not* been created by a
> > previous version, and some upgrade scripts would do pretty weird
> > stuff. There's several that do things like updating catalogs directly
> > etc.  It seems to me that FROM UNPACKAGED shouldn't support trusted.
>
> Hmm, seems like a reasonable idea, but I'm not quite sure how to mechanize
> it given that "unpackaged" isn't magic in any way so far as extension.c
> is concerned.  Maybe we could decide that the time for supporting easy
> updates from pre-9.1 is past, and just remove all the unpackaged-to-XXX
> scripts?  Maybe even remove the "FROM version" option altogether.

I agree in general with dropping the unpackaged-to-XXX bits.

Thanks,

Stephen

On Thu, Feb 13, 2020 at 07:09:18PM -0500, Tom Lane wrote:
> I wrote:
> > Andres Freund <andres@anarazel.de> writes:
> >> It seems to me that FROM UNPACKAGED shouldn't support trusted.
> 
> > Hmm, seems like a reasonable idea, but I'm not quite sure how to mechanize
> > it given that "unpackaged" isn't magic in any way so far as extension.c
> > is concerned.  Maybe we could decide that the time for supporting easy
> > updates from pre-9.1 is past, and just remove all the unpackaged-to-XXX
> > scripts?  Maybe even remove the "FROM version" option altogether.
> 
> [ thinks some more... ]  A less invasive idea would be to insist that
> you be superuser to use the FROM option.  But I'm thinking that the
> unpackaged-to-XXX scripts are pretty much dead letters anymore.  Has
> anyone even tested them in years?  How much longer do we want to be
> on the hook to fix them?

PostGIS uses unpackaged-to-XXX pretty heavily, and has it under
automated testing (which broke since "FROM unpackaged" support was
removed, see 14514.1581638958@sss.pgh.pa.us)

We'd be ok with requiring SUPERUSER for doing that, since that's
what is currently required so nothing would change for us.

Instead, dropping UPGRADE..FROM completely puts us in trouble of
having to find another way to "package" postgis objects.

--strk;

Hi,

On 2020-02-26 09:11:21 +0100, Sandro Santilli wrote:
> PostGIS uses unpackaged-to-XXX pretty heavily, and has it under
> automated testing (which broke since "FROM unpackaged" support was
> removed, see 14514.1581638958@sss.pgh.pa.us)
> 
> We'd be ok with requiring SUPERUSER for doing that, since that's
> what is currently required so nothing would change for us.
> 
> Instead, dropping UPGRADE..FROM completely puts us in trouble of
> having to find another way to "package" postgis objects.

Coul you explain what postgis is trying to achieve with FROM unpackaged?

Greetings,

Andres Freund

On Wed, Feb 26, 2020 at 12:17:37AM -0800, Andres Freund wrote:
> Hi,
> 
> On 2020-02-26 09:11:21 +0100, Sandro Santilli wrote:
> > PostGIS uses unpackaged-to-XXX pretty heavily, and has it under
> > automated testing (which broke since "FROM unpackaged" support was
> > removed, see 14514.1581638958@sss.pgh.pa.us)
> > 
> > We'd be ok with requiring SUPERUSER for doing that, since that's
> > what is currently required so nothing would change for us.
> > 
> > Instead, dropping UPGRADE..FROM completely puts us in trouble of
> > having to find another way to "package" postgis objects.
> 
> Coul you explain what postgis is trying to achieve with FROM unpackaged?

We're turning a non-extension based install into an extension-based
one. Common need for those who came to PostGIS way before EXTENSION
was even invented and for those who remained there for the bigger
flexibility (for example to avoid the raster component, which was
unavoidable with EXTENSION mechanism until PostGIS 3.0).

For the upgrades to 3.0.0 when coming from a previous version we're
using that `FROM unpackaged` SYNTAX for re-packaging the raster
component for those who still want it (raster objects are unpackaged
from 'postgis' extension on EXTENSION UPDATE because there was no other
way to move them from an extension to another).

I guess it would be ok for us to do the packaging directly from the
scripts that would run on `CREATE EXTENSION postgis`, but would that
mean we'd take the security risk you're trying to avoid by dropping
the `FROM unpackaged` syntax ?

--strk;