rewrite logging in console

- enhance logging throughout the codebase
- all packages at pkg/ should never log
  or perform log.Fatal() instead packages
  should return errors through functions.
- simplified various user, group mapping
  and removed redundant functions.
- deprecate older flags like --tls-certificate
  --tls-key and --tls-ca as we do not use
  them anymore, keep them for backward compatibility
  for some time.

Author: harshavardhana

cmd/console/server.go

@@ -20,10 +20,7 @@ import (
 	"context"
 	"fmt"
 	"io/ioutil"
-	"log"
-	"os"
 	"path/filepath"
-	"strconv"
 	"time"
 
 	"github.com/go-openapi/loads"
@@ -38,18 +35,18 @@ import (
 var serverCmd = cli.Command{
 	Name:    "server",
 	Aliases: []string{"srv"},
-	Usage:   "starts Console server",
+	Usage:   "Start MinIO Console server",
 	Action:  StartServer,
 	Flags: []cli.Flag{
 		cli.StringFlag{
 			Name:  "host",
 			Value: restapi.GetHostname(),
-			Usage: "hostname",
+			Usage: "bind to a specific HOST, HOST can be an IP or hostname",
 		},
 		cli.IntFlag{
 			Name:  "port",
 			Value: restapi.GetPort(),
-			Usage: "HTTP port",
+			Usage: "bind to specific HTTP port",
 		},
 		// This is kept here for backward compatibility,
 		// hostname's do not have HTTP or HTTPs
@@ -58,52 +55,52 @@ var serverCmd = cli.Command{
 		cli.StringFlag{
 			Name:   "tls-host",
 			Value:  restapi.GetHostname(),
-			Usage:  "HTTPS hostname",
 			Hidden: true,
 		},
+		cli.StringFlag{
+			Name:  "certs-dir",
+			Value: certs.GlobalCertsCADir.Get(),
+			Usage: "path to certs directory",
+		},
 		cli.IntFlag{
 			Name:  "tls-port",
 			Value: restapi.GetTLSPort(),
-			Usage: "HTTPS port",
+			Usage: "bind to specific HTTPS port",
 		},
 		cli.StringFlag{
 			Name:  "tls-redirect",
 			Value: restapi.GetTLSRedirect(),
 			Usage: "toggle HTTP->HTTPS redirect",
 		},
 		cli.StringFlag{
-			Name:  "certs-dir",
-			Value: certs.GlobalCertsCADir.Get(),
-			Usage: "path to certs directory",
-		},
-		cli.StringFlag{
-			Name:  "tls-certificate",
-			Value: "",
-			Usage: "path to TLS public certificate",
+			Name:   "tls-certificate",
+			Value:  "",
+			Usage:  "path to TLS public certificate",
+			Hidden: true,
 		},
 		cli.StringFlag{
-			Name:  "tls-key",
-			Value: "",
-			Usage: "path to TLS private key",
+			Name:   "tls-key",
+			Value:  "",
+			Usage:  "path to TLS private key",
+			Hidden: true,
 		},
 		cli.StringFlag{
-			Name:  "tls-ca",
-			Value: "",
-			Usage: "path to TLS Certificate Authority",
+			Name:   "tls-ca",
+			Value:  "",
+			Usage:  "path to TLS Certificate Authority",
+			Hidden: true,
 		},
 	},
 }
 
-// StartServer starts the console service
-func StartServer(ctx *cli.Context) error {
+func buildServer() (*restapi.Server, error) {
 	swaggerSpec, err := loads.Embedded(restapi.SwaggerJSON, restapi.FlatSwaggerJSON)
 	if err != nil {
-		log.Fatalln(err)
+		return nil, err
 	}
 
 	api := operations.NewConsoleAPI(swaggerSpec)
 	server := restapi.NewServer(api)
-	defer server.Shutdown()
 
 	parser := flags.NewParser(server, flags.Default)
 	parser.ShortDescription = "MinIO Console Server"
@@ -114,33 +111,31 @@ func StartServer(ctx *cli.Context) error {
 	for _, optsGroup := range api.CommandLineOptionsGroups {
 		_, err := parser.AddGroup(optsGroup.ShortDescription, optsGroup.LongDescription, optsGroup.Options)
 		if err != nil {
-			log.Fatalln(err)
+			return nil, err
 		}
 	}
 
 	if _, err := parser.Parse(); err != nil {
-		code := 1
-		if fe, ok := err.(*flags.Error); ok {
-			if fe.Type == flags.ErrHelp {
-				code = 0
-			}
-		}
-		os.Exit(code)
+		return nil, err
 	}
 
-	server.Host = ctx.String("host")
-	server.Port = ctx.Int("port")
-	restapi.Hostname = server.Host
-	restapi.Port = strconv.Itoa(server.Port)
+	return server, nil
+}
 
+func loadAllCerts(ctx *cli.Context) error {
+	var err error
 	// Set all certs and CAs directories path
-	certs.GlobalCertsDir, _ = certs.NewConfigDirFromCtx(ctx, "certs-dir", certs.DefaultCertsDir.Get)
-	certs.GlobalCertsCADir = &certs.ConfigDir{Path: filepath.Join(certs.GlobalCertsDir.Get(), certs.CertsCADir)}
+	certs.GlobalCertsDir, _, err = certs.NewConfigDirFromCtx(ctx, "certs-dir", certs.DefaultCertsDir.Get)
+	if err != nil {
+		return err
+	}
 
+	certs.GlobalCertsCADir = &certs.ConfigDir{Path: filepath.Join(certs.GlobalCertsDir.Get(), certs.CertsCADir)}
 	// check if certs and CAs directories exists or can be created
-	if err := certs.MkdirAllIgnorePerm(certs.GlobalCertsCADir.Get()); err != nil {
-		log.Println(fmt.Sprintf("Unable to create certs CA directory at %s", certs.GlobalCertsCADir.Get()))
+	if err = certs.MkdirAllIgnorePerm(certs.GlobalCertsCADir.Get()); err != nil {
+		return fmt.Errorf("Unable to create certs CA directory at %s: with %w", certs.GlobalCertsCADir.Get(), err)
 	}
+
 	// load the certificates and the CAs
 	restapi.GlobalRootCAs, restapi.GlobalPublicCerts, restapi.GlobalTLSCertsManager = certs.GetAllCertificatesAndCAs()
 
@@ -153,7 +148,7 @@ func StartServer(ctx *cli.Context) error {
 		if swaggerServerCertificate != "" && swaggerServerCertificateKey != "" {
 			if err = certs.AddCertificate(context.Background(),
 				restapi.GlobalTLSCertsManager, swaggerServerCertificate, swaggerServerCertificateKey); err != nil {
-				log.Fatalln(err)
+				return err
 			}
 			if x509Certs, err := certs.ParsePublicCertFile(swaggerServerCertificate); err == nil {
 				restapi.GlobalPublicCerts = append(restapi.GlobalPublicCerts, x509Certs...)
@@ -169,25 +164,40 @@ func StartServer(ctx *cli.Context) error {
 		}
 	}
 
-	if len(restapi.GlobalPublicCerts) > 0 {
-		// If TLS certificates are provided enforce the HTTPS schema, meaning console will redirect
-		// plain HTTP connections to HTTPS server
-		server.EnabledListeners = []string{"http", "https"}
-		server.TLSPort = ctx.Int("tls-port")
-		// Need to store tls-port, tls-host un config variables so secure.middleware can read from there
-		restapi.TLSPort = strconv.Itoa(server.TLSPort)
-		restapi.Hostname = ctx.String("host")
-		restapi.TLSRedirect = ctx.String("tls-redirect")
+	return nil
+}
+
+// StartServer starts the console service
+func StartServer(ctx *cli.Context) error {
+	if err := loadAllCerts(ctx); err != nil {
+		restapi.LogError("Unable to load certs: %v", err)
+		return err
+	}
+
+	var rctx restapi.Context
+	if err := rctx.Load(ctx); err != nil {
+		restapi.LogError("argument validation failed: %v", err)
+		return err
 	}
 
-	server.ConfigureAPI()
+	server, err := buildServer()
+	if err != nil {
+		restapi.LogError("Unable to initialize console server: %v", err)
+		return err
+	}
+
+	s := server.Configure(rctx)
+	defer s.Shutdown()
 
 	// subnet license refresh process
 	go func() {
+		// start refreshing subnet license after 5 seconds..
+		time.Sleep(time.Second * 5)
+
 		failedAttempts := 0
 		for {
 			if err := restapi.RefreshLicense(); err != nil {
-				log.Println(err)
+				restapi.LogError("Refreshing subnet license failed: %v", err)
 				failedAttempts++
 				// end license refresh after 3 consecutive failed attempts
 				if failedAttempts >= 3 {
@@ -204,8 +214,5 @@ func StartServer(ctx *cli.Context) error {
 		}
 	}()
 
-	if err := server.Serve(); err != nil {
-		log.Fatalln(err)
-	}
-	return nil
+	return s.Serve()
 }

pkg/auth/idp/oauth2/provider.go

@@ -22,7 +22,6 @@ import (
 	"encoding/base64"
 	"errors"
 	"fmt"
-	"log"
 	"net/http"
 	"net/url"
 	"strconv"
@@ -37,10 +36,6 @@ import (
 	xoauth2 "golang.org/x/oauth2"
 )
 
-var (
-	errGeneric = errors.New("an error occurred, please try again")
-)
-
 type Configuration interface {
 	Exchange(ctx context.Context, code string, opts ...xoauth2.AuthCodeOption) (*xoauth2.Token, error)
 	AuthCodeURL(state string, opts ...xoauth2.AuthCodeOption) string
@@ -168,8 +163,8 @@ type User struct {
 // VerifyIdentity will contact the configured IDP and validate the user identity based on the authorization code
 func (client *Provider) VerifyIdentity(ctx context.Context, code, state string) (*credentials.Credentials, error) {
 	// verify the provided state is valid (prevents CSRF attacks)
-	if !validateOauth2State(state) {
-		return nil, errGeneric
+	if err := validateOauth2State(state); err != nil {
+		return nil, err
 	}
 	getWebTokenExpiry := func() (*credentials.WebIdentityToken, error) {
 		oauth2Token, err := client.oauth2Config.Exchange(ctx, code)
@@ -210,29 +205,30 @@ func (client *Provider) VerifyIdentity(ctx context.Context, code, state string)
 // validateOauth2State validates the provided state was originated using the same
 // instance (or one configured using the same secrets) of Console, this is basically used to prevent CSRF attacks
 // https://security.stackexchange.com/questions/20187/oauth2-cross-site-request-forgery-and-state-parameter
-func validateOauth2State(state string) bool {
+func validateOauth2State(state string) error {
 	// state contains a base64 encoded string that may ends with "==", the browser encodes that to "%3D%3D"
 	// query unescape is need it before trying to decode the base64 string
 	encodedMessage, err := url.QueryUnescape(state)
 	if err != nil {
-		log.Println(err)
-		return false
+		return err
 	}
 	// decode the state parameter value
 	message, err := base64.StdEncoding.DecodeString(encodedMessage)
 	if err != nil {
-		log.Println(err)
-		return false
+		return err
 	}
 	s := strings.Split(string(message), ":")
 	// Validate that the decoded message has the right format "message:hmac"
 	if len(s) != 2 {
-		return false
+		return fmt.Errorf("invalid number of tokens, expected only 2, got %d instead", len(s))
 	}
 	// extract the state and hmac
 	incomingState, incomingHmac := s[0], s[1]
 	// validate that hmac(incomingState + pbkdf2(secret, salt)) == incomingHmac
-	return utils.ComputeHmac256(incomingState, derivedKey) == incomingHmac
+	if calculatedHmac := utils.ComputeHmac256(incomingState, derivedKey); calculatedHmac != incomingHmac {
+		return fmt.Errorf("oauth2 state is invalid, expected %d, got %d", calculatedHmac, incomingHmac)
+	}
+	return nil
 }
 
 // GetRandomStateWithHMAC computes message + hmac(message, pbkdf2(key, salt)) to be used as state during the oauth authorization

pkg/auth/operator.go

@@ -18,7 +18,6 @@ package auth
 
 import (
 	"context"
-	"log"
 
 	"github.com/minio/console/cluster"
 	"github.com/minio/minio-go/v7/pkg/credentials"
@@ -64,16 +63,12 @@ func (c *operatorClient) Authenticate(ctx context.Context) ([]byte, error) {
 	return c.client.RESTClient().Verb("GET").RequestURI("/api").DoRaw(ctx)
 }
 
-// isServiceAccountTokenValid will make an authenticated request against kubernetes api, if the
+// checkServiceAccountTokenValid will make an authenticated request against kubernetes api, if the
 // request success means the provided jwt its a valid service account token and the console user can use it for future
 // requests until it expires
-func isServiceAccountTokenValid(ctx context.Context, operatorClient OperatorClient) bool {
+func checkServiceAccountTokenValid(ctx context.Context, operatorClient OperatorClient) error {
 	_, err := operatorClient.Authenticate(ctx)
-	if err != nil {
-		log.Println(err)
-		return false
-	}
-	return true
+	return err
 }
 
 // GetConsoleCredentialsForOperator will validate the provided JWT (service account token) and return it in the form of credentials.Login
@@ -86,8 +81,8 @@ func GetConsoleCredentialsForOperator(jwt string) (*credentials.Credentials, err
 	opClient := &operatorClient{
 		client: opClientClientSet,
 	}
-	if isServiceAccountTokenValid(ctx, opClient) {
-		return credentials.New(operatorCredentialsProvider{serviceAccountJWT: jwt}), nil
+	if err = checkServiceAccountTokenValid(ctx, opClient); err != nil {
+		return nil, errInvalidCredentials
 	}
-	return nil, errInvalidCredentials
+	return credentials.New(operatorCredentialsProvider{serviceAccountJWT: jwt}), nil
 }

pkg/auth/operator_test.go

@@ -20,8 +20,7 @@ func (c *operatorClientTest) Authenticate(ctx context.Context) ([]byte, error) {
 	return operatorAuthenticateMock(ctx)
 }
 
-func Test_isServiceAccountTokenValid(t *testing.T) {
-
+func Test_checkServiceAccountTokenValid(t *testing.T) {
 	successResponse := func() {
 		operatorAuthenticateMock = func(ctx context.Context) ([]byte, error) {
 			return nil, nil
@@ -70,12 +69,17 @@ func Test_isServiceAccountTokenValid(t *testing.T) {
 		},
 	}
 	for _, tt := range tests {
+		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			if tt.args.mockFunction != nil {
 				tt.args.mockFunction()
 			}
-			if got := isServiceAccountTokenValid(tt.args.ctx, tt.args.operatorClient); got != tt.want {
-				t.Errorf("isServiceAccountTokenValid() = %v, want %v", got, tt.want)
+			got := checkServiceAccountTokenValid(tt.args.ctx, tt.args.operatorClient)
+			if got != nil && tt.want {
+				t.Errorf("checkServiceAccountTokenValid() = expected success but got %s", got)
+			}
+			if got == nil && !tt.want {
+				t.Error("checkServiceAccountTokenValid() = expected failure but got success")
 			}
 		})
 	}