Re: [v9.2] Add GUC sepgsql.client_label - Mailing list pgsql-hackers

On 2012-03-09 21:49, Robert Haas wrote:
> On Tue, Mar 6, 2012 at 9:14 AM, Kohei KaiGai<kaigai@kaigai.gr.jp>  wrote:
>> [ new patch ]
> Are we absolutely certain that we want the semantics of
> sepgsql_setcon() to be transactional?  Because if we made them
> non-transactional, this would be a whole lot simpler, and it would
> still meet the originally proposed use case, which is to allow the
> security context of a connection to be changed on a one-time basis
> before handing it off to a client application.

It would meet the original use case, but outside of that use case it 
would be very easy to get POLA violations. Imagine a transaction like
1- do stuff under label A
2- setcon to B
3- do stuff under label B

When that transaction fails due to a serialization error, one would 
expect that when the transaction is replayed, the initial actions are 
executed under label A. If it was B, or any other further label in the 
original transaction, it would be very hard to develop software in user 
space that could cope with this behaviour.
> As a separate but related note, the label management here seems to be
> excessively complicated.  In particular, it seems to me that the
> semantics of sepgsql_get_client_label() become quite muddled under
> this patch.  An explicitly-set label overrides the default label, but
> a trusted procedure's temporary label overrides that.  So if you enter
> a trusted procedure, and it calls sepgsql_setcon(), it'll change the
> label, but no actual security transition will occur; then, when you
> exit the trusted procedure, you'll get the new label in place of
> whatever the caller had before.  That seems like one heck of a
> surprising set of semantics.

I agree that sepgsql_get_*client*_label does not really match with a 
trusted procedure temporarily changing it.
> It seems to me that it would make more sense to view the set of
> security labels in effect as a stack.  When we enter a trusted
> procedure, it pushes a new label on the stack; when we exit a trusted
> procedure, it pops the top label off the stack.  sepgsql_setcon()
> changes the top label on the stack.  If we want to retain
> transactional semantics, then you can also have a toplevel label that
> doesn't participate in the stack; a commit copies the sole item on the
> stack into the toplevel label, and transaction start copies the
> toplevel label into an empty stack.

Yes the additions be sepgsql_setcon look like a stack for pushing. 
However, the current code that rollbacks the pending list to a certain 
savepoint matches code from e.g. StandbyReleaseLocks(), so having a 
concept like this as a list is not unknown to the current codebase.
>    In the current coding, I think
> client_label_peer is redundant with client_label_committed - once the
> latter is set, the former is unused, IIUC

client_label_peer is used on reset of the client label, i.e. calling 
sepgsql_setcon with NULL.

>   - and what I'm proposing is
> that client_label_func shouldn't be separate, but rather should mutate
> the stack of labels maintained by client_label_pending.

The drawback of having trusted procedures push things on this stack is 
that it would add a memory alloc and size overhead when calling trusted 
functions, especially for recursive functions.

> The docs need updating, both to reflect the version bump in
> sepgsql-regtest.te and to describe the actual feature.

I can probably write some docs tomorrow.

regards,
Yeb Havinga