Bootstrap Token by ivanhata · Pull Request #760 · micromdm/micromdm

ivanhata · 2021-06-14T20:41:25Z

Rebased to current main and corrected the branch, otherwise this is identical with closed #755

This is a partial implementation of Bootstrap Token as discussed in #735
I had a short chat with @jessepeterson. He recommended doing a PR to get this worked on further.

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. Everything else should work.

Also earlier code for Bootstrap Token from commit 58c3782 "mdm: add SetBootstrapToken command." is removed

I have spent some time with the code trying to understand and figure out a way to implement what is missing. I'm quote stucked now. If someone wants to continue from this and implement what is needed, please do so, I'll be just happy to get the feature 🙂. Anyhow, I'm ready to continue, but I'd need help. Any tips or advice is appreciated.

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

jessepeterson

This all looks really good!

The primary thing I think we need to do is to couple a new "BootstrapTokenGetter" (or similar) interface to the MDMService which knows how to retrieve the Bootstrap token. We can't really do this over the pub/sub interface because the request-response model here doesn't really fit the pub/sub paradigm. Basically we'll make the device service implement this new "BootstrapTokenGetter" interface and when we init the MDM interface we'll provide it this service (which is actually just the device interface).

I hope that makes sense and let me know of any questions!

jessepeterson · 2021-06-16T05:40:12Z

mdm/internal/checkinproto/checkin.pb.go

@@ -1,11 +1,15 @@
-// Code generated by protoc-gen-go. DO NOT EDIT.
+// Code generated by protoc-gen-gogo. DO NOT EDIT.


This doesn't seem right. Seems like protoc-gen-gogo generated this file but this file should be generated with normal protoc-gen-go. How did you create this file? go generate should be all that's needed once the required protoc and protoc-gen-go is installed.

I had some trouble to run just go generate . I don't recall what I did exactly.

I tried to do everything by the book in the later commit. Still couldn't get the go generate to exit without errors. After some googling found a hint to add extra option to the proto file. After doing that, go generate exited ok.

I noticed there was some discussion in #757 about same kind of difficulties.

mdm/mdm/command.go

mdm/service.go

jessepeterson · 2021-06-16T05:53:20Z

platform/device/internal/deviceproto/device.pb.go

@@ -1,21 +1,15 @@
-// Code generated by protoc-gen-go.
+// Code generated by protoc-gen-gogo. DO NOT EDIT.


Here, too, I think this needs to be generated with protoc-gen-go instead of -gogo. I'm surprised you got this if you just ran go generate.

jessepeterson · 2021-06-16T05:55:27Z

platform/device/worker.go


 }

+func (w *Worker) updateFromGetBootstrapToken(ctx context.Context, message []byte) error {


So I'm not sure there's any need to listen for a get request. Because we need to actively return data there's nothing really to "listen" for here. Indeed it only looks like we update the LastSeen and/or Await state here. I think this can go in lieu of coupling the device service to the check-in service, mentioned elsewhere.

I forgot to do anything for this one. I need to check the code again if I figure out how to change it. And if you think that this is still a good idea.

jessepeterson · 2021-06-18T00:52:14Z

I put together #764 which will probably help for the bootstrap token retrieval.

jessepeterson · 2021-07-01T23:14:40Z

hey @ivanhata i've merged the other PR. this sets you up to do the work mentioned in the review after a rebase. no rush though, just letting you know. :)

jessepeterson · 2021-08-26T03:08:06Z

just a ping on this :) @ivanhata

…ootstrap token back to the client, when client requests it

ivanhata · 2021-09-14T12:48:43Z

Sorry for the horrible delivery time!

These commits should bring the earlier partial implementation to fully working feature. I have done only some testing, like

watching both micromdm and mdmclients logs in debug mode
toggling the state of bootstrap token with profiles command
evaluating the output of sysadminctl, diskutil and fdesetup for bootstrap and secure token

Let me know if further testing is needed in general or something needs to be tested more thoroughly.

Huge thank you to @jessepeterson for his patience and advice.

jessepeterson

this is great progress! in addition to the code changes I think you'll need to: rebase the PR on main as well as confirm working & testing (though it sounds like @korylprince might be willing to help test).

I suspect the biggest pain is going to be not changing the protobuf files/definitions (we want to consolidate that into #773). this implies running some older protoc and protobuf compiler versions.

jessepeterson · 2021-10-01T03:37:48Z

mdm/checkin.go

+	return base64.StdEncoding.EncodeToString(b)
+}
+
+type BootstrapToken struct {


Needs a comment for exported (uppercased) symbol.

jessepeterson · 2021-10-01T03:41:12Z

mdm/checkin.go

+		var bt BootstrapToken
+		bt = BootstrapToken{b64Data(btBytes)}


Suggested change

var bt BootstrapToken

bt = BootstrapToken{b64Data(btBytes)}

bt := &BootstrapToken{b64Data(btBytes)}

jessepeterson · 2021-10-01T03:43:24Z

mdm/checkin.go

+			return nil, errors.Wrap(err, "marshal bootstrap token")
+		}
+
+		return resp, errors.Wrap(err, "getting bootstrap token")


