Conversation
Fetching the token from the db and sending it back to the client when the client asks for it ie sends the GetBootstrapTokenRequest is not implemented Also earlier code for Bootstrap Token from commit 58c3782 "mdm: add SetBootstrapToken command." is removed
…ootstrap token back to the client, when client requests it
checkin.pb.go: golang/[email protected] mdm.pb.go: gogo/[email protected] device.pb.go: golang/[email protected]
jessepeterson
left a comment
jessepeterson
left a comment
There was a problem hiding this comment.
This looks good. Much thanks to you and @ivanhata!
each file needed a different protoc compiler version to match existing code
Yikes, how annoying. Thanks for slogging through! Mentioned in #760 I really want to unify all that with #773; but it'll take time & testing.
Added a small test for GetBootstrapToken
❤️
Note: the []byte returned from MDMService.Checkin isn't sent to pubsub/webhook. Not sure if that's something that would be wanted, but that's probably for another PR. I'm not sure it could be done without breaking compatibility somewhere.
Yeah the issue here is, conceptually, the webhook publishes the Checkin request messages of which GetBootstrapToken is just that — a request. We don't really publish Checkin responses — which actually were only added recently. This is true of any Checkin, or Acknowledge (command) requests, too. We publish the request, but not the MDM server response. There could be another PubSub topic, I suppose, for GetBootstrapToken responses, but as you said: probably another discussion from another PR (if it's even something that's worth doing). It works the same way in NanoMDM fwiw, too.
except for his note on platform/device/worker.go: Worker.updateFromGetBootstrapToken. From my understanding, you're saying to move the logic from the worker to mdm/checkin.go: MDMService.Checkin. I guess where I'm confused, is why you wouldn't do the same thing for updateFromCheckout, updateFromTokenUpdate, etc. It seems like pretty much everything in the work is just accessing the db and pubsub.
Thanks, yeah. This was a misconception from the original suggestion to include the 'full' device database in the Checkin service when, in fact, we only have a BootstrapTokenRetriever interface. If it were the full device database then we wouldn't need to publish to the PubSub because we'd have the device right there (which we don't, of course). So, that's my bad. And as you note the code in the PR is actually consistent with the other checkin requests, too. So: ignore me, carry on. :)
Again, thanks for this! I'll have another look-see hopefully tomorrow and we can get this in!
|
This is great! Thanks to @korylprince for fixing all the issues and getting this done! I'm totally fine with it if this PR gets merged. I guess the #760 can be just closed. |
Co-authored-by: Jesse Peterson <[email protected]>
jessepeterson
left a comment
jessepeterson
left a comment
There was a problem hiding this comment.
Just a couple really minor things, but overall looking good!
mdm/checkin_event.go
Outdated
| NotOnConsole bool `plist:",omitempty"` | ||
| } | ||
|
|
||
| // Get Bootstrap Token Message Type # GetBootstrapTokenRequest |
There was a problem hiding this comment.
There was a problem hiding this comment.
I updated these to match existing code. For example auth struct has the comment // Authenticate Message Type, not auth Message Type. These are private structs so they're not going to show up in godoc anyways. If you want it to actually be //getBootstrap ... I can do that too.
mdm/checkin_event.go
Outdated
| GetAwaitingConfiguration bool | ||
| } | ||
|
|
||
| // Set Bootstrap Token Message Type # SetBootstrapTokenRequest |
There was a problem hiding this comment.
| DEPProfileAssignedDate time.Time `db:"dep_profile_assigned_date"` | ||
| DEPProfileAssignedBy string `db:"dep_profile_assigned_by"` | ||
| LastSeen time.Time `db:"last_seen"` | ||
| BootstrapToken string `db:"bootstrap_token"` |
There was a problem hiding this comment.
Any reason this, too, shouldn't be a byte slice? I see a few string() and []byte() type conversions and I wonder if they're necessary.
There was a problem hiding this comment.
I mainly left these as is because it matches existing code (namely, Token and Unlock Token are also stored as strings). I agree it would make more sense to store it as a byte (same in the protobuf), but figured consistency with the rest of the codebase was better. []byte-string conversions cost very little I would think. If you want to move it to a []byte, I can make those changes (proto file, generated code, services, etc). Just let me know.
3 participants
I hope I'm not stepping on @ivanhata's toes (:heart:). I've taken his existing work in #760 and made the following changes to hopefully get this PR merged:
GetBootstrapToken[]bytereturned fromMDMService.Checkinisn't sent to pubsub/webhook. Not sure if that's something that would be wanted, but that's probably for another PR. I'm not sure it could be done without breaking compatibility somewhere.I've done some testing with a few different MacBooks:
sudo profiles install -type bootstraptokenthensudo profiles validate -type bootstraptokento verify proper escrowsudo profiles status -type bootstraptokenwill lie to you and tell you the server has the token just because it sent it. You need to usevalidateto verify the MacBook can actually get the token from the server and use it properly.GetBootstrapTokenrequest is sent each time a user without a SecureToken logs inAs a final note, this PR addresses all of @jessepeterson's reviews on #740 except for his note on
platform/device/worker.go: Worker.updateFromGetBootstrapToken. From my understanding, you're saying to move the logic from the worker tomdm/checkin.go: MDMService.Checkin. I guess where I'm confused, is why you wouldn't do the same thing forupdateFromCheckout,updateFromTokenUpdate, etc. It seems like pretty much everything in the work is just accessing the db and pubsub.In any case, I can make any changes to this PR to get it merged, so just let me know!