-
Notifications
You must be signed in to change notification settings - Fork 432
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 Key Escrow - Agent #23771
base: 23585-zenity
Are you sure you want to change the base?
Linux Key Escrow - Agent #23771
Conversation
server/service/orbit_client.go
Outdated
@@ -666,3 +666,8 @@ func (oc *OrbitClient) GetSetupExperienceStatus() (*fleet.SetupExperienceStatusP | |||
|
|||
return resp.Results, nil | |||
} | |||
|
|||
func (oc *OrbitClient) EscrowLinuxKey(key []byte) error { |
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.
stub for testing
randPassphraseLength = 32 | ||
) | ||
|
||
type KeyEscrower interface { |
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.
@iansltx the implementation is currently stubbed
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 Do we need to push a client error here sometimes?
85e5465
to
37d03e3
Compare
44fbc9d
to
eee937b
Compare
infoFailedText = "Failed to escrow key. Please try again later." | ||
infoSuccessTitle = "Encryption key escrow" | ||
infoSuccessText = "Key escrowed successfully." | ||
maxKeySlots = 8 |
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 is somewhat of a guess at an upper bounds, but I expect slot 1 to be available 90% of the time.
orbit/pkg/luks/luks_linux.go
Outdated
"github.com/siderolabs/go-blockdevice/v2/encryption" | ||
"github.com/siderolabs/go-blockdevice/v2/encryption/luks" |
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.
a divergence from the POC package used as I ran into cross compilation issues with go-cryptsetup. This package still only compiles on linux, thus the use of luks_stub.go
} | ||
|
||
// Validate the passphrase | ||
for { |
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.
purposeful loop as product requirement is to continue to reprompt on invalid password attempts. A 1 minute timeout is passed into zenity which should protect a leaked go routine.
} | ||
|
||
func (lr *LuksRunner) passphraseIsValid(ctx context.Context, device *luks.LUKS, devicePath string, passphrase []byte) (bool, error) { | ||
if len(passphrase) == 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.
empty slice passed into device.CheckKey
produces an undesired error
|
||
var ErrKeySlotFull = regexp.MustCompile(`Key slot \d+ is full`) | ||
|
||
func (lr *LuksRunner) Run(oc *fleet.OrbitConfig) error { |
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.
acknowledging a lack of unit test coverage on this method. Much manual testing was done here on a physical machine (could not encrypt a VM) interacting with zenity prompts and monitoring LUKS states with cryptsetup
orbit-linux
Outdated
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.
Please remove me!
@@ -0,0 +1 @@ | |||
- adding functionality to support linux disk encryption key escrow including end user prompts and LUKS key management |
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.
Please use past tense. (and *
:)
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.
Looks good!
Added a few questions/comments.
|
||
if err := lr.escrower.SendLinuxKeyEscrowResponse(response); err != nil { | ||
if err := lr.infoPrompt(ctx, infoFailedTitle, infoFailedText); err != nil { | ||
log.Debug().Err(err).Msg("failed to show failed escrow key dialog") |
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.
Maybe log.Info()
for troubleshooting purposes.
|
||
if response.Err != "" { | ||
if err := lr.infoPrompt(ctx, infoFailedTitle, response.Err); err != nil { | ||
log.Debug().Err(err).Msg("failed to show failed escrow key dialog") |
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.
Distinguish log message from the one above (and also use log.Info
).
orbit/pkg/luks/luks_linux.go
Outdated
return nil, fmt.Errorf("Failed to find LUKS Root Partition: %w", err) | ||
} | ||
|
||
device := luks.New(luks.AESXTSPlain64Cipher) |
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 the default for the distributions we are attempting to support? Could a user use something else?
} | ||
|
||
break | ||
} |
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.
Before returning should we run passphraseIsValid
on escrowPassphrase
?
response.Key = string(key) | ||
|
||
if err := lr.escrower.SendLinuxKeyEscrowResponse(response); err != nil { | ||
if err := lr.infoPrompt(ctx, infoFailedTitle, infoFailedText); err != nil { |
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.
Should we remove the generated escrow key if it failed to send it to Fleet? (e.g. Fleet or network is down.)
orbit/pkg/luks/luks_linux.go
Outdated
@@ -0,0 +1,221 @@ | |||
//go:build linux | |||
|
|||
package luks_runner |
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.
Nit: Rename package to just luks
?
|
||
var ErrKeySlotFull = regexp.MustCompile(`Key slot \d+ is full`) | ||
|
||
func (lr *LuksRunner) Run(oc *fleet.OrbitConfig) error { |
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.
Just a comment, nothing to fix AFAICS:
This runner might delay the next run of orbit "config runners" [*] for up to a minute (plus a few seconds of show dialog timeout), I don't see an issue with it, just something to be aware of.
[*] Config runners run in a goroutine each, and all runners are iterated every 30s (this would delay the frequency a bit but not much).
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
// sample from real LUKS encrypted Ubuntu disk |
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.
Here's another sample from my Fedora VM:
{
"blockdevices": [
{
"name": "sr0",
"maj:min": "11:0",
"rm": true,
"size": "2.1G",
"ro": false,
"type": "rom",
"mountpoints": [
"/run/media/luk/Fedora-WS-Live-40-1-14"
]
},{
"name": "zram0",
"maj:min": "252:0",
"rm": false,
"size": "1.9G",
"ro": false,
"type": "disk",
"mountpoints": [
"[SWAP]"
]
},{
"name": "nvme0n1",
"maj:min": "259:0",
"rm": false,
"size": "20G",
"ro": false,
"type": "disk",
"mountpoints": [
null
],
"children": [
{
"name": "nvme0n1p1",
"maj:min": "259:1",
"rm": false,
"size": "600M",
"ro": false,
"type": "part",
"mountpoints": [
"/boot/efi"
]
},{
"name": "nvme0n1p2",
"maj:min": "259:2",
"rm": false,
"size": "1G",
"ro": false,
"type": "part",
"mountpoints": [
"/boot"
]
},{
"name": "nvme0n1p3",
"maj:min": "259:3",
"rm": false,
"size": "18.4G",
"ro": false,
"type": "part",
"mountpoints": [
null
],
"children": [
{
"name": "luks-21fc9b67-752e-42fb-83bb-8c92864382e9",
"maj:min": "253:0",
"rm": false,
"size": "18.4G",
"ro": false,
"type": "crypt",
"mountpoints": [
"/home", "/"
]
}
]
}
]
}
]
}
server/service/orbit_client.go
Outdated
@@ -666,3 +667,8 @@ func (oc *OrbitClient) GetSetupExperienceStatus() (*fleet.SetupExperienceStatusP | |||
|
|||
return resp.Results, nil | |||
} | |||
|
|||
func (oc *OrbitClient) SendLinuxKeyEscrowResponse(lr luks_runner.LuksResponse) error { |
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.
Please add a TODO comment :)
server/service/orbit_client.go
Outdated
@@ -666,3 +667,8 @@ func (oc *OrbitClient) GetSetupExperienceStatus() (*fleet.SetupExperienceStatusP | |||
|
|||
return resp.Results, nil | |||
} | |||
|
|||
func (oc *OrbitClient) SendLinuxKeyEscrowResponse(lr luks_runner.LuksResponse) error { | |||
log.Debug().Msg(fmt.Sprintf("Escrowing key: %s", lr.Key)) |
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.
And not forget to remove logging
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 23585-zenity #23771 +/- ##
================================================
- Coverage 63.16% 63.10% -0.07%
================================================
Files 1561 1567 +6
Lines 148732 149256 +524
Branches 3788 3788
================================================
+ Hits 93945 94184 +239
- Misses 47365 47645 +280
- Partials 7422 7427 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
4c0dac3
to
2f6f56c
Compare
ea2a5d0
to
300b04d
Compare
2f6f56c
to
5377d91
Compare
#23586
PR stacked diff on #23585. Will not merge until that PR is merged.
Adds a lvm orbit package to detect the proper encrypted disk partition for use with LUKS tools
Adds a luks orbit package managing the process of gathering a user passphrase and creating a keyslot and escrowable passphrase
Changes file added for user-visible changes in
changes/
,orbit/changes/
oree/fleetd-chrome/changes
.See Changes files for more information.
Added/updated tests
Manual QA for all new/changed functionality
For Orbit and Fleet Desktop changes:
runtime.GOOS
).