Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: include crypto diagnostics in /debug/vars output #23948

Merged
merged 5 commits into from
Dec 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
86 changes: 85 additions & 1 deletion services/httpd/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ import (
"go.uber.org/zap"
)

var ErrDiagnosticsValueMissing = errors.New("expected diagnostic value missing")

const (
// DefaultChunkSize specifies the maximum number of points that will
// be read before sending results back to the engine.
Expand Down Expand Up @@ -872,7 +874,6 @@ func (h *Handler) async(q *influxql.Query, results <-chan *query.Result) {
// in the database URL query value. It is encoded using a forward slash like
// "database/retentionpolicy" and we should be able to simply split that string
// on the forward slash.
//
func bucket2dbrp(bucket string) (string, string, error) {
// test for a slash in our bucket name.
switch idx := strings.IndexByte(bucket, '/'); idx {
Expand Down Expand Up @@ -2250,6 +2251,40 @@ func (h *Handler) serveExpvar(w http.ResponseWriter, r *http.Request) {
first = false
fmt.Fprintf(w, "\"cmdline\": %s", val)
}

// We're going to print some kind of crypto data, we just
// need to find the proper source for it.
{
var jv map[string]interface{}
val := diags["crypto"]
if val != nil {
jv, err = parseCryptoDiagnostics(val)
if err != nil {
if errors.Is(err, ErrDiagnosticsValueMissing) {
// log missing values, but don't error out
h.Logger.Warn(err.Error())
} else {
h.httpError(w, err.Error(), http.StatusInternalServerError)
return
}
}
} else {
jv = ossCryptoDiagnostics()
}

data, err := json.Marshal(jv)
if err != nil {
h.httpError(w, err.Error(), http.StatusInternalServerError)
return
}

if !first {
fmt.Fprintln(w, ",")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still in favor of an h.Logger.Error(err) if Fprintln fails here.

}
first = false
fmt.Fprintf(w, "\"crypto\": %s", data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, let's log Fprintf errors, as unlikely as they may be.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go a step further and say the fact there are fmt.Printf calls to check when generating JSON is problematic. I don't understand why we don't build the sections into a map[string]interface{} and then serialize that. There's a lot more fmt.Printf's in there than just the ones I added.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely agree. But I didn't want to generate more work for you rewriting everything.

}

if val := expvar.Get("memstats"); val != nil {
if !first {
fmt.Fprintln(w, ",")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, let's log Fprintln errors.

Expand Down Expand Up @@ -2434,6 +2469,55 @@ func parseBuildInfo(d *diagnostics.Diagnostics) (map[string]interface{}, error)
return m, nil
}

// ossCryptoDiagnostics creates a default crypto diagnostics map that
// can be marshaled into JSON for /debug/vars.
func ossCryptoDiagnostics() map[string]interface{} {
return map[string]interface{}{
"ensureFIPS": false,
"FIPS": false,
"implementation": "Go",
"passwordHash": "bcrypt",
}
}

// parseCryptoDiagnostics converts the crypto diagnostics into an appropriate
// format for marshaling to JSON in the /debug/vars format.
func parseCryptoDiagnostics(d *diagnostics.Diagnostics) (map[string]interface{}, error) {
// We use ossCryptoDiagnostics as a template for columns we need to pull from d.
// If the column is missing from d, we will nil out the value in m to avoid lying
// about a value to the user and making troubleshooting harder.
m := ossCryptoDiagnostics()
var missing []string

for key := range m {
// Find the associated column.
ci := -1
for i, col := range d.Columns {
if col == key {
ci = i
break
}
}

// Don't error out if we can't find the column or cell for a given key, just nil
// out the value in m. There could still be other useful information we gather.
// Column not found or data cell not found
if ci == -1 || len(d.Rows) < 1 || len(d.Rows[0]) <= ci {
m[key] = nil
missing = append(missing, key)
continue
}

m[key] = d.Rows[0][ci]
}

if len(missing) > 0 {
// If you're getting this error, you probably need to update enterprise.
return m, fmt.Errorf("parseCryptoDiagnostics: missing %s: %w", strings.Join(missing, ","), ErrDiagnosticsValueMissing)
}
return m, nil
}

// httpError writes an error to the client in a standard format.
func (h *Handler) httpError(w http.ResponseWriter, errmsg string, code int) {
if code == http.StatusUnauthorized {
Expand Down
45 changes: 44 additions & 1 deletion services/httpd/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2616,6 +2616,12 @@ func TestHandlerDebugVars(t *testing.T) {
return res
}

newDiagFn := func(d map[string]*diagnostics.Diagnostics) func() (map[string]*diagnostics.Diagnostics, error) {
return func() (map[string]*diagnostics.Diagnostics, error) {
return d, nil
}
}

var Ignored = []string{"memstats", "cmdline"}
read := func(t *testing.T, b *bytes.Buffer, del ...string) map[string]interface{} {
t.Helper()
Expand Down Expand Up @@ -2652,17 +2658,19 @@ func TestHandlerDebugVars(t *testing.T) {
stat("shard", tags("path", "/mnt/foo", "id", "111"), nil),
)
}
h.Monitor.DiagnosticsFn = newDiagFn(map[string]*diagnostics.Diagnostics{})
req := MustNewRequest("GET", "/debug/vars", nil)
w := httptest.NewRecorder()
h.ServeHTTP(w, req)
got := keys(read(t, w.Body, Ignored...))
exp := []string{"database:foo", "hh:/mnt/foo/bar", "httpd:https:127.0.0.1:8088", "other", "shard:/mnt/foo:111"}
exp := []string{"crypto", "database:foo", "hh:/mnt/foo/bar", "httpd:https:127.0.0.1:8088", "other", "shard:/mnt/foo:111"}
if !cmp.Equal(got, exp) {
t.Errorf("unexpected keys; -got/+exp\n%s", cmp.Diff(got, exp))
}
})

t.Run("generates numbered keys for collisions", func(t *testing.T) {
// This also implicitly tests the case where no `crypto` diagnostics are not set by application.
h := NewHandler(false)
h.Monitor.StatisticsFn = func(_ map[string]string) ([]*monitor.Statistic, error) {
return stats(
Expand All @@ -2677,6 +2685,12 @@ func TestHandlerDebugVars(t *testing.T) {
h.ServeHTTP(w, req)
got := read(t, w.Body, Ignored...)
exp := map[string]interface{}{
"crypto": map[string]interface{}{
"FIPS": false,
"ensureFIPS": false,
"passwordHash": "bcrypt",
"implementation": "Go",
},
"hh_processor": map[string]interface{}{
"name": "hh_processor",
"tags": map[string]interface{}{"db": "foo", "shardID": "10"},
Expand All @@ -2703,6 +2717,35 @@ func TestHandlerDebugVars(t *testing.T) {
}
})
})

t.Run("checks crypto diagnostic handling", func(t *testing.T) {
h := NewHandler(false)
// intentionally leave out "ensureFIPS" to test that code path
h.Monitor.DiagnosticsFn = newDiagFn(
map[string]*diagnostics.Diagnostics{
"crypto": diagnostics.RowFromMap(map[string]interface{}{
"FIPS": true,
"passwordHash": "pbkdf2-sha256",
"implementation": "BoringCrypto",
}),
})
req := MustNewRequest("GET", "/debug/vars", nil)
w := httptest.NewRecorder()
h.ServeHTTP(w, req)
got := read(t, w.Body, Ignored...)
exp := map[string]interface{}{
"crypto": map[string]interface{}{
"FIPS": true,
"ensureFIPS": nil,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to omit this entirely, rather than having a nil entry?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nil is intentional. Here's my reasoning:

Assumptions:

  • Seeing nil values means enterprise was not updated to generate the appropriate diagnostic values.
  • You should not see nil in release versions, because enterprise and OSS agree on the appropriate attributes.
  • Incomplete data is better than no data, as long as there's no misleading data.

Option 1: If enterprise doesn't provide an attribute value, GET /debug/vars will return a 500. This is what other sections like build do.
I don't like this option because OSS regression tests will pass, but enterprise currently has no tests for /debug/vars. Hopefully we'd catch this, but maybe not. We might have a situation where a customer can't pull /debug/vars, even though it has lots of other useful info.

Option 2: Leave the field out if enterprise doesn't set it.
Let's say in the future we add PAM support, and add PAMEnabled and PAMModules to OSS, but enterprise hasn't been updated yet. If we see /debug/vars without PAMEnabled and PAMModules, is that because the version is too old to have those fields, or does enterprise have a bug and isn't passing them through. If we see those fields as nil, we know something is wrong. This also applies to automated tests. If we forget to add fields to enterprise and hide them, our automated tests still pass so we don't realize we need to add something to enterprise.

Option 3: Missing fields are given a nil value.
Looks wrong because it is wrong, so a human will flag it when they see it. Once we add automated tests to enterprise, these tests will fail when suddenly there's a new field that enterprise doesn't set. Either way, hopefully we really would catch it during development time instead of RC or even release time.

"passwordHash": "pbkdf2-sha256",
"implementation": "BoringCrypto",
},
}
if !cmp.Equal(got, exp) {
t.Errorf("unexpected keys; -got/+exp\n%s", cmp.Diff(got, exp))
}
})

}

// NewHandler represents a test wrapper for httpd.Handler.
Expand Down