[PATCH] make capabilities support optional

[PATCH] make capabilities support optional

[Mike Frysinger]

The new code using libcap is quite minor, so rather than always reqiure
libcap support, make it a normal --enable type flag.  Current default
behavior is retained -- if libcap is found, it is enabled, else it is
disabled like every nfs-utils version in the past.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 aclocal/libcap.m4  |   18 +++++++++++++-----
 support/nsm/file.c |    4 ++++
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/aclocal/libcap.m4 b/aclocal/libcap.m4
index eabe507..68a624c 100644
--- a/aclocal/libcap.m4
+++ b/aclocal/libcap.m4
@@ -5,11 +5,19 @@ AC_DEFUN([AC_LIBCAP], [
   dnl look for prctl
   AC_CHECK_FUNC([prctl], , )
 
-  dnl look for the library; do not add to LIBS if found
-  AC_CHECK_LIB([cap], [cap_get_proc], [LIBCAP=-lcap], ,)
-  AC_SUBST(LIBCAP)
+  AC_ARG_ENABLE([caps],
+    [AS_HELP_STRING([--disable-caps], [Disable capabilities support])])
+
+  LIBCAP=
+
+  if test "x$enable_caps" != "xno" ; then
+    dnl look for the library; do not add to LIBS if found
+    AC_CHECK_LIB([cap], [cap_get_proc], [LIBCAP=-lcap], ,)
 
-  AC_CHECK_HEADERS([sys/capability.h], ,
-                   [AC_MSG_ERROR([libcap headers not found.])])
+    AC_CHECK_HEADERS([sys/capability.h], ,
+      [test "x$enable_caps" = "xyes" && AC_MSG_ERROR([libcap headers not found.])])
+  fi
+
+  AC_SUBST(LIBCAP)
 
 ])dnl
diff --git a/support/nsm/file.c b/support/nsm/file.c
index d469219..f4baeb9 100644
--- a/support/nsm/file.c
+++ b/support/nsm/file.c
@@ -67,7 +67,9 @@
 #endif
 
 #include <sys/types.h>
+#ifdef HAVE_SYS_CAPABILITY_H
 #include <sys/capability.h>
+#endif
 #include <sys/prctl.h>
 #include <sys/stat.h>
 
@@ -347,6 +349,7 @@ nsm_is_default_parentdir(void)
 static _Bool
 nsm_clear_capabilities(void)
 {
+#ifdef HAVE_SYS_CAPABILITY_H
 	cap_t caps;
 
 	caps = cap_from_text("cap_net_bind_service=ep");
@@ -362,6 +365,7 @@ nsm_clear_capabilities(void)
 	}
 
 	(void)cap_free(caps);
+#endif
 	return true;
 }
 
-- 
1.7.0.2

Re: [PATCH] make capabilities support optional

[Steve Dickson]

On 04/20/2010 04:46 AM, Mike Frysinger wrote:
> The new code using libcap is quite minor, so rather than always reqiure
> libcap support, make it a normal --enable type flag.  Current default
> behavior is retained -- if libcap is found, it is enabled, else it is
> disabled like every nfs-utils version in the past.
> 
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>
Committed...

steved.

Re: [PATCH] make capabilities support optional

[Chuck Lever]

On 04/23/2010 12:29 PM, Steve Dickson wrote:
> On 04/20/2010 04:46 AM, Mike Frysinger wrote:
>> The new code using libcap is quite minor, so rather than always reqiure
>> libcap support, make it a normal --enable type flag.  Current default
>> behavior is retained -- if libcap is found, it is enabled, else it is
>> disabled like every nfs-utils version in the past.
>>
>> Signed-off-by: Mike Frysinger<vapier@gentoo.org>
>>
> Committed...

I somehow missed this one.  Why are we disabling libcap?  And why are we 
adding another --enable flag when everyone has agreed that we should 
avoid that if at all possible?

It is especially on older systems that nfs-utils will break without 
libcap support.  Without CAP_NET_BIND, pmap_unregister() will fail when 
statd is shut down, leaving NSM registered with the portmapper, but with 
no active listeners.  When statd is started up again, it won't be able 
to register the new NSM listener ports.

That's the bug that was fixed by the addition of libcap.  I don't agree 
that is "quite minor."

-- 
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] make capabilities support optional

[Steve Dickson]

On 04/23/2010 01:28 PM, Chuck Lever wrote:
> On 04/23/2010 12:29 PM, Steve Dickson wrote:
>> On 04/20/2010 04:46 AM, Mike Frysinger wrote:
>>> The new code using libcap is quite minor, so rather than always reqiure
>>> libcap support, make it a normal --enable type flag.  Current default
>>> behavior is retained -- if libcap is found, it is enabled, else it is
>>> disabled like every nfs-utils version in the past.
>>>
>>> Signed-off-by: Mike Frysinger<vapier@gentoo.org>
>>>
>> Committed...
> 
> I somehow missed this one.  Why are we disabling libcap?  And why are we
> adding another --enable flag when everyone has agreed that we should
> avoid that if at all possible?
The justification I was used was it made nfs-utils more portable on 
systems/distros that may not have the libcap support. 

> 
> It is especially on older systems that nfs-utils will break without
> libcap support.  Without CAP_NET_BIND, pmap_unregister() will fail when
> statd is shut down, leaving NSM registered with the portmapper, but with
> no active listeners.  When statd is started up again, it won't be able
> to register the new NSM listener ports.
Hmm... I agree the unregister() would fail on exit, but that's the reason 
an unregister() (and then an register) is done on start up before the 
privileges are drop... Actually this how it worked for a very long time,
well before the capabilities support added... 

steved.

Re: [PATCH] make capabilities support optional

[Chuck Lever]

Hi Steve-

On 04/23/2010 02:22 PM, Steve Dickson wrote:
> On 04/23/2010 01:28 PM, Chuck Lever wrote:
>> On 04/23/2010 12:29 PM, Steve Dickson wrote:
>>> On 04/20/2010 04:46 AM, Mike Frysinger wrote:
>>>> The new code using libcap is quite minor, so rather than always reqiure
>>>> libcap support, make it a normal --enable type flag.  Current default
>>>> behavior is retained -- if libcap is found, it is enabled, else it is
>>>> disabled like every nfs-utils version in the past.
>>>>
>>>> Signed-off-by: Mike Frysinger<vapier@gentoo.org>
>>>>
>>> Committed...
>>
>> I somehow missed this one.  Why are we disabling libcap?  And why are we
>> adding another --enable flag when everyone has agreed that we should
>> avoid that if at all possible?
> The justification I was used was it made nfs-utils more portable on
> systems/distros that may not have the libcap support.

As an aside, the patch description is where we should be documenting the 
thinking behind these decisions in an audit-able and transparent manner. 
  The description for this patch doesn't have a strong justification 
IMHO.  It would be hard for any of us to come back to this patch a year 
from now and figure out exactly why this change was made.  (I say this 
having spent the last year doing just that for a long history of patches 
to statd and mount).

Back on topic: I get it that, in general, we want to allow older distros 
to build the latest nfs-utils.  However I don't think we can blithely 
rip this libcap support out, even just for old configurations.

If we really do need to drop libcap for some configurations, then such a 
change should be thoroughly tested in those environments.  Some features 
won't always work without libcap, and appropriate warnings should be 
added to man pages and/or should be displayed by statd.

>> It is especially on older systems that nfs-utils will break without
>> libcap support.  Without CAP_NET_BIND, pmap_unregister() will fail when
>> statd is shut down, leaving NSM registered with the portmapper, but with
>> no active listeners.  When statd is started up again, it won't be able
>> to register the new NSM listener ports.
> Hmm... I agree the unregister() would fail on exit, but that's the reason
> an unregister() (and then an register) is done on start up before the
> privileges are drop... Actually this how it worked for a very long time,
> well before the capabilities support added...

When I was working on it, subsequent attempts to register would always 
fail if an NSM service was already registered.  In other words, this was 
broken when I found it.

Commits e2446fda and 7dd13420 explain why CAP_NET_BIND was introduced, 
and what bugs are addressed.  Without CAP_NET_BIND, we can't guarantee 
that the NSM service can be unregistered, and neither can we guarantee 
that a privileged port, when requested, can be used for listening.

The problem is that statd drops its root privileges, so any subsequent 
attempts to acquire a privileged port (such as to do a 
pmap_unregister()) will fail, and leave the NSM service registered.

Since rpcbind registration is done via AF_UNIX, it can work without 
CAP_NET_BIND.  But it requires that the registering UID be the same as 
the UID used to unregister it.  Thus both registration and 
unregistration must be root, or both must be done as "rpcuser."  Since 
statd drops its privileges just after start-up, I chose the latter.

However, using lower privileges means a pmap_unregister() will always 
fail in common cases.  So CAP_NET_BIND is retained for this purpose.

We also have to worry about mount.nfs these days, as it pings the statd 
service when mounting with "lock".  If NSM is registered, but no statd 
is listening (as would be the case if statd couldn't unregister itself 
on its way down), then most subsequent NFSv2/v3 mounts would hang.  This 
is why even "unregister/register" at start-up isn't always adequate.

-- 
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] make capabilities support optional

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1097 bytes --]

On Friday 23 April 2010 15:12:33 Chuck Lever wrote:
> If we really do need to drop libcap for some configurations, then such a
> change should be thoroughly tested in those environments.  Some features
> won't always work without libcap, and appropriate warnings should be
> added to man pages and/or should be displayed by statd.

there should be appropriate documentation regardless.  current nfs-utils lists 
no information at all in ChangeLog/NEWS/README/INSTALL or any other document 
explaining why/what/how libcap is needed/used.  you cant do documentless dumps 
on distro maintainers and expect them to "just know" what is going on.  this 
isnt the first time the nfs related packages suddenly started requiring new 
libraries out of the blue when in reality things could be done optionally, nor 
is this the first patch ive sent to try and address what appears to be 
unnecessarily hard deps.  kerberos readily comes to mind.

i'd point out that from a distro POV, the DAEMON section of the top level 
README is single handily the best thing added in a long time.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] make capabilities support optional

[Chuck Lever]

On 04/23/2010 03:29 PM, Mike Frysinger wrote:
> On Friday 23 April 2010 15:12:33 Chuck Lever wrote:
>> If we really do need to drop libcap for some configurations, then such a
>> change should be thoroughly tested in those environments.  Some features
>> won't always work without libcap, and appropriate warnings should be
>> added to man pages and/or should be displayed by statd.
>
> there should be appropriate documentation regardless.  current nfs-utils lists
> no information at all in ChangeLog/NEWS/README/INSTALL or any other document
> explaining why/what/how libcap is needed/used.  you cant do documentless dumps
> on distro maintainers and expect them to "just know" what is going on.

"git log" has served as the ChangeLog for some time now.  The commits I 
referenced in my last e-mail explain exactly why libcap was introduced.

You can also use "git annotate <some file>" to see exactly when 
particular lines of code were introduced or change, who changed them, 
and what git commit you can look at for more information.

The nfs-utils git repo is available on linux-nfs.org, and gitweb can 
guide you through all of this information with a nice GUI.

If all of that fails, you can still post a question here on this mailing 
list.

> this
> isnt the first time the nfs related packages suddenly started requiring new
> libraries out of the blue when in reality things could be done optionally, nor
> is this the first patch ive sent to try and address what appears to be
> unnecessarily hard deps.  kerberos readily comes to mind.

Patches are posted on this mailing list for review before they are 
committed.  Anyone has a chance to object, comment, or suggest a simpler 
way to do things.

Of course, that only works if you post to this list too.  I don't see 
this patch in the mailing list archives, but I may have missed something.

It's important to realize that it's much harder to make things optional 
than to insist that they be built in.  Adding build options means 
there's more work for distributors to configure the build, and it 
exponentially increases our test matrix (which is already out of 
control).  Every change now has to be tested with each combination of 
build options.  Add one more --enable option, and that doubles the 
number of combinations.

I don't have a problem with making new features optional, when it's 
necessary.  What I'm objecting to here is that it's easy to add 
autotools machinery, but it's a lot more difficult to check that all of 
nfs-utils still works in all cases after such a change, and that the new 
option doesn't introduce a regression (which it does in this case).

I didn't see a clear explanation of why your proposed change was 
necessary, nor was it clear from the patch description that you 
understood why libcap was added in the first place, nor does it seem 
that the regressions caused by disabling libcap are adequately addressed.

So, why do you want to make libcap optional?  And why is yet another 
build option needed (rather than just using AC_FUNCTIONS and HAVE_LIBCAP) ?

-- 
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] make capabilities support optional

[Steve Dickson]

On 04/23/2010 03:12 PM, Chuck Lever wrote:
> Hi Steve-
> 
> On 04/23/2010 02:22 PM, Steve Dickson wrote:
>> On 04/23/2010 01:28 PM, Chuck Lever wrote:
>>> On 04/23/2010 12:29 PM, Steve Dickson wrote:
>>>> On 04/20/2010 04:46 AM, Mike Frysinger wrote:
>>>>> The new code using libcap is quite minor, so rather than always
>>>>> reqiure
>>>>> libcap support, make it a normal --enable type flag.  Current default
>>>>> behavior is retained -- if libcap is found, it is enabled, else it is
>>>>> disabled like every nfs-utils version in the past.
>>>>>
>>>>> Signed-off-by: Mike Frysinger<vapier@gentoo.org>
>>>>>
>>>> Committed...
>>>
>>> I somehow missed this one.  Why are we disabling libcap?  And why are we
>>> adding another --enable flag when everyone has agreed that we should
>>> avoid that if at all possible?
>> The justification I was used was it made nfs-utils more portable on
>> systems/distros that may not have the libcap support.
> 
> As an aside, the patch description is where we should be documenting the
> thinking behind these decisions in an audit-able and transparent manner.
>  The description for this patch doesn't have a strong justification
> IMHO.  It would be hard for any of us to come back to this patch a year
> from now and figure out exactly why this change was made.  (I say this
> having spent the last year doing just that for a long history of patches
> to statd and mount).
True, the patch description could have been a bit more verbose, but I 
feel I understood the reason for the patch and that reason the made
sense to me... I feel backwards compatibility is important..  

> 
> Back on topic: I get it that, in general, we want to allow older distros
> to build the latest nfs-utils.  However I don't think we can blithely
> rip this libcap support out, even just for old configurations.
> 
> If we really do need to drop libcap for some configurations, then such a
> change should be thoroughly tested in those environments.  Some features
> won't always work without libcap, and appropriate warnings should be
> added to man pages and/or should be displayed by statd.
Well dropping libcap is not the default and I don't see us (i.e. upstream) 
ever making it the default... If people want set that config flag, its up to 
them to document the ramifications, IMHO...

> 
>>> It is especially on older systems that nfs-utils will break without
>>> libcap support.  Without CAP_NET_BIND, pmap_unregister() will fail when
>>> statd is shut down, leaving NSM registered with the portmapper, but with
>>> no active listeners.  When statd is started up again, it won't be able
>>> to register the new NSM listener ports.
>> Hmm... I agree the unregister() would fail on exit, but that's the reason
>> an unregister() (and then an register) is done on start up before the
>> privileges are drop... Actually this how it worked for a very long time,
>> well before the capabilities support added...
> 
> When I was working on it, subsequent attempts to register would always
> fail if an NSM service was already registered.  In other words, this was
> broken when I found it.
> 
> Commits e2446fda and 7dd13420 explain why CAP_NET_BIND was introduced,
> and what bugs are addressed.  Without CAP_NET_BIND, we can't guarantee
> that the NSM service can be unregistered, and neither can we guarantee
> that a privileged port, when requested, can be used for listening.
> 
> The problem is that statd drops its root privileges, so any subsequent
> attempts to acquire a privileged port (such as to do a
> pmap_unregister()) will fail, and leave the NSM service registered.
> 
> Since rpcbind registration is done via AF_UNIX, it can work without
> CAP_NET_BIND.  But it requires that the registering UID be the same as
> the UID used to unregister it.  Thus both registration and
> unregistration must be root, or both must be done as "rpcuser."  Since
> statd drops its privileges just after start-up, I chose the latter.
> 
> However, using lower privileges means a pmap_unregister() will always
> fail in common cases.  So CAP_NET_BIND is retained for this purpose.
> 
> We also have to worry about mount.nfs these days, as it pings the statd
> service when mounting with "lock".  If NSM is registered, but no statd
> is listening (as would be the case if statd couldn't unregister itself
> on its way down), then most subsequent NFSv2/v3 mounts would hang.  This
> is why even "unregister/register" at start-up isn't always adequate.
> 
I can't disagree with any of the above... but the above assumes that
the --disable libcap will some how become the default... that is 
not the case... 

All that config flag allows is backwards compatibility,  which I know 
we both think is a good thing... 

steved.

Re: [PATCH] make capabilities support optional

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 2838 bytes --]

On Friday 23 April 2010 16:09:27 Chuck Lever wrote:
> On 04/23/2010 03:29 PM, Mike Frysinger wrote:
> > On Friday 23 April 2010 15:12:33 Chuck Lever wrote:
> >> If we really do need to drop libcap for some configurations, then such a
> >> change should be thoroughly tested in those environments.  Some features
> >> won't always work without libcap, and appropriate warnings should be
> >> added to man pages and/or should be displayed by statd.
> > 
> > there should be appropriate documentation regardless.  current nfs-utils
> > lists no information at all in ChangeLog/NEWS/README/INSTALL or any
> > other document explaining why/what/how libcap is needed/used.  you cant
> > do documentless dumps on distro maintainers and expect them to "just
> > know" what is going on.
> 
> "git log" has served as the ChangeLog for some time now.  The commits I
> referenced in my last e-mail explain exactly why libcap was introduced.

none of the scm metadata is relevant to distro maintainers.  that info is fine 
for developers of nfs-utils, but that's it.  people attempting to package nfs-
utils shouldnt need to crawl these backends to try and glean info themselves.

> Patches are posted on this mailing list for review before they are
> committed.  Anyone has a chance to object, comment, or suggest a simpler
> way to do things.

again, this isnt relevant to distro maintainers.

> It's important to realize that it's much harder to make things optional
> than to insist that they be built in.  Adding build options means
> there's more work for distributors to configure the build, and it
> exponentially increases our test matrix (which is already out of
> control).  Every change now has to be tested with each combination of
> build options.  Add one more --enable option, and that doubles the
> number of combinations.

hardcoding optional features in autotools is worse for distro maintainers than 
proper optional configure flags.  dont kid yourself in this regard.

> I didn't see a clear explanation of why your proposed change was
> necessary, nor was it clear from the patch description that you
> understood why libcap was added in the first place, nor does it seem
> that the regressions caused by disabling libcap are adequately addressed.

things worked before libcap was introduced, so clearly it's possible.  it may 
be reduced security footprint, but plenty of people are fine with it.

> So, why do you want to make libcap optional?

there are plenty of systems where privileges are meaningless (like embedded) 
and so libcap is pure cruft.

> And why is yet another build option needed (rather than just using
> AC_FUNCTIONS and HAVE_LIBCAP) ?

magic detections are terrible for distro maintainers and one of the things we 
spend a lot of time fixing.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] make capabilities support optional

[Chuck Lever]

On 04/24/2010 12:42 AM, Mike Frysinger wrote:
> On Friday 23 April 2010 16:09:27 Chuck Lever wrote:
>> On 04/23/2010 03:29 PM, Mike Frysinger wrote:
>>> On Friday 23 April 2010 15:12:33 Chuck Lever wrote:
>>>> If we really do need to drop libcap for some configurations, then such a
>>>> change should be thoroughly tested in those environments.  Some features
>>>> won't always work without libcap, and appropriate warnings should be
>>>> added to man pages and/or should be displayed by statd.
>>>
>>> there should be appropriate documentation regardless.  current nfs-utils
>>> lists no information at all in ChangeLog/NEWS/README/INSTALL or any
>>> other document explaining why/what/how libcap is needed/used.  you cant
>>> do documentless dumps on distro maintainers and expect them to "just
>>> know" what is going on.
>>
>> "git log" has served as the ChangeLog for some time now.  The commits I
>> referenced in my last e-mail explain exactly why libcap was introduced.
>
> none of the scm metadata is relevant to distro maintainers.  that info is fine
> for developers of nfs-utils, but that's it.

Obviously, that metadata _is_ relevant to distro maintainers, as your 
example shows.  The nfs-utils ChangeLog is an exact copy of the the git 
log (up to about mid 2006).  Why keep an extra copy?

However, as soon as a distributor sends patches (rather than, say, 
simply posting a bug report), you become a developer, and are thus 
obligated to act like one by reviewing the content of the local source 
management system before making changes, and by posting your patches to 
this list for us to review.

> people attempting to package nfs-
> utils shouldnt need to crawl these backends to try and glean info themselves.

As I pointed out, you don't need git on your local system to look at 
this metadata: it's already available on linux-nfs.org if you have a web 
browser.

>> Patches are posted on this mailing list for review before they are
>> committed.  Anyone has a chance to object, comment, or suggest a simpler
>> way to do things.
>
> again, this isnt relevant to distro maintainers.

How are nfs-utils developers supposed to know of a problem if distro 
maintainers don't tell us?

I specifically asked on this list about libcap before adding it.  We've 
been discussing the addition of libsqlite and libtirpc as well, and I 
specifically requested feedback from distributors.  There was no 
response.  So how are we supposed to know these are problems?  Where 
else should I have asked this question?

>> It's important to realize that it's much harder to make things optional
>> than to insist that they be built in.  Adding build options means
>> there's more work for distributors to configure the build, and it
>> exponentially increases our test matrix (which is already out of
>> control).  Every change now has to be tested with each combination of
>> build options.  Add one more --enable option, and that doubles the
>> number of combinations.
>
> hardcoding optional features in autotools is worse for distro maintainers than
> proper optional configure flags.  dont kid yourself in this regard.

>> I didn't see a clear explanation of why your proposed change was
>> necessary, nor was it clear from the patch description that you
>> understood why libcap was added in the first place, nor does it seem
>> that the regressions caused by disabling libcap are adequately addressed.
>
> things worked before libcap was introduced, so clearly it's possible.  it may
> be reduced security footprint, but plenty of people are fine with it.

I claim that "things" did not work.  When statd was shut down, it left a 
dangling rpcbind registration, and that's a bug in all environments.

If my bug fix is not complete or is inappropriate for some environments, 
then we should have a discussion about that.  It's pretty hard to tell 
what you were trying to address from your patch description (and that's 
all I have right now because the actual patch was never publicly posted).

>> So, why do you want to make libcap optional?
>
> there are plenty of systems where privileges are meaningless (like embedded)
> and so libcap is pure cruft.

In that case, your patch description should explain that so we can 
understand why you've added another --enable switch.  These switches are 
overused, so a clear rationalization is needed when adding yet another.

By and large, those who participate on this list felt that "--enable" 
flags are less desirable than automatic feature checking.  Your view is 
novel, I think.

>> And why is yet another build option needed (rather than just using
>> AC_FUNCTIONS and HAVE_LIBCAP) ?
>
> magic detections are terrible for distro maintainers and one of the things we
> spend a lot of time fixing.

nfs-utils uses autotools, so that's what we have.  How else should it be 
done?

-- 
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] make capabilities support optional

[Chuck Lever]

On 04/23/2010 06:22 PM, Steve Dickson wrote:
>
>
> On 04/23/2010 03:12 PM, Chuck Lever wrote:
>> Hi Steve-
>>
>> On 04/23/2010 02:22 PM, Steve Dickson wrote:
>>> On 04/23/2010 01:28 PM, Chuck Lever wrote:
>>>> On 04/23/2010 12:29 PM, Steve Dickson wrote:
>>>>> On 04/20/2010 04:46 AM, Mike Frysinger wrote:
>>>>>> The new code using libcap is quite minor, so rather than always
>>>>>> reqiure
>>>>>> libcap support, make it a normal --enable type flag.  Current default
>>>>>> behavior is retained -- if libcap is found, it is enabled, else it is
>>>>>> disabled like every nfs-utils version in the past.
>>>>>>
>>>>>> Signed-off-by: Mike Frysinger<vapier@gentoo.org>
>>>>>>
>>>>> Committed...
>>>>
>>>> I somehow missed this one.  Why are we disabling libcap?  And why are we
>>>> adding another --enable flag when everyone has agreed that we should
>>>> avoid that if at all possible?
>>> The justification I was used was it made nfs-utils more portable on
>>> systems/distros that may not have the libcap support.
>>
>> As an aside, the patch description is where we should be documenting the
>> thinking behind these decisions in an audit-able and transparent manner.
>>   The description for this patch doesn't have a strong justification
>> IMHO.  It would be hard for any of us to come back to this patch a year
>> from now and figure out exactly why this change was made.  (I say this
>> having spent the last year doing just that for a long history of patches
>> to statd and mount).
> True, the patch description could have been a bit more verbose, but I
> feel I understood the reason for the patch and that reason the made
> sense to me...

The problem is that this patch was never posted publicly, so you were 
the only reviewer.  The rest of us have no idea what this is about.

> I feel backwards compatibility is important..

Sure, but this isn't necessarily the way to go about it, in this case. 
It's hard to know though, since the patch description was vague, and the 
patch itself was never publicly posted.

I'm all for choice, but let's make sure it's _informed_ choice where 
ever possible.

>> Back on topic: I get it that, in general, we want to allow older distros
>> to build the latest nfs-utils.  However I don't think we can blithely
>> rip this libcap support out, even just for old configurations.
>>
>> If we really do need to drop libcap for some configurations, then such a
>> change should be thoroughly tested in those environments.  Some features
>> won't always work without libcap, and appropriate warnings should be
>> added to man pages and/or should be displayed by statd.
> Well dropping libcap is not the default and I don't see us (i.e. upstream)
> ever making it the default... If people want set that config flag, its up to
> them to document the ramifications, IMHO...

Er, I think it _is_ up to us chickens to document the ramifications.  If 
some new --enable flag just shows up on ./configure, how am I going to 
know whether I should set it or not?  I certainly couldn't tell why 
another --enable flag was needed just for libcap until Mike spelled it out.

>>>> It is especially on older systems that nfs-utils will break without
>>>> libcap support.  Without CAP_NET_BIND, pmap_unregister() will fail when
>>>> statd is shut down, leaving NSM registered with the portmapper, but with
>>>> no active listeners.  When statd is started up again, it won't be able
>>>> to register the new NSM listener ports.
>>> Hmm... I agree the unregister() would fail on exit, but that's the reason
>>> an unregister() (and then an register) is done on start up before the
>>> privileges are drop... Actually this how it worked for a very long time,
>>> well before the capabilities support added...
>>
>> When I was working on it, subsequent attempts to register would always
>> fail if an NSM service was already registered.  In other words, this was
>> broken when I found it.
>>
>> Commits e2446fda and 7dd13420 explain why CAP_NET_BIND was introduced,
>> and what bugs are addressed.  Without CAP_NET_BIND, we can't guarantee
>> that the NSM service can be unregistered, and neither can we guarantee
>> that a privileged port, when requested, can be used for listening.
>>
>> The problem is that statd drops its root privileges, so any subsequent
>> attempts to acquire a privileged port (such as to do a
>> pmap_unregister()) will fail, and leave the NSM service registered.
>>
>> Since rpcbind registration is done via AF_UNIX, it can work without
>> CAP_NET_BIND.  But it requires that the registering UID be the same as
>> the UID used to unregister it.  Thus both registration and
>> unregistration must be root, or both must be done as "rpcuser."  Since
>> statd drops its privileges just after start-up, I chose the latter.
>>
>> However, using lower privileges means a pmap_unregister() will always
>> fail in common cases.  So CAP_NET_BIND is retained for this purpose.
>>
>> We also have to worry about mount.nfs these days, as it pings the statd
>> service when mounting with "lock".  If NSM is registered, but no statd
>> is listening (as would be the case if statd couldn't unregister itself
>> on its way down), then most subsequent NFSv2/v3 mounts would hang.  This
>> is why even "unregister/register" at start-up isn't always adequate.
>>
> I can't disagree with any of the above... but the above assumes that
> the --disable libcap will some how become the default... that is
> not the case...
>
> All that config flag allows is backwards compatibility,  which I know
> we both think is a good thing...

Now that we've answered the question of "why are we doing this," the 
question in my mind is how we want statd to behave if libcap isn't 
available.

At the very least, I think the man page should mention this bug somehow, 
and maybe a warning should be generated during the build.  Since this is 
mostly for embedded systems, according to Mike, I doubt an actual 
generated run-time warning would be visible to anyone.

-- 
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] make capabilities support optional

[Steve Dickson]

On 04/26/2010 11:24 AM, Chuck Lever wrote:
> On 04/23/2010 06:22 PM, Steve Dickson wrote:
>>> As an aside, the patch description is where we should be documenting the
>>> thinking behind these decisions in an audit-able and transparent manner.
>>>   The description for this patch doesn't have a strong justification
>>> IMHO.  It would be hard for any of us to come back to this patch a year
>>> from now and figure out exactly why this change was made.  (I say this
>>> having spent the last year doing just that for a long history of patches
>>> to statd and mount).
>> True, the patch description could have been a bit more verbose, but I
>> feel I understood the reason for the patch and that reason the made
>> sense to me...
> 
> The problem is that this patch was never posted publicly, so you were
> the only reviewer.  The rest of us have no idea what this is about.
Well here is where I got the patch: 
    http://marc.info/?l=linux-nfs&m=127175324400983&w=2
off the mailing list.

> 
>> I feel backwards compatibility is important..
> 
> Sure, but this isn't necessarily the way to go about it, in this case.
> It's hard to know though, since the patch description was vague, and the
> patch itself was never publicly posted.
I agree the description was a bit vague, but the patch was posted 
publicly... 

>> Well dropping libcap is not the default and I don't see us (i.e.
>> upstream)
>> ever making it the default... If people want set that config flag, its
>> up to
>> them to document the ramifications, IMHO...
> 
> Er, I think it _is_ up to us chickens to document the ramifications.  If
> some new --enable flag just shows up on ./configure, how am I going to
> know whether I should set it or not?  
Hopefully the setting of an configure flag would come out in the 
discussion about the problem... Any changing of default values,
especially ones that will cause a recompile needed to be 
relayed early in any bug discussion... 

> 
> Now that we've answered the question of "why are we doing this," the
> question in my mind is how we want statd to behave if libcap isn't
> available.
> 
> At the very least, I think the man page should mention this bug somehow,
> and maybe a warning should be generated during the build.  Since this is
> mostly for embedded systems, according to Mike, I doubt an actual
> generated run-time warning would be visible to anyone.
> 
Mike would you be willing to test this out and document any
differences in statd behaviour with and without that config
flag set?

steved.

Re: [PATCH] make capabilities support optional

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 5488 bytes --]

