Add force_readonly option for owner of a FOREIGN SERVER in context of SQLite connection management

[mkgrgis]

In this PR:

• Implementation of force_readonly option based on sqlite3_open_v2 function SQLITE_OPEN_READONLY mode and can disallow any changes in SQLite database. Hence if force_readonly set to true any values of updatable options are ignored. If force_readonly set to false or not set there is usual behaviour of updatable option like in postgres_fdw.
• Improve SQL query execution errors to separate context and hint parts
• Improve other global SQLite errors for connection to more common PostgreSQL style
• Fix some linter hints about trailing spaces in some files

[t-kataym]

@mkgrgis , Please let me share my thought.
Because the functionality for a user can be provided by updatable option, providing similar options may causes a confusion to user.
I don't understand the necessity of force_readonly option now, and cannot explain a decisive usage to distinguish them (what kind of situation user should use these option respectively) .
And I agree with @son-phamngoc 's comment: it had better not to introduce a feature which assumes an uncertain case.

[mkgrgis]

what kind of situation user should use these option respectively

This option is usefully in multi-user environment for security reasons.

1. There is server with owner u1 and updatable = false.
2. u1 granted table rights to u2, u3 etc.
3. Normally u2 or u3 can overwrite updatable value for a table
4. If force_readonly = true only owner of the server can determine if change SQL are available for all represented objects from any SQLite DB file. If PostgreSQL works with important SQLite database this option helps to guarantee unchangeable content.

Please explain where have I mistaken, @t-kataym.

[mkgrgis]

@t-kataym , let's continue review of this PR.

SQLite (and Firebird) data access model is not only SQL like in Oracle, MariaDB or PostgreSQL, because significantly based on OS file permissions and I think we should give to foreign server owner some instruments with similar security effect without access to PostgreSQL server file system. In the new version of README.md there is full motivation in Data modification access section. Could you please write me your opinion about low level access permissions in context of this PR?

[t-kataym]

Hello, @mkgrgis

I'm sorry for my late reply.

This option is usefully in multi-user environment for security reasons.

1. There is server with owner u1 and updatable = false.
2. u1 granted table rights to u2, u3 etc.
3. Normally u2 or u3 can overwrite updatable value for a table
4. If force_readonly = true only owner of the server can determine if change SQL are available for all represented objects from any SQLite DB file. If PostgreSQL works with important SQLite database this option helps to guarantee unchangeable content.

I think you can control it with PostgreSQL feature without this PR.
If you wants not to allow u2 to update the table, you take away the privilege of UPDATE for the table from u2 on PostgreSQL core layer (not FDW layer) with REVOKE query.

Could you explain a situation which cannot realize on only PostgreSQL feature?

[mkgrgis]

Could you explain a situation which cannot realize on only PostgreSQL feature?

I am trying to prepare this SQL mutiuser use case.
I know, that for Firebird database file permissions and file open modes also are access control instruments and many years before there was many commits to Firebird to support readonly files as possible data source. SQLite by default supports readonly file mode, hence there is no programmatically or conceptual problem, we need only explaining of SQL context for this PR. I'll do this.

[mkgrgis]

@t-kataym , I have made a research about most popular relational FDWs for PostgreSQL.
Such FDWs as oracle_fdw, firebird_fdw, mysql_fdw, postgres_fdw can have non-effective updatable=true for both server and table.
For example remote PostgreSQL user in postgres_fdw can have readonly data access property as result of remote ALTER USER ... SET default_transaction_read_only = on; or SET SESSION CHARACTERISTICS AS TRANSACTION READ ONLY;. Also in Oracle remote user, used for oracle_fdw can have no data modification permissions. In Firebird there is two popular methods to deny data modification: usual SQL GRANT/REVOKE commands and readonly fdb file on filesystem level similar to implementation for SQLite in this PR.

In this PR I have implemented data access control under remote connection level. In case of SQLite there is only primitive file open mode, no SQL GRANT/REVOKE which can make non-effective updatable=true for both server and table in other relational DBs.
I was wrong in #78 (comment), this PR is not about multi-user context. For user grouping there is GRANT role TO user. This PR is about foreign object grouping as united entity with a data modification property value. In other FDWs this is result of data access grants of remote user, here we have only primitive file open mode.

