On Tue, Feb 04, 2014 at 05:00:32PM +0100, Santi Raffa wrote:
> Luxid as it is can leak private and secret parameters by logging
> all requests as they arrive, before any preprocessing is done.
>
> Warn the user stern warnings about this.
>
> Signed-off-by: Santi Raffa <[email protected]>
> ---
> lib/cli.py | 2 +-
> lib/daemon.py | 11 ++++++++++-
> lib/server/noded.py | 3 ++-
> src/Ganeti/Constants.hs | 6 ++++++
> src/Ganeti/Daemon.hs | 6 ++++++
> src/Ganeti/Logging.hs | 8 ++++++++
> src/Ganeti/UDSServer.hs | 15 ++++++++++++++-
> 7 files changed, 47 insertions(+), 4 deletions(-)
>
> diff --git a/lib/cli.py b/lib/cli.py
> index e7f1755..e257b4e 100644
> --- a/lib/cli.py
> +++ b/lib/cli.py
> @@ -2585,7 +2585,7 @@ def GenericMain(commands, override=None, aliases=None,
> utils.SetupLogging(pathutils.LOG_COMMANDS, logname, debug=options.debug,
> stderr_logging=True)
>
> - logging.info("Command line: %s", cmdline)
> + logging.debug("Command line: %s", cmdline)
>
> try:
> result = func(options, args)
> diff --git a/lib/daemon.py b/lib/daemon.py
> index 299563d..efe22bf 100644
> --- a/lib/daemon.py
> +++ b/lib/daemon.py
> @@ -670,7 +670,8 @@ def _HandleSigHup(reopen_fn, signum, frame): # pylint:
> disable=W0613
> def GenericMain(daemon_name, optionparser,
> check_fn, prepare_fn, exec_fn,
> multithreaded=False, console_logging=False,
> - default_ssl_cert=None, default_ssl_key=None):
> + default_ssl_cert=None, default_ssl_key=None,
> + warn_about_private_values_and_debug_mode=False):
Duuuuude... really? -> warn_about_private_values_and_debug_mode
Make it small.
> """Shared main function for daemons.
>
> @type daemon_name: string
> @@ -697,6 +698,10 @@ def GenericMain(daemon_name, optionparser,
> @param default_ssl_cert: Default SSL certificate path
> @type default_ssl_key: string
> @param default_ssl_key: Default SSL key path
> + @type warn_about_private_values_and_debug_mode: bool
> + @param warn_about_private_values_and_debug_mode: issue a warning at daemon
> + launch time, before daemonizing, about the possibility of breaking
> + parameter privacy invariants through otherwise helpful debug logging.
s/through otherwise/through the otherwise/
> """
> optionparser.add_option("-f", "--foreground", dest="fork",
> @@ -800,6 +805,10 @@ def GenericMain(daemon_name, optionparser,
>
> log_filename = constants.DAEMONS_LOGFILES[daemon_name]
>
> + # node-deamon logging in lib/http/server.py, _HandleServerRequestInner
s/deamon/daemon/
> + if options.debug and warn_about_private_values_and_debug_mode:
> + sys.stderr.write(constants.DEBUG_MODE_CONFIDENTIALITY_WARNING %
> daemon_name)
> +
> if options.fork:
> utils.CloseFDs()
> (wpipe, stdio_reopen_fn) = utils.Daemonize(logfile=log_filename)
> diff --git a/lib/server/noded.py b/lib/server/noded.py
> index 9731925..994c8d7 100644
> --- a/lib/server/noded.py
> +++ b/lib/server/noded.py
> @@ -1297,4 +1297,5 @@ def Main():
> daemon.GenericMain(constants.NODED, parser, CheckNoded, PrepNoded,
> ExecNoded,
> default_ssl_cert=pathutils.NODED_CERT_FILE,
> default_ssl_key=pathutils.NODED_CERT_FILE,
> - console_logging=True)
> + console_logging=True,
> + warn_about_private_values_and_debug_mode=True)
> diff --git a/src/Ganeti/Constants.hs b/src/Ganeti/Constants.hs
> index e16b32c..10f58e5 100644
> --- a/src/Ganeti/Constants.hs
> +++ b/src/Ganeti/Constants.hs
> @@ -4774,3 +4774,9 @@ privateParametersBlacklist = [ "osparams_private"
> , "osparams_secret"
> , "osparams_private_cluster" ]
>
> +-- | Warn the user that the logging level is too low for production use.
> +debugModeConfidentialityWarning :: String
> +debugModeConfidentialityWarning = "ALERT: %s started in debug mode.\n\
> + \ Private and secret parameters WILL\
> + \ be logged!\n"
> +
Newline after equals sign will make the message more readable.
debugModeConfidentialityWarning :: String
debugModeConfidentialityWarning =
"ALERT: %s started in debug mode.\n" ++
" Private and secret parameters WILL be logged!\n"
> diff --git a/src/Ganeti/Daemon.hs b/src/Ganeti/Daemon.hs
> index 7bd88f0..ac87156 100644
> --- a/src/Ganeti/Daemon.hs
> +++ b/src/Ganeti/Daemon.hs
> @@ -49,6 +49,7 @@ import Control.Concurrent
> import Control.Exception
> import Control.Monad
> import Data.Maybe (fromMaybe, listToMaybe)
> +import Text.Printf
> import Data.Word
> import GHC.IO.Handle (hDuplicateTo)
> import Network.BSD (getHostName)
> @@ -383,6 +384,11 @@ genericMain daemon options check_fn prep_fn exec_fn = do
>
> (opts, args) <- parseArgs progname options
>
> + -- Modify handleClient in UDSServer.hs to compile this logging out of
> luxid.
> + when (optDebug opts && daemon == GanetiLuxid) .
> + hPutStrLn stderr $
> + printf C.debugModeConfidentialityWarning (daemonName daemon)
> +
> ensureNode daemon
>
> exitUnless (null args) "This program doesn't take any arguments"
> diff --git a/src/Ganeti/Logging.hs b/src/Ganeti/Logging.hs
> index b7b05b1..abb8d46 100644
> --- a/src/Ganeti/Logging.hs
> +++ b/src/Ganeti/Logging.hs
> @@ -47,6 +47,7 @@ module Ganeti.Logging
> , syslogUsageToRaw
> , syslogUsageFromRaw
> , withErrorLogAt
> + , isDebugMode
> ) where
>
> import Control.Monad
> @@ -175,6 +176,13 @@ logAlert = logAt ALERT
> logEmergency :: (MonadLog m) => String -> m ()
> logEmergency = logAt EMERGENCY
>
> +-- | Check if the logging is at DEBUG level.
> +-- DEBUG logging is unacceptable for production.
> +isDebugMode :: IO Bool
> +isDebugMode = do
> + logger <- getRootLogger
> + return $ getLevel logger == Just DEBUG
> +
I think this might work (untested):
isDebugMode :: IO Bool
isDebugMode = (Just DEBUG ==) . getLevel <$> getRootLogger
> -- * Logging in an error monad with rethrowing errors
>
> -- | If an error occurs within a given computation, it annotated
> diff --git a/src/Ganeti/UDSServer.hs b/src/Ganeti/UDSServer.hs
> index d30851c..4fd4c78 100644
> --- a/src/Ganeti/UDSServer.hs
> +++ b/src/Ganeti/UDSServer.hs
> @@ -59,6 +59,7 @@ import Control.Applicative
> import Control.Concurrent (forkIO)
> import Control.Exception (catch)
> import Data.IORef
> +import Data.List
Please fix the alphabetic sorting.
> import qualified Data.ByteString as B
> import qualified Data.ByteString.Lazy as BL
> import qualified Data.ByteString.UTF8 as UTF8
> @@ -81,7 +82,7 @@ import Ganeti.Logging
> import Ganeti.Runtime (GanetiDaemon(..), MiscGroup(..), GanetiGroup(..))
> import Ganeti.THH
> import Ganeti.Utils
> -
> +import Ganeti.Constants (privateParametersBlacklist)
>
> -- * Utility functions
>
> @@ -359,7 +360,14 @@ handleClient
> -> IO Bool
> handleClient handler client = do
> msg <- recvMsgExt client
> +
> + debugMode <- isDebugMode
> + when (debugMode && isRisky msg) $
> + logAlert "POSSIBLE LEAKING OF CONFIDENTIAL PARAMETERS. \
> + \Daemon is running in debug mode. \
> + \The text of the request has been logged."
> logDebug $ "Received message: " ++ show msg
> +
> case msg of
> RecvConnClosed -> logDebug "Connection closed" >>
> return False
> @@ -370,6 +378,11 @@ handleClient handler client = do
> sendMsg client outMsg
> return close
>
> +isRisky :: RecvResult -> Bool
> +isRisky msg = case msg of
> + RecvOk payload -> any (`isInfixOf` payload) privateParametersBlacklist
> + _ -> False
> +
Move this above handleClient to keep the linear order of definitions.
Thanks,
Jose
> -- | Main client loop: runs one loop of 'handleClient', and if that
> -- doesn't report a finished (closed) connection, restarts itself.
> clientLoop
> --
> 1.9.0.rc1.175.g0b1dcb5
>
--
Jose Antonio Lopes
Ganeti Engineering
Google Germany GmbH
Dienerstr. 12, 80331, München
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Graham Law, Christine Elizabeth Flores
Steuernummer: 48/725/00206
Umsatzsteueridentifikationsnummer: DE813741370
