Re: Please advice TODO Item pg_hba.conf

Gevik Babakhani wrote:

Hi,

> 2) The file parsenodes.h is updated to support
> #define ACL_DATABASE_CONNECT

I wouldn't call it ACL_DATABASE_CONNECT, just ACL_CONNECT.  Currently we
don't have any objects other than databases that need connecting to, but
you never know.

(It should be very easy to change this if you edit the patch itself,
un-apply the old one and then re-apply the edited patch.)

> 7) File postinit.c method "ReverifyMyDatabase" is updated by following:
> First we check to make sure we are not in bootstrap processing mode.
> If not, we check to see if the connected user has ACL_DATABASE_CONNECT.
> If not, ereport(FATAL,.....)
> (Perhaps we should change the error message later)

Please make sure you use the established style for writing the ereport()
call.  The newlines you placed are not consistent with the rest of the
code.  Also the message text does not follow the guidelines (no initial
capital letter, no ending period for the primary message; the other way
around for secondary messages).  I think it should be

ereport(FATAL,    (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),         errmsg("couldn't connect to database %s",
database_name),    errdetail("User %s doesn't have the CONNECT privilege for database %s.",
user_name,database_name)));
 

Or something like that.  Not sure about the wording.  Also: are we
giving too much information so that it can be considered a security
problem?  (I don't think so, because user and database name were passed
by the user).

> 8) File dbcommands.c method "createdb" is updated by following:
> When a new database is being created we add a default ACL by 
> calling acldefault(ACL_OBJECT_DATABASE,.... and adding the default ACL
> by new_record[Anum_pg_database_datacl - 1] =
> PointerGetDatum(defaultAcl);

If I'm not mistaken, the general principle for creating objects is leave
their ACLs as NULLs.  Later, when a privilege is going to be checked, a
NULL is treated as if it contained whatever default privilege the object
class has.  So you should leave this code alone, and have the checking
code replace the default ACL when it gets a NULL (this way, it's even
more backwards compatible).

> At this moment the owner of the database CAN REVOKE himself form the
> ACL_OBJECT_DATABASE. If the implementation above is acceptable then I
> can work on this one :)

Hmm, what do you want to do about it?  ISTM the owner should be able to
revoke the privilege from himself ... (Maybe we could raise a WARNING
whenever anyone revokes the last CONNECT privilege to a database, so
that he can GRANT it to somebody before disconnecting.)


Also I'm not sure if we have discussed what's the default (initial)
privilege state.  Do we want PUBLIC to have CONNECT privilege?

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.