[PATCH] POSIX ACL support

[PATCH] POSIX ACL support

[Romain Francoise]

Hi,

The following patch adds support for preserving POSIX ACL entries of
files, which at the moment are lost when Emacs saves a buffer (unless
`backup-by-copying' is set). This is a similar problem as preserving the
SELinux context, and as a consequence the implementation is also very
similar.

Only the "portable" interface is used, I tested this patch successfully on
GNU/Linux (using libacl) and FreeBSD.

Comments welcome!

Thanks,

diff --git a/configure.ac b/configure.ac
index 1884cc7..fb5d8dd 100644
--- a/configure.ac
+++ b/configure.ac
@@ -184,6 +184,7 @@ OPTION_DEFAULT_ON([dbus],[don't compile with D-Bus support])
 OPTION_DEFAULT_ON([gconf],[don't compile with GConf support])
 OPTION_DEFAULT_ON([gsettings],[don't compile with GSettings support])
 OPTION_DEFAULT_ON([selinux],[don't compile with SELinux support])
+OPTION_DEFAULT_ON([acl],[don't compile with ACL support])
 OPTION_DEFAULT_ON([gnutls],[don't use -lgnutls for SSL/TLS support])
 
 ## For the times when you want to build Emacs but don't have
@@ -2151,6 +2152,24 @@ fi
 AC_SUBST(LIBGNUTLS_LIBS)
 AC_SUBST(LIBGNUTLS_CFLAGS)
 
+dnl On GNU/Linux, ACL support is provided by libacl. On BSD,
+dnl ACL support is directly provided by libc.
+HAVE_ACL=no
+LIBACL_LIBS=
+if test "${with_acl}" = "yes"; then
+  AC_CHECK_LIB([acl], [acl_set_file], HAVE_ACL=yes, HAVE_ACL=no)
+  if test "$HAVE_ACL" = yes; then
+    AC_DEFINE(HAVE_ACL, 1, [Define to 1 if using ACL support.])
+    LIBACL_LIBS=-lacl
+  else
+    AC_CHECK_FUNC(acl_set_file, HAVE_ACL=yes, HAVE_ACL=no)
+    if test "$HAVE_ACL" = yes; then
+      AC_DEFINE(HAVE_ACL, 1, [Define to 1 if using ACL support.])
+    fi
+ fi
+fi
+AC_SUBST(LIBACL_LIBS)
+
 dnl Do not put whitespace before the #include statements below.
 dnl Older compilers (eg sunos4 cc) choke on it.
 HAVE_XAW3D=no
@@ -4435,6 +4454,7 @@ echo "  Does Emacs use -ldbus?                                  ${HAVE_DBUS}"
 echo "  Does Emacs use -lgconf?                                 ${HAVE_GCONF}"
 echo "  Does Emacs use GSettings?                               ${HAVE_GSETTINGS}"
 echo "  Does Emacs use -lselinux?                               ${HAVE_LIBSELINUX}"
+echo "  Does Emacs use ACL support?                             ${HAVE_ACL}"
 echo "  Does Emacs use -lgnutls?                                ${HAVE_GNUTLS}"
 echo "  Does Emacs use -lxml2?                                  ${HAVE_LIBXML2}"
 
diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
index a5710c7..03fe290 100644
--- a/doc/lispref/files.texi
+++ b/doc/lispref/files.texi
@@ -1350,6 +1350,24 @@ not support SELinux, or if Emacs was not compiled with SELinux
 support, then the return value is @code{(nil nil nil nil)}.
 @end defun
 
+@cindex POSIX ACL
+  Several operating systems, like GNU/Linux and BSD, support
+fine-grained discretionary access rights for files and directories using
+the POSIX ACL interface.  If Emacs has been compiled with ACL support,
+you can use the function @code{file-posix-acl} to retrieve a file's ACL
+entries.  For the function @code{set-file-posix-acl}, see @ref{Changing
+Files}.
+
+@defun file-posix-acl filename
+This function returns the POSIX ACL entries of the file @var{filename}.
+The return value is a string containing the textual representation of
+the ACL entries.  See the @file{acl} man page for details about this
+representation.
+
+If the file does not exist or is inaccessible, of if Emacs was not
+compiled with ACL support, then the return value is @code{nil}.
+@end defun
+
 @node Locating Files
 @subsection How to Locate Files in Standard Places
 @cindex locate file in path
@@ -1542,6 +1560,10 @@ the correct permissions to do so.
 If the optional argument @var{preserve-selinux} is non-@code{nil}, and
 Emacs has been compiled with SELinux support, this function attempts
 to copy the file's SELinux context (@pxref{File Attributes}).
+
+If the optional argument @var{preserve-posix-acl} is non-@code{nil}, and
+Emacs has been compiled with ACL support, this function attempts to copy
+the file's POSIX ACL entries (@pxref{File Attributes}).
 @end deffn
 
 @deffn Command make-symbolic-link filename newname  &optional ok-if-exists
@@ -1682,6 +1704,14 @@ nothing if SELinux is disabled, or if Emacs was compiled without
 SELinux support.
 @end defun
 
+@defun set-file-posix-acl filename acl-string
+This function sets the POSIX ACL entries of the file @var{filename} to
+@var{acl-string}.  @xref{File Attributes}, for a brief description of
+POSIX ACLs.  The @var{acl-string} argument should be a string containing
+the textual representation of the desired ACL entries.  The function
+does nothing if Emacs was compiled without ACL support.
+@end defun
+
 @node File Names
 @section File Names
 @cindex file names
diff --git a/lisp/files.el b/lisp/files.el
index 8e8a178..e507894 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -3878,13 +3878,14 @@ variable `make-backup-files'.  If it's done by renaming, then the file is
 no longer accessible under its old name.
 
 The value is non-nil after a backup was made by renaming.
-It has the form (MODES SELINUXCONTEXT BACKUPNAME).
+It has the form (MODES SELINUXCONTEXT ACL BACKUPNAME).
 MODES is the result of `file-modes' on the original
 file; this means that the caller, after saving the buffer, should change
 the modes of the new file to agree with the old modes.
 SELINUXCONTEXT is the result of `file-selinux-context' on the original
 file; this means that the caller, after saving the buffer, should change
 the SELinux context of the new file to agree with the old context.
+Similarly, ACL is the result of `file-posix-acl' on the original file.
 BACKUPNAME is the backup file name, which is the old file renamed."
   (if (and make-backup-files (not backup-inhibited)
 	   (not buffer-backed-up)
@@ -3913,7 +3914,8 @@ BACKUPNAME is the backup file name, which is the old file renamed."
 				(y-or-n-p (format "Delete excess backup versions of %s? "
 						  real-file-name)))))
 		      (modes (file-modes buffer-file-name))
-		      (context (file-selinux-context buffer-file-name)))
+		      (context (file-selinux-context buffer-file-name))
+		      (acl (file-posix-acl buffer-file-name)))
 		  ;; Actually write the back up file.
 		  (condition-case ()
 		      (if (or file-precious-flag
@@ -3933,10 +3935,10 @@ BACKUPNAME is the backup file name, which is the old file renamed."
 						   (<= (nth 2 attr) backup-by-copying-when-privileged-mismatch)))
 					  (or (nth 9 attr)
 					      (not (file-ownership-preserved-p real-file-name)))))))
-			  (backup-buffer-copy real-file-name backupname modes context)
+			  (backup-buffer-copy real-file-name backupname modes context acl)
 			;; rename-file should delete old backup.
 			(rename-file real-file-name backupname t)
-			(setq setmodes (list modes context backupname)))
+			(setq setmodes (list modes context acl backupname)))
 		    (file-error
 		     ;; If trouble writing the backup, write it in
 		     ;; .emacs.d/%backup%.
@@ -3944,7 +3946,7 @@ BACKUPNAME is the backup file name, which is the old file renamed."
 		     (message "Cannot write backup file; backing up in %s"
 			      backupname)
 		     (sleep-for 1)
-		     (backup-buffer-copy real-file-name backupname modes context)))
+		     (backup-buffer-copy real-file-name backupname modes context acl)))
 		  (setq buffer-backed-up t)
 		  ;; Now delete the old versions, if desired.
 		  (if delete-old-versions
@@ -3956,7 +3958,7 @@ BACKUPNAME is the backup file name, which is the old file renamed."
 		  setmodes)
 	    (file-error nil))))))
 
-(defun backup-buffer-copy (from-name to-name modes context)
+(defun backup-buffer-copy (from-name to-name modes context acl)
   (let ((umask (default-file-modes)))
     (unwind-protect
 	(progn
@@ -3985,7 +3987,9 @@ BACKUPNAME is the backup file name, which is the old file renamed."
   (and modes
        (set-file-modes to-name (logand modes #o1777)))
   (and context
-       (set-file-selinux-context to-name context)))
+       (set-file-selinux-context to-name context))
+  (and acl
+       (set-file-posix-acl to-name acl)))
 
 (defvar file-name-version-regexp
   "\\(?:~\\|\\.~[-[:alnum:]:#@^._]+\\(?:~[[:digit:]]+\\)?~\\)"
@@ -4561,7 +4565,8 @@ Before and after saving the buffer, this function runs
 		(condition-case ()
 		    (progn
 		      (set-file-modes buffer-file-name (car setmodes))
-		      (set-file-selinux-context buffer-file-name (nth 1 setmodes)))
+		      (set-file-selinux-context buffer-file-name (nth 1 setmodes))
+		      (set-file-posix-acl buffer-file-name (nth 2 setmodes)))
 		  (error nil))))
 	  ;; If the auto-save file was recent before this command,
 	  ;; delete it now.
@@ -4574,7 +4579,7 @@ Before and after saving the buffer, this function runs
 ;; This does the "real job" of writing a buffer into its visited file
 ;; and making a backup file.  This is what is normally done
 ;; but inhibited if one of write-file-functions returns non-nil.
-;; It returns a value (MODES SELINUXCONTEXT BACKUPNAME), like backup-buffer.
+;; It returns a value (MODES SELINUXCONTEXT ACL BACKUPNAME), like backup-buffer.
 (defun basic-save-buffer-1 ()
   (prog1
       (if save-buffer-coding-system
@@ -4586,7 +4591,7 @@ Before and after saving the buffer, this function runs
       (setq buffer-file-coding-system-explicit
 	    (cons last-coding-system-used nil)))))
 
-;; This returns a value (MODES SELINUXCONTEXT BACKUPNAME), like backup-buffer.
+;; This returns a value (MODES SELINUXCONTEXT ACL BACKUPNAME), like backup-buffer.
 (defun basic-save-buffer-2 ()
   (let (tempsetmodes setmodes)
     (if (not (file-writable-p buffer-file-name))
@@ -4659,9 +4664,10 @@ Before and after saving the buffer, this function runs
 	    ;; Since we have created an entirely new file,
 	    ;; make sure it gets the right permission bits set.
 	    (setq setmodes (or setmodes
- 			       (list (or (file-modes buffer-file-name)
+			       (list (or (file-modes buffer-file-name)
 					 (logand ?\666 umask))
 				     (file-selinux-context buffer-file-name)
+				     (file-posix-acl buffer-file-name)
 				     buffer-file-name)))
 	    ;; We succeeded in writing the temp file,
 	    ;; so rename it.
@@ -4674,9 +4680,11 @@ Before and after saving the buffer, this function runs
 	       ;; Change the mode back, after writing.
 	       (setq setmodes (list (file-modes buffer-file-name)
 				    (file-selinux-context buffer-file-name)
+				    (file-posix-acl buffer-file-name)
 				    buffer-file-name))
 	       (set-file-modes buffer-file-name (logior (car setmodes) 128))
-	       (set-file-selinux-context buffer-file-name (nth 1 setmodes)))))
+	       (set-file-selinux-context buffer-file-name (nth 1 setmodes))
+	       (set-file-posix-acl (nth 2 setmodes)))))
 	(let (success)
 	  (unwind-protect
 	      (progn
@@ -4690,7 +4698,7 @@ Before and after saving the buffer, this function runs
 	    ;; the backup by renaming, undo the backing-up.
 	    (and setmodes (not success)
 		 (progn
-		   (rename-file (nth 2 setmodes) buffer-file-name t)
+		   (rename-file (nth 3 setmodes) buffer-file-name t)
 		   (setq buffer-backed-up nil))))))
     setmodes))
 
diff --git a/src/Makefile.in b/src/Makefile.in
index d034ad0..060d62a 100644
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -283,6 +283,8 @@ LIBSELINUX_LIBS = @LIBSELINUX_LIBS@
 LIBGNUTLS_LIBS = @LIBGNUTLS_LIBS@
 LIBGNUTLS_CFLAGS = @LIBGNUTLS_CFLAGS@
 
+LIBACL_LIBS = @LIBACL_LIBS@
+
 LIB_PTHREAD_SIGMASK = @LIB_PTHREAD_SIGMASK@
 
 INTERVALS_H = dispextern.h intervals.h composite.h
@@ -398,7 +400,7 @@ LIBES = $(LIBS) $(W32_LIBS) $(LIBX_BASE) $(LIBIMAGE) \
    $(LIBXML2_LIBS) $(LIBGPM) $(LIBRESOLV) $(LIBS_SYSTEM) \
    $(LIBS_TERMCAP) $(GETLOADAVG_LIBS) $(SETTINGS_LIBS) $(LIBSELINUX_LIBS) \
    $(FREETYPE_LIBS) $(FONTCONFIG_LIBS) $(LIBOTF_LIBS) $(M17N_FLT_LIBS) \
-   $(LIBGNUTLS_LIBS) $(LIB_PTHREAD) $(LIB_PTHREAD_SIGMASK) \
+   $(LIBACL_LIBS) $(LIBGNUTLS_LIBS) $(LIB_PTHREAD) $(LIB_PTHREAD_SIGMASK) \
    $(LIB_GCC) $(LIB_MATH) $(LIB_STANDARD) $(LIB_GCC)
 
 all: emacs$(EXEEXT) $(OTHER_FILES)
diff --git a/src/fileio.c b/src/fileio.c
index 572f6d8..7654668 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -36,6 +36,10 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #include <selinux/context.h>
 #endif
 
+#ifdef HAVE_ACL
+#include <sys/acl.h>
+#endif
+
 #include <c-ctype.h>
 
 #include "lisp.h"
@@ -236,6 +240,8 @@ static Lisp_Object Qset_file_modes;
 static Lisp_Object Qset_file_times;
 static Lisp_Object Qfile_selinux_context;
 static Lisp_Object Qset_file_selinux_context;
+static Lisp_Object Qfile_posix_acl;
+static Lisp_Object Qset_file_posix_acl;
 static Lisp_Object Qfile_newer_than_file_p;
 Lisp_Object Qinsert_file_contents;
 Lisp_Object Qwrite_region;
@@ -1829,7 +1835,7 @@ barf_or_query_if_file_exists (Lisp_Object absname, const char *querystring,
   return;
 }
 
-DEFUN ("copy-file", Fcopy_file, Scopy_file, 2, 6,
+DEFUN ("copy-file", Fcopy_file, Scopy_file, 2, 7,
        "fCopy file: \nGCopy %s to file: \np\nP",
        doc: /* Copy FILE to NEWNAME.  Both args must be strings.
 If NEWNAME names a directory, copy FILE there.
@@ -1854,8 +1860,11 @@ If PRESERVE-UID-GID is non-nil, we try to transfer the
 uid and gid of FILE to NEWNAME.
 
 If PRESERVE-SELINUX-CONTEXT is non-nil and SELinux is enabled
-on the system, we copy the SELinux context of FILE to NEWNAME.  */)
-  (Lisp_Object file, Lisp_Object newname, Lisp_Object ok_if_already_exists, Lisp_Object keep_time, Lisp_Object preserve_uid_gid, Lisp_Object preserve_selinux_context)
+on the system, we copy the SELinux context of FILE to NEWNAME.
+
+If PRESERVE-POSIX-ACL is non-nil and Emacs is built with ACL support,
+we copy the ACL of FILE to NEWNAME.  */)
+  (Lisp_Object file, Lisp_Object newname, Lisp_Object ok_if_already_exists, Lisp_Object keep_time, Lisp_Object preserve_uid_gid, Lisp_Object preserve_selinux_context, Lisp_Object preserve_posix_acl)
 {
   int ifd, ofd;
   int n;
@@ -1870,6 +1879,9 @@ on the system, we copy the SELinux context of FILE to NEWNAME.  */)
   security_context_t con;
   int conlength = 0;
 #endif
+#ifdef HAVE_ACL
+  acl_t acl = NULL;
+#endif
 
   encoded_file = encoded_newname = Qnil;
   GCPRO4 (file, newname, encoded_file, encoded_newname);
@@ -1955,6 +1967,15 @@ on the system, we copy the SELinux context of FILE to NEWNAME.  */)
     }
 #endif
 
+#ifdef HAVE_ACL
+  if (!NILP (preserve_posix_acl))
+    {
+      acl = acl_get_fd (ifd);
+      if (acl == NULL && errno != ENOTSUP)
+	report_file_error ("Getting ACL", Fcons (file, Qnil));
+    }
+#endif
+
   if (out_st.st_mode != 0
       && st.st_dev == out_st.st_dev && st.st_ino == out_st.st_ino)
     {
@@ -2045,6 +2066,17 @@ on the system, we copy the SELinux context of FILE to NEWNAME.  */)
     }
 #endif
 
+#ifdef HAVE_ACL
+  if (acl != NULL)
+    {
+      bool fail = acl_set_fd (ofd, acl) != 0;
+      if (fail && errno != ENOTSUP)
+	report_file_error ("Setting ACL", Fcons (newname, Qnil));
+
+      acl_free (acl);
+    }
+#endif
+
   if (input_file_statable_p)
     {
       if (!NILP (keep_time))
@@ -2262,7 +2294,7 @@ This is what happens in interactive use with M-x.  */)
 	       have copy-file prompt again.  */
 	    Fcopy_file (file, newname,
 			NILP (ok_if_already_exists) ? Qnil : Qt,
-			Qt, Qt, Qt);
+			Qt, Qt, Qt, Qt);
 
 	  count = SPECPDL_INDEX ();
 	  specbind (Qdelete_by_moving_to_trash, Qnil);
@@ -2934,6 +2966,104 @@ compiled with SELinux support.  */)
   return Qnil;
 }
 \f
+DEFUN ("file-posix-acl", Ffile_posix_acl, Sfile_posix_acl, 1, 1, 0,
+       doc: /* Return POSIX ACL entries of file named FILENAME, as a string.
+Return nil if file does not exist or is not accessible, or if Emacs was
+not compiled with ACL support.  */)
+  (Lisp_Object filename)
+{
+  Lisp_Object absname;
+  Lisp_Object handler;
+#ifdef HAVE_ACL
+  acl_t acl;
+  Lisp_Object acl_string;
+  char *str;
+#endif
+
+  absname = expand_and_dir_to_file (filename,
+				    BVAR (current_buffer, directory));
+
+  /* If the file name has special constructs in it,
+     call the corresponding file handler.  */
+  handler = Ffind_file_name_handler (absname, Qfile_posix_acl);
+  if (!NILP (handler))
+    return call2 (handler, Qfile_posix_acl, absname);
+
+#ifdef HAVE_ACL
+  absname = ENCODE_FILE (absname);
+
+  acl = acl_get_file (SSDATA (absname), ACL_TYPE_ACCESS);
+  if (acl == NULL)
+    return Qnil;
+
+  str = acl_to_text (acl, NULL);
+  if (str == NULL)
+    {
+      acl_free (acl);
+      return Qnil;
+    }
+
+  acl_string = build_string (str);
+  acl_free (str);
+  acl_free (acl);
+
+  return acl_string;
+#endif
+
+  return Qnil;
+}
+
+DEFUN ("set-file-posix-acl", Fset_file_posix_acl, Sset_file_posix_acl,
+       2, 2, 0,
+       doc: /* Set POSIX ACL of file named FILENAME to ACL-STRING.
+ACL-STRING should contain the textual representation of the ACL
+entries in either long or short form.
+
+This function does nothing if Emacs was not compiled with ACL
+support.  */)
+  (Lisp_Object filename, Lisp_Object acl_string)
+{
+  Lisp_Object absname;
+  Lisp_Object handler;
+#ifdef HAVE_ACL
+  Lisp_Object encoded_absname;
+  acl_t acl;
+  bool fail;
+#endif
+
+  absname = Fexpand_file_name (filename, BVAR (current_buffer, directory));
+
+  /* If the file name has special constructs in it,
+     call the corresponding file handler.  */
+  handler = Ffind_file_name_handler (absname, Qset_file_posix_acl);
+  if (!NILP (handler))
+    return call3 (handler, Qset_file_posix_acl, absname, acl_string);
+
+#ifdef HAVE_ACL
+  if (STRINGP (acl_string))
+    {
+      acl = acl_from_text (SSDATA (acl_string));
+      if (acl == NULL)
+	{
+	  report_file_error ("Converting ACL", Fcons (absname, Qnil));
+	  return Qnil;
+	}
+
+      encoded_absname = ENCODE_FILE (absname);
+
+      fail = (acl_set_file (SSDATA (encoded_absname), ACL_TYPE_ACCESS,
+			    acl)
+	      != 0);
+      if (fail && errno != ENOTSUP)
+	report_file_error ("Setting ACL", Fcons (absname, Qnil));
+
+      acl_free (acl);
+    }
+#endif
+
+  return Qnil;
+}
+\f
 DEFUN ("file-modes", Ffile_modes, Sfile_modes, 1, 1, 0,
        doc: /* Return mode bits of file named FILENAME, as an integer.
 Return nil, if file does not exist or is not accessible.  */)
@@ -5812,6 +5942,8 @@ This includes interactive calls to `delete-file' and
   defsubr (&Sset_file_modes);
   defsubr (&Sset_file_times);
   defsubr (&Sfile_selinux_context);
+  defsubr (&Sfile_posix_acl);
+  defsubr (&Sset_file_posix_acl);
   defsubr (&Sset_file_selinux_context);
   defsubr (&Sset_default_file_modes);
   defsubr (&Sdefault_file_modes);

Re: [PATCH] POSIX ACL support

[Eli Zaretskii]

> From: Romain Francoise <romain@orebokech.com>
> Date: Sun, 18 Nov 2012 23:27:22 +0100
> 
> The following patch adds support for preserving POSIX ACL entries of
> files, which at the moment are lost when Emacs saves a buffer (unless
> `backup-by-copying' is set). This is a similar problem as preserving the
> SELinux context, and as a consequence the implementation is also very
> similar.
> 
> Only the "portable" interface is used, I tested this patch successfully on
> GNU/Linux (using libacl) and FreeBSD.
> 
> Comments welcome!

Thanks.

I'd like to suggest to drop the "posix" part from the APIs, because
that will make it easier to extend this support to other types of
ACLs.

Re: [PATCH] POSIX ACL support

[Romain Francoise]

Hi Eli,

Eli Zaretskii <eliz@gnu.org> writes:

> I'd like to suggest to drop the "posix" part from the APIs, because
> that will make it easier to extend this support to other types of
> ACLs.

Sure, I can do that. Do you think that a string-based interface is
portable enough for all platforms? From what I can see, you can use a
string interface on Windows to set ACEs, but I'm no expert. If it requires
passing binary information, the new functions wouldn't be general enough.

Thanks!

Re: [PATCH] POSIX ACL support

[Eli Zaretskii]

> From: Romain Francoise <romain@orebokech.com>
> Date: Mon, 19 Nov 2012 22:41:57 +0100
> Cc: emacs-devel@gnu.org
> 
> Do you think that a string-based interface is portable enough for
> all platforms?

I think so.  There are functions like acl_to_text and acl_from_text to
do that.

> From what I can see, you can use a string interface on Windows to
> set ACEs, but I'm no expert.

Yes, I once wrote acl_from_text and acl_to_text for Windows.  The
strings look very differently from what you see with Posix ACLs,
though, but I don't think this matters.

> If it requires passing binary information, the new functions
> wouldn't be general enough.

I think binary data will be inconvenient in Lisp.

Re: [PATCH] POSIX ACL support

[Glenn Morris]

Romain Francoise wrote:

> The following patch adds support for preserving POSIX ACL entries of
> files, which at the moment are lost when Emacs saves a buffer (unless
> `backup-by-copying' is set).

Thanks for writing this. It is definitely needed IMO.

> -(defun backup-buffer-copy (from-name to-name modes context)
> +(defun backup-buffer-copy (from-name to-name modes context acl)

[and several other such]

This is really a general comment, not an issue with your patch in
particular, so don't let it distract. I just wonder if it is possible to
find a different way to pass this information around, so that we don't
need to have an ever increasing list of function arguments and return
values (preserve-this, preserve-that, preserve-the-other, etc...).
I don't have a good suggestion for how to do that.
(I have a vague memory this was mentioned when SELinux was added.)

Re: [PATCH] POSIX ACL support

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Date: Mon, 19 Nov 2012 12:11:38 -0500
> Cc: emacs-devel@gnu.org
> 
> > -(defun backup-buffer-copy (from-name to-name modes context)
> > +(defun backup-buffer-copy (from-name to-name modes context acl)
> 
> [and several other such]
> 
> This is really a general comment, not an issue with your patch in
> particular, so don't let it distract. I just wonder if it is possible to
> find a different way to pass this information around, so that we don't
> need to have an ever increasing list of function arguments and return
> values (preserve-this, preserve-that, preserve-the-other, etc...).

Agreed.

> I don't have a good suggestion for how to do that.

How about a cons cell of the form (TYPE . DATA), where TYPE can be
selinux-context, posix-acls, etc., and DATA is whatever value is
appropriate for TYPE?  Or maybe (:TYPE DATA)?

Re: [PATCH] POSIX ACL support

[Stefan Monnier]

>> The following patch adds support for preserving POSIX ACL entries of
>> files, which at the moment are lost when Emacs saves a buffer (unless
>> `backup-by-copying' is set).
> Thanks for writing this. It is definitely needed IMO.
>> -(defun backup-buffer-copy (from-name to-name modes context)
>> +(defun backup-buffer-copy (from-name to-name modes context acl)
> [and several other such]
> This is really a general comment, not an issue with your patch in
> particular, so don't let it distract. I just wonder if it is possible to
> find a different way to pass this information around, so that we don't
> need to have an ever increasing list of function arguments and return
> values (preserve-this, preserve-that, preserve-the-other, etc...).
> I don't have a good suggestion for how to do that.
> (I have a vague memory this was mentioned when SELinux was added.)

Yes, that's a problem.  We should consolidate `modes', `context', and
`acl' into a `metadata' argument.


        Stefan

Re: [PATCH] POSIX ACL support

[Romain Francoise]

Hi everyone,

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> This is really a general comment, not an issue with your patch in
>> particular, so don't let it distract. I just wonder if it is possible to
>> find a different way to pass this information around, so that we don't
>> need to have an ever increasing list of function arguments and return
>> values (preserve-this, preserve-that, preserve-the-other, etc...).
>> I don't have a good suggestion for how to do that.
>> (I have a vague memory this was mentioned when SELinux was added.)

> Yes, that's a problem.  We should consolidate `modes', `context', and
> `acl' into a `metadata' argument.

Ok, in files.el I can refactor the code to pass all the metadata in one go
and avoid the extra ACL argument here and there, no problem.

However, I think Glenn was also referring to the additional argument to
`copy-file', which doesn't preserve the modes. But maybe we can change the
existing `preserve-selinux-context' argument into a more general
`preserve-platform-attributes' argument and do both ACL and SELinux? Any
caller that wants to preserve the SELinux context probably wants to
preserve the ACL entries as well, anyway.

Ideally we'd get rid of `file-modes', `file-selinux-context' and
`file-posix-acl' and just add the info to `file-attributes', which already
returns a list of 12 different attributes. Then `set-file-attributes'
would be created to set all those different attributes in one go. But that
would be a lot of work...

Thanks!

Re: [PATCH] POSIX ACL support

[Stefan Monnier]

> However, I think Glenn was also referring to the additional argument to
> `copy-file', which doesn't preserve the modes.

Yes, this also needs to be handled in a similar way (tho I'm not sure
what you mean by "which doesn't preserve the modes" since the docstring
says "This function always sets the file modes of the output file to match
the input file.").

> But maybe we can change the existing `preserve-selinux-context'
> argument into a more general `preserve-platform-attributes' argument
> and do both ACL and SELinux? Any caller that wants to preserve the
> SELinux context probably wants to preserve the ACL entries as
> well, anyway.

Actually, I'd go even further and merge PRESERVE-UID-GID
PRESERVE-SELINUX-CONTEXT into a single argument.

> Ideally we'd get rid of `file-modes', `file-selinux-context' and
> `file-posix-acl' and just add the info to `file-attributes', which already
> returns a list of 12 different attributes.  Then `set-file-attributes'
> would be created to set all those different attributes in one go.  But that
> would be a lot of work...

I'm not sure I'm ready to add the info to file-attributes.
But we could start by providing a new file-extended-attributes which
returns a self-descriptive list of attributes, and then
set-file-extended-attributes which takes that list and applies it.


        Stefan

Re: [PATCH] POSIX ACL support

[Romain Francoise]

Hi,

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> Yes, this also needs to be handled in a similar way (tho I'm not sure
> what you mean by "which doesn't preserve the modes" since the docstring
> says "This function always sets the file modes of the output file to match
> the input file.").

Oh, right. I guess I meant "which doesn't take a `preserve-modes'
argument"...

> But we could start by providing a new file-extended-attributes which
> returns a self-descriptive list of attributes, and then
> set-file-extended-attributes which takes that list and applies it.

Okay. Do you envision this as a Lisp-level API which uses lower-level
subroutines for each type (set-file-foo, similar to what we have today),
or as a C-level API which abstracts the different metadata types (in which
case we would remove file-foo/set-file-foo)?

Thanks!

Re: [PATCH] POSIX ACL support

[Stefan Monnier]

> Okay. Do you envision this as a Lisp-level API which uses lower-level
> subroutines for each type (set-file-foo, similar to what we have today),
> or as a C-level API which abstracts the different metadata types (in which
> case we would remove file-foo/set-file-foo)?

I'd start with whichever is easiest.


        Stefan

Re: [PATCH] POSIX ACL support

[Paul Eggert]

On 11/21/2012 06:25 AM, Stefan Monnier wrote:
> I'd start with whichever is easiest.

I suggest something that's even simpler at the C level:
an API that simply clones attributes from one file to
another.  The two files can be represented by file names
and file descriptors.  There's no need to build a runtime
representation in C or Lisp of the metadata.  Just copy
the metadata directly, from one file to the other.

Re: [PATCH] POSIX ACL support

[Romain Francoise]

Hi Paul,

Paul Eggert <eggert@cs.ucla.edu> writes:

> I suggest something that's even simpler at the C level:
> an API that simply clones attributes from one file to
> another.  The two files can be represented by file names
> and file descriptors.  There's no need to build a runtime
> representation in C or Lisp of the metadata.  Just copy
> the metadata directly, from one file to the other.

That's indeed much simpler, but makes it impossible to preserve
attributes for files that have special handlers. For example, Tramp
currently preserves the SELinux context for remote SSH files by getting it
from the local file via the C API and reapplying it remotely using the
`chcon' utility. See `tramp-sh-handle-set-file-selinux-context' in
tramp-sh.el.

Whether or not that is a desirable feature to have is a matter of taste,
I suppose...

Re: [PATCH] POSIX ACL support

[Eli Zaretskii]

> Date: Wed, 21 Nov 2012 07:36:09 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: emacs-devel@gnu.org
> 
> On 11/21/2012 06:25 AM, Stefan Monnier wrote:
> > I'd start with whichever is easiest.
> 
> I suggest something that's even simpler at the C level:
> an API that simply clones attributes from one file to
> another.  The two files can be represented by file names
> and file descriptors.  There's no need to build a runtime
> representation in C or Lisp of the metadata.  Just copy
> the metadata directly, from one file to the other.

But that could be impossible in some cases, no?  Can a user on a Posix
host always copy the ACLs, no matter what access right these ACLs
specify?

And in addition, this could be inconvenient or even dangerous: you
could set access rights on a file you created that give you no access
at all, not even read access.  IOW, something like umask filtering
might be necessary to avoid shooting yourself in the foot.  Right?

Re: [PATCH] POSIX ACL support

[Paul Eggert]

On 11/21/12 10:17, Eli Zaretskii wrote:
> Can a user on a Posix
> host always copy the ACLs, no matter what access right these ACLs
> specify?

I don't know of any exception, no, though I don't pretend to be
an expert on Posix ACLS.

> And in addition, this could be inconvenient or even dangerous: you
> could set access rights on a file you created that give you no access
> at all, not even read access.

This couldn't happen for files that you're copying, since if you can't
read them you can't copy them anyway.  But anyway, it's OK to create
files that you can't read -- Emacs already can do that, I expect.

Romain's point about special file handlers is a good one.  If we
want this to work with Tramp we have to have a way in C or in Lisp
to represent all the file metadata, so that we can copy it.  Doing
this correctly would solve not only this problem, but some other bugs
in this area (e.g., group or group permission is wrong, time stamps
are truncated).

Re: [PATCH] POSIX ACL support

[Achim Gratz]

Paul Eggert writes:
> On 11/21/12 10:17, Eli Zaretskii wrote:
>> Can a user on a Posix
>> host always copy the ACLs, no matter what access right these ACLs
>> specify?
>
> I don't know of any exception, no, though I don't pretend to be
> an expert on Posix ACLS.

On windows at least it is possible to have all access rights, but
specifically not to create or change ACL.  For POSIX, you need
CAP_FOWNER privileges, which you should have for a newly created file (I
don't know if that capability could be removed via SElinux policy,
though).


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf rackAttack V1.04R1:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: [PATCH] POSIX ACL support

[Eli Zaretskii]

> From: Achim Gratz <Stromeko@nexgo.de>
> Date: Wed, 21 Nov 2012 20:09:12 +0100
> 
> On windows at least it is possible to have all access rights, but
> specifically not to create or change ACL.

True.  But I think this is unlikely to be a problem when _copying_ a
file, because the copy is created by you, so you get full access by
default, and that includes the "write DAC" (a.k.a. "change ACL")
privilege.  However, it _can_ happen that after copying the ACL from
the original, you no longer can change the access rights, if that
privilege was denied in the original.  Which is okay, I think.

Re: [PATCH] POSIX ACL support

[Achim Gratz]

Eli Zaretskii writes:
> True.  But I think this is unlikely to be a problem when _copying_ a
> file, because the copy is created by you, so you get full access by
> default, and that includes the "write DAC" (a.k.a. "change ACL")
> privilege.  However, it _can_ happen that after copying the ACL from
> the original, you no longer can change the access rights, if that
> privilege was denied in the original.  Which is okay, I think.

Well, talk to our corporate IT department then which happens to think
that it is a sensible thing to not let you do this and in addition don't
let you even look at the existing ACL (which means I can't be sure how
they accomplished that, but I believe it's some inheritable property
that makes all files, including new ones, owned by a service account).

As I said, I'm almost certain you could achieve something similar with
SElinux policies, but then I haven't tried it yet.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Terratec KOMPLEXER:
http://Synth.Stromeko.net/Downloads.html#KomplexerWaves

Re: [PATCH] POSIX ACL support

[Paul Eggert]

On 11/19/12 10:11, Stefan Monnier wrote:
> Yes, that's a problem.  We should consolidate `modes', `context', and
> `acl' into a `metadata' argument.

Or we could take a simpler approach, and just copy all the metadata
from the original file to the backup.  It's pretty rare that one wants
to back up just some of this stuff, and not the rest.  This would be a
simpler than having individual arguments to copy just this and that,
arguments that I expect in practice are rarely used properly.

Which reminds me, this area is busted for a different reason:
the backup-by-copying-when-mismatch algorithm is done incorrectly on
most modern GNUish or POSIXish hosts, in the sense that copies are
not being made when they should be, and the resulting file metadata
is incorrect (wrong group ID).  I'll file a bug report about that
when I find the time.

Re: [PATCH] POSIX ACL support

[Stefan Monnier]

>> Yes, that's a problem.  We should consolidate `modes', `context', and
>> `acl' into a `metadata' argument.
> Or we could take a simpler approach, and just copy all the metadata
> from the original file to the backup.  It's pretty rare that one wants
> to back up just some of this stuff, and not the rest.  This would be a
> simpler than having individual arguments to copy just this and that,
> arguments that I expect in practice are rarely used properly.

I see we're in violent agreement.


        Stefan