-
Notifications
You must be signed in to change notification settings - Fork 430
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
Linux disk encryption: frontend changes, backend error on update DE setting with missing private key #23714
base: 22074-linux-encryption
Are you sure you want to change the base?
Conversation
* Add Linux column to Disk encryption table * Add "linux" to type for expected platforms with disk encryption * Widen capabilities of `SectionHeader` to include: * Vertically aligned title/subtitle * Optional grey subtitle text coloring
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 22074-linux-encryption #23714 +/- ##
==========================================================
- Coverage 63.14% 63.13% -0.01%
==========================================================
Files 1557 1558 +1
Lines 147617 147653 +36
Branches 3687 3742 +55
==========================================================
+ Hits 93212 93227 +15
- Misses 47033 47053 +20
- Partials 7372 7373 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
- Allows rendering of Disk Encryption section even when MDM is not enabled
@@ -1523,6 +1523,9 @@ func unmarshalWithGlobalDefaults(b *json.RawMessage) (fleet.Features, error) { | |||
func (svc *Service) updateTeamMDMDiskEncryption(ctx context.Context, tm *fleet.Team, enable *bool) error { | |||
var didUpdate, didUpdateMacOSDiskEncryption bool | |||
if enable != nil { | |||
if len(svc.config.Server.PrivateKey) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more idiomatic to use == ""
for string evaluation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After consulting with MDM, let's use == ""
going forward, and update existing len
-based checks as we come to them, no need to update now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked through this PR to get my bearings about this feature. Some of the questions have probably been answered elsewhere; thanks in advance for answering these for the Nth time.
setup. | ||
</li> | ||
<li> | ||
Close this window and select <b>Refetch</b> on your <b>My device</b>{" "} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mostlikelee Can we force a refetch on Orbit when the passphrase is entered?
@@ -353,14 +374,15 @@ const DeviceUserPage = ({ | |||
mdmEnabledAndConfigured={ | |||
!!globalConfig?.mdm.enabled_and_configured | |||
} | |||
mdmConnectedToFleet={!!host.mdm.connected_to_fleet} | |||
diskEncryptionStatus={ | |||
connectedToFleetMdm={!!host.mdm.connected_to_fleet} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to verify that this doesn't show up for Linux hosts as there isn't a concept of Linux MDM the same way there is for Mac/Win.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be testable now with a Linux host as nothing we're doing on the backend changes the fact that there's no Linux MDM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prop is only used in conjunction with (a) macDiskEncryptionStatus === "action_required"
to determine whether to (b) showMacDiskEncryptionKeyResetRequired
– since (a) should only be possible for darwin
hosts, there shouldn't be any issue here.
Here's a clarifying rename that should make those relationships more clear.
Does that make sense or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will, as noted, test all of this today
}, | ||
}); | ||
} | ||
return sendRequest("PATCH", teamsEndpoint, { | ||
return sendRequest("POST", teamsEndpoint, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating this call from using a deprecated endpoint to using the latest – see here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually looks like we can unify logic for which endpoint to hit here, as the new endpoint supports a nil team ID for editing no-team config, vs. needing to split between a team-specific endpoint and a no-team-specific endpoint. Does that match what you're seeing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, nice!
// TODO: API INTEGRATION: remove macos_settings when API change is merged in. | ||
macos_settings: { enable_disk_encryption: enableDiskEncryption }, | ||
// enable_disk_encryption: enableDiskEncryption, | ||
enable_disk_encryption: enableDiskEncryption, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supported on the backend, or does this tweak need to be built so we don't lose support for macOS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above should answer this question as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(yes, it's supported 🙂)
@@ -706,8 +706,8 @@ func attachFleetAPIRoutes(r *mux.Router, svc fleet.Service, config config.FleetC | |||
|
|||
// Deprecated: GET /mdm/profiles/summary is now deprecated, replaced by the | |||
// GET /configuration_profiles/summary endpoint. | |||
mdmAnyMW.GET("/api/_version_/fleet/mdm/profiles/summary", getMDMProfilesSummaryEndpoint, getMDMProfilesSummaryRequest{}) | |||
mdmAnyMW.GET("/api/_version_/fleet/configuration_profiles/summary", getMDMProfilesSummaryEndpoint, getMDMProfilesSummaryRequest{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL 👍
Addresses #22702, #23713, #23756, and #23747
-Note that much of this code as is will render as expected only once integrated with the backend or if manipulated manually for testing purposes
Frontend:
Create key
CreateLinuxKeyModal
to inform user of next steps after clickingCreate key
SectionHeader
component, apply to new UI-Other TODO:
Backend:
For "No team" and teams, error when trying to update disk encryption enabled while no server private key is present.
Changes file added for user-visible changes in
changes/
Added/updated tests
Manual QA for all new/changed functionality