Apartment::Tenant.create fails with PG::DuplicateTable when PostgreSQL is upgraded to 9.5.12

aldrinmartoq commented 6 years ago

Steps to reproduce

Upgrade your postgresql database to 9.5.12
Setup Apartment to use_sql = true
Create a new Tenant with Apartment::Tenant.create('foo')

Expected behavior

It should create a new schema for the tenant

Actual behavior

Because of CVE-2018-1058, postgresql changed the way it pg_dump objects. Before it looked something like this:

# pg_dump postgresql 9.5.11
SET search_path = public, pg_catalog;
CREATE TABLE ahoy_events (
    id bigint NOT NULL,
    visit_id integer,
    user_id integer,
    name character varying,
    properties jsonb,
    "time" timestamp without time zone
);

Now the dumps look something like this:

# pg_dump postgresql 9.5.12
CREATE TABLE public.ahoy_events (
    id bigint NOT NULL,
    visit_id integer,
    user_id integer,
    name character varying,
    properties jsonb,
    "time" timestamp without time zone
);

IE: every object has the schema name prepended. This make the clone_pg_schema method to fail in the postgresql_adapter, because the format of the dump changed.

https://github.com/influitive/apartment/blob/80a21f2e1cdcbe5b0bd976f88c14332657804536/lib/apartment/adapters/postgresql_adapter.rb#L124-L127

System configuration

Database: PostgreSQL 9.5.12
Apartment version: 2.1.0

Apartment config (in config/initializers/apartment.rb or so):

Apartment.configure do |config|
config.excluded_models = %w{ Tenant Delayed::Job }
config.tenant_names = lambda { Tenant.pluck(:token) }
config.use_sql = true
config.persistent_schemas = %w{ shared_extensions }

use_schemas: true

Rails (or ActiveRecord) version: 5.1.5
Ruby version: 2.4.3

squidget commented 6 years ago

This will cause failures to create new tenants on any production system that security patches Postgres. 👍 Just had this happen on our dev environments that are auto patching.

The solution may not be very clean if Apartment can only get a copy of the public schema via pg_dump's output. Search/replace for public. anyone?

aldrinmartoq commented 6 years ago

@squidget

I'm working here https://github.com/aldrinmartoq/apartment/commit/8429b852bd9c15067661b1b75f07a7c0e5d1629c

But It's not finished. There are two specs not passing, and I'm not sure yet if it works correctly.

Apartment::Adapters::PostgresqlAdapter
  using schemas with SQL dump
    allows for dashes in the schema name
    it should behave like a generic apartment adapter
      #reset
        should reset connection
      #each
        iterates over each tenant by default
        iterates over the given tenants
      #drop
        should remove the db
      #current
        should return the current db name
      #switch
        should not throw exception if current is no longer accessible
        connects and resets the tenant
      #create
        should create the new databases
        should raise error when the schema.rb is missing unless Apartment.use_sql is set to true
        should load schema.rb to new schema
==>     should yield to block if passed and reset (FAILED - 1)
      #init
        should not retain a connection after railtie
      #switch!
        should connect to new db
        should raise an error if database is invalid
        should reset connection if database is nil
    it should behave like a schema based apartment adapter
      #switch!
        should connect to new schema
        should reset connection if database is nil
        should raise an error if schema is invalid
        persistent_schemas
          prioritizes the switched schema to front of schema_search_path
          maintains the persistent schemas in the schema_search_path
        numeric databases
          should connect to them
        with default_schema specified
          should switch out the default schema rather than public
          should still switch to the switched schema
      #reset
        should reset connection
        persistent_schemas
          maintains the persistent schemas in the schema_search_path
          with default_schema
            prioritizes the switched schema to front of schema_search_path
        with default_schema
          should reset to the default schema
      #switch
        connects and resets
      #current
        should return the current schema name
        persistent_schemas
          should exlude persistent_schemas
      #create
        should load schema.rb to new schema
==>     should yield to block if passed and reset (FAILED - 2)
        numeric database names
          should allow them
      #drop
        should raise an error for unknown database
        numeric database names
          should be able to drop them
      #init
        should process model exclusions
        with a default_schema
          sets the search_path correctly
          should set the proper table_name on excluded_models
        persistent_schemas
          sets the persistent schemas in the schema_search_path

squidget commented 6 years ago

The problem is the data coming back from pg_dump now contains "public." prefixes on all table references (including inside sql within function definitions).

We've created a patch for our prod environment as follows, which basically just search/replaces the string "public." with blank in the pg_dump contents so it is logically equivalent to the pg_dump generated prior to this change. The risk is the string "public." may exist in your pg_dump in a place where it should not be replaced - this is not the case for us, but it's a risk in this solution.

require 'apartment/adapters/postgresql_adapter'