[son-phamngoc]

Hello @mkgrgis , thanks for your effort. I would like to share my opinion about this PR.

In my understanding, you are going to provide an option to allow user to control data access at FDW level, via 2 options force_readonly and updatable. GRANT/REVOKE can help us to control it, but at object access level.
At FDW level, we already have updatable option, but its behavior is not as the purpose of this PR.
For updatable option, the value of FOREIGN TABLE has higher priority than FOREIGN SERVER.
So, to control it, you defined force_readonly option, which ignores updatable when it is true. Is that correct?

In my opinion, I think it is confusing when providing 2 options to do the same thing.
From user viewpoint, it is confusing to configure 2 options which has similar meaning. There can be a lot of combination of those 2 options, for both FOREIGN TABLE and FOREIGN SERVER, and make user harder to determine correct behavior.
Actually, if we change the behavior of updatable to prioritize the value of FOREIGN SERVER, it will reach the purpose of this PR without creating any new option. However, it will be inconsistent compared to the behavior of postgres_fdw, which is our base FDW. Please consider this point.

[mkgrgis]

Hello, @son-phamngoc ! Thanks for Your opinion, I'll comment it.

In my understanding, you are going to provide an option to allow user to control data access at FDW level, via 2 options force_readonly and updatable. GRANT/REVOKE can help us to control it, but at object access level.

Please note, for other relational FDWs also there is remote GRANT/REVOKE in source databases for a remote user of a foreign server. This remote GRANT/REVOKE allow to group all foreign server objects as potentially changeable or strongly read-only independent of updatable option values and dependant only on connection. This PR implements similar behaviour.

So, to control it, you defined force_readonly option, which ignores updatable when it is true. Is that correct?

Yes. Grants or revokes of Oracle or PosrgreSQL remote user also ignores all of updatable options inside of FDW foreign objects.

In SQLite we have no "remote"-like global access control except to file open mode. The specifics of our FDW is that both access controls methods concentrates inside of FDW: universal but not obligatory updatable (based on connection, after connection) and something like access control before connection implemented as file open mode in this PR.

In my opinion, I think it is confusing when providing 2 options to do the same thing. From user viewpoint, it is confusing to configure 2 options which has similar meaning.

Not the same thing, @son-phamngoc. With force_readonly I can once group all foreign server objects as read-only like REVOKE INSERT, UPDATE, DELETE for remote user in Oracle, MariaDB or Firebird. Without this option I can only generate a scripts by list of objects of a foreign server to make this objects read-only for a users or roles.

There can be a lot of combination of those 2 options, for both FOREIGN TABLE and FOREIGN SERVER, and make user harder to determine correct behavior.

Users of other relational FDWs can understand force_readonly option as fully compatible with revoked change permissions for remote user. I am sure, with other FDWs there is much of remote users without change rights and de facto ignorable updatable option values.

Actually, if we change the behavior of updatable to prioritize the value of FOREIGN SERVER, it will reach the purpose of this PR without creating any new option. However, it will be inconsistent compared to the behavior of postgres_fdw, which is our base FDW.
Please consider this point.

Yes, this was my first not correct proposal. To avoid inconsistency with postgres_fdw I propose to understand force_readonly option as some kind of reduced remote access control, implemented inside of our FDW because SQLite works just with a file and have no code outside of FDW.
Now I can implement similar access control only on OS level - postgres OS user have no write access to SQLite file with a sensitive data and it's directory. But I thinks it's better to give similar possibility to FOREIGN SERVER owner.

[son-phamngoc]

@mkgrgis Thanks for your explanation.

With force_readonly I can once group all foreign server objects as read-only like REVOKE INSERT, UPDATE, DELETE for remote user in Oracle, MariaDB or Firebird. Without this option I can only generate a scripts by list of objects of a foreign server to make this objects read-only for a users or roles.

I understand that force_readonly can help you mark all objects which belong to a foreign server as read-only at once, instead of setting each object respectively.
However, introducing an option that ignores another option causes a confusion and makes complex. It is better to avoid it.
Therefore, we need a stronger reason to be able to accept this option.