On Monday 26 April 2010 11:12:40 Chuck Lever wrote:
> On 04/24/2010 12:42 AM, Mike Frysinger wrote:
> > On Friday 23 April 2010 16:09:27 Chuck Lever wrote:
> >> "git log" has served as the ChangeLog for some time now.  The commits I
> >> referenced in my last e-mail explain exactly why libcap was introduced.
> > 
> > none of the scm metadata is relevant to distro maintainers.  that info is
> > fine for developers of nfs-utils, but that's it.
> 
> Obviously, that metadata _is_ relevant to distro maintainers, as your
> example shows.  The nfs-utils ChangeLog is an exact copy of the the git
> log (up to about mid 2006).  Why keep an extra copy?

it's trivial to include the full ChangeLog in the dist tarball without keeping 
it in git.  plenty of GNU projects are doing this already.

> However, as soon as a distributor sends patches (rather than, say,
> simply posting a bug report), you become a developer, and are thus
> obligated to act like one by reviewing the content of the local source
> management system before making changes, and by posting your patches to
> this list for us to review.

expecting anyone who sends a patch to be fully versed in NFS internals and the 
full history is completely unreasonable.  i'm not saying people shouldnt do a 
little research here, but your position appears to be poor: if you want nfs-
utils info, regardless of who you are, you must go to the scm source and dive 
through mounds of history and divine the answer yourself.

> > people attempting to package nfs-
> > utils shouldnt need to crawl these backends to try and glean info
> > themselves.
> 
> As I pointed out, you don't need git on your local system to look at
> this metadata: it's already available on linux-nfs.org if you have a web
> browser.

the interface is irrelevant -- you're still saying people have to dig through 
piles of information that is not relevant to packaging.

> >> Patches are posted on this mailing list for review before they are
> >> committed.  Anyone has a chance to object, comment, or suggest a simpler
> >> way to do things.
> > 
> > again, this isnt relevant to distro maintainers.
> 
> How are nfs-utils developers supposed to know of a problem if distro
> maintainers don't tell us?

you're saying distro maintainers are expected to subscribe to the development 
list and review patches ?  that's bunk.

> I specifically asked on this list about libcap before adding it.  We've
> been discussing the addition of libsqlite and libtirpc as well, and I
> specifically requested feedback from distributors.  There was no
> response.  So how are we supposed to know these are problems?  Where
> else should I have asked this question?

create a list/alias maintainers can subscribe to and anytime you want to ask 
them a question, cc that.  i used to be on the nfs list but there is way too 
much noise going through about crap i dont care about.  i'm not going to wade 
through it all to find the one e-mail a month that is relevant to me.

> I claim that "things" did not work.  When statd was shut down, it left a
> dangling rpcbind registration, and that's a bug in all environments.

perhaps, but some people are ok with that.  considering that has always been 
the behavior then, ive never seen one complaint about this via Gentoo.  clear 
documentation explaining the tradeoffs would be sufficient and there are 
plenty of systems where this bug is meaningless.  on my nfs servers, either 
everything is up, or everything is down.  there is no "in between" where some 
services are left running while others are not.

> If my bug fix is not complete or is inappropriate for some environments,
> then we should have a discussion about that.  It's pretty hard to tell
> what you were trying to address from your patch description

i already stated my logic pretty clearly in previous e-mails.  nfs-utils 
provides no documentation, libraries get dropped in arbitrarily, so i made 
things optional.

> (and that's all I have right now because the actual patch was never publicly
> posted).

go read a mailing archive then.  not my problem if your client ate it.

> >> So, why do you want to make libcap optional?
> > 
> > there are plenty of systems where privileges are meaningless (like
> > embedded) and so libcap is pure cruft.
> 
> In that case, your patch description should explain that so we can
> understand why you've added another --enable switch.  These switches are
> overused, so a clear rationalization is needed when adding yet another.
> 
> By and large, those who participate on this list felt that "--enable"
> flags are less desirable than automatic feature checking.  Your view is
> novel, I think.

the view of distro maintainers in this regard matters more than that of 
developers.  the configure script is designed for the end packagers to use, 
not developers.  my position here is not unique to me or something i pulled 
out of nowhere.

> >> And why is yet another build option needed (rather than just using
> >> AC_FUNCTIONS and HAVE_LIBCAP) ?
> > 
> > magic detections are terrible for distro maintainers and one of the
> > things we spend a lot of time fixing.
> 
> nfs-utils uses autotools, so that's what we have.  How else should it be
> done?

i already told you how and posted a patch: a proper configure flag should 
*always* be used to control optional non-C library linking.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] make capabilities support optional

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 970 bytes --]

On Monday 26 April 2010 12:10:00 Steve Dickson wrote:
> On 04/26/2010 11:24 AM, Chuck Lever wrote:
> > At the very least, I think the man page should mention this bug somehow,
> > and maybe a warning should be generated during the build.  Since this is
> > mostly for embedded systems, according to Mike, I doubt an actual
> > generated run-time warning would be visible to anyone.
> 
> Mike would you be willing to test this out and document any
> differences in statd behaviour with and without that config
> flag set?

i think a BUGS section in statd is sufficient, but if you wanted to go 
further, then a simple syslog() would be OK as well.  dumping warnings to 
stderr/stdout is obnoxious and non-standard for daemons.  those really only 
should be written when a daemon is quitting due to misconfiguration.

i can send a patch for the man page, but i'd only copy and paste what Chuck 
wrote (about statd remaining registered upon exit).
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] make capabilities support optional

[Steve Dickson]

