Merge pull request #548 from oauth2-proxy/move-logging-options

Separate logging options out of main options structure

CHANGELOG.md

@@ -48,9 +48,14 @@
     This lead to confusion and misconfiguration as it was not obvious when a session should be encrypted.
   - Cookie Secrets must now be 16, 24 or 32 bytes.
   - If you need to change your secret, this will force users to reauthenticate.
+- [#548](https://github.com/oauth2-proxy/oauth2-proxy/pull/548) Separate logging options out of main options structure
+  - Fixes an inconsistency in the `--exclude-logging-paths` option by renaming it to `--exclude-logging-option`.
+  - This flag may now be given multiple times as with other list options
+  - This flag also accepts comma separated values
 
 ## Changes since v5.1.1
 
+- [#548](https://github.com/oauth2-proxy/oauth2-proxy/pull/548) Separate logging options out of main options structure (@JoelSpeed)
 - [#536](https://github.com/oauth2-proxy/oauth2-proxy/pull/536) Improvements to Session State code (@JoelSpeed)
 - [#573](https://github.com/oauth2-proxy/oauth2-proxy/pull/573) Properly parse redis urls for cluster and sentinel connections (@amnay-mo)
 - [#574](https://github.com/oauth2-proxy/oauth2-proxy/pull/574) render error page on 502 proxy status (@amnay-mo)

pkg/apis/options/logging.go

@@ -0,0 +1,74 @@
+package options
+
+import (
+	"github.com/oauth2-proxy/oauth2-proxy/pkg/logger"
+	"github.com/spf13/pflag"
+)
+
+// Logging contains all options required for configuring the logging
+type Logging struct {
+	AuthEnabled     bool           `flag:"auth-logging" cfg:"auth_logging"`
+	AuthFormat      string         `flag:"auth-logging-format" cfg:"auth_logging_format"`
+	RequestEnabled  bool           `flag:"request-logging" cfg:"request_logging"`
+	RequestFormat   string         `flag:"request-logging-format" cfg:"request_logging_format"`
+	StandardEnabled bool           `flag:"standard-logging" cfg:"standard_logging"`
+	StandardFormat  string         `flag:"standard-logging-format" cfg:"standard_logging_format"`
+	ExcludePaths    []string       `flag:"exclude-logging-path" cfg:"exclude_logging_paths"`
+	LocalTime       bool           `flag:"logging-local-time" cfg:"logging_local_time"`
+	SilencePing     bool           `flag:"silence-ping-logging" cfg:"silence_ping_logging"`
+	File            LogFileOptions `cfg:",squash"`
+}
+
+// LogFileOptions contains options for configuring logging to a file
+type LogFileOptions struct {
+	Filename   string `flag:"logging-filename" cfg:"logging_filename"`
+	MaxSize    int    `flag:"logging-max-size" cfg:"logging_max_size"`
+	MaxAge     int    `flag:"logging-max-age" cfg:"logging_max_age"`
+	MaxBackups int    `flag:"logging-max-backups" cfg:"logging_max_backups"`
+	Compress   bool   `flag:"logging-compress" cfg:"logging_compress"`
+}
+
+func loggingFlagSet() *pflag.FlagSet {
+	flagSet := pflag.NewFlagSet("logging", pflag.ExitOnError)
+
+	flagSet.Bool("auth-logging", true, "Log authentication attempts")
+	flagSet.String("auth-logging-format", logger.DefaultAuthLoggingFormat, "Template for authentication log lines")
+	flagSet.Bool("standard-logging", true, "Log standard runtime information")
+	flagSet.String("standard-logging-format", logger.DefaultStandardLoggingFormat, "Template for standard log lines")
+	flagSet.Bool("request-logging", true, "Log HTTP requests")
+	flagSet.String("request-logging-format", logger.DefaultRequestLoggingFormat, "Template for HTTP request log lines")
+
+	flagSet.StringSlice("exclude-logging-path", []string{}, "Exclude logging requests to paths (eg: '/path1,/path2,/path3')")
+	flagSet.Bool("logging-local-time", true, "If the time in log files and backup filenames are local or UTC time")
+	flagSet.Bool("silence-ping-logging", false, "Disable logging of requests to ping endpoint")
+
+	flagSet.String("logging-filename", "", "File to log requests to, empty for stdout")
+	flagSet.Int("logging-max-size", 100, "Maximum size in megabytes of the log file before rotation")
+	flagSet.Int("logging-max-age", 7, "Maximum number of days to retain old log files")
+	flagSet.Int("logging-max-backups", 0, "Maximum number of old log files to retain; 0 to disable")
+	flagSet.Bool("logging-compress", false, "Should rotated log files be compressed using gzip")
+
+	return flagSet
+}
+
+// loggingDefaults creates a Logging structure, populating each field with its default value
+func loggingDefaults() Logging {
+	return Logging{
+		ExcludePaths:    nil,
+		LocalTime:       true,
+		SilencePing:     false,
+		AuthEnabled:     true,
+		AuthFormat:      logger.DefaultAuthLoggingFormat,
+		RequestEnabled:  true,
+		RequestFormat:   logger.DefaultRequestLoggingFormat,
+		StandardEnabled: true,
+		StandardFormat:  logger.DefaultStandardLoggingFormat,
+		File: LogFileOptions{
+			Filename:   "",
+			MaxSize:    100,
+			MaxAge:     7,
+			MaxBackups: 0,
+			Compress:   false,
+		},
+	}
+}

pkg/apis/options/options.go

@@ -9,7 +9,6 @@ import (
 	oidc "github.com/coreos/go-oidc"
 	ipapi "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/ip"
 	sessionsapi "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/sessions"
-	"github.com/oauth2-proxy/oauth2-proxy/pkg/logger"
 	"github.com/oauth2-proxy/oauth2-proxy/providers"
 	"github.com/spf13/pflag"
 )
@@ -61,6 +60,7 @@ type Options struct {
 
 	Cookie  CookieOptions  `cfg:",squash"`
 	Session SessionOptions `cfg:",squash"`
+	Logging Logging        `cfg:",squash"`
 
 	Upstreams                     []string      `flag:"upstream" cfg:"upstreams"`
 	SkipAuthRegex                 []string      `flag:"skip-auth-regex" cfg:"skip_auth_regex"`
@@ -101,27 +101,12 @@ type Options struct {
 	ApprovalPrompt                     string `flag:"approval-prompt" cfg:"approval_prompt"` // Deprecated by OIDC 1.0
 	UserIDClaim                        string `flag:"user-id-claim" cfg:"user_id_claim"`
 
-	// Configuration values for logging
+	SignatureKey    string `flag:"signature-key" cfg:"signature_key"`
-	LoggingFilename       string `flag:"logging-filename" cfg:"logging_filename"`
+	AcrValues       string `flag:"acr-values" cfg:"acr_values"`
-	LoggingMaxSize        int    `flag:"logging-max-size" cfg:"logging_max_size"`
+	JWTKey          string `flag:"jwt-key" cfg:"jwt_key"`
-	LoggingMaxAge         int    `flag:"logging-max-age" cfg:"logging_max_age"`
+	JWTKeyFile      string `flag:"jwt-key-file" cfg:"jwt_key_file"`
-	LoggingMaxBackups     int    `flag:"logging-max-backups" cfg:"logging_max_backups"`
+	PubJWKURL       string `flag:"pubjwk-url" cfg:"pubjwk_url"`
-	LoggingLocalTime      bool   `flag:"logging-local-time" cfg:"logging_local_time"`
+	GCPHealthChecks bool   `flag:"gcp-healthchecks" cfg:"gcp_healthchecks"`
-	LoggingCompress       bool   `flag:"logging-compress" cfg:"logging_compress"`
-	StandardLogging       bool   `flag:"standard-logging" cfg:"standard_logging"`
-	StandardLoggingFormat string `flag:"standard-logging-format" cfg:"standard_logging_format"`
-	RequestLogging        bool   `flag:"request-logging" cfg:"request_logging"`
-	RequestLoggingFormat  string `flag:"request-logging-format" cfg:"request_logging_format"`
-	ExcludeLoggingPaths   string `flag:"exclude-logging-paths" cfg:"exclude_logging_paths"`
-	SilencePingLogging    bool   `flag:"silence-ping-logging" cfg:"silence_ping_logging"`
-	AuthLogging           bool   `flag:"auth-logging" cfg:"auth_logging"`
-	AuthLoggingFormat     string `flag:"auth-logging-format" cfg:"auth_logging_format"`
-	SignatureKey          string `flag:"signature-key" cfg:"signature_key"`
-	AcrValues             string `flag:"acr-values" cfg:"acr_values"`
-	JWTKey                string `flag:"jwt-key" cfg:"jwt_key"`
-	JWTKeyFile            string `flag:"jwt-key-file" cfg:"jwt_key_file"`
-	PubJWKURL             string `flag:"pubjwk-url" cfg:"pubjwk_url"`
-	GCPHealthChecks       bool   `flag:"gcp-healthchecks" cfg:"gcp_healthchecks"`
 
 	// internal values that are set after config validation
 	redirectURL        *url.URL
@@ -197,20 +182,7 @@ func NewOptions() *Options {
 		UserIDClaim:                      "email",
 		InsecureOIDCAllowUnverifiedEmail: false,
 		SkipOIDCDiscovery:                false,
-		LoggingFilename:                  "",
+		Logging:                          loggingDefaults(),
-		LoggingMaxSize:                   100,
-		LoggingMaxAge:                    7,
-		LoggingMaxBackups:                0,
-		LoggingLocalTime:                 true,
-		LoggingCompress:                  false,
-		ExcludeLoggingPaths:              "",
-		SilencePingLogging:               false,
-		StandardLogging:                  true,
-		StandardLoggingFormat:            logger.DefaultStandardLoggingFormat,
-		RequestLogging:                   true,
-		RequestLoggingFormat:             logger.DefaultRequestLoggingFormat,
-		AuthLogging:                      true,
-		AuthLoggingFormat:                logger.DefaultAuthLoggingFormat,
 	}
 }
 
@@ -293,24 +265,6 @@ func NewFlagSet() *pflag.FlagSet {
 	flagSet.Bool("redis-use-cluster", false, "Connect to redis cluster. Must set --redis-cluster-connection-urls to use this feature")
 	flagSet.StringSlice("redis-cluster-connection-urls", []string{}, "List of Redis cluster connection URLs (eg redis://HOST[:PORT]). Used in conjunction with --redis-use-cluster")
 
-	flagSet.String("logging-filename", "", "File to log requests to, empty for stdout")
-	flagSet.Int("logging-max-size", 100, "Maximum size in megabytes of the log file before rotation")
-	flagSet.Int("logging-max-age", 7, "Maximum number of days to retain old log files")
-	flagSet.Int("logging-max-backups", 0, "Maximum number of old log files to retain; 0 to disable")
-	flagSet.Bool("logging-local-time", true, "If the time in log files and backup filenames are local or UTC time")
-	flagSet.Bool("logging-compress", false, "Should rotated log files be compressed using gzip")
-
-	flagSet.Bool("standard-logging", true, "Log standard runtime information")
-	flagSet.String("standard-logging-format", logger.DefaultStandardLoggingFormat, "Template for standard log lines")
-
-	flagSet.Bool("request-logging", true, "Log HTTP requests")
-	flagSet.String("request-logging-format", logger.DefaultRequestLoggingFormat, "Template for HTTP request log lines")
-	flagSet.String("exclude-logging-paths", "", "Exclude logging requests to paths (eg: '/path1,/path2,/path3')")
-	flagSet.Bool("silence-ping-logging", false, "Disable logging of requests to ping endpoint")
-
-	flagSet.Bool("auth-logging", true, "Log authentication attempts")
-	flagSet.String("auth-logging-format", logger.DefaultAuthLoggingFormat, "Template for authentication log lines")
-
 	flagSet.String("provider", "google", "OAuth provider")
 	flagSet.String("provider-display-name", "", "Provider display name")
 	flagSet.String("oidc-issuer-url", "", "OpenID Connect issuer URL (ie: https://accounts.google.com)")
@@ -336,5 +290,7 @@ func NewFlagSet() *pflag.FlagSet {
 
 	flagSet.String("user-id-claim", "email", "which claim contains the user ID")
 
+	flagSet.AddFlagSet(loggingFlagSet())
+
 	return flagSet
 }

pkg/validation/logging.go

@@ -0,0 +1,62 @@
+package validation
+
+import (
+	"os"
+
+	"github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options"
+	"github.com/oauth2-proxy/oauth2-proxy/pkg/logger"
+	"gopkg.in/natefinch/lumberjack.v2"
+)
+
+// configureLogger is responsible for configuring the logger based on the options given
+func configureLogger(o options.Logging, pingPath string, msgs []string) []string {
+	// Setup the log file
+	if len(o.File.Filename) > 0 {
+		// Validate that the file/dir can be written
+		file, err := os.OpenFile(o.File.Filename, os.O_WRONLY|os.O_CREATE, 0666)
+		if err != nil {
+			if os.IsPermission(err) {
+				return append(msgs, "unable to write to log file: "+o.File.Filename)
+			}
+		}
+		file.Close()
+
+		logger.Printf("Redirecting logging to file: %s", o.File.Filename)
+
+		logWriter := &lumberjack.Logger{
+			Filename:   o.File.Filename,
+			MaxSize:    o.File.MaxSize, // megabytes
+			MaxAge:     o.File.MaxAge,  // days
+			MaxBackups: o.File.MaxBackups,
+			LocalTime:  o.LocalTime,
+			Compress:   o.File.Compress,
+		}
+
+		logger.SetOutput(logWriter)
+	}
+
+	// Supply a sanity warning to the logger if all logging is disabled
+	if !o.StandardEnabled && !o.AuthEnabled && !o.RequestEnabled {
+		logger.Print("Warning: Logging disabled. No further logs will be shown.")
+	}
+
+	// Pass configuration values to the standard logger
+	logger.SetStandardEnabled(o.StandardEnabled)
+	logger.SetAuthEnabled(o.AuthEnabled)
+	logger.SetReqEnabled(o.RequestEnabled)
+	logger.SetStandardTemplate(o.StandardFormat)
+	logger.SetAuthTemplate(o.AuthFormat)
+	logger.SetReqTemplate(o.RequestFormat)
+
+	excludePaths := o.ExcludePaths
+	if o.SilencePing {
+		excludePaths = append(excludePaths, pingPath)
+	}
+	logger.SetExcludePaths(excludePaths)
+
+	if !o.LocalTime {
+		logger.SetFlags(logger.Flags() | logger.LUTC)
+	}
+
+	return msgs
+}

pkg/validation/options.go

@@ -23,7 +23,6 @@ import (
 	"github.com/oauth2-proxy/oauth2-proxy/pkg/requests"
 	"github.com/oauth2-proxy/oauth2-proxy/pkg/sessions"
 	"github.com/oauth2-proxy/oauth2-proxy/providers"
-	"gopkg.in/natefinch/lumberjack.v2"
 )
 
 // Validate checks that required options are set and validates those that they
@@ -265,7 +264,7 @@ func Validate(o *options.Options) error {
 
 	msgs = parseSignatureKey(o, msgs)
 	msgs = validateCookieName(o, msgs)
-	msgs = setupLogger(o, msgs)
+	msgs = configureLogger(o.Logging, o.PingPath, msgs)
 
 	if o.ReverseProxy {
 		parser, err := ip.GetRealClientIPParser(o.RealClientIPHeader)
@@ -273,6 +272,11 @@ func Validate(o *options.Options) error {
 			msgs = append(msgs, fmt.Sprintf("real_client_ip_header (%s) not accepted parameter value: %v", o.RealClientIPHeader, err))
 		}
 		o.SetRealClientIPParser(parser)
+
+		// Allow the logger to get client IPs
+		logger.SetGetClientFunc(func(r *http.Request) string {
+			return ip.GetClientString(o.GetRealClientIPParser(), r, false)
+		})
 	}
 
 	if len(msgs) != 0 {
@@ -453,63 +457,6 @@ func validateCookieName(o *options.Options, msgs []string) []string {
 	return msgs
 }
 
-func setupLogger(o *options.Options, msgs []string) []string {
-	// Setup the log file
-	if len(o.LoggingFilename) > 0 {
-		// Validate that the file/dir can be written
-		file, err := os.OpenFile(o.LoggingFilename, os.O_WRONLY|os.O_CREATE, 0666)
-		if err != nil {
-			if os.IsPermission(err) {
-				return append(msgs, "unable to write to log file: "+o.LoggingFilename)
-			}
-		}
-		file.Close()
-
-		logger.Printf("Redirecting logging to file: %s", o.LoggingFilename)
-
-		logWriter := &lumberjack.Logger{
-			Filename:   o.LoggingFilename,
-			MaxSize:    o.LoggingMaxSize, // megabytes
-			MaxAge:     o.LoggingMaxAge,  // days
-			MaxBackups: o.LoggingMaxBackups,
-			LocalTime:  o.LoggingLocalTime,
-			Compress:   o.LoggingCompress,
-		}
-
-		logger.SetOutput(logWriter)
-	}
-
-	// Supply a sanity warning to the logger if all logging is disabled
-	if !o.StandardLogging && !o.AuthLogging && !o.RequestLogging {
-		logger.Print("Warning: Logging disabled. No further logs will be shown.")
-	}
-
-	// Pass configuration values to the standard logger
-	logger.SetStandardEnabled(o.StandardLogging)
-	logger.SetAuthEnabled(o.AuthLogging)
-	logger.SetReqEnabled(o.RequestLogging)
-	logger.SetStandardTemplate(o.StandardLoggingFormat)
-	logger.SetAuthTemplate(o.AuthLoggingFormat)
-	logger.SetReqTemplate(o.RequestLoggingFormat)
-	logger.SetGetClientFunc(func(r *http.Request) string {
-		return ip.GetClientString(o.GetRealClientIPParser(), r, false)
-	})
-
-	excludePaths := make([]string, 0)
-	excludePaths = append(excludePaths, strings.Split(o.ExcludeLoggingPaths, ",")...)
-	if o.SilencePingLogging {
-		excludePaths = append(excludePaths, o.PingPath)
-	}
-
-	logger.SetExcludePaths(excludePaths)
-
-	if !o.LoggingLocalTime {
-		logger.SetFlags(logger.Flags() | logger.LUTC)
-	}
-
-	return msgs
-}
-
 // jwtIssuer hold parsed JWT issuer info that's used to construct a verifier.
 type jwtIssuer struct {
 	issuerURI string