i think we've already handled the errors, above, so shouldn't need to wrap this one.

Suggested change

return resp, errors.Wrap(err, "getting bootstrap token")

return resp, nil

jessepeterson · 2021-10-01T03:45:25Z

mdm/internal/checkinproto/checkin.proto

 syntax = "proto3";

 package checkinproto;
+option go_package = "/;checkinproto";


we should try not to modify our protobuf file too much in this diff - I'm hoping to refactor them in #773 but until then we should try and hobble along with the older gogo (to keep the changes minimal) :(

jessepeterson · 2021-10-01T03:46:08Z

mdm/internal/checkinproto/checkin.proto

+}
+
+message SetBootstrapToken {
+	bytes  bootstrap_token = 1;


spaces not tabs?

Suggested change

bytes bootstrap_token = 1;

bytes bootstrap_token = 1;

jessepeterson · 2021-10-01T03:56:26Z

platform/device/internal/deviceproto/device.proto


 package deviceproto;
-
+option go_package = "/;deviceproto";


same, see other comment.

jessepeterson · 2021-10-01T03:57:30Z

go.mod

 	golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9
 	golang.org/x/net v0.0.0-20201021035429-f5854403a974
 	google.golang.org/appengine v1.2.0 // indirect
+	google.golang.org/protobuf v1.23.0


see comment below about protobuf

jessepeterson · 2021-10-01T04:00:34Z

platform/device/builtin/db.go

+
+	opts.FilterUDID = udids
+
+	devices, err := db.List(ctx, opts)


We should avoid db.List here. It enumerates every device and only filters after the fact. We should instead use db.DeviceByUDID() which then obviates the need for all the UDID stuff.

jessepeterson · 2021-10-01T04:01:31Z

platform/device/builtin/db.go

+
+	devices, err := db.List(ctx, opts)
+	for _, d := range devices {
+		btstring, _ := hex.DecodeString(string(d.BootstrapToken))


Any reason to store/encode/decode as hex? We can store the raw binary in the device record.

jessepeterson · 2021-10-01T04:02:09Z

platform/device/builtin/db.go

 	return datastore, nil
 }

+func (db *DB) GetBootstrapToken(ctx context.Context, udid string) ([]byte, error) {


nit: maybe return b64Data from the other definition?

Suggested change

func (db *DB) GetBootstrapToken(ctx context.Context, udid string) ([]byte, error) {

func (db *DB) GetBootstrapToken(ctx context.Context, udid string) (B64Data, error) {

ivanhata · 2021-10-05T08:22:54Z

I guess we can just close this PR as everything is solved in #781 now.

jessepeterson self-assigned this Jun 16, 2021

jessepeterson requested changes Jun 16, 2021

View reviewed changes

jessepeterson mentioned this pull request Aug 30, 2021

Cutting a release? micromdm/nanomdm#31

Closed

ivanhata added 3 commits September 14, 2021 14:01

merge upstream/main

09eae92

implements rest of the bootstrap token feature - micromdm now sends b…

a4670af

…ootstrap token back to the client, when client requests it

regenerate some proto files

62e4d38

jessepeterson requested changes Oct 1, 2021

View reviewed changes

korylprince mentioned this pull request Oct 5, 2021

Bootstrap Token #781

Merged

jessepeterson closed this Oct 5, 2021

		@@ -1,11 +1,15 @@
		// Code generated by protoc-gen-go. DO NOT EDIT.
		// Code generated by protoc-gen-gogo. DO NOT EDIT.

		@@ -1,21 +1,15 @@
		// Code generated by protoc-gen-go.
		// Code generated by protoc-gen-gogo. DO NOT EDIT.


		}

		func (w *Worker) updateFromGetBootstrapToken(ctx context.Context, message []byte) error {

	var bt BootstrapToken
	bt = BootstrapToken{b64Data(btBytes)}
	bt := &BootstrapToken{b64Data(btBytes)}

	return resp, errors.Wrap(err, "getting bootstrap token")
	return resp, nil

	func (db *DB) GetBootstrapToken(ctx context.Context, udid string) ([]byte, error) {
	func (db *DB) GetBootstrapToken(ctx context.Context, udid string) (B64Data, error) {

Conversation

ivanhata commented Jun 14, 2021

Uh oh!

jessepeterson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessepeterson commented Jun 18, 2021

Uh oh!

jessepeterson commented Jul 1, 2021

Uh oh!

jessepeterson commented Aug 26, 2021

Uh oh!

ivanhata commented Sep 14, 2021

Uh oh!

jessepeterson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanhata commented Oct 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants