Print errors on extra keys in server config

This does NOT apply to the backend config, since each backend config
could have a variation of options that differ based off of the
configured backend itself. This may be an optimization that can be made
in the future, but I think each backend should be responsible for
performing its own configuration validation instead of overloading the
config itself with this functionality.

command/server/config.go

@@ -4,13 +4,15 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"log"
 	"os"
 	"path/filepath"
 	"strings"
 	"time"
 
+	"github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/hcl"
-	hclobj "github.com/hashicorp/hcl/hcl"
+	"github.com/hashicorp/hcl/hcl/ast"
 )
 
 // Config is the configuration for the vault server.
@@ -157,9 +159,12 @@ func LoadConfigFile(path string) (*Config, error) {
 	if err != nil {
 		return nil, err
 	}
+	return ParseConfig(string(d))
+}
 
+func ParseConfig(d string) (*Config, error) {
 	// Parse!
-	obj, err := hcl.Parse(string(d))
+	obj, err := hcl.Parse(d)
 	if err != nil {
 		return nil, err
 	}
@@ -181,49 +186,82 @@ func LoadConfigFile(path string) (*Config, error) {
 		}
 	}
 
-	if objs := obj.Get("listener", false); objs != nil {
+	list, ok := obj.Node.(*ast.ObjectList)
-		result.Listeners, err = loadListeners(objs)
+	if !ok {
-		if err != nil {
+		return nil, fmt.Errorf("error parsing: file doesn't contain a root object")
+	}
+
+	valid := []string{
+		"backend",
+		"ha_backend",
+		"listener",
+		"disable_cache",
+		"disable_mlock",
+		"telemetry",
+		"default_lease_ttl",
+		"max_lease_ttl",
+
+		// TODO: Remove in 0.6.0
+		// Deprecated keys
+		"statsd_addr",
+		"statsite_addr",
+	}
+	if err := checkHCLKeys(list, valid); err != nil {
+		return nil, err
+	}
+
+	// TODO: Remove in 0.6.0
+	// Preflight checks for deprecated keys
+	sda := list.Filter("statsd_addr")
+	ssa := list.Filter("statsite_addr")
+	if len(sda.Items) > 0 || len(ssa.Items) > 0 {
+		log.Println("[WARN] The top-level keys 'statsd_addr' and 'statsite_addr' " +
+			"have been moved into a 'telemetry' block instead. Please update your " +
+			"Vault configuration as this deprecation will be removed in the next " +
+			"major release. Values specified in a 'telemetry' block will take " +
+			"precendence.")
+
+		t := struct {
+			StatsdAddr   string `hcl:"statsd_addr"`
+			StatsiteAddr string `hcl:"statsite_addr"`
+		}{}
+		if err := hcl.DecodeObject(&t, list); err != nil {
 			return nil, err
 		}
-	}
+
-	if objs := obj.Get("backend", false); objs != nil {
+		result.Telemetry = &Telemetry{
-		result.Backend, err = loadBackend(objs)
+			StatsdAddr:   t.StatsdAddr,
-		if err != nil {
+			StatsiteAddr: t.StatsiteAddr,
-			return nil, err
-		}
-	}
-	if objs := obj.Get("ha_backend", false); objs != nil {
-		result.HABackend, err = loadBackend(objs)
-		if err != nil {
-			return nil, err
 		}
 	}
 
-	// A little hacky but upgrades the old stats config directives to the new way
+	if o := list.Filter("backend"); len(o.Items) > 0 {
-	if result.Telemetry == nil {
+		if err := parseBackends(&result, o); err != nil {
-		statsdAddr := obj.Get("statsd_addr", false)
+			return nil, fmt.Errorf("error parsing 'backend': %s", err)
-		statsiteAddr := obj.Get("statsite_addr", false)
+		}
+	}
 
-		if statsdAddr != nil || statsiteAddr != nil {
+	if o := list.Filter("ha_backend"); len(o.Items) > 0 {
-			result.Telemetry = &Telemetry{
+		if err := parseHABackends(&result, o); err != nil {
-				StatsdAddr:   getString(statsdAddr),
+			return nil, fmt.Errorf("error parsing 'ha_backend': %s", err)
-				StatsiteAddr: getString(statsiteAddr),
+		}
-			}
+	}
+
+	if o := list.Filter("listener"); len(o.Items) > 0 {
+		if err := parseListeners(&result, o); err != nil {
+			return nil, fmt.Errorf("error parsing 'listener': %s", err)
+		}
+	}
+
+	if o := list.Filter("telemetry"); len(o.Items) > 0 {
+		if err := parseTelemetry(&result, o); err != nil {
+			return nil, fmt.Errorf("error parsing 'telemetry': %s", err)
 		}
 	}
 
 	return &result, nil
 }
 
-func getString(o *hclobj.Object) string {
-	if o == nil || o.Type != hclobj.ValueTypeString {
-		return ""
-	}
-
-	return o.Value.(string)
-}
-
 // LoadConfigDir loads all the configurations in the given directory
 // in alphabetical order.
 func LoadConfigDir(dir string) (*Config, error) {
@@ -301,106 +339,163 @@ func isTemporaryFile(name string) bool {
 		(strings.HasPrefix(name, "#") && strings.HasSuffix(name, "#")) // emacs
 }
 
-func loadListeners(os *hclobj.Object) ([]*Listener, error) {
+func parseBackends(result *Config, list *ast.ObjectList) error {
-	var allNames []*hclobj.Object
+	if len(list.Items) > 1 {
-
+		return fmt.Errorf("only one 'backend' block is permitted")
-	// Really confusing iteration. The key is the false/true parameter
-	// of whether we're expanding or not. We first iterate over all
-	// the "listeners"
-	for _, o1 := range os.Elem(false) {
-		// Iterate expand to get the list of types
-		for _, o2 := range o1.Elem(true) {
-			switch o2.Type {
-			case hclobj.ValueTypeList:
-				// This switch is for JSON, to allow them to do this:
-				//
-				// "tcp": [{ ... }, { ... }]
-				//
-				// To configure multiple listeners of the same type.
-				for _, o3 := range o2.Elem(true) {
-					o3.Key = o2.Key
-					allNames = append(allNames, o3)
-				}
-			case hclobj.ValueTypeObject:
-				// This is for the standard `listener "tcp" { ... }` syntax
-				allNames = append(allNames, o2)
-			}
-		}
 	}
 
-	if len(allNames) == 0 {
+	// Get our item
-		return nil, nil
+	item := list.Items[0]
+
+	key := "backend"
+	if len(item.Keys) > 0 {
+		key = item.Keys[0].Token.Value().(string)
 	}
 
-	// Now go over all the types and their children in order to get
+	var m map[string]string
-	// all of the actual resources.
+	if err := hcl.DecodeObject(&m, item.Val); err != nil {
-	result := make([]*Listener, 0, len(allNames))
+		return multierror.Prefix(err, fmt.Sprintf("backend.%s:", key))
-	for _, obj := range allNames {
+	}
-		k := obj.Key
 
-		var config map[string]string
+	// Pull out the advertise address since it's commong to all backends
-		if err := hcl.DecodeObject(&config, obj); err != nil {
+	var advertiseAddr string
-			return nil, fmt.Errorf(
+	if v, ok := m["advertise_addr"]; ok {
-				"Error reading config for %s: %s",
+		advertiseAddr = v
-				k,
+		delete(m, "advertise_addr")
-				err)
+	}
+
+	result.Backend = &Backend{
+		AdvertiseAddr: advertiseAddr,
+		Type:          strings.ToLower(key),
+		Config:        m,
+	}
+	return nil
+}
+
+func parseHABackends(result *Config, list *ast.ObjectList) error {
+	if len(list.Items) > 1 {
+		return fmt.Errorf("only one 'ha_backend' block is permitted")
+	}
+
+	// Get our item
+	item := list.Items[0]
+
+	key := "backend"
+	if len(item.Keys) > 0 {
+		key = item.Keys[0].Token.Value().(string)
+	}
+
+	var m map[string]string
+	if err := hcl.DecodeObject(&m, item.Val); err != nil {
+		return multierror.Prefix(err, fmt.Sprintf("ha_backend.%s:", key))
+	}
+
+	// Pull out the advertise address since it's commong to all backends
+	var advertiseAddr string
+	if v, ok := m["advertise_addr"]; ok {
+		advertiseAddr = v
+		delete(m, "advertise_addr")
+	}
+
+	result.HABackend = &Backend{
+		AdvertiseAddr: advertiseAddr,
+		Type:          strings.ToLower(key),
+		Config:        m,
+	}
+	return nil
+}
+
+func parseListeners(result *Config, list *ast.ObjectList) error {
+	listeners := make([]*Listener, 0, len(list.Items))
+	for _, item := range list.Items {
+		key := "listener"
+		if len(item.Keys) > 0 {
+			key = item.Keys[0].Token.Value().(string)
 		}
 
-		result = append(result, &Listener{
+		valid := []string{
-			Type:   k,
+			"address",
-			Config: config,
+			"tls_disable",
+			"tls_cert_file",
+			"tls_key_file",
+			"tls_min_version",
+		}
+		if err := checkHCLKeys(item.Val, valid); err != nil {
+			return multierror.Prefix(err, fmt.Sprintf("listeners.%s:", key))
+		}
+
+		var m map[string]string
+		if err := hcl.DecodeObject(&m, item.Val); err != nil {
+			return multierror.Prefix(err, fmt.Sprintf("listeners.%s:", key))
+		}
+
+		listeners = append(listeners, &Listener{
+			Type:   strings.ToLower(key),
+			Config: m,
 		})
 	}
 
-	return result, nil
+	result.Listeners = listeners
+	return nil
 }
 
-func loadBackend(os *hclobj.Object) (*Backend, error) {
+func parseTelemetry(result *Config, list *ast.ObjectList) error {
-	var allNames []*hclobj.Object
+	if len(list.Items) > 1 {
+		return fmt.Errorf("only one 'telemetry' block is permitted")
+	}
 
-	// See loadListeners
+	// Get our one item
-	for _, o1 := range os.Elem(false) {
+	item := list.Items[0]
-		// Iterate expand to get the list of types
+
-		for _, o2 := range o1.Elem(true) {
+	// Check for invalid keys
-			// Iterate non-expand to get the full list of types
+	valid := []string{
-			for _, o3 := range o2.Elem(false) {
+		"statsite_address",
-				allNames = append(allNames, o3)
+		"statsd_address",
-			}
+		"disable_hostname",
+	}
+	if err := checkHCLKeys(item.Val, valid); err != nil {
+		return multierror.Prefix(err, "telemetry:")
+	}
+
+	var t Telemetry
+	if err := hcl.DecodeObject(&t, item.Val); err != nil {
+		return multierror.Prefix(err, "telemetry:")
+	}
+
+	if result.Telemetry == nil {
+		result.Telemetry = &Telemetry{}
+	}
+
+	if err := hcl.DecodeObject(&result.Telemetry, item.Val); err != nil {
+		return multierror.Prefix(err, "telemetry:")
+	}
+	return nil
+}
+
+func checkHCLKeys(node ast.Node, valid []string) error {
+	var list *ast.ObjectList
+	switch n := node.(type) {
+	case *ast.ObjectList:
+		list = n
+	case *ast.ObjectType:
+		list = n.List
+	default:
+		return fmt.Errorf("cannot check HCL keys of type %T", n)
+	}
+
+	validMap := make(map[string]struct{}, len(valid))
+	for _, v := range valid {
+		validMap[v] = struct{}{}
+	}
+
+	var result error
+	for _, item := range list.Items {
+		key := item.Keys[0].Token.Value().(string)
+		if _, ok := validMap[key]; !ok {
+			result = multierror.Append(result, fmt.Errorf(
+				"invalid key '%s' on line %d", key, item.Assign.Line))
 		}
 	}
 
-	if len(allNames) == 0 {
+	return result
-		return nil, nil
-	}
-	if len(allNames) > 1 {
-		keys := make([]string, 0, len(allNames))
-		for _, o := range allNames {
-			keys = append(keys, o.Key)
-		}
-
-		return nil, fmt.Errorf(
-			"Multiple backends declared. Only one is allowed: %v", keys)
-	}
-
-	// Now go over all the types and their children in order to get
-	// all of the actual resources.
-	var result Backend
-	obj := allNames[0]
-	result.Type = obj.Key
-
-	var config map[string]string
-	if err := hcl.DecodeObject(&config, obj); err != nil {
-		return nil, fmt.Errorf(
-			"Error reading config for backend %s: %s",
-			result.Type,
-			err)
-	}
-
-	if v, ok := config["advertise_addr"]; ok {
-		result.AdvertiseAddr = v
-		delete(config, "advertise_addr")
-	}
-
-	result.Config = config
-	return &result, nil
 }

command/server/config_test.go

@@ -2,6 +2,7 @@ package server
 
 import (
 	"reflect"
+	"strings"
 	"testing"
 	"time"
 )
@@ -53,7 +54,7 @@ func TestLoadConfigFile(t *testing.T) {
 		DefaultLeaseTTLRaw: "10h",
 	}
 	if !reflect.DeepEqual(config, expected) {
-		t.Fatalf("bad: %#v", config)
+		t.Fatalf("expected \n\n%#v\n\n to be \n\n%#v\n\n", config, expected)
 	}
 }
 
@@ -92,7 +93,7 @@ func TestLoadConfigFile_json(t *testing.T) {
 		DefaultLeaseTTLRaw: "10h",
 	}
 	if !reflect.DeepEqual(config, expected) {
-		t.Fatalf("bad: %#v", config)
+		t.Fatalf("expected \n\n%#v\n\n to be \n\n%#v\n\n", config, expected)
 	}
 }
 
@@ -133,7 +134,7 @@ func TestLoadConfigFile_json2(t *testing.T) {
 		},
 	}
 	if !reflect.DeepEqual(config, expected) {
-		t.Fatalf("bad: %#v", config)
+		t.Fatalf("expected \n\n%#v\n\n to be \n\n%#v\n\n", config, expected)
 	}
 }
 
@@ -176,3 +177,67 @@ func TestLoadConfigDir(t *testing.T) {
 		t.Fatalf("bad: %#v", config)
 	}
 }
+
+func TestParseConfig_badTopLevel(t *testing.T) {
+	_, err := ParseConfig(strings.TrimSpace(`
+backend {}
+bad  = "one"
+nope = "yes"
+`))
+
+	if err == nil {
+		t.Fatal("expected error")
+	}
+
+	if !strings.Contains(err.Error(), "invalid key 'bad' on line 2") {
+		t.Errorf("bad error: %q", err)
+	}
+
+	if !strings.Contains(err.Error(), "invalid key 'nope' on line 3") {
+		t.Errorf("bad error: %q", err)
+	}
+}
+
+func TestParseConfig_badListener(t *testing.T) {
+	_, err := ParseConfig(strings.TrimSpace(`
+listener "tcp" {
+	address = "1.2.3.3"
+	bad  = "one"
+	nope = "yes"
+}
+`))
+
+	if err == nil {
+		t.Fatal("expected error")
+	}
+
+	if !strings.Contains(err.Error(), "listeners.tcp: invalid key 'bad' on line 3") {
+		t.Errorf("bad error: %q", err)
+	}
+
+	if !strings.Contains(err.Error(), "listeners.tcp: invalid key 'nope' on line 4") {
+		t.Errorf("bad error: %q", err)
+	}
+}
+
+func TestParseConfig_badTelemetry(t *testing.T) {
+	_, err := ParseConfig(strings.TrimSpace(`
+telemetry {
+	statsd_address = "1.2.3.3"
+	bad  = "one"
+	nope = "yes"
+}
+`))
+
+	if err == nil {
+		t.Fatal("expected error")
+	}
+
+	if !strings.Contains(err.Error(), "telemetry: invalid key 'bad' on line 3") {
+		t.Errorf("bad error: %q", err)
+	}
+
+	if !strings.Contains(err.Error(), "telemetry: invalid key 'nope' on line 4") {
+		t.Errorf("bad error: %q", err)
+	}
+}