Skip to content
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

Draft
wants to merge 26 commits into
base: 23585-zenity
Choose a base branch
from
Draft

Linux Key Escrow - Agent #23771

wants to merge 26 commits into from

Conversation

mostlikelee
Copy link
Contributor

@mostlikelee mostlikelee commented Nov 13, 2024

#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/ or ee/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:

    • Orbit runs on macOS, Linux and Windows. Check if the orbit feature/bugfix should only apply to one platform (runtime.GOOS).
    • Manual QA must be performed in the three main OSs, macOS, Windows and Linux.
    • Auto-update manual QA, from released version of component to new version (see tools/tuf/test).

@@ -666,3 +666,8 @@ func (oc *OrbitClient) GetSetupExperienceStatus() (*fleet.SetupExperienceStatusP

return resp.Results, nil
}

func (oc *OrbitClient) EscrowLinuxKey(key []byte) error {
Copy link
Contributor Author

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 {
Copy link
Contributor Author

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

Copy link
Member

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?

infoFailedText = "Failed to escrow key. Please try again later."
infoSuccessTitle = "Encryption key escrow"
infoSuccessText = "Key escrowed successfully."
maxKeySlots = 8
Copy link
Contributor Author

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.

Comment on lines 18 to 19
"github.com/siderolabs/go-blockdevice/v2/encryption"
"github.com/siderolabs/go-blockdevice/v2/encryption/luks"
Copy link
Contributor Author

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 {
Copy link
Contributor Author

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 {
Copy link
Contributor Author

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

@mostlikelee mostlikelee marked this pull request as ready for review November 14, 2024 01:41
@mostlikelee mostlikelee requested a review from a team as a code owner November 14, 2024 01:41

var ErrKeySlotFull = regexp.MustCompile(`Key slot \d+ is full`)

func (lr *LuksRunner) Run(oc *fleet.OrbitConfig) error {
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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 * :)

Copy link
Member

@lucasmrod lucasmrod left a 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")
Copy link
Member

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")
Copy link
Member

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).

return nil, fmt.Errorf("Failed to find LUKS Root Partition: %w", err)
}

device := luks.New(luks.AESXTSPlain64Cipher)
Copy link
Member

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
}
Copy link
Member

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 {
Copy link
Member

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.)

@@ -0,0 +1,221 @@
//go:build linux

package luks_runner
Copy link
Member

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 {
Copy link
Member

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
Copy link
Member

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", "/"
                     ]
                  }
               ]
            }
         ]
      }
   ]
}

@@ -666,3 +667,8 @@ func (oc *OrbitClient) GetSetupExperienceStatus() (*fleet.SetupExperienceStatusP

return resp.Results, nil
}

func (oc *OrbitClient) SendLinuxKeyEscrowResponse(lr luks_runner.LuksResponse) error {
Copy link
Member

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 :)

@@ -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))
Copy link
Member

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

Copy link

codecov bot commented Nov 15, 2024

Codecov Report

Attention: Patch coverage is 45.11149% with 320 lines in your changes missing coverage. Please review.

Project coverage is 63.10%. Comparing base (f390db7) to head (a5d1663).
Report is 27 commits behind head on 23585-zenity.

Files with missing lines Patch % Lines
orbit/pkg/luks/luks_linux.go 0.00% 156 Missing ⚠️
orbit/pkg/execuser/execuser_linux.go 0.00% 69 Missing ⚠️
orbit/pkg/zenity/zenity.go 73.49% 22 Missing ⚠️
orbit/pkg/lvm/lvm.go 77.58% 12 Missing and 1 partial ⚠️
orbit/pkg/execuser/execuser.go 0.00% 12 Missing ⚠️
server/service/orbit_client.go 0.00% 12 Missing ⚠️
server/service/hosts.go 50.00% 4 Missing and 2 partials ⚠️
server/service/orbit.go 76.00% 4 Missing and 2 partials ⚠️
orbit/pkg/luks/luks.go 0.00% 5 Missing ⚠️
...41116233322_AddLuksDataToHostDiskEncryptionKeys.go 58.33% 4 Missing and 1 partial ⚠️
... and 5 more
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     
Flag Coverage Δ
backend 63.93% <45.11%> (-0.08%) ⬇️

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

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


🚨 Try these New Features:

@mostlikelee mostlikelee marked this pull request as draft November 19, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants