bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux


From: Bruno Haible
Subject: Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux
Date: Thu, 01 Dec 2022 12:52:43 +0100

Hi Ondrej,

Ondrej Valousek wrote:
> Just sending the modified patch (fixed formatting + few suggestions from 
> Andreas included)
> Does it look OK now?

Some formatting issues, still. Details below.

> diff --git a/doc/acl-nfsv4.txt b/doc/acl-nfsv4.txt
> new file mode 100644
> index 000000000..552a62ca3
> --- /dev/null
> +++ b/doc/acl-nfsv4.txt

Thanks for adding this file. It will help maintenance.

> @@ -0,0 +1,17 @@
> +General introduction:
> +   https://linux.die.net/man/5/nfs4_acl
> +
> +The NFSv4 acls are defined in RFC7530 and as such, every NFSv4 server 
> supporting ACLs
> +will support this kind of ACLs (note the difference from POSIX draft ACLs)
> +
> +The ACLs can be obtained via the nfsv4-acl-tools, i.e.
> +
> +$ nfs4_getfacl <file>
> +
> +# file: <file>
> +A::OWNER@:rwaDxtTnNcCy
> +A::GROUP@:rwaDxtTnNcy
> +A::EVERYONE@:rwaDxtTnNcy
> +
> +GNULib is aiming to only provide a basic support of these, i.e. recognize 
> trivial

Please spell it 'Gnulib', not 'GNULib'.

> +and non-trivial ACLs
> diff --git a/lib/acl-internal.c b/lib/acl-internal.c
> index be244c67a..86df38854 100644
> --- a/lib/acl-internal.c
> +++ b/lib/acl-internal.c
> @@ -25,6 +25,9 @@
>  
>  #if USE_ACL && HAVE_ACL_GET_FILE /* Linux, FreeBSD, Mac OS X, IRIX, Tru64, 
> Cygwin >= 2.5 */
>  
> +#include <string.h>
> +# include <arpa/inet.h>

We try to keep a visible indentation level for preprocessor directives/
statements/instructions. The indentation level is indicated by spaces
after the '#'. So, here please insert a space before 'include <string.h>'.

> +
>  # if HAVE_ACL_TYPE_EXTENDED /* Mac OS X */
>  
>  /* ACL is an ACL, from a file, stored as type ACL_TYPE_EXTENDED.
> @@ -122,6 +125,98 @@ acl_default_nontrivial (acl_t acl)
>    return (acl_entries (acl) > 0);
>  }
>  
> +#define ACE4_WHO_OWNER    "OWNER@"
> +#define ACE4_WHO_GROUP    "GROUP@"
> +#define ACE4_WHO_EVERYONE "EVERYONE@"
> +
> +#define ACE4_ACCESS_ALLOWED_ACE_TYPE   0
> +#define ACE4_ACCESS_DENIED_ACE_TYPE    1

In these preprocessor statements, we are at preprocessor indentation
level 2. So please change '#define' to '#  define' here.

> +/* ACE flag values */

What does this comment belong to? The two lines above or the line below?
If it's the lines above, then the comment should be moved up. If it's
the line below, a blank line should be removed.

