On Thu, Feb 06, 2014 at 05:24:19PM +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/Daemon.hs | 6 ++++++ > src/Ganeti/Logging.hs | 7 +++++++ > src/Ganeti/UDSServer.hs | 20 +++++++++++++++++--- > 6 files changed, 43 insertions(+), 6 deletions(-) > > diff --git a/lib/cli.py b/lib/cli.py > index 39ef2df..aa3d968 100644 > --- a/lib/cli.py > +++ b/lib/cli.py > @@ -2594,7 +2594,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..082af2b 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_breach=False): > """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_breach: bool > + @param warn_breach: issue a warning at daemon launch time, before > + daemonizing, about the possibility of breaking parameter privacy > + invariants through the otherwise helpful debug logging. > > """ > optionparser.add_option("-f", "--foreground", dest="fork", > @@ -800,6 +805,10 @@ def GenericMain(daemon_name, optionparser, > > log_filename = constants.DAEMONS_LOGFILES[daemon_name] > > + # node-daemon logging in lib/http/server.py, _HandleServerRequestInner > + if options.debug and warn_breach: > + 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..507c2ae 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_breach=True) > 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.
It's better to refer to module names instead of filename, for example, Instead of UDSServer.hs have Ganeti.UDSServer I only noticed this now, but this occurs in other patches as well. Rest LGTM. No need to resend. Thanks, Jose > + 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..4cc5c62 100644 > --- a/src/Ganeti/Logging.hs > +++ b/src/Ganeti/Logging.hs > @@ -47,8 +47,10 @@ module Ganeti.Logging > , syslogUsageToRaw > , syslogUsageFromRaw > , withErrorLogAt > + , isDebugMode > ) where > > +import Control.Applicative ((<$>)) > import Control.Monad > import Control.Monad.Error (Error(..), MonadError(..), catchError) > import Control.Monad.Reader > @@ -175,6 +177,11 @@ 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 = (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..a1622d1 100644 > --- a/src/Ganeti/UDSServer.hs > +++ b/src/Ganeti/UDSServer.hs > @@ -58,13 +58,14 @@ module Ganeti.UDSServer > import Control.Applicative > import Control.Concurrent (forkIO) > import Control.Exception (catch) > -import Data.IORef > +import Control.Monad > import qualified Data.ByteString as B > import qualified Data.ByteString.Lazy as BL > import qualified Data.ByteString.UTF8 as UTF8 > import qualified Data.ByteString.Lazy.UTF8 as UTF8L > +import Data.IORef > +import Data.List > import Data.Word (Word8) > -import Control.Monad > import qualified Network.Socket as S > import System.Directory (removeFile) > import System.IO (hClose, hFlush, hWaitForInput, Handle, IOMode(..)) > @@ -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 > > @@ -350,6 +351,11 @@ handleRawMessage handler payload = > let (status, response) = prepareMsg call_result_json > return (close, buildResponse status response) > > +isRisky :: RecvResult -> Bool > +isRisky msg = case msg of > + RecvOk payload -> any (`isInfixOf` payload) privateParametersBlacklist > + _ -> False > + > -- | Reads a request, passes it to a handler and sends a response back to the > -- client. > handleClient > @@ -359,7 +365,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 +383,7 @@ handleClient handler client = do > sendMsg client outMsg > return close > > + > -- | 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
