Re: [PATCH] Re: [pgsql-advocacy] Why READ ONLY transactions?

> > Josh Berkus wrote:
> >> I thought that this was rejected thouroughly by Tom some months ago. He
> >> argued pretty strongly that READ ONLY transactions were *not* a security
> >> feature and that trying to make them one would work very poorly.
>
> > I remember something like that, but I thought the patch was the result
> > of that discussion. Tom?
>
> Hm, I can't find anything in the archives in which I said that. I
> did argue that using GUC to control a security feature would be a
> mistake:
> http://archives.postgresql.org/pgsql-patches/2003-07/msg00198.php
> and after watching Bruce struggle with trying to make
> logging-related GUC settings secure, I think my point is pretty much
> proved ;-). I don't want to see more cruft like that added to the
> GUC logic.

http://archives.postgresql.org/pgsql-patches/2003-07/msg00204.php

Sure sounds like you said READ ONLY xacts can't be used for security. :)

> Another thing to think about is that the semantics of START
> TRANSACTION READ ONLY are constrained by the SQL standard, and they
> are not exactly "read only" in the traditional sense (eg, you can
> still create and use temp tables). If we go down this path, I would
> be unsurprised to run into a showstopper conflict between what's
> needed for reasonably "secure" behavior and what the spec dictates.
> It would be less risky to use some other approach, if we are really
> interested in creating read-only users.

Hence the term, "security policy." I want read only
users/transactions, but I also need temp tables to work and for
transactions to be committed out of temp tables into the real tables
via a proc with elevated privs. Other people who don't want to have
malicious read only users fill up the disk may want TEMP tables to be
disabled.

> So I'm still of the opinion I gave in the above-mentioned thread,
> that I'd rather make "read only user" be a concept driven by a flag
> in the user's pg_shadow entry.

I think a boolean "read only" user flag will fall well short of
letting admins finely tune the database's behavior given the example
above. I think using ALTER USER [username] SET is a much better
mechanism for securing users than setting a boolean in pg_shadow.
Taking the boolean in pg_shadow to its extreme, we'll either get to
the point where we've got a gazillion different columns (think of how
nasty MySQL's mysql database and it's host/user/table/db is) that are
unneeded 99% of the time. <sarcasm>To avoid that, we could get smart
and replace the single boolean value with an int4 "options" field
where we could toggle various bits to mean different things. Bit 1
would be read only. Bit 2 would be allow temp tables. Then we could
teach admins to xor bits and negate bits and hope that no one makes a
mistake, thus opening up their DB to abuse because the admin made a
mistake because instead of bit 15, they flipped bit 14, nevermind that
we'll have made the assumption that every DBA has a good working
understanding of binary.</sarcasm>

The patch doesn't prevent write(2), but this tunable isn't used to
prevent writing to disk, it's meant to prevent changes to the database
by a given user. If you want a truly read-only database (in the case
of NFS), mount the filesystem as readonly (not sure if that works, but
it'd be a useful exercise). One step better, centralize the
postmaster's write() calls and add a level of indirection with a few
function pointers. If the backend is in read-only mode, use a
different func that aborts the transaction.

I think Tom's big objection is the abuse of the GUC system for
maintaining this information. Having thought about this some, I think
the GUC system is pretty well suited for this and that Tom's objection
(correct me if I'm wrong here) is that GUC has a non-hierarchical
naming structure/convention. With a hierarchical structure, lumping
of GUC variables becomes more reasonable and the naming is more
systematic. Instead of, "jail_read_only_transaction=true" it'd be
"security.force_readonly=true" or "transaction.readonly_always=true".

-sc

--
Sean Chittenden