Could you tell me an actual use case that you need to use force_readonly option?

[mkgrgis]

Could you tell me an actual use case that you need to use force_readonly option?

Yes, let's discuss real example.
There is collective PostgreSQL of college connected to SQLite file which is updated by outer system. Data schema of source SQLite is not very stable, but changeable approximate once in 3..4 weeks for 2..3 tables. Professor gives full data read access to students to all of SQLite tables (25..35), but sometimes edit source data anomalies in SQlite as FOREIGN SERVER owner. Now professor generates SQL like REVOKE INSERT UPDATE DELETE for all objects for student's role and works with PostgreSQL admin to allow temporary write access for postgres OS user to the SQLite file and the directory.
Also students have separate SQLite files for exam works connected to this collective PostgreSQL. Students can freely share and read all of the data, but before exam professor blocks (with help of OS admins) all personal student's SQLite files to avoid changes. With force_readonly option professor can block changes in all of the student's SQLite files after he become owner of all of student's FOREIGN SERVERS. Ownership changed if a student tell his exam works in SQLite are ready.
This case is typical for remote relational databases, but with SQLite so called remote connection control in not really remote, because SQLite file processing is not outside of PostgreSQL. I hope , @son-phamngoc, you have understood that so called remote control access which allows so called remote connection admins ignore updatable values is something independent of DB technical form - both needed for network or file DB.

[mkgrgis]

Short description of this PR: implement remote database admin role for FOREIGN SERVER owner, because there is no role or access conception in SQLite, only file open mode. This remote database admin role implemented inside of FDW because of SQLite is not really remote database, but some file accessible from PostgreSQL server.

[son-phamngoc]

@mkgrgis Could you help me to confirm which way of understanding matches your example?

1. There is only 1 FOREIGN SERVER, and the Professor is the owner of that FOREIGN SERVER, which can change value of force_readonly. The professor creates FOREIGN TABLE and GRANT INSERT UPDATE DELETE on FOREIGN TABLE for students.
   The students have the permission to read and write FOREIGN TABLE. They do not have permission to access or change FOREIGN SERVER.
   For example: student 1 can read and write FOREIGN TABLE 1, student 2 can read and write FOREIGN TABLE 2.
   In normal case, the professor does not set value of force_readonly (or set it to false) to allow students to access or update data.
   Before exam, the professor change force_readonly to true to block all students from updating.
2. There are several FOREIGN SERVERs and the Professor is the owner of them.
   The professor GRANT USAGE on FOREIGN SERVER to students, so that the students can access and modify server.
   For example, FOREIGN SERVER 1 is granted to student 1, FOREIGN SERVER 2 is granted to student 2.
   Students can create FOREIGN TABLE on their granted FOREIGN SERVER.
   Before exam, firstly, the professor REVOKE USAGE on FOREIGN SERVER from students, so that the students cannot update the FOREIGN SERVER.
   Next, he changes force_readonly of all FOREIGN SERVER to disallow student update data.

I think the 2nd matches your example.
If I have any misunderstanding, please explain to me.

[mkgrgis]

@son-phamngoc , I am sorry for not clear clarification, really the example is near your 2 version.

1. There is only 1 FOREIGN SERVER, and the Professor is the owner of that FOREIGN SERVER, which can change value of force_readonly.

No, there are many foreign servers in PostgreSQL, but there is main FOREIGN SERVER which also works with outer system and have very big and data quality sensitive SQLite file. This server owned by the Professor and usually is read only even for the Professor. Sometimes he changed some data.

The students have the permission to read and write FOREIGN TABLE. They do not have permission to access or change FOREIGN SERVER.

Students does not have permission to change any data of main FOREIGN SERVER owned by the Professor. They can read and write any data on personal FOREIGN SERVER, sometimes student can write some data on FOREIGN SERVER of other student if there was GRANT from other student. By default there are no read restrictions on all of FOREIGN SERVERs.

For example: student 1 can read and write FOREIGN TABLE 1, student 2 can read and write FOREIGN TABLE 2.

