Fleetd bitlocker management by marcosd4h · Pull Request #14022 · fleetdm/fleet

marcosd4h · 2023-09-20T15:45:32Z

Checklist for submitter

This relates to #12842

If some of the following don't apply, delete the relevant line.

Changes file added for user-visible changes in changes/ or orbit/changes/.
See Changes files for more information.
[ ] Added/updated tests
Manual QA for all new/changed functionality

…anch

codecov · 2023-09-20T16:04:24Z

Codecov Report

Patch coverage has no change and project coverage change: -0.10% ⚠️

Comparison is base (32ec1e1) 58.82% compared to head (935991a) 58.73%.
Report is 3 commits behind head on feat-bitlocker.

❗ Current head 935991a differs from pull request most recent head ed06454. Consider uploading reports for the commit ed06454 to get more accurate results

Additional details and impacted files

@@                Coverage Diff                 @@
##           feat-bitlocker   #14022      +/-   ##
==================================================
- Coverage           58.82%   58.73%   -0.10%     
==================================================
  Files                 903      903              
  Lines               74510    74565      +55     
  Branches             2100     2100              
==================================================
- Hits                43831    43793      -38     
- Misses              27188    27282      +94     
+ Partials             3491     3490       -1

Flag	Coverage Δ
backend	`59.27% <0.00%> (-0.11%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
orbit/pkg/update/execwinapi_stub.go	`0.00% <0.00%> (ø)`
orbit/pkg/update/notifications.go	`58.43% <0.00%> (-12.53%)`	⬇️
server/service/orbit_client.go	`0.00% <0.00%> (ø)`

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

roperzh · 2023-09-20T19:26:46Z

+
+	err = w.EncryptionResult.SetOrUpdateDiskEncryptionKey(payload)
+	if err != nil {
+		log.Error().Err(err).Msgf("failed to get send encryption result to Fleet Server - Error %v", err)


Suggested change

log.Error().Err(err).Msgf("failed to get send encryption result to Fleet Server - Error %v", err)

log.Error().Err(err).Msg("failed to get send encryption result to Fleet Server")

roperzh · 2023-09-20T19:27:48Z

+		return
+	}
+
+	// Performing Bitlocker encryption operation against C: volum


old comment?

roperzh · 2023-09-20T19:31:24Z

+	payload := fleet.OrbitHostDiskEncryptionKeyPayload{
+		EncryptionKey: []byte(recoveryKey),
+		ClientError:   err.Error(),
+	}
+


two notes:

if err != nil we're early returning, so we will never report the error

if err == nil, isn't this going to panic? I think we probably need to add the error to the payload inside the if that's right above

Those are great points. I've fixed them on the code

#2 would have crashed during the E2E testing! I missed this scenario on the mocked-up version of bitlocker.EncryptVolume().

mna · 2023-09-20T19:41:16Z

+	// executions of the windows MDM enrollment attempt.
+	Frequency time.Duration
+	// HostUUID is the current host's UUID.
+	HostUUID string


Looks like the host uuid is not needed after all?

yes, not needed. Just removed it from the type

mna · 2023-09-20T19:50:32Z

+	verb, path := "POST", "/api/fleet/orbit/disk_encryption_key"
+
+	var resp orbitPostDiskEncryptionKeyResponse
+	if err := oc.authenticatedRequest(verb, path, &diskEncryptionStatus, &resp); err != nil {


This won't work because diskEncryptionStatus var is not a struct type that can receive the orbit node key. You need to transfer your fleet.OrbitHostDiskEncryptionKeyPayload struct to the proper request struct for that endpoint:

Suggested change

if err := oc.authenticatedRequest(verb, path, &diskEncryptionStatus, &resp); err != nil {

if err := oc.authenticatedRequest(verb, path, &orbitPostDiskEncryptionKeyRequest{

EncryptionKey: diskEncryptionStatus.EncryptionKey,

ClientError: diskEncryptionStatus.ClientError,

}, &resp); err != nil {

Thanks! I'm working on getting test coverage for this.

mna · 2023-09-20T19:51:34Z

 	return constant.OrbitEnrollRetrySleep
 }
+
+type HostScriptResultPayload struct {


I think this was added by mistake? It's a real struct, but it already exists in the hosts.go file, should probably not be here.

It was added by mistake, it is fixed now

mna · 2023-09-20T19:52:16Z

@@ -0,0 +1,574 @@
+//go:build windows
+// +build windows


The old +build comment is not needed anymore.

I forgot to remove this, sorry. It is now fixed

mna · 2023-09-20T19:57:25Z

+	resultRaw, err := oleutil.CallMethod(v.handle, "Encrypt", int32(method), int32(flags))
+	if err != nil {
+		return fmt.Errorf("encrypt(%s): %w", v.letter, err)
+	} else if val, ok := resultRaw.Value().(int32); val != 0 || !ok {


Just to double-check, I presume you'll know soon enough when testing on a Win host if this is ok, but the microsoft docs mention that it returns an uint32.

mna · 2023-09-20T20:00:59Z

+		return fmt.Errorf("protectWithPassphrase(%s): %w", v.letter, encryptErrHandler(val))
+	}
+
+	volumeKeyProtectorID.ToString()


This seems like something that needed to be removed or stored somewhere?

Thanks, I've just fixed this. This function is not currently used. I've left the logic implemented for future usecase (fixed drives encryption)

mna · 2023-09-20T20:02:58Z

+	var conversionStatus int32 = 0
+	var encryptionPercentage int32 = 0
+	var encryptionFlags int32 = 0
+	var wipingStatus int32 = 0
+	var wipingPercentage int32 = 0
+	var precisionFactor int32 = 4
+	var protectionStatus int32 = 0


Nit, but 0 will be automatically set, and you can group in a var ( ) block:

Suggested change

var conversionStatus int32 = 0

var encryptionPercentage int32 = 0

var encryptionFlags int32 = 0

var wipingStatus int32 = 0

var wipingPercentage int32 = 0

var precisionFactor int32 = 4

var protectionStatus int32 = 0

var (

conversionStatus int32

encryptionPercentage int32

encryptionFlags int32

wipingStatus int32

wipingPercentage int32

precisionFactor int32 = 4

protectionStatus int32

)

Thanks for taking the time to explain how Go manages this! This is useful for me to keep learning the language.

mna · 2023-09-20T20:58:07Z

@marcosd4h Just a heads-up, as I likely won't be around for the final review, don't block waiting for me - feel free to go ahead and merge when you have @roperzh 's approval!

marcosd4h · 2023-09-20T21:03:33Z

@marcosd4h Just a heads-up, as I likely won't be around for the final review, don't block waiting for me - feel free to go ahead and merge when you have @roperzh 's approval!

Thanks for the heads up! I currently had the merge on hold because I'm seeing a weird build error on the Github actions

The GA build fails with the following error about a var being declared, but that code does not exist on the code in the branch see here. I think I'm going to merge and fix it on the feature branch if this is still happening

Addressing code review findings and picking up latest from feature br…

935991a

…anch

marcosd4h temporarily deployed to Docker Hub September 20, 2023 15:45 — with GitHub Actions Inactive

marcosd4h marked this pull request as ready for review September 20, 2023 15:53

marcosd4h requested a review from a team as a code owner September 20, 2023 15:53

roperzh reviewed Sep 20, 2023

View reviewed changes

Addressing code review findings

b2244de

marcosd4h had a problem deploying to Docker Hub September 20, 2023 20:00 — with GitHub Actions Failure

mna reviewed Sep 20, 2023

View reviewed changes

Addressing code review findings

e97f0e4

marcosd4h had a problem deploying to Docker Hub September 20, 2023 20:42 — with GitHub Actions Error

Addressing code review findings

ed06454

marcosd4h had a problem deploying to Docker Hub September 20, 2023 20:44 — with GitHub Actions Failure

marcosd4h merged commit dc8a086 into feat-bitlocker Sep 20, 2023

marcosd4h deleted the 12842-fleetd-bitlocker-management-feat-branch branch September 20, 2023 21:11

	log.Error().Err(err).Msgf("failed to get send encryption result to Fleet Server - Error %v", err)
	log.Error().Err(err).Msg("failed to get send encryption result to Fleet Server")

-	if err := oc.authenticatedRequest(verb, path, &diskEncryptionStatus, &resp); err != nil {
+	if err := oc.authenticatedRequest(verb, path, &orbitPostDiskEncryptionKeyRequest{
+	        EncryptionKey: diskEncryptionStatus.EncryptionKey,
+	        ClientError: diskEncryptionStatus.ClientError,
+	}, &resp); err != nil {

Uh oh!

Conversation

marcosd4h commented Sep 20, 2023

Checklist for submitter

Uh oh!

codecov Bot commented Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

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!

mna commented Sep 20, 2023

Uh oh!

marcosd4h commented Sep 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Sep 20, 2023 •

edited

Loading