From 6747c546afc598f7e1911a3ed3f607685ca7c9e2 Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Fri, 24 Feb 2023 13:00:09 -0500 Subject: [PATCH] Address some small issues within pki health-check (#19295) * Address some small issues within pki health-check - Notify user yaml output mode is not support with --list argument - Output pure JSON in json output mode with --list argument - If a checker returns a nil response, convert to an empty slice - Add handler for permission errors to too many certs checker - Add checks for permission issues within hardware_backed_root and root_issued_leaves * Identify the role that contained the permission issue in role based checks - Augument the role health checks to identify the role(s) that we have insufficient permissions to read instead of an overall read failure - Treat the failure to list roles as a complete failure for the check --- command/healthcheck/healthcheck.go | 4 ++ .../healthcheck/pki_hardware_backed_root.go | 26 +++++++++- .../pki_role_allows_glob_wildcards.go | 39 ++++++++++---- .../healthcheck/pki_role_allows_localhost.go | 40 ++++++++++---- .../healthcheck/pki_role_no_store_false.go | 52 ++++++++++++------- command/healthcheck/pki_root_issued_leaves.go | 31 ++++++++++- command/healthcheck/pki_too_many_certs.go | 22 +++++++- command/pki_health_check.go | 10 +++- command/pki_health_check_test.go | 8 ++- 9 files changed, 187 insertions(+), 45 deletions(-) diff --git a/command/healthcheck/healthcheck.go b/command/healthcheck/healthcheck.go index 1949de04e..2ce9c2dee 100644 --- a/command/healthcheck/healthcheck.go +++ b/command/healthcheck/healthcheck.go @@ -119,6 +119,10 @@ func (e *Executor) Execute() (map[string][]*Result, error) { return nil, fmt.Errorf("failed to evaluate %v: %w", checker.Name(), err) } + if results == nil { + results = []*Result{} + } + for _, result := range results { result.Endpoint = e.templatePath(result.Endpoint) result.StatusDisplay = ResultStatusNameMap[result.Status] diff --git a/command/healthcheck/pki_hardware_backed_root.go b/command/healthcheck/pki_hardware_backed_root.go index 199a781fe..89d3550ea 100644 --- a/command/healthcheck/pki_hardware_backed_root.go +++ b/command/healthcheck/pki_hardware_backed_root.go @@ -13,12 +13,14 @@ type HardwareBackedRoot struct { UnsupportedVersion bool + FetchIssues map[string]*PathFetch IssuerKeyMap map[string]string KeyIsManaged map[string]string } func NewHardwareBackedRootCheck() Check { return &HardwareBackedRoot{ + FetchIssues: make(map[string]*PathFetch), IssuerKeyMap: make(map[string]string), KeyIsManaged: make(map[string]string), } @@ -64,6 +66,7 @@ func (h *HardwareBackedRoot) FetchResources(e *Executor) error { if err != nil { return err } + h.FetchIssues[issuer] = ret continue } @@ -83,13 +86,15 @@ func (h *HardwareBackedRoot) FetchResources(e *Executor) error { } h.IssuerKeyMap[issuer] = keyId - skip, _, keyEntry, err := pkiFetchKeyEntry(e, keyId, func() { + skip, ret, keyEntry, err := pkiFetchKeyEntry(e, keyId, func() { h.UnsupportedVersion = true }) if skip || err != nil || keyEntry == nil { if err != nil { return err } + + h.FetchIssues[issuer] = ret continue } @@ -112,6 +117,25 @@ func (h *HardwareBackedRoot) Evaluate(e *Executor) (results []*Result, err error return []*Result{&ret}, nil } + for issuer, fetchPath := range h.FetchIssues { + if fetchPath != nil && fetchPath.IsSecretPermissionsError() { + delete(h.IssuerKeyMap, issuer) + ret := Result{ + Status: ResultInsufficientPermissions, + Endpoint: fetchPath.Path, + Message: "Without this information, this health check is unable to function.", + } + + if e.Client.Token() == "" { + ret.Message = "No token available so unable for the endpoint for this mount. " + ret.Message + } else { + ret.Message = "This token lacks permission for the endpoint for this mount. " + ret.Message + } + + results = append(results, &ret) + } + } + for name, keyId := range h.IssuerKeyMap { var ret Result ret.Status = ResultInformational diff --git a/command/healthcheck/pki_role_allows_glob_wildcards.go b/command/healthcheck/pki_role_allows_glob_wildcards.go index 6978f3517..34fb09927 100644 --- a/command/healthcheck/pki_role_allows_glob_wildcards.go +++ b/command/healthcheck/pki_role_allows_glob_wildcards.go @@ -10,14 +10,16 @@ import ( type RoleAllowsGlobWildcards struct { Enabled bool UnsupportedVersion bool - NoPerms bool - RoleEntryMap map[string]map[string]interface{} + RoleListFetchIssue *PathFetch + RoleFetchIssues map[string]*PathFetch + RoleEntryMap map[string]map[string]interface{} } func NewRoleAllowsGlobWildcardsCheck() Check { return &RoleAllowsGlobWildcards{ - RoleEntryMap: make(map[string]map[string]interface{}), + RoleFetchIssues: make(map[string]*PathFetch), + RoleEntryMap: make(map[string]map[string]interface{}), } } @@ -49,7 +51,7 @@ func (h *RoleAllowsGlobWildcards) FetchResources(e *Executor) error { }) if exit || err != nil { if f != nil && f.IsSecretPermissionsError() { - h.NoPerms = true + h.RoleListFetchIssue = f } return err } @@ -60,7 +62,7 @@ func (h *RoleAllowsGlobWildcards) FetchResources(e *Executor) error { }) if skip || err != nil || entry == nil { if f != nil && f.IsSecretPermissionsError() { - h.NoPerms = true + h.RoleFetchIssues[role] = f } if err != nil { return err @@ -84,18 +86,37 @@ func (h *RoleAllowsGlobWildcards) Evaluate(e *Executor) (results []*Result, err } return []*Result{&ret}, nil } - if h.NoPerms { + if h.RoleListFetchIssue != nil && h.RoleListFetchIssue.IsSecretPermissionsError() { ret := Result{ Status: ResultInsufficientPermissions, - Endpoint: "/{{mount}}/roles", - Message: "lacks permission either to list the roles or to read a specific role. This may restrict the ability to fully execute this health check.", + Endpoint: h.RoleListFetchIssue.Path, + Message: "lacks permission either to list the roles. This restricts the ability to fully execute this health check.", } if e.Client.Token() == "" { ret.Message = "No token available and so this health check " + ret.Message } else { ret.Message = "This token " + ret.Message } - results = append(results, &ret) + return []*Result{&ret}, nil + } + + for role, fetchPath := range h.RoleFetchIssues { + if fetchPath != nil && fetchPath.IsSecretPermissionsError() { + delete(h.RoleEntryMap, role) + ret := Result{ + Status: ResultInsufficientPermissions, + Endpoint: fetchPath.Path, + Message: "Without this information, this health check is unable to function.", + } + + if e.Client.Token() == "" { + ret.Message = "No token available so unable for the endpoint for this mount. " + ret.Message + } else { + ret.Message = "This token lacks permission the endpoint for this mount. " + ret.Message + } + + results = append(results, &ret) + } } for role, entry := range h.RoleEntryMap { diff --git a/command/healthcheck/pki_role_allows_localhost.go b/command/healthcheck/pki_role_allows_localhost.go index c725f86d1..568aa3a5f 100644 --- a/command/healthcheck/pki_role_allows_localhost.go +++ b/command/healthcheck/pki_role_allows_localhost.go @@ -9,14 +9,16 @@ import ( type RoleAllowsLocalhost struct { Enabled bool UnsupportedVersion bool - NoPerms bool - RoleEntryMap map[string]map[string]interface{} + RoleListFetchIssue *PathFetch + RoleFetchIssues map[string]*PathFetch + RoleEntryMap map[string]map[string]interface{} } func NewRoleAllowsLocalhostCheck() Check { return &RoleAllowsLocalhost{ - RoleEntryMap: make(map[string]map[string]interface{}), + RoleFetchIssues: make(map[string]*PathFetch), + RoleEntryMap: make(map[string]map[string]interface{}), } } @@ -48,7 +50,7 @@ func (h *RoleAllowsLocalhost) FetchResources(e *Executor) error { }) if exit || err != nil { if f != nil && f.IsSecretPermissionsError() { - h.NoPerms = true + h.RoleListFetchIssue = f } return err } @@ -59,7 +61,7 @@ func (h *RoleAllowsLocalhost) FetchResources(e *Executor) error { }) if skip || err != nil || entry == nil { if f != nil && f.IsSecretPermissionsError() { - h.NoPerms = true + h.RoleFetchIssues[role] = f } if err != nil { return err @@ -83,18 +85,38 @@ func (h *RoleAllowsLocalhost) Evaluate(e *Executor) (results []*Result, err erro } return []*Result{&ret}, nil } - if h.NoPerms { + + if h.RoleListFetchIssue != nil && h.RoleListFetchIssue.IsSecretPermissionsError() { ret := Result{ Status: ResultInsufficientPermissions, - Endpoint: "/{{mount}}/roles", - Message: "lacks permission either to list the roles or to read a specific role. This may restrict the ability to fully execute this health check", + Endpoint: h.RoleListFetchIssue.Path, + Message: "lacks permission either to list the roles. This restricts the ability to fully execute this health check.", } if e.Client.Token() == "" { ret.Message = "No token available and so this health check " + ret.Message } else { ret.Message = "This token " + ret.Message } - results = append(results, &ret) + return []*Result{&ret}, nil + } + + for role, fetchPath := range h.RoleFetchIssues { + if fetchPath != nil && fetchPath.IsSecretPermissionsError() { + delete(h.RoleEntryMap, role) + ret := Result{ + Status: ResultInsufficientPermissions, + Endpoint: fetchPath.Path, + Message: "Without this information, this health check is unable to function.", + } + + if e.Client.Token() == "" { + ret.Message = "No token available so unable for the endpoint for this mount. " + ret.Message + } else { + ret.Message = "This token lacks permission the endpoint for this mount. " + ret.Message + } + + results = append(results, &ret) + } } for role, entry := range h.RoleEntryMap { diff --git a/command/healthcheck/pki_role_no_store_false.go b/command/healthcheck/pki_role_no_store_false.go index 67d686a7a..4fa7ba5ac 100644 --- a/command/healthcheck/pki_role_no_store_false.go +++ b/command/healthcheck/pki_role_no_store_false.go @@ -11,19 +11,20 @@ import ( type RoleNoStoreFalse struct { Enabled bool UnsupportedVersion bool - NoPerms bool AllowedRoles map[string]bool - CertCounts int - RoleEntryMap map[string]map[string]interface{} - CRLConfig *PathFetch + RoleListFetchIssue *PathFetch + RoleFetchIssues map[string]*PathFetch + RoleEntryMap map[string]map[string]interface{} + CRLConfig *PathFetch } func NewRoleNoStoreFalseCheck() Check { return &RoleNoStoreFalse{ - AllowedRoles: make(map[string]bool), - RoleEntryMap: make(map[string]map[string]interface{}), + RoleFetchIssues: make(map[string]*PathFetch), + AllowedRoles: make(map[string]bool), + RoleEntryMap: make(map[string]map[string]interface{}), } } @@ -64,7 +65,7 @@ func (h *RoleNoStoreFalse) FetchResources(e *Executor) error { }) if exit || err != nil { if f != nil && f.IsSecretPermissionsError() { - h.NoPerms = true + h.RoleListFetchIssue = f } return err } @@ -75,7 +76,7 @@ func (h *RoleNoStoreFalse) FetchResources(e *Executor) error { }) if skip || err != nil || entry == nil { if f != nil && f.IsSecretPermissionsError() { - h.NoPerms = true + h.RoleFetchIssues[role] = f } if err != nil { return err @@ -86,14 +87,6 @@ func (h *RoleNoStoreFalse) FetchResources(e *Executor) error { h.RoleEntryMap[role] = entry } - exit, _, leaves, err := pkiFetchLeavesList(e, func() { - h.UnsupportedVersion = true - }) - if exit || err != nil { - return err - } - h.CertCounts = len(leaves) - // Check if the issuer is fetched yet. configRet, err := e.FetchIfNotFetched(logical.ReadOperation, "/{{mount}}/config/crl") if err != nil { @@ -116,18 +109,37 @@ func (h *RoleNoStoreFalse) Evaluate(e *Executor) (results []*Result, err error) return []*Result{&ret}, nil } - if h.NoPerms { + if h.RoleListFetchIssue != nil && h.RoleListFetchIssue.IsSecretPermissionsError() { ret := Result{ Status: ResultInsufficientPermissions, - Endpoint: "/{{mount}}/roles", - Message: "lacks permission either to list the roles or to read a specific role. This may restrict the ability to fully execute this health check", + Endpoint: h.RoleListFetchIssue.Path, + Message: "lacks permission either to list the roles. This restricts the ability to fully execute this health check.", } if e.Client.Token() == "" { ret.Message = "No token available and so this health check " + ret.Message } else { ret.Message = "This token " + ret.Message } - results = append(results, &ret) + return []*Result{&ret}, nil + } + + for role, fetchPath := range h.RoleFetchIssues { + if fetchPath != nil && fetchPath.IsSecretPermissionsError() { + delete(h.RoleEntryMap, role) + ret := Result{ + Status: ResultInsufficientPermissions, + Endpoint: fetchPath.Path, + Message: "Without this information, this health check is unable to function.", + } + + if e.Client.Token() == "" { + ret.Message = "No token available so unable for the endpoint for this mount. " + ret.Message + } else { + ret.Message = "This token lacks permission the endpoint for this mount. " + ret.Message + } + + results = append(results, &ret) + } } crlAutoRebuild := false diff --git a/command/healthcheck/pki_root_issued_leaves.go b/command/healthcheck/pki_root_issued_leaves.go index 3252a91fb..e858794b6 100644 --- a/command/healthcheck/pki_root_issued_leaves.go +++ b/command/healthcheck/pki_root_issued_leaves.go @@ -14,12 +14,14 @@ type RootIssuedLeaves struct { CertsToFetch int + FetchIssues map[string]*PathFetch RootCertMap map[string]*x509.Certificate LeafCertMap map[string]*x509.Certificate } func NewRootIssuedLeavesCheck() Check { return &RootIssuedLeaves{ + FetchIssues: make(map[string]*PathFetch), RootCertMap: make(map[string]*x509.Certificate), LeafCertMap: make(map[string]*x509.Certificate), } @@ -64,9 +66,10 @@ func (h *RootIssuedLeaves) FetchResources(e *Executor) error { } for _, issuer := range issuers { - skip, _, cert, err := pkiFetchIssuer(e, issuer, func() { + skip, pathFetch, cert, err := pkiFetchIssuer(e, issuer, func() { h.UnsupportedVersion = true }) + h.FetchIssues[issuer] = pathFetch if skip || err != nil { if err != nil { return err @@ -85,10 +88,15 @@ func (h *RootIssuedLeaves) FetchResources(e *Executor) error { h.RootCertMap[issuer] = cert } - exit, _, leaves, err := pkiFetchLeavesList(e, func() { + exit, f, leaves, err := pkiFetchLeavesList(e, func() { h.UnsupportedVersion = true }) if exit || err != nil { + if f != nil && f.IsSecretPermissionsError() { + for _, issuer := range issuers { + h.FetchIssues[issuer] = f + } + } return err } @@ -130,6 +138,25 @@ func (h *RootIssuedLeaves) Evaluate(e *Executor) (results []*Result, err error) return []*Result{&ret}, nil } + for issuer, fetchPath := range h.FetchIssues { + if fetchPath != nil && fetchPath.IsSecretPermissionsError() { + delete(h.RootCertMap, issuer) + ret := Result{ + Status: ResultInsufficientPermissions, + Endpoint: fetchPath.Path, + Message: "Without this information, this health check is unable to function.", + } + + if e.Client.Token() == "" { + ret.Message = "No token available so unable for the endpoint for this mount. " + ret.Message + } else { + ret.Message = "This token lacks permission for the endpoint for this mount. " + ret.Message + } + + results = append(results, &ret) + } + } + issuerHasLeaf := make(map[string]bool) for serial, leaf := range h.LeafCertMap { if len(issuerHasLeaf) == len(h.RootCertMap) { diff --git a/command/healthcheck/pki_too_many_certs.go b/command/healthcheck/pki_too_many_certs.go index 8bd61003b..6b07b5dfe 100644 --- a/command/healthcheck/pki_too_many_certs.go +++ b/command/healthcheck/pki_too_many_certs.go @@ -14,6 +14,7 @@ type TooManyCerts struct { CountWarning int CertCounts int + FetchIssue *PathFetch } func NewTooManyCertsCheck() Check { @@ -60,7 +61,9 @@ func (h *TooManyCerts) FetchResources(e *Executor) error { exit, leavesRet, _, err := pkiFetchLeavesList(e, func() { h.UnsupportedVersion = true }) - if exit { + h.FetchIssue = leavesRet + + if exit || err != nil { return err } @@ -80,6 +83,23 @@ func (h *TooManyCerts) Evaluate(e *Executor) (results []*Result, err error) { return []*Result{&ret}, nil } + if h.FetchIssue != nil && h.FetchIssue.IsSecretPermissionsError() { + ret := Result{ + Status: ResultInsufficientPermissions, + Endpoint: h.FetchIssue.Path, + Message: "Without this information, this health check is unable to function.", + } + + if e.Client.Token() == "" { + ret.Message = "No token available so unable to list the endpoint for this mount. " + ret.Message + } else { + ret.Message = "This token lacks permission to list the endpoint for this mount. " + ret.Message + } + + results = append(results, &ret) + return + } + ret := Result{ Status: ResultOK, Endpoint: "/{{mount}}/certs", diff --git a/command/pki_health_check.go b/command/pki_health_check.go index 39d3ba73f..8c56a2c1f 100644 --- a/command/pki_health_check.go +++ b/command/pki_health_check.go @@ -223,7 +223,15 @@ func (c *PKIHealthCheckCommand) Run(args []string) int { // Handle listing, if necessary. if c.flagList { - c.UI.Output("Default health check config:") + uiFormat := Format(c.UI) + if uiFormat == "yaml" { + c.UI.Error("YAML output format is not supported by the --list command") + return pkiRetUsage + } + + if uiFormat != "json" { + c.UI.Output("Default health check config:") + } config := map[string]map[string]interface{}{} for _, checker := range executor.Checkers { config[checker.Name()] = checker.DefaultConfig() diff --git a/command/pki_health_check_test.go b/command/pki_health_check_test.go index bdd491a04..af3cf337a 100644 --- a/command/pki_health_check_test.go +++ b/command/pki_health_check_test.go @@ -271,6 +271,8 @@ func testPKIHealthCheckCommand(tb testing.TB) (*cli.MockUi, *PKIHealthCheckComma } func execPKIHC(t *testing.T, client *api.Client, ok bool) (int, string, map[string][]map[string]interface{}) { + t.Helper() + stdout := bytes.NewBuffer(nil) stderr := bytes.NewBuffer(nil) runOpts := &RunOptions{ @@ -295,6 +297,8 @@ func execPKIHC(t *testing.T, client *api.Client, ok bool) (int, string, map[stri } func validateExpectedPKIHC(t *testing.T, expected, results map[string][]map[string]interface{}) { + t.Helper() + for test, subtest := range expected { actual, ok := results[test] require.True(t, ok, fmt.Sprintf("expected top-level test %v to be present", test)) @@ -615,7 +619,7 @@ var expectedNoPerm = map[string][]map[string]interface{}{ }, "root_issued_leaves": { { - "status": "ok", + "status": "insufficient_permissions", }, }, "tidy_last_run": { @@ -625,7 +629,7 @@ var expectedNoPerm = map[string][]map[string]interface{}{ }, "too_many_certs": { { - "status": "ok", + "status": "insufficient_permissions", }, }, }