> +
> +#define ACE4_IDENTIFIER_GROUP          0x00000040
> +#define ROUNDUP(x, y)                  (((x) + (y) - 1) & - (y))
> +
> +int
> +acl_nfs4_nontrivial (char *xattr, int len)
> +{
> +  int ret = 0,wholen,bufs = len;

Please define one variable per line. When reading the code, we want to see
quickly where a certain variable is declared, and that is significantly
harder if multiple variables are declared on the same line, especially
with initializations.

> +  uint32_t flag,type,num_aces = ntohl(*((uint32_t*)(xattr))); /* Grab the 
> number of aces in the acl */

Likewise. Also, please add a space after 'ntohl' because it's a function call.

> +  char *bufp = xattr;
> +  int num_a_aces = 0, num_d_aces = 0;

Please define one variable per line.

> +  ret = 0;
> +  bufp += 4;  /* sizeof(uint32_t); */
> +  bufs -= 4;
> +
> +  for(uint32_t ace_n = 0; num_aces > ace_n ; ace_n++) {

Please add a space after 'for'. Also put the opening brace on a separate line.

> +    int d_ptr;
> +
> +    /* Get the acl type */
> +    if(bufs <= 0) 

Please add a space after 'if'.

> +      return -1;
> +
> +    type = ntohl(*((uint32_t*)bufp));

Please add a space after 'ntohl' because it's a function call.

> +    bufp += 4;
> +    bufs -= 4;
> +    if (bufs <= 0)
> +      return -1;
> +
> +    flag = ntohl(*((uint32_t*)bufp));

Likewise.

> +    /* As per RFC 7530, the flag should be 0, but we are just generous to 
> Netapp
> +     * and also accept the Group flag
> +     */
> +    if (flag & ~ACE4_IDENTIFIER_GROUP)
> +      return 1;
> +
> +    /* we skip mask - 
> +     * it's too risky to test it and it does not seem to be actually needed 
> */
> +    bufp += 2*4;
> +    bufs -= 2*4;
> +
> +    if (bufs <= 0) 
> +      return -1;
> +
> +    wholen = ntohl(*((uint32_t*)bufp));

Likewise.

> +
> +    bufp += 4;
> +    bufs -= 4;
> +
> +    /* Get the who string */
> +    if (bufs <= 0)
> +      return -1;
> +
> +    /* for trivial ACL, we expect max 5 (typically 3) ACES, 3 Allow, 2 deny 
> */ 
> +    if (((strncmp(bufp,ACE4_WHO_OWNER,wholen) == 0) ||

Please move the operator || to the next line. This allows reading the multi-line
expressions faster, just by looking at the first few characters of each line.

> +        (strncmp(bufp,ACE4_WHO_GROUP,wholen) == 0)) &&

Likewise, please move the operator && to the next line.

> +         wholen == 6 )

You can remove the space before the closing parenthesis.

> +      {
> +        if(type == ACE4_ACCESS_ALLOWED_ACE_TYPE)

Please add a space after 'if'.

> +          num_a_aces++;
> +        if(type == ACE4_ACCESS_DENIED_ACE_TYPE)

Likewise.

> +          num_d_aces++;
> +      } else 

Please put the closing brace and the 'else' on separate lines.

> +        if ((strncmp(bufp,ACE4_WHO_EVERYONE,wholen) == 0) &&

Please move the operator && to the next line.

> +            (type == ACE4_ACCESS_ALLOWED_ACE_TYPE) &&

Likewise.

> +            (wholen == 9))
> +          num_a_aces++;
> +        else
> +          return 1;
> +
> +    d_ptr = ROUNDUP(wholen,4);

Please add a space after ROUNDUP, since it's an invocation of a function-like
macro.

> +    bufp += d_ptr;
> +    bufs -= d_ptr;
> +
> +    /* Make sure we aren't outside our domain */
> +    if (bufs < 0)
> +      return -1;
> +
> +  }
> +  return (num_a_aces <= 3) && (num_d_aces <= 2) &&

Please move the operator && to the next line.

> +         ( num_a_aces + num_d_aces == num_aces) ? 0 : 1;

You can remove the space after the opening parenthesis.

> +
> +}
> +
>  # endif
>  
>  #elif USE_ACL && HAVE_FACL && defined GETACL /* Solaris, Cygwin < 2.5, not 
> HP-UX */
> diff --git a/lib/acl-internal.h b/lib/acl-internal.h
> index 94553fab2..88f1d8f1d 100644
> --- a/lib/acl-internal.h
> +++ b/lib/acl-internal.h
> @@ -146,6 +146,9 @@ rpl_acl_set_fd (int fd, acl_t acl)
>  #   define acl_entries rpl_acl_entries
>  extern int acl_entries (acl_t);
>  #  endif
> +/* Return 1 if given ACL in XDR format is non-trivial 
> + * Return 0 if it is trivial */
> +extern int acl_nfs4_nontrivial (char *,int);
>  
>  #  if HAVE_ACL_TYPE_EXTENDED /* Mac OS X */
>  /* ACL is an ACL, from a file, stored as type ACL_TYPE_EXTENDED.
> diff --git a/lib/file-has-acl.c b/lib/file-has-acl.c
> index e02f0626a..8e43dd70f 100644
> --- a/lib/file-has-acl.c
> +++ b/lib/file-has-acl.c
> @@ -32,6 +32,11 @@
>  #if GETXATTR_WITH_POSIX_ACLS
>  # include <sys/xattr.h>
>  # include <linux/xattr.h>
> +# include <arpa/inet.h>
> +#ifndef XATTR_NAME_NFSV4_ACL
> +#define XATTR_NAME_NFSV4_ACL "system.nfs4_acl"
> +#endif
> +#define TRIVIAL_NFS4_ACL_MAX_LENGTH 128

Please add spaces after '#', to indicate the preprocessor statement
indentation level.

>  #endif
>  
>  /* Return 1 if NAME has a nontrivial access control list,
> @@ -67,6 +72,22 @@ file_has_acl (char const *name, struct stat const *sb)
>              return 1;
>          }
>  
> +      if (ret < 0)
> +        { /* we might be on NFS, so try to check NFSv4 ACLs too */
> +          char xattr[TRIVIAL_NFS4_ACL_MAX_LENGTH];
> +
> +          errno = 0; /* we need to reset errno set by the previous 
> getxattr() */
> +          ret = getxattr (name, XATTR_NAME_NFSV4_ACL, xattr, 
> TRIVIAL_NFS4_ACL_MAX_LENGTH);
> +          if (ret < 0 && errno == ENODATA)
> +            ret = 0;
> +          else
> +            if (ret < 0 && errno == ERANGE)
> +              return 1;  /* we won't fit into the buffer, so non-trivial ACL 
> is presented */
> +          else