On 04/26/2010 12:51 PM, Mike Frysinger wrote:
> On Monday 26 April 2010 12:10:00 Steve Dickson wrote:
>> On 04/26/2010 11:24 AM, Chuck Lever wrote:
>>> At the very least, I think the man page should mention this bug somehow,
>>> and maybe a warning should be generated during the build.  Since this is
>>> mostly for embedded systems, according to Mike, I doubt an actual
>>> generated run-time warning would be visible to anyone.
>>
>> Mike would you be willing to test this out and document any
>> differences in statd behaviour with and without that config
>> flag set?
> 
> i think a BUGS section in statd is sufficient, but if you wanted to go 
> further, then a simple syslog() would be OK as well.  dumping warnings to 
> stderr/stdout is obnoxious and non-standard for daemons.  those really only 
> should be written when a daemon is quitting due to misconfiguration.
I agree... a blurb in the BUGS section is all that is needed..
> 
> i can send a patch for the man page, but i'd only copy and paste what Chuck 
> wrote (about statd remaining registered upon exit).
That would be good... thanks!

steved.

Re: [PATCH] make capabilities support optional

[Chuck Lever]

On 04/26/2010 12:46 PM, Mike Frysinger wrote:
> On Monday 26 April 2010 11:12:40 Chuck Lever wrote:

>> However, as soon as a distributor sends patches (rather than, say,
>> > simply posting a bug report), you become a developer, and are thus
>> > obligated to act like one by reviewing the content of the local source
>> > management system before making changes, and by posting your patches to
>> > this list for us to review.
> expecting anyone who sends a patch to be fully versed in NFS internals and the
> full history is completely unreasonable.

I didn't say "anyone."  We were specifically discussing distributors, 
and specifically distributors that regularly send patches.

> i'm not saying people shouldnt do a
> little research here, but your position appears to be poor: if you want nfs-
> utils info, regardless of who you are, you must go to the scm source and dive
> through mounds of history and divine the answer yourself.

That's not my position at all.  After you claimed the ChangeLog is not 
kept updated, I merely observed that ChangeLog information is available 
in the git log, and on the web.  Asking the maintainer to copy the git 
log into the ChangeLog during every release for tarball users is 
perfectly reasonable.

>>> > > people attempting to package nfs-
>>> > > utils shouldnt need to crawl these backends to try and glean info
>>> > > themselves.
>> >
>> > As I pointed out, you don't need git on your local system to look at
>> > this metadata: it's already available on linux-nfs.org if you have a web
>> > browser.
> the interface is irrelevant -- you're still saying people have to dig through
> piles of information that is not relevant to packaging.

Have you looked at the output of git log?  It's basically the same 
content as the ChangeLog file, up to 2006.  It's exactly the information 
you were asking for.

>>>> > >> Patches are posted on this mailing list for review before they are
>>>> > >> committed.  Anyone has a chance to object, comment, or suggest a simpler
>>>> > >> way to do things.
>>> > >
>>> > > again, this isnt relevant to distro maintainers.
>> >
>> > How are nfs-utils developers supposed to know of a problem if distro
>> > maintainers don't tell us?
> you're saying distro maintainers are expected to subscribe to the development
> list and review patches ?  that's bunk.

Anyone who is a regular contributor should subscribe to the mailing list 
and review patches they care about to stay up to date.  If you happen to 
choose not to do that, then you really shouldn't be surprised when asked 
to provide more detail on a patch, or to follow the normal patch 
submission rules.

If you are a distro maintainer, you can either report problems to us, 
ask for new features, or post patches.  As a maintainer, if you want to 
post a lot of patches, you should follow the patch submission rules the 
rest of us are beholden to.  That's only fair to the rest of us, and it 
means we can do a better job reviewing everyone's changes.  How is that 
unreasonable?

>> I claim that "things" did not work.  When statd was shut down, it left a
>> dangling rpcbind registration, and that's a bug in all environments.
>
> perhaps, but some people are ok with that.  considering that has always been
> the behavior then, ive never seen one complaint about this via Gentoo.  clear
> documentation explaining the tradeoffs would be sufficient and there are
> plenty of systems where this bug is meaningless.  on my nfs servers, either
> everything is up, or everything is down.  there is no "in between" where some
> services are left running while others are not.

My point is that explanation should have been included in the patch 
description at the very beginning, because you should realize that not 
everyone else is served by your changes, either, and many of us don't 
run Gentoo and would not have known the details of your server 
configuration.

>>>> So, why do you want to make libcap optional?
>>>
>>> there are plenty of systems where privileges are meaningless (like
>>> embedded) and so libcap is pure cruft.
>>
>> In that case, your patch description should explain that so we can
>> understand why you've added another --enable switch.  These switches are
>> overused, so a clear rationalization is needed when adding yet another.
>>
>> By and large, those who participate on this list felt that "--enable"
>> flags are less desirable than automatic feature checking.  Your view is
>> novel, I think.
>
> the view of distro maintainers in this regard matters more than that of
> developers.  the configure script is designed for the end packagers to use,
> not developers.  my position here is not unique to me or something i pulled
> out of nowhere.

It may be true that maintainers care more about the configure script 
than developers, but none of those maintainers were here to express that 
opinion or comment on any patches that changed the configure script.

Basically, how are we supposed to know your opinions if you refuse to 
participate?

-- 
chuck[dot]lever[at]oracle[dot]com