Conversation
majst01
left a comment
majst01
left a comment
There was a problem hiding this comment.
first small improvements
Co-authored-by: Stefan Majer <stefan.majer@gmail.com>
Gerrit91
left a comment
Gerrit91
left a comment
There was a problem hiding this comment.
Cool. Not an expert on this, but looks pretty good from the code perspective.
| if encryptedPath != "" { | ||
| // For encrypted volumes: extend LV without filesystem resize, then resize LUKS | ||
| output, err := lvm.ExtendLVS(d.log, d.vgName, volID, uint64(capacity), true) //nolint:gosec | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to extend lv: %w output:%s", err, output) | ||
| } | ||
|
|
||
| if err := lvm.LuksResize(d.log, mapperName); err != nil { | ||
| return nil, fmt.Errorf("unable to resize LUKS device: %w", err) | ||
| } | ||
|
|
||
| // For block volumes we're done; for filesystem volumes, resize the filesystem on the mapper device | ||
| if !isBlock { | ||
| resizer := mountutils.NewResizeFs(utilexec.New()) | ||
| if _, err := resizer.Resize(encryptedPath, volPath); err != nil { | ||
| return nil, fmt.Errorf("unable to resize filesystem on encrypted device %s: %w", encryptedPath, err) | ||
| } | ||
| } | ||
| } else { | ||
| output, err := lvm.ExtendLVS(d.log, d.vgName, volID, uint64(capacity), isBlock) //nolint:gosec | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to extend lv: %w output:%s", err, output) | ||
| } |
There was a problem hiding this comment.
I would prefer to have the default "happy" case in the if condition and the encryption logic inside else
ostempel
left a comment
ostempel
left a comment
There was a problem hiding this comment.
Really nice feature. Have some feedback for you
| args := []string{ | ||
| "luksOpen", devicePath, mapperName, | ||
| "--disable-keyring", // LUKS2 volumes require passphrase on resize if keyring is not disabled on open | ||
| "--key-file", "/dev/stdin", |
There was a problem hiding this comment.
| "--key-file", "/dev/stdin", | |
| "--key-file", os.Stdin.Name(), |
| "--cipher", defaultLuksCipher, | ||
| "--key-size", defaultLuksKeySize, | ||
| "--key-file", os.Stdin.Name(), | ||
| "--pbkdf-memory=65535", |
There was a problem hiding this comment.
Should we make this configurable since it affects the security?
| } | ||
|
|
||
| for _, line := range strings.Split(string(data), "\n") { | ||
| if strings.HasPrefix(line, "flags") && strings.Contains(line, " aes") { |
There was a problem hiding this comment.
This will most likely not work for arm architecture:
On my pi: output has the field features and this will silently fail.
Also gets this function called somewhere?
processor : 0
BogoMIPS : 108.00
Features : fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm lrcpc dcpop asimddp
CPU implementer : 0x41
CPU architecture: 8
CPU variant : 0x4
CPU part : 0xd0b
CPU revision : 1
| var ( | ||
| volID = req.GetVolumeId() | ||
| mapperName = lvm.LuksMapperName(volID) | ||
| devicePath = "" |
There was a problem hiding this comment.
What if we move the unencrypted device path here?
| devicePath = "" | |
| devicePath = fmt.Sprintf("/dev/%s/%s", d.vgName, volID) |
Then we can remove the defaulting logic in the lvm.go:
| req.GetVolumeContext()["csi.storage.k8s.io/ephemeral"] == "" && d.ephemeral // Kubernetes 1.15 doesn't have csi.storage.k8s.io/ephemeral. | ||
|
|
||
| // if ephemeral is specified, create volume here | ||
| if ephemeralVolume { |
There was a problem hiding this comment.
ephemeralVolumes won't go through the NodeStageVolume -> only through NodePublishVolume
So ephemeral volumes won't get encrypted. Do we want to handle this with logs?
|
|
||
| // LuksClose closes the LUKS device with the given mapper name. | ||
| func LuksClose(log *slog.Logger, mapperName string) error { | ||
| mapperPath := diskMapperPath + mapperName |
There was a problem hiding this comment.
| mapperPath := diskMapperPath + mapperName | |
| mapperPath := path.Join(diskMapperPath, mapperName) |
|
|
||
| // LuksResize resizes the LUKS device with the given mapper name. | ||
| func LuksResize(log *slog.Logger, mapperName string) error { | ||
| mapperPath := diskMapperPath + mapperName |
There was a problem hiding this comment.
| mapperPath := diskMapperPath + mapperName | |
| mapperPath := path.Join(diskMapperPath, mapperName) |
| linearEncrypted: | ||
| enabled: true | ||
| additionalAnnotations: [] | ||
| reclaimPolicy: Delete | ||
| encryptionSecret: | ||
| name: csi-lvm-encryption-secret | ||
| namespace: default | ||
| stripedEncrypted: | ||
| enabled: true | ||
| additionalAnnotations: [] | ||
| reclaimPolicy: Delete | ||
| encryptionSecret: | ||
| name: csi-lvm-encryption-secret | ||
| namespace: default | ||
| mirrorEncrypted: | ||
| enabled: true | ||
| additionalAnnotations: [] | ||
| reclaimPolicy: Delete | ||
| encryptionSecret: | ||
| name: csi-lvm-encryption-secret | ||
| namespace: default |
There was a problem hiding this comment.
Probably make these storageclasses/feature opt-in instead of default. Users first need to deploy the csi-lvm-encryption-secret.
| run kubectl exec -t volume-encrypted-writing-test -- cat /remount/output.log | grep "Happily writing encrypted" | ||
| [ "$status" -eq 0 ] | ||
| } | ||
|
|
There was a problem hiding this comment.
Tests so far validate, that the driver is working accordingly via the LUKS mapper, but doesn't test if the volumes are encrypted from the outside.
Maybe we want to add here something like this:
- Exec into daemonset
- Confirm the raw device is LUKS-formatted with cryptsetup isLuks
- Confirm the plaintext data is not readable from the raw LV (maybe with
stringscommand, but didn't dive into this)
4 participants
Description
This PR adds LUKS2 encryption support for volumes (raw block and filesystem).
The test framework has been extended and all tests pass in a local test run.
Closes #29.