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

Add Logging for plist parse errors #54

Merged
merged 4 commits into from
Jul 27, 2022

Conversation

4e554c4c
Copy link
Contributor

This is useful to debug errors that occur because of bad plists sent by
clients. Usually in this case, one receives a not-very-useful error such
as HTTP status 400 (Bad Request): <opaque error>.

Test Plan

  1. Patch mdmb so that it sends bad checkins
diff --git a/internal/device/mdm.go b/internal/device/mdm.go
index 33a4c50..cda3d69 100644
--- a/internal/device/mdm.go
+++ b/internal/device/mdm.go
@@ -185,10 +185,7 @@ func (c *MDMClient) connect(client *http.Client, connReq interface{}) error {
                return errors.New("device not enrolled")
        }

-       plistBytes, err := plist.Marshal(connReq)
-       if err != nil {
-               return err
-       }
+       plistBytes := []byte("aoeuaoeuaoeu")
  1. Run
$ mdmb -uuids $uuid devices-connect
  1. Observe more verbose NanoMDM log:
2022/07/21 15:52:43 level=info handler=checkin-command msg=command report results err=HTTP status 400 (Bad Request): decoding command results: parse error: EOF: raw body: aoeuaoeuaoeu

@4e554c4c 4e554c4c force-pushed the unmarshal-context branch from 20a07b9 to c046ca5 Compare July 21, 2022 23:06
This is useful to debug errors that occur because of bad plists sent by
clients. Usually in this case, one receives a not-very-useful error such
as `HTTP status 400 (Bad Request): <opaque error>`.

 ## Test Plan

1. Patch `mdmb` so that it sends bad checkins
```patch
diff --git a/internal/device/mdm.go b/internal/device/mdm.go
index 33a4c50..cda3d69 100644
--- a/internal/device/mdm.go
+++ b/internal/device/mdm.go
@@ -185,10 +185,7 @@ func (c *MDMClient) connect(client *http.Client, connReq interface{}) error {
                return errors.New("device not enrolled")
        }

-       plistBytes, err := plist.Marshal(connReq)
-       if err != nil {
-               return err
-       }
+       plistBytes := []byte("aoeuaoeuaoeu")
```

2. Run
```
$ mdmb -uuids $uuid devices-connect
```

3. Observe more verbose NanoMDM log:
```
2022/07/21 15:52:43 level=info handler=checkin-command trace_id=1af087fdc6de5381 msg=command report results err=HTTP status 400 (Bad Request): decoding command results: parse error: EOF: raw body: aoeuaoeuaoeu
```
@4e554c4c 4e554c4c force-pushed the unmarshal-context branch from c046ca5 to 5dc2c16 Compare July 21, 2022 23:07
Copy link
Member

@jessepeterson jessepeterson left a comment

Choose a reason for hiding this comment

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

Nice! I'd also like to see some error checking such that the body is logged as its own individual column. Something along these lines:

diff --git a/http/mdm/mdm.go b/http/mdm/mdm.go
index 0325280..ae4f493 100644
--- a/http/mdm/mdm.go
+++ b/http/mdm/mdm.go
@@ -2,6 +2,7 @@ package mdm
 
 import (
        "errors"
+       "fmt"
        "net/http"
        "strings"
 
@@ -37,12 +38,20 @@ func CheckinHandler(svc service.Checkin, logger log.Logger) http.HandlerFunc {
                }
                respBytes, err := service.CheckinRequest(svc, mdmReqFromHTTPReq(r), bodyBytes)
                if err != nil {
-                       logger.Info("msg", "check-in request", "err", err)
+                       logs := []interface{}{"msg", "check-in request"}
                        httpStatus := http.StatusInternalServerError
                        var statusErr *service.HTTPStatusError
                        if errors.As(err, &statusErr) {
                                httpStatus = statusErr.Status
+                               err = statusErr.Err
+                               var parseErr *mdm.ParseError
+                               if errors.As(statusErr.Err, &parseErr) {
+                                       err = fmt.Errorf("parse: %w", parseErr.Err)
+                                       logs = append(logs, "body", parseErr.Body)
+                               }
                        }
+                       logs = append(logs, "http_status", httpStatus, "err", err)
+                       logger.Info(logs...)
                        http.Error(w, http.StatusText(httpStatus), httpStatus)
                }
                w.Write(respBytes)

(And same for command endpoint)

However perhaps this is future thing — it's.. complicated to do the right set of logging by clobbering multiple errors into a single log line and potentially losing some context along the way. I think the above patch would look something like this with your example log line:

2022/07/21 15:52:43 level=info handler=checkin-command msg=command report results body=aoeuaoeuaoeu http_status=400 err=parse: EOF

But in any case there's at least a couple small requested changes. :)

mdm/mdm.go Outdated Show resolved Hide resolved
mdm/mdm.go Outdated Show resolved Hide resolved
mdm/mdm.go Show resolved Hide resolved
@4e554c4c
Copy link
Contributor Author

new log line:

2022/07/25 11:26:09 level=info handler=checkin-command trace_id=222d85887f605374 msg=command report results err=HTTP status 400 (Bad Request): decoding command results: parse error: EOF: raw content: aoeuaoeuaoeu http_status=400 content=aoeuaoeuaoeu

@4e554c4c 4e554c4c force-pushed the unmarshal-context branch from 91a5252 to 0f31875 Compare July 25, 2022 18:30
@4e554c4c 4e554c4c requested a review from jessepeterson July 25, 2022 19:59
http/mdm/mdm.go Outdated Show resolved Hide resolved
@4e554c4c 4e554c4c force-pushed the unmarshal-context branch 3 times, most recently from 74635fd to 61867b1 Compare July 26, 2022 22:39
@4e554c4c 4e554c4c requested a review from jessepeterson July 26, 2022 22:40
@4e554c4c 4e554c4c force-pushed the unmarshal-context branch from 61867b1 to 6336e05 Compare July 26, 2022 22:43
http/mdm/mdm.go Outdated Show resolved Hide resolved
http/mdm/mdm.go Outdated Show resolved Hide resolved
@4e554c4c 4e554c4c requested a review from jessepeterson July 26, 2022 22:55
Copy link
Member

@jessepeterson jessepeterson left a comment

Choose a reason for hiding this comment

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

lgtm! thanks for the changes. :)

@jessepeterson jessepeterson merged commit d0bcd3c into micromdm:main Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants