From 417c20a3c275b8540bf8839e01e2abdc62997a54 Mon Sep 17 00:00:00 2001 From: Boshi Lian Date: Sat, 24 Feb 2018 18:33:12 +0800 Subject: [PATCH] move logger out from global and add flags args --- sshpiperd/cmd.go | 7 +--- sshpiperd/log.go | 20 +++++---- sshpiperd/sshpiperd.go | 15 +++---- sshpiperd/sshpiperd_test.go | 81 ++++++++++++++++++++++++++++++++----- 4 files changed, 93 insertions(+), 30 deletions(-) diff --git a/sshpiperd/cmd.go b/sshpiperd/cmd.go index 84b0ed6a..2ddacde6 100644 --- a/sshpiperd/cmd.go +++ b/sshpiperd/cmd.go @@ -161,7 +161,7 @@ func main() { config := &struct { piperdConfig - Logfile string `long:"log" description:"Logfile path. Leave empty or any error occurs will fall back to stdout" env:"SSHPIPERD_LOG_PATH" ini-name:"log-path"` + loggerConfig // need to be shown in help, or will be moved to populate config ConfigFile flags.Filename `long:"config" description:"Config file path. Will be overwriten by arg options and environment variables" default:"/etc/sshpiperd.ini" env:"SSHPIPERD_CONFIG_FILE" no-ini:"true"` @@ -190,13 +190,10 @@ func main() { return fmt.Errorf("unknown command %v", args) } - // init log - initLogger(config.Logfile) - showVersion() dumpConfig() - return startPiper(&config.piperdConfig) + return startPiper(&config.piperdConfig, config.createLogger()) } return command.Execute(args) diff --git a/sshpiperd/log.go b/sshpiperd/log.go index 2fdde3b0..25939780 100644 --- a/sshpiperd/log.go +++ b/sshpiperd/log.go @@ -5,14 +5,18 @@ import ( "os" ) -var ( - logger = log.New(os.Stdout, "", log.Ldate|log.Ltime) -) +type loggerConfig struct { + LogFile string `long:"log" description:"LogFile path. Leave empty or any error occurs will fall back to stdout" env:"SSHPIPERD_LOG_PATH" ini-name:"log-path"` -func initLogger(file string) { - // change this value for display might be not a good idea - if file != "" { - f, err := os.OpenFile(file, os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0644) + LogFlags int `long:"log-flags" default:"3" description:"Flags for logger see https://godoc.org/log, default LstdFlags" env:"SSHPIPERD_LOG_FLAGS" ini-name:"log-flags"` +} + +func (l loggerConfig) createLogger() (logger *log.Logger) { + + logger = log.New(os.Stdout, "", l.LogFlags) + + if l.LogFile != "" { + f, err := os.OpenFile(l.LogFile, os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0644) if err != nil { logger.Printf("cannot open log file %v", err) return @@ -20,4 +24,6 @@ func initLogger(file string) { logger = log.New(f, "", logger.Flags()) } + + return } diff --git a/sshpiperd/sshpiperd.go b/sshpiperd/sshpiperd.go index 3aaee933..58505856 100644 --- a/sshpiperd/sshpiperd.go +++ b/sshpiperd/sshpiperd.go @@ -11,6 +11,7 @@ import ( "github.com/tg123/sshpiper/sshpiperd/challenger" "github.com/tg123/sshpiper/sshpiperd/registry" "github.com/tg123/sshpiper/sshpiperd/upstream" + "log" ) type piperdConfig struct { @@ -23,7 +24,7 @@ type piperdConfig struct { AuditorDriver string `long:"auditor-driver" description:"Auditor for ssh connections piped by SSH Piper " env:"SSHPIPERD_AUDITOR" ini-name:"auditor-driver"` } -func getAndInstall(name string, get func(n string) registry.Plugin, install func(plugin registry.Plugin) error) error { +func getAndInstall(name string, get func(n string) registry.Plugin, install func(plugin registry.Plugin) error, logger *log.Logger) error { if name == "" { return nil } @@ -41,7 +42,7 @@ func getAndInstall(name string, get func(n string) registry.Plugin, install func return install(p) } -func installDrivers(piper *ssh.SSHPiperConfig, config *piperdConfig) (auditor.Provider, error) { +func installDrivers(piper *ssh.SSHPiperConfig, config *piperdConfig, logger *log.Logger) (auditor.Provider, error) { // install upstreamProvider driver if config.UpstreamDriver == "" { @@ -65,7 +66,7 @@ func installDrivers(piper *ssh.SSHPiperConfig, config *piperdConfig) (auditor.Pr handler := plugin.(upstream.Provider).GetHandler() if handler == nil { - return fmt.Errorf("driver return nil handler") + return fmt.Errorf("upstream driver return nil handler") } piper.FindUpstream = handler @@ -82,7 +83,7 @@ func installDrivers(piper *ssh.SSHPiperConfig, config *piperdConfig) (auditor.Pr handler := plugin.(challenger.Provider).GetHandler() if handler == nil { - return fmt.Errorf("driver return nil handler") + return fmt.Errorf("challenger driver return nil handler") } piper.AdditionalChallenge = handler @@ -101,7 +102,7 @@ func installDrivers(piper *ssh.SSHPiperConfig, config *piperdConfig) (auditor.Pr }, }, } { - err := getAndInstall(d.name, d.get, d.install) + err := getAndInstall(d.name, d.get, d.install, logger) if err != nil { return nil, err } @@ -110,13 +111,13 @@ func installDrivers(piper *ssh.SSHPiperConfig, config *piperdConfig) (auditor.Pr return bigbro, nil } -func startPiper(config *piperdConfig) error { +func startPiper(config *piperdConfig, logger *log.Logger) error { logger.Println("sshpiper is about to start") piper := &ssh.SSHPiperConfig{} - bigbro, err := installDrivers(piper, config) + bigbro, err := installDrivers(piper, config, logger) privateBytes, err := ioutil.ReadFile(config.PiperKeyFile) if err != nil { diff --git a/sshpiperd/sshpiperd_test.go b/sshpiperd/sshpiperd_test.go index 51607c0f..e441dd17 100644 --- a/sshpiperd/sshpiperd_test.go +++ b/sshpiperd/sshpiperd_test.go @@ -43,7 +43,7 @@ func Test_getAndInstall(t *testing.T) { }, func(plugin registry.Plugin) error { t.Errorf("should not call install") return nil - }) + }, nil) // fail when not found err := getAndInstall("test", func(n string) registry.Plugin { @@ -54,7 +54,7 @@ func Test_getAndInstall(t *testing.T) { }, func(plugin registry.Plugin) error { t.Errorf("should not call install") return nil - }) + }, nil) if err == nil { t.Errorf("should err when not found") @@ -69,7 +69,7 @@ func Test_getAndInstall(t *testing.T) { } }, func(plugin registry.Plugin) error { return fmt.Errorf("test") - }) + }, nil) if err == nil { t.Errorf("should err when not found") @@ -95,7 +95,7 @@ func Test_getAndInstall(t *testing.T) { installed = true return nil - }) + }, nil) if !installed { t.Errorf("not installed") @@ -155,9 +155,11 @@ func (t *testauditor) Close() error { func Test_installDriver(t *testing.T) { var ( - upstreamName = fmt.Sprintf("u_%v", time.Now().UTC().UnixNano()) - challengerName = fmt.Sprintf("c_%v", time.Now().UTC().UnixNano()) - auditorName = fmt.Sprintf("a_%v", time.Now().UTC().UnixNano()) + upstreamName = fmt.Sprintf("u_%v", time.Now().UTC().UnixNano()) + challengerName = fmt.Sprintf("c_%v", time.Now().UTC().UnixNano()) + auditorName = fmt.Sprintf("a_%v", time.Now().UTC().UnixNano()) + upstreamErrName = fmt.Sprintf("ue_%v", time.Now().UTC().UnixNano()) + upstreamNilName = fmt.Sprintf("un_%v", time.Now().UTC().UnixNano()) ) findUpstream := func(conn ssh.ConnMetadata) (net.Conn, *ssh.SSHPiperAuthPipe, error) { @@ -171,6 +173,23 @@ func Test_installDriver(t *testing.T) { h: findUpstream, }) + upstream.Register(upstreamErrName, &testupstream{ + testplugin: testplugin{ + name: upstreamErrName, + init: func(logger *log.Logger) error { + return fmt.Errorf("test err") + }, + }, + h: findUpstream, + }) + + upstream.Register(upstreamNilName, &testupstream{ + testplugin: testplugin{ + name: upstreamNilName, + }, + h: nil, + }) + challenger.Register(challengerName, &testchallenger{ testplugin: testplugin{ name: upstreamName, @@ -187,7 +206,7 @@ func Test_installDriver(t *testing.T) { piper := &ssh.SSHPiperConfig{} _, err := installDrivers(piper, &piperdConfig{ UpstreamDriver: "", - }) + }, nil) if err == nil { t.Errorf("should fail when empty driver") @@ -199,7 +218,7 @@ func Test_installDriver(t *testing.T) { piper := &ssh.SSHPiperConfig{} _, err := installDrivers(piper, &piperdConfig{ UpstreamDriver: upstreamName, - }) + }, nil) if err != nil { t.Errorf("install failed %v", err) @@ -214,13 +233,53 @@ func Test_installDriver(t *testing.T) { } } + // install upstream with failed init + { + piper := &ssh.SSHPiperConfig{} + _, err := installDrivers(piper, &piperdConfig{ + UpstreamDriver: upstreamErrName, + }, nil) + + if err == nil { + t.Errorf("install should fail") + } + + if piper.FindUpstream != nil { + t.Errorf("should not install upstream provider") + } + + if piper.AdditionalChallenge != nil { + t.Errorf("should not install challenger") + } + } + + // install upstream with nil handler + { + piper := &ssh.SSHPiperConfig{} + _, err := installDrivers(piper, &piperdConfig{ + UpstreamDriver: upstreamNilName, + }, nil) + + if err == nil { + t.Errorf("install should fail") + } + + if piper.FindUpstream != nil { + t.Errorf("should not install upstream provider") + } + + if piper.AdditionalChallenge != nil { + t.Errorf("should not install challenger") + } + } + // install challenger { piper := &ssh.SSHPiperConfig{} _, err := installDrivers(piper, &piperdConfig{ UpstreamDriver: upstreamName, ChallengerDriver: challengerName, - }) + }, nil) if err != nil { t.Errorf("install failed %v", err) @@ -237,7 +296,7 @@ func Test_installDriver(t *testing.T) { ap, err := installDrivers(piper, &piperdConfig{ UpstreamDriver: upstreamName, AuditorDriver: auditorName, - }) + }, nil) if err != nil { t.Errorf("install failed %v", err)