No. This limitation is very rare, even if a student tell to the Professor not to see some data of the main FOREIGN SERVER.

2. There are several FOREIGN SERVERs and the Professor is the owner of them.

Yes. There is not only main FOREIGN SERVER, but also there are many FOREIGN SERVERs of students.

The professor GRANT USAGE on FOREIGN SERVER to students, so that the students can access and modify server.

Yes. Students works with data from the main FOREIGN SERVER and, sometimes, with data of FOREIGN SERVERs of other students.

For example, FOREIGN SERVER 1 is granted to student 1, FOREIGN SERVER 2 is granted to student 2.

Yes. This is group of FOREIGN SERVERs for students, usually with not big SQLite files.

Students can create FOREIGN TABLE on their granted FOREIGN SERVER.

Yes. But students cannot create or change a FOREIGN TABLE on the main FOREIGN SERVER owned by the Professor.

Before exam, firstly, the professor REVOKE USAGE on FOREIGN SERVER from students, so that the students cannot update the FOREIGN SERVER.
Next, he changes force_readonly of all FOREIGN SERVER to disallow student update data.

Yes.

I think the 2nd matches your example.

Yes, but this is not 1st and 2nd versions, this is only different aspects of the example. I am sorry for not good clarification, @son-phamngoc . I hope now we have no misunderstanding.

[mkgrgis]

In my opinion, I think it is confusing when providing 2 options ...

@son-phamngoc , in README added detailed description with diagnostics description in 087b3a3 and 6e6c74e

[son-phamngoc]

Hello @mkgrgis , thanks for your answer. I think I understood the necessity of this option.

I would like to describe the necessity of this option like this:

Without `force_readonly` option, we cannot support A and B at the same time.
A. Grant user permission to create a foreign table
B. Revoke user permission to modify table data.

Could you add it to README?

About the implementation, I have not started review yet, but please let me share my thought.
I think we can simply reach the target of this MR by modifying sqliteIsForeignRelUpdatable .
When sqliteIsForeignRelUpdatable return False, which does not allow modification, the Postgres core will raise an error, and the modification stops immediately.
Therefore, I think the implementation of connection.c is redundant. There is no case that sqlite_fdw opens a read-only connection.

flags = flags | (entry->readonly ? SQLITE_OPEN_READONLY : SQLITE_OPEN_READWRITE);
rc = sqlite3_open_v2(dbpath, &entry->conn, flags, zVfs);

I remember that we have already discussed about that in #59.
If it is redundant, we should remove it.

[mkgrgis]

Without force_readonly option, we cannot support A and B at the same time.
A. Grant user permission to create a foreign table
B. Revoke user permission to modify table data.

Yes. There is no use case difference with other FDWs, but there is difference of implementation. The description have completed in f92ee42

This option is useful if you need grant user permission to create a foreign tables on the foreign server and revoke user permission to modify any table data on this foreign server. This option with true value can disallow any write operations on foreign server table data through SQLite file readonly access mode. This option driven only by foreign server owner role can not be overwritten by any updatable option value. This is a strong restriction given by PostgreSQL foreign server owner user not to modify data in any foreign server tables. Also see Connection to SQLite database file and access control

About the implementation...

I think we can simply reach the target of this MR by modifying sqliteIsForeignRelUpdatable .

Yes, this is implemented, but not only. In other FDWs similar "grant user permission to create a foreign table and revoke user permission to modify table data" conditions implementation based on remote database internal structures. I think in our case we also can use remote database structures which is internal SQLite function in our case. Any FOREIGN SERVER option change calls a new connection and ends old connection for a new option combination. Also force_readonly is a classical security option and double implementation is useful for any security options. In our case sqlite3_open_v2 with SQLITE_OPEN_READONLY needs additional 1-2 ms. If there will be found any CVE in the Postgres core or sqlite3_open_v2 our double implementation will be not vulnerable. Maybe, I should add comments about this in the code?

[son-phamngoc]

Yes. There is no use case difference with other FDWs, but there is difference of implementation. The description have completed in f92ee42

Thank you for your update.