This 'else' statement and the following lines should be indented by 2 more
spaces.

> +            if (ret > 0)
> +              return acl_nfs4_nontrivial(xattr,ret);

Please add spaces before the opening parenthesis and after the comma.

> +              /* looks like trivial ACL, but we need to investigate further 
> */

It looks like this comment refers to the previous line? Then move it up one 
line.

> +        }
>        if (ret < 0)
>          return - acl_errno_valid (errno);
>        return ret;
> diff --git a/m4/acl.m4 b/m4/acl.m4
> index 8909442d7..9445edf9c 100644
> --- a/m4/acl.m4
> +++ b/m4/acl.m4
> @@ -197,7 +197,10 @@ AC_DEFUN([gl_FILE_HAS_ACL],
>           [gl_cv_getxattr_with_posix_acls=yes])])
>    fi
>    if test "$gl_cv_getxattr_with_posix_acls" = yes; then
> -    LIB_HAS_ACL=
> +    dnl We need to pull libacl back to make linker happy, but strictly
> +    dnl speaking, we do not need it
> +    LIB_HAS_ACL=$LIB_ACL
> +    gl_need_lib_has_acl=1
>      AC_DEFINE([GETXATTR_WITH_POSIX_ACLS], 1,
>        [Define to 1 if getxattr works with XATTR_NAME_POSIX_ACL_ACCESS
>         and XATTR_NAME_POSIX_ACL_DEFAULT.])
> 

I see nothing in the changes above that needs to pull in libacl. Therefore
please leave the change to m4/acl.m4 out for the moment. We'll need to discuss
this part in a separate thread; as first part of the thread please describe
the problem (link error?) that you are having — since Andreas said that he
doesn't get a link error, and I haven't seen a link error regarding libacl
in years either.

Thanks!

Bruno






reply via email to

[Prev in Thread] Current Thread [Next in Thread]