Make KeyDirectory's GroupReadable behave the same as CacheDirectory's.

In #26913 we solved a bug where CacheDirectoryGroupReadable would
override DataDirectoryGroupReadable when the two directories are the
same.  We never did the same for KeyDirectory, though, because
that's a rare setting.

Now that I'm testing this code, though, fixing this issue seems
fine.  Fixes bug #27992; bugfix on 0.3.3.1-alpha.

changes/ticket27992

@@ -0,0 +1,5 @@
+  o Minor bugfixes (configuration):
+    - When creating a KeyDirectory with the same location as the
+      DataDirectory (not recommended), respect the DataDirectory's
+      group-readable setting if one has not been set for the KeyDirectory.
+      Fixes bug 27992; bugfix on 0.3.3.1-alpha.

doc/tor.1.txt

@@ -2589,10 +2589,12 @@ is non-zero):
     running.
     (Default: the "keys" subdirectory of DataDirectory.)
 
-[[KeyDirectoryGroupReadable]] **KeyDirectoryGroupReadable** **0**|**1**::
+[[KeyDirectoryGroupReadable]] **KeyDirectoryGroupReadable** **0**|**1**|**auto**::
     If this option is set to 0, don't allow the filesystem group to read the
-    KeywDirectory. If the option is set to 1, make the KeyDirectory readable
-    by the default GID. (Default: 0)
+    KeyDirectory. If the option is set to 1, make the KeyDirectory readable
+    by the default GID. If the option is "auto", then we use the
+    setting for DataDirectoryGroupReadable when the KeyDirectory is the
+    same as the DataDirectory, and 0 otherwise. (Default: auto)
 
 [[RephistTrackTime]] **RephistTrackTime** __N__ **seconds**|**minutes**|**hours**|**days**|**weeks**::
     Tells an authority, or other node tracking node reliability and history,

src/app/config/config.c

@@ -540,7 +540,7 @@ static const config_var_t option_vars_[] = {
   V(Socks5ProxyUsername,         STRING,   NULL),
   V(Socks5ProxyPassword,         STRING,   NULL),
   VAR_IMMUTABLE("KeyDirectory",  FILENAME, KeyDirectory_option, NULL),
-  V(KeyDirectoryGroupReadable,   BOOL,     "0"),
+  V(KeyDirectoryGroupReadable,   AUTOBOOL, "auto"),
   VAR_D("HSLayer2Nodes",         ROUTERSET,  HSLayer2Nodes,  NULL),
   VAR_D("HSLayer3Nodes",         ROUTERSET,  HSLayer3Nodes,  NULL),
   V(KeepalivePeriod,             INTERVAL, "5 minutes"),
@@ -1515,11 +1515,39 @@ options_switch_id(char **msg_out)
   return 0;
 }
 
+/**
+ * Helper. Given a data directory (<b>datadir</b>) and another directory
+ * (<b>subdir</b>) with respective group-writable permissions
+ * <b>datadir_gr</b> and <b>subdir_gr</b>, compute whether the subdir should
+ * be group-writeable.
+ **/
+static int
+compute_group_readable_flag(const char *datadir,
+                            const char *subdir,
+                            int datadir_gr,
+                            int subdir_gr)
+{
+  if (subdir_gr != -1) {
+    /* The user specified a default for "subdir", so we always obey it. */
+    return subdir_gr;
+  }
+
+  /* The user left the subdir_gr option on "auto." */
+  if (0 == strcmp(subdir, datadir)) {
+    /* The directories are the same, so we use the group-readable flag from
+     * the datadirectory */
+    return datadir_gr;
+  } else {
+    /* The directores are different, so we default to "not group-readable" */
+    return 0;
+  }
+}
+
 /**
  * Create our DataDirectory, CacheDirectory, and KeyDirectory, and
  * set their permissions correctly.
  */
-static int
+STATIC int
 options_create_directories(char **msg_out)
 {
   const or_options_t *options = get_options();
@@ -1536,30 +1564,29 @@ options_create_directories(char **msg_out)
                                       msg_out) < 0) {
     return -1;
   }
+
+  /* We need to handle the group-readable flag for the cache directory and key
+   * directory specially, since they may be the same as the data directory */
+  const int key_dir_group_readable = compute_group_readable_flag(
+                                        options->DataDirectory,
+                                        options->KeyDirectory,
+                                        options->DataDirectoryGroupReadable,
+                                        options->KeyDirectoryGroupReadable);
+
   if (check_and_create_data_directory(running_tor /* create */,
                                       options->KeyDirectory,
-                                      options->KeyDirectoryGroupReadable,
+                                      key_dir_group_readable,
                                       options->User,
                                       msg_out) < 0) {
     return -1;
   }
 
-  /* We need to handle the group-readable flag for the cache directory
-   * specially, since the directory defaults to being the same as the
-   * DataDirectory. */
-  int cache_dir_group_readable;
-  if (options->CacheDirectoryGroupReadable != -1) {
-    /* If the user specified a value, use their setting */
-    cache_dir_group_readable = options->CacheDirectoryGroupReadable;
-  } else if (!strcmp(options->CacheDirectory, options->DataDirectory)) {
-    /* If the user left the value as "auto", and the cache is the same as the
-     * datadirectory, use the datadirectory setting.
-     */
-    cache_dir_group_readable = options->DataDirectoryGroupReadable;
-  } else {
-    /* Otherwise, "auto" means "not group readable". */
-    cache_dir_group_readable = 0;
-  }
+  const int cache_dir_group_readable = compute_group_readable_flag(
+                                        options->DataDirectory,
+                                        options->CacheDirectory,
+                                        options->DataDirectoryGroupReadable,
+                                        options->CacheDirectoryGroupReadable);
+
   if (check_and_create_data_directory(running_tor /* create */,
                                       options->CacheDirectory,
                                       cache_dir_group_readable,

src/app/config/config.h

@@ -303,6 +303,8 @@ STATIC int open_and_add_file_log(const log_severity_list_t *severity,
 STATIC int options_init_logs(const or_options_t *old_options,
                              const or_options_t *options, int validate_only);
 
+STATIC int options_create_directories(char **msg_out);
+
 #ifdef TOR_UNIT_TESTS
 int options_validate(const or_options_t *old_options,
                      or_options_t *options,

src/test/test_options_act.c

@@ -109,11 +109,7 @@ test_options_act_create_dirs(void *arg)
   opts->KeyDirectory = tor_strdup(fn);
   opts->DataDirectoryGroupReadable = 1;
   opts->CacheDirectoryGroupReadable = -1; /* default. */
-#if 1
-  /* Bug 27992: this setting shouldn't be needed, but for now it is, in the
-   * unusual case that DataDirectory == KeyDirectory */
-  opts->KeyDirectoryGroupReadable = 1;
-#endif
+  opts->KeyDirectoryGroupReadable = -1; /* default */
   r = options_create_directories(&msg);
   tt_int_op(r, OP_EQ, 0);
   tt_ptr_op(msg, OP_EQ, NULL);