Yes, this is implemented, but not only. In other FDWs similar "grant user permission to create a foreign table and revoke user permission to modify table data" conditions implementation based on remote database internal structures. I think in our case we also can use remote database structures which is internal SQLite function in our case. Any FOREIGN SERVER option change calls a new connection and ends old connection for a new option combination. Also force_readonly is a classical security option and double implementation is useful for any security options. In our case sqlite3_open_v2 with SQLITE_OPEN_READONLY needs additional 1-2 ms. If there will be found any CVE in the Postgres core or sqlite3_open_v2 our double implementation will be not vulnerable. Maybe, I should add comments about this in the code?

About the implementation of SQLite connection, I commented about it before, in #59.
We are not sure that if SQLITE_OPEN_READONLY can actually prevent any CVE.
If there is any CVE, we need to analyze so many things: the root cause, possibility, reproduction, suitable solution ...
And solution may be not SQLITE_OPEN_READONLY. Maybe we need to modify PostgreSQL core, or another routine of FDW, or other places. There are many cases that can happen.
Currently, we have no basis for it: no logical issue found, no CVE detected, no test case ...
Therefore, we don't want to leave a code for an uncertain case. All the code merging into master branch should be tested, and make sure that it is effective. Please understand it.
Seen from usability, the logic can be well implemented in sqliteIsForeignRelUpdatable. I do not see any logic problem.
If you can create any test case that leads to problem, we will consider again.

[mkgrgis]

All the code merging into master branch should be tested, and make sure that it is effective. Please understand it.

Added tests allow us ensure there are no problems for any existed functions with SQLITE_OPEN_READONLY. Have I added destroyed or unusable TCs in this PR, @son-phamngoc ? Does I need to give you scripts with benchmark tests and my results with some millions SQLITE_OPEN_READONLY and SQLITE_OPEN_READWRITE open connection operations?

Seen from usability, the logic can be well implemented in sqliteIsForeignRelUpdatable. I do not see any logic problem.

Please allow me share opinion of antivirus or SELinux security administrator. There is security rule: "you shouldn't collect unusable privileges". PostgreSQL server is well-tested program, but it's hard to run it with 0 (root) user rights, because authors of PostgreSQL try to prevent unusable privileges for the server and running from 0 user documented as unwanted.
Let's imagine system monitoring log about opened files. A security administrator knows sensitive SQLite file as readonly for PostgreSQL server process, but there is rw (readwrite) alert in log in case of force_readonly = 'true'. This is false security alert and unusable write privilege. If FOREIGN SERVER options will be changed this will be a new connection with new privileges. This can allow security administrators to log when somebody changes FOREIGN SERVER rights. Also SQLITE_OPEN_READONLY mode doesn't destroy any sqlite_fdw work and doesn't decrease formal productivity of any SQL operations. What do you think about security administrator view and opinion, @son-phamngoc ?

[son-phamngoc]

@mkgrgis Sorry for late respond.
We had an internal discussion. We understood your viewpoint and the reason for your modification.
We can accept this PR after finished reviewing and testing.

[mkgrgis]

@mkgrgis Sorry for late respond.

No problems, @son-phamngoc. I understand there is unusual for you external C programming in the PR. Really this is more to POSIX side, not to SQL.

We had an internal discussion.

Does I need to provide some external tests for file access mode descriptors r and rw ? I don't understand how we can integrate this tests, because it based on shell commands out of PostgreSQL server context.

[son-phamngoc]

Does I need to provide some external tests for file access mode descriptors r and rw ? I don't understand how we can integrate this tests, because it based on shell commands out of PostgreSQL server context.

No, you don't need to provide more test.
I will start reviewing soon.

[mkgrgis]

I will start reviewing soon.

Thanks, @son-phamngoc , common description is updated #78 (comment)

[son-phamngoc]

@mkgrgis I added some comments. Could you check them?

[mkgrgis]

Thanks for 1st review, @son-phamngoc ! Let's begin a 2nd review round?

[son-phamngoc]

Thanks for your fixing @mkgrgis.
I see no more problem. @t-kataym will decide next step.

[mkgrgis]

Ping, @t-kataym ?

[t-kataym]

@mkgrgis Thank you for your contribution. I will accept the PR.