Thread: Re: [COMMITTERS] pgsql: sepgsql: Support for new post-ALTER access hook.
On Wed, Mar 27, 2013 at 8:44 AM, Thom Brown <thom@linux.com> wrote: > On 27 March 2013 12:33, Robert Haas <rhaas@postgresql.org> wrote: >> sepgsql: Support for new post-ALTER access hook. > > I notice that due to commit bc5334d8 I can't actually build the docs > at the moment. > > But I think the language here definitely needs improving: > > "On CREATE FUNCTION, install permission will be checked if leakproof > attribute was given, not only create on the new function. This > permission will be also checked when user tries to turn on leakproof > attribute using ALTER FUNCTION command, with setattr permission on the > function being altered." What do you suggest? I thought about changing the wording but the new wording is parallel to what's already in that paragraph, so likely the whole thing needs to be rewritten if we change any of it. That seemed beyond the scope of this commit, but I'm happy to have us do it. > And are the literals there capitalised when rendered? If not, could I > suggest they be capitalised in the SGML? AFAIK, there's nothing that would change capitalization automatically in our doc toolchain. Possibly LEAKPROOF should be capitalized but the rest look right. setattr, etc. should not be capitalized, at least according to my limited understanding of how SELinux capitalization conventions work. > s/for each object types/for each object type/ In the interest of avoiding a proliferation of tiny commits, I'll hold off on fixing this until we figure out what to do about the rest of it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 27 March 2013 12:58, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Mar 27, 2013 at 8:44 AM, Thom Brown <thom@linux.com> wrote:
>> On 27 March 2013 12:33, Robert Haas <rhaas@postgresql.org> wrote:
>>> sepgsql: Support for new post-ALTER access hook.
>>
>> I notice that due to commit bc5334d8 I can't actually build the docs
>> at the moment.
>>
>> But I think the language here definitely needs improving:
>>
>> "On CREATE FUNCTION, install permission will be checked if leakproof
>> attribute was given, not only create on the new function. This
>> permission will be also checked when user tries to turn on leakproof
>> attribute using ALTER FUNCTION command, with setattr permission on the
>> function being altered."
>
> What do you suggest? I thought about changing the wording but the new
> wording is parallel to what's already in that paragraph, so likely the
> whole thing needs to be rewritten if we change any of it. That seemed
> beyond the scope of this commit, but I'm happy to have us do it.
Perhaps something along the lines of:
"When a CREATE FUNCTION command is executed, the install permission
will be checked to determine whether the LEAKPROOF attribute was
present. This permission will also be checked when the user tries to
apply the LEAKPROOF attribute using the ALTER FUNCTION command."
I'm not sure what the last part is actually describing ("with setattr
permission on the function being altered."), so I'm not sure how that
should be read. It doesn't help that I'm not familiar with SELinux
terms.
>> And are the literals there capitalised when rendered? If not, could I
>> suggest they be capitalised in the SGML?
>
> AFAIK, there's nothing that would change capitalization automatically
> in our doc toolchain. Possibly LEAKPROOF should be capitalized but
> the rest look right. setattr, etc. should not be capitalized, at
> least according to my limited understanding of how SELinux
> capitalization conventions work.
I was really just thinking of CREATE and LEAKPROOF, but I'm not sure
"CREATE" should be in there anyway.
--
Thom
On Wed, Mar 27, 2013 at 9:09 AM, Thom Brown <thom@linux.com> wrote:
> Perhaps something along the lines of:
>
> "When a CREATE FUNCTION command is executed, the install permission
> will be checked to determine whether the LEAKPROOF attribute was
> present. This permission will also be checked when the user tries to
> apply the LEAKPROOF attribute using the ALTER FUNCTION command."
>
> I'm not sure what the last part is actually describing ("with setattr
> permission on the function being altered."), so I'm not sure how that
> should be read. It doesn't help that I'm not familiar with SELinux
> terms.
Right, so what it's trying to say is: whenever you modify an object,
we check whether you've got {setattr} permission for that object and
disallow the operation if not. However, for some operations on some
object types, {setattr} is necessary but not sufficient. The
paragraph is recapping, for various cases, which operations require
additional permissions, and what those additional things are.
> I was really just thinking of CREATE and LEAKPROOF, but I'm not sure
> "CREATE" should be in there anyway.
create here is referring to the sepgsql permission, not the SQL
command, so it's correct as-is.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 27 March 2013 14:50, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Mar 27, 2013 at 9:09 AM, Thom Brown <thom@linux.com> wrote:
>> Perhaps something along the lines of:
>>
>> "When a CREATE FUNCTION command is executed, the install permission
>> will be checked to determine whether the LEAKPROOF attribute was
>> present. This permission will also be checked when the user tries to
>> apply the LEAKPROOF attribute using the ALTER FUNCTION command."
>>
>> I'm not sure what the last part is actually describing ("with setattr
>> permission on the function being altered."), so I'm not sure how that
>> should be read. It doesn't help that I'm not familiar with SELinux
>> terms.
>
> Right, so what it's trying to say is: whenever you modify an object,
> we check whether you've got {setattr} permission for that object and
> disallow the operation if not. However, for some operations on some
> object types, {setattr} is necessary but not sufficient. The
> paragraph is recapping, for various cases, which operations require
> additional permissions, and what those additional things are.
>
>> I was really just thinking of CREATE and LEAKPROOF, but I'm not sure
>> "CREATE" should be in there anyway.
>
> create here is referring to the sepgsql permission, not the SQL
> command, so it's correct as-is.
My bad.
--
Thom
On Wed, Mar 27, 2013 at 10:51 AM, Thom Brown <thom@linux.com> wrote: >> create here is referring to the sepgsql permission, not the SQL >> command, so it's correct as-is. > > My bad. Here's an attempt at reworking the whole section to be more understandable. I took the liberty of rearranging the text into bulleted lists, which I hope is more clear. Comments welcome. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On 27 March 2013 15:19, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Mar 27, 2013 at 10:51 AM, Thom Brown <thom@linux.com> wrote: >>> create here is referring to the sepgsql permission, not the SQL >>> command, so it's correct as-is. >> >> My bad. > > Here's an attempt at reworking the whole section to be more > understandable. I took the liberty of rearranging the text into > bulleted lists, which I hope is more clear. > > Comments welcome. This looks a lot better, apart from: s/permssions/permissions/ +1 from me. -- Thom
On Wed, Mar 27, 2013 at 11:27 AM, Thom Brown <thom@linux.com> wrote: > On 27 March 2013 15:19, Robert Haas <robertmhaas@gmail.com> wrote: >> On Wed, Mar 27, 2013 at 10:51 AM, Thom Brown <thom@linux.com> wrote: >>>> create here is referring to the sepgsql permission, not the SQL >>>> command, so it's correct as-is. >>> >>> My bad. >> >> Here's an attempt at reworking the whole section to be more >> understandable. I took the liberty of rearranging the text into >> bulleted lists, which I hope is more clear. >> >> Comments welcome. > > This looks a lot better, apart from: > > s/permssions/permissions/ > > +1 from me. OK, committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company