module Apartment
  module Adapters
    class PostgresqlSchemaFromSqlAdapter < PostgresqlSchemaAdapter
      def _unqualify_dump(qualified_dump)
        # remove all references to the default schema
        unqualified_dump = qualified_dump.gsub(Regexp.new("\\b#{Regexp.escape(default_tenant)}\\."), '')
        # prevent the unsetting of search_path
        unqualified_dump.gsub(/^\s*select pg_catalog.set_config\('search_path'[^\n]+\n/i, '')
      end

      #   Dump postgres default schema
      #
      #   @return {String} raw SQL contaning only postgres schema dump
      #
      def pg_dump_schema
        _unqualify_dump(with_pg_env { `pg_dump -s -x -O -n #{default_tenant} #{dbname}` })
      end

      #   Dump data from schema_migrations table
      #
      #   @return {String} raw SQL contaning inserts with data from schema_migrations
      #
      def pg_dump_schema_migrations_data
        _unqualify_dump(with_pg_env { `pg_dump -a --inserts -t schema_migrations -t ar_internal_metadata -n #{default_tenant} #{dbname}` })
      end
    end
  end
end

maschiner commented 6 years ago

@squidget Thanks, your patch works on our staging env. I only had to change module PostgresqlSchemaFromSqlAdapter to class PostgresqlSchemaFromSqlAdapter.

Currently we don't need new schemas in production, so we will hold it back for now.

squidget commented 6 years ago

@maschiner Thanks. Updated with that change

aldrinmartoq commented 6 years ago

I was looking at my patch and what @squidget have done, and I feel this will be a mess. We fixed the issue by just going back to use_sql = false, but I'm looking for a more robust solution.

In the meantime, use @squidget code if it works; if it doesn't, don't use Apartment::Tenant.create and create the schema manually.

jonathanmartins commented 6 years ago

I got a similar error this week. Some upgrade in my system/softwares broke my project in my machine, I ended up here, so I reinstalled everything to be in the same version as my coworkers.

Postgresql 9.6.3, Apartment 2.0.0, and now it works again.

IngusSkaistkalns commented 6 years ago

Same applies for postgresql 10.3. It looks related to some postgresql pg_dump security related fixes

squidget commented 6 years ago

@aldrinmartoq I agree regex replacing in arbitrary pg_dump contents is an unreliable solution that would result in confusing errors if/when it replaced the wrong thing.

We can't use use_sql = false as a temporary solution here, as we're using PostgreSQL specific features that can't be represented in schema.rb.

Unfortunately, unless the Postgres team add an optional flag to pg_dump to generate without the schema (the previous format), I don't think there IS a clean solution.

The only ways I'm aware of to effectively duplicate a Postgres schema is to: 1) Use pg_dump 2) Write a custom stored procedure that inspects Postgres system tables & constructs create statements for the applicable objects in the new schema. (Ok for the common case of tables, but unreliable as a general solution, as it would need to support all current & future Postgres object types) 3) Rerun all database migrations (also unreliable, as migrations are not designed to be reusable long term) 4) Put in a request for the Postgres team to add an optional flag to pg_dump that returns the previous format.

This is probably why pg_dump was chosen as the original solution for Apartment gem in the first place.

squidget commented 6 years ago

I've posted to the pgsql-general@postgersql.org mailing list to see if the Postgres devs have a better solution for a more robust way to clone the public schema.

squidget commented 6 years ago

The best solution the mailing list suggested so far is to check in a sql file to version control which you run to populate new schemas, instead of inferring the schema definitions from pg_dump.

This solution would lose some of the automation that Apartment provides, (You'd have to specify your create tenant SQL) but would be more reliable than our current fix.

The risk is that if the user does not update this file whenever they create new migrations, it will get out of sync with the schema. It could maybe be made less painful by hooking into rake db:migrate so it also recreates a 'template' version of this create tenant SQL file via pg_dump, allowing the end user to then manually replace public. prefixes themselves.

aldrinmartoq commented 6 years ago

@squidget Sorry not telling you, I've already posted to pgsql-general@postgresql.org last week, same responses than you.

https://www.postgresql.org/message-id/ED39C2F0-21E9-4F7C-B0CF-14A62E78AFD6%40gmail.com

Before posting to the mailing list, I've already seen the clone_schema.sql script from Emanuel/Melvin but it wasn't robust enough for me (didn't run well in 10.3). But I think it could be a better solution than fixing pg_dump in the short/med term. I suggest we change apartment to do the following:

Create an apartment_clone_schema.sql script that dumps the SQL, but making sure it works with current major postgresql versions (maybe 8-9-10).
Instead of pg_dump, run that script.

I will work in that direction, and tell you when it's ready. Cheers,

ritchiey commented 6 years ago

The Postgres devs seem to be saying that the only supported use of pg_dump is to backup and restore without changing the schema name.

There is an alter schema ... rename to ... command though.

Would it be feasible to clone the schema with something like:

  # Restore the old schema in to a temp database
  pg_dump -Fc -d rails_db --schema=old_schema -f old_schema.dump
  pg_restore -d temp_database -f old_schema.dump

  # Rename it in the temp database
  psql temp_database -c "ALTER SCHEMA old_schema RENAME TO new_schema"

  # Transfer the renamed database back into the Rails database
  pg_dump -Fc -d temp_database --schema=new_schema -f new_schema.dump
  pg_restore -d rails_database -f new_schema.dump

Apart from being slower is there any reason this shouldn't work?

smmr0 commented 6 years ago

We ran into a similar problem, and we managed to get around it by renaming each schema (including public) to a long (63 characters), unique string. Then in the dump, we do a global string replace to switch out the source schema name for the target schema name before we restore it. There is an infinitesimal chance that the source schema name will occur in the data, but for our purposes it's close enough to zero that we decided it was worth the risk.

Obviously this approach probably isn't suitable for a library where you don't have control over the schema names that the user chooses, but I'll leave it here in case it matches anyone else's use case.

meganemura commented 6 years ago

Fixed by #537.

Mayurifag commented 6 years ago

@meganemura are you sure its fixed?

I have latest apartment (2.2.0), postgresql (9.5.12), rails (5.0.6) and I can't create a tenant in production. I'm fine with it on development, though. And my app had created and successfully used some tenants earlier.

Getting this error:

...%long_messages%...
--
-- Name: fk_rails_pickup_issuer_admin_admins; Type: FK CONSTRAINT; Schema: public; Owner: -
--

ALTER TABLE ONLY "newtenant".orders
    ADD CONSTRAINT fk_rails_pickup_issuer_admin_admins FOREIGN KEY (pickup_issuer_id) REFERENCES "newtenant".admin_admins(id);

--
-- PostgreSQL database dump complete
--
D, [2018-04-14T04:01:55.420083 #30232] DEBUG -- :    (0.5ms)  ROLLBACK
ActiveRecord::StatementInvalid: PG::InFailedSqlTransaction: ERROR:  current transaction is aborted, commands ignored until end of transaction block
: SET search_path TO "public", "public"
from /.../shared/bundle/ruby/2.4.0/gems/activerecord-5.0.6/lib/active_record/connection_adapters/postgresql/database_statements.rb:98:in `async_exec'

Sure I googled it for 2 days already, but had no luck. I have no problems with migrations, nor ar_internal_metadata noticed on stackoverflow and other stuff at github.

The only thing I noticed that probably affects my problem is weird config of pg_dump or smth like that. When I'm trying to create the tenant from development, i dont have the string SELECT pg_catalog.set_config('search_path', '', false);, noticed in the beginning of the log on production env:

D, [2018-04-14T04:01:53.597875 #30232] DEBUG -- :    (0.1ms)  BEGIN
D, [2018-04-14T04:01:53.605534 #30232] DEBUG -- :    (0.4ms)  CREATE SCHEMA "newtenant"
D, [2018-04-14T04:01:53.606296 #30232] DEBUG -- :    (0.1ms)  show search_path
D, [2018-04-14T04:01:55.388625 #30232] DEBUG -- :    (1097.1ms)  SET search_path = "newtenant", public;
--
-- PostgreSQL database dump
--

-- Dumped from database version 9.5.12
-- Dumped by pg_dump version 9.5.12

SET statement_timeout = 0;
SET client_encoding = 'UTF8';
SET standard_conforming_strings = on;
SELECT pg_catalog.set_config('search_path', '', false);
SET check_function_bodies = false;
SET client_min_messages = warning;

--
-- Name: russian_ispell; Type: TEXT SEARCH DICTIONARY; Schema: public; Owner: -
--

CREATE TEXT SEARCH DICTIONARY "newtenant".russian_ispell (
    TEMPLATE = pg_catalog.ispell,
    dictfile = 'russian', afffile = 'russian', stopwords = 'russian' );

...%la-la-la%...

There are no other errors or things i can somehow notice. I'm sure I have no configuration in postgresql.conf or in database.yml related with search_path.

And my apartment.rb also contains config.use_sql = true and config.persistent_schemas = %w{ public } if its needed for future investigation.

meganemura commented 6 years ago

Thanks @Mayurifag for reporting. I reopen this.

Mayurifag commented 6 years ago

@meganemura eh im sorry, i have done with my problem, I will write the solution some hours later for anyone who will google this error.

Mayurifag commented 6 years ago

@meganemura I want to sorry once again, finally I don't know what to think about the stuff happened to my production. This issue can be closed once again I guess.

Somehow I solved my problem with ALTER EXTENSION pg_trgm SET SCHEMA pg_catalog; (be careful with such stuff on production env). Everything worked fine half a year ago with schema public on production and still works in dev with same schema [but postgres version 10.1 instead of 9.5.12], I don't know how this affected the search_path, I hope someone will investigate further.

So it seems, my issue wasn't caused by gem apartment bug, happily. But I guess we need more error explanations to get these things debugged easier.

Thanks for all the work you guys have done here :+1:

influitive / apartment