Feature/jwt keyfile (#7863)

* Added jwt-keyfile option and warning for old option.
* CHANGELOG
* Add trimming to --auth.jwt-secret-keyfile
* Adjust some comments.

CHANGELOG

@@ -1,6 +1,8 @@
 devel
 -----
 
+* Added --server.jwt-secret-keyfile option.
+
 * speed up data-modification operations in exclusive transactions in the RocksDB
   storage engine
 
@@ -8,7 +10,7 @@ devel
   the edge can now be executed in a more optimized way, e.g.
 
     FOR edge IN edgeCollection FILTER edge._from == "v/1" RETURN edge._to
-    
+
   is fully covered by RocksDB edge index. For MMFiles this rule does not apply.
 
 * reverted accidental change to error handling in geo index
@@ -27,10 +29,10 @@ devel
 * make `--help-all` now also show all hidden program options
 
   Previously hidden program options were only returned when invoking arangod or
-  a client tool with the cryptic `--help-.` option. Now `--help-all` simply 
+  a client tool with the cryptic `--help-.` option. Now `--help-all` simply
   retuns them as well.
 
-  The program options JSON description returned by `--dump-options` was also 
+  The program options JSON description returned by `--dump-options` was also
   improved as follows:
 
   - the new boolean attribute "dynamic" indicates whether the option has a dynamic
@@ -56,26 +58,26 @@ devel
   a COLLECT statement as documented. The documentation for variable invalidation claims
   that
 
-      The COLLECT statement will eliminate all local variables in the current scope. 
+      The COLLECT statement will eliminate all local variables in the current scope.
       After COLLECT only the variables introduced by COLLECT itself are available.
 
   However, the described behavior was not enforced when a COLLECT was preceded by a
   FOR loop that was itself preceded by a COLLECT. In the following query the final
-  RETURN statement accesses variable `key1` though the variable should have been 
+  RETURN statement accesses variable `key1` though the variable should have been
   invalidated by the COLLECT directly before it:
 
-      FOR x1 IN 1..2 
+      FOR x1 IN 1..2
-        COLLECT key1 = x1 
+        COLLECT key1 = x1
-        FOR x2 IN 1..2 
+        FOR x2 IN 1..2
-          COLLECT key2 = x2 
+          COLLECT key2 = x2
-          RETURN [key2, key1] 
+          RETURN [key2, key1]
-  
+
   In previous releases, this query was
   parsed ok, but the contents of variable `key1` in the final RETURN statement were
-  undefined. 
+  undefined.
-  
+
-  This change is about making queries as the above fail with a parse error, as an 
+  This change is about making queries as the above fail with a parse error, as an
-  unknown variable `key1` is accessed here, avoiding the undefined behavior. This is 
+  unknown variable `key1` is accessed here, avoiding the undefined behavior. This is
   also in line with what the documentation states about variable invalidation.
 
 * fixed issue #7763: Collect after update does not execute updates
@@ -99,8 +101,8 @@ devel
 
 * Updated joi library (Web UI), improved foxx mount path validation
 
-* do not create `_routing` collection for new installations/new databases, 
+* do not create `_routing` collection for new installations/new databases,
-  as it is not needed anymore. Redirects to the web interface's login screen, which 
+  as it is not needed anymore. Redirects to the web interface's login screen, which
   were previously handled by entries in the `_routing` collection are now handled
   from the responsible REST action handler directly
 
@@ -160,10 +162,10 @@ v3.4.1 (XXXX-XX-XX)
 * make `--help-all` now also show all hidden program options
 
   Previously hidden program options were only returned when invoking arangod or
-  a client tool with the cryptic `--help-.` option. Now `--help-all` simply 
+  a client tool with the cryptic `--help-.` option. Now `--help-all` simply
   retuns them as well.
 
-  The program options JSON description returned by `--dump-options` was also 
+  The program options JSON description returned by `--dump-options` was also
   improved as follows:
 
   - the new boolean attribute "dynamic" indicates whether the option has a dynamic
@@ -191,21 +193,21 @@ v3.4.0-rc.5 (2018-11-29)
 ------------------------
 
 * Persist and check default language (locale) selection.
-  Previously we would not check if the language (`--default-language`) had changed 
+  Previously we would not check if the language (`--default-language`) had changed
-  when the server was restarted. This could cause issues with indexes over text fields, 
+  when the server was restarted. This could cause issues with indexes over text fields,
-  as it will resulted in undefined behavior within RocksDB (potentially missing entries, 
+  as it will resulted in undefined behavior within RocksDB (potentially missing entries,
   corruption, etc.). Now if the language is changed, ArangoDB will print out an error
   message on startup and abort.
 
 * fixed issue #7522: FILTER logic totally broke for my query in 3.4-rc4
 
-* export version and storage engine in `_admin/cluster/health` for Coordinators 
+* export version and storage engine in `_admin/cluster/health` for Coordinators
-  and DBServers. 
+  and DBServers.
 
 * restrict the total amount of data to build up in all in-memory RocksDB write buffers
-  by default to a certain fraction of the available physical RAM. This helps restricting 
+  by default to a certain fraction of the available physical RAM. This helps restricting
-  memory usage for the arangod process, but may have an effect on the RocksDB storage 
+  memory usage for the arangod process, but may have an effect on the RocksDB storage
-  engine's write performance. 
+  engine's write performance.
 
   In ArangoDB 3.3 the governing configuration option `--rocksdb.total-write-buffer-size`
   had a default value of `0`, which meant that the memory usage was not limited. ArangoDB
@@ -215,7 +217,7 @@ v3.4.0-rc.5 (2018-11-29)
 * lower default value for `--cache.size` startup option from about 30% of physical RAM to
   about 25% percent of physical RAM.
 
-* fix internal issue #2786: improved confirmation dialog when clicking the truncate 
+* fix internal issue #2786: improved confirmation dialog when clicking the truncate
   button in the web UI
 
 * Updated joi library (web UI), improved Foxx mount path validation
@@ -223,17 +225,17 @@ v3.4.0-rc.5 (2018-11-29)
 * disable startup warning for Linux kernel variable `vm.overcommit_memory` settings
   values of 0 or 1.
   Effectively `overcommit_memory` settings value of 0 or 1 fix two memory-allocation
-  related issues with the default memory allocator used in ArangoDB release builds on 
+  related issues with the default memory allocator used in ArangoDB release builds on
-  64bit Linux. 
+  64bit Linux.
   The issues will remain when running with an `overcommit_memory` settings value of 2,
   so this is now discouraged.
   Setting `overcommit_memory` to 0 or 1 (0 is the Linux kernel's default) fixes issues
   with increasing numbers of memory mappings for the arangod process (which may lead
   to an out-of-memory situation if the kernel's maximum number of mappings threshold
   is hit) and an increasing amount of memory that the kernel counts as "committed".
-  With an `overcommit_memory` setting of 0 or 1, an arangod process may either be 
+  With an `overcommit_memory` setting of 0 or 1, an arangod process may either be
   killed by the kernel's OOM killer or will die with a segfault when accessing memory
-  it has allocated before but the kernel could not provide later on. This is still 
+  it has allocated before but the kernel could not provide later on. This is still
   more acceptable than the kernel not providing any more memory to the process when
   there is still physical memory left, which may have occurred with an `overcommit_memory`
   setting of 2 after the arangod process had done lots of allocations.
@@ -273,8 +275,8 @@ v3.4.0-rc.4 (2018-11-04)
 
 * stop printing `connection class corrupted` in arangosh
 
- when just starting the arangosh without a connection to a server and running 
+ when just starting the arangosh without a connection to a server and running
- code such as `require("internal")`, the shell always printed "connection class 
+ code such as `require("internal")`, the shell always printed "connection class
  corrupted", which was somewhat misleading.
 
 * add separate option `--query.slow-streaming-threshold` for tracking slow

arangod/GeneralServer/AuthenticationFeature.cpp

@@ -30,6 +30,8 @@
 #include "ProgramOptions/ProgramOptions.h"
 #include "Random/RandomGenerator.h"
 #include "RestServer/QueryRegistryFeature.h"
+#include "Basics/FileUtils.h"
+#include "Basics/StringUtils.h"
 
 #if USE_ENTERPRISE
 #include "Enterprise/Ldap/LdapAuthenticationHandler.h"
@@ -101,13 +103,39 @@ void AuthenticationFeature::collectOptions(std::shared_ptr<ProgramOptions> optio
                      new BooleanParameter(&_authenticationUnixSockets));
 #endif
 
+  // Maybe deprecate this option in devel
   options->addOption("--server.jwt-secret",
                      "secret to use when doing jwt authentication",
                      new StringParameter(&_jwtSecretProgramOption));
+
+  options->addOption("--server.jwt-secret-keyfile",
+                     "file containing jwt secret to use when doing jwt authentication.",
+                     new StringParameter(&_jwtSecretKeyfileProgramOption));
 }
 
 void AuthenticationFeature::validateOptions(std::shared_ptr<ProgramOptions>) {
-  if (!_jwtSecretProgramOption.empty()) {
+  if (!_jwtSecretKeyfileProgramOption.empty()) {
+
+    try {
+      // Note that the secret is trimmed for whitespace, because whitespace
+      // at the end of a file can easily happen. We do not base64-encode,
+      // though, so the bytes count as given. Zero bytes might be a problem
+      // here.
+      _jwtSecretProgramOption = basics::StringUtils::trim(
+          basics::FileUtils::slurp(_jwtSecretKeyfileProgramOption),
+          " \t\n\r");
+    } catch (std::exception const& ex) {
+      LOG_TOPIC(FATAL, Logger::STARTUP)
+          << "unable to read content of jwt-secret file '"
+          << _jwtSecretKeyfileProgramOption << "': " << ex.what()
+          << ". please make sure the file/directory is readable for the "
+             "arangod process and user";
+      FATAL_ERROR_EXIT();
+    }
+
+  } else if (!_jwtSecretProgramOption.empty()) {
+    LOG_TOPIC(WARN, arangodb::Logger::FIXME)
+      << "--server.jwt-secret is insecure. Use --server.jwt-secret-keyfile instead.";
     if (_jwtSecretProgramOption.length() > _maxSecretLength) {
       LOG_TOPIC(FATAL, arangodb::Logger::FIXME)
           << "Given JWT secret too long. Max length is " << _maxSecretLength;

arangod/GeneralServer/AuthenticationFeature.h

@@ -79,6 +79,7 @@ class AuthenticationFeature final : public application_features::ApplicationFeat
   double _authenticationTimeout;
 
   std::string _jwtSecretProgramOption;
+  std::string _jwtSecretKeyfileProgramOption;
 
   static AuthenticationFeature* INSTANCE;
 };