add S390x Cex base disk encryption #1820
Conversation
internal/exec/stages/disks/luks.go
Outdated
| // args manually by verifying the keysize and keytype. | ||
| func zkeySecCryptGenArg(luks types.Luks) []string { | ||
| ret := []string{ | ||
| "--master-key-file", |
There was a problem hiding this comment.
Looks like that name is deprecated and it's now --volume-key-file.
internal/exec/stages/disks/luks.go
Outdated
| func zkeySecCryptGenArg(luks types.Luks) []string { | ||
| ret := []string{ | ||
| "--master-key-file", | ||
| "/etc/zkey/repository/root_xts.skey", |
There was a problem hiding this comment.
We'll need to adjust this accordingly.
internal/exec/stages/disks/luks.go
Outdated
| if !util.NilOrEmpty(luks.Cexmode.Keytype) && *luks.Cexmode.Keytype == "CCA-AESDATA" { | ||
| ret = append(ret, "--key-size", "1024") | ||
| } else { | ||
| ret = append(ret, "--key-size", "2176") | ||
| } |
There was a problem hiding this comment.
Do I understand correctly that this arg doesn't actually control the key size since we're explicitly passing in the volume key file to use, but that it's just for checking its size?
There was a problem hiding this comment.
As far as i understand , the cryptsetup wants --key-size when we use the --volume-key-file in xts mode. if we do not provide key size the cryptsetup somehow use default 2*256 bits from the file while formatting the disk and set the key size to 512bits (64bytes).
Without --key-size somehow it wont do the protected key mechanism with CEX card and no longer protected; The volume can be open with passkey without CEX card . Also The zkey validation will fail as well as it fails to set the verification pattern in the Token.
zkey-cryptsetup: # Device /dev/dasdb1 READ lock released.
zkey-cryptsetup: # PBKDF pbkdf2-sha256, time_ms 2000 (iterations 0).
zkey-cryptsetup: # PBKDF pbkdf2-sha256, time_ms 2000 (iterations 0).
zkey-cryptsetup: Installing SIGINT/SIGTERM handler
zkey-cryptsetup: Volume key size: 64
zkey-cryptsetup: Integrity: 'none'
zkey-cryptsetup: Integrity key size: 0
zkey-cryptsetup: The volume key size 64 is not valid for the cipher mode 'xts-plain64'
zkey-cryptsetup: # Releasing crypt device /dev/dasdb1 context.
zkey-cryptsetup: # Releasing device-mapper backend.
zkey-cryptsetup: # Closing read only fd for /dev/dasdb1.
eg:
I tried in one of the OCP worker node by formatting the disk as per the --key-size and the cryptsetup luksDump show the key size exactly as the key-size we provided.
sh-5.1# cryptsetup luksFormat /dev/dasdb1 --pbkdf pbkdf2 --volume-key-file '/etc/zkey/repository/secure_AESCipherKey4.skey' --key-size 2176 --cipher paes-xts-plain64
WARNING: Device /dev/dasdb1 already contains a 'crypto_LUKS' superblock signature.
WARNING!
========
This will overwrite data on /dev/dasdb1 irrevocably.
Are you sure? (Type 'yes' in capital letters): YES
Enter passphrase for /dev/dasdb1:
Verify passphrase:
sh-5.1# cryptsetup luksDump /dev/dasdb1
LUKS header information
Version: 2
Epoch: 3
Metadata area: 16384 [bytes]
Keyslots area: 16744448 [bytes]
UUID: 62910b38-1e69-4ed0-9e13-b641c423d0a0
Label: (no label)
Subsystem: (no subsystem)
Flags: (no flags)
Data segments:
0: crypt
offset: 16777216 [bytes]
length: (whole device)
cipher: paes-xts-plain64
sector: 4096 [bytes]
Keyslots:
0: luks2
Key: 2176 bits
Priority: normal
Cipher: aes-xts-plain64
Cipher key: 512 bits
PBKDF: pbkdf2
Hash: sha256
Iterations: 1865792
Salt: cf ac 97 b8 49 92 cf 53 33 85 7e 7c 5e b1 bc 48
5d 1a 3d f9 39 46 16 b8 3c 7f 84 7d 72 e7 64 78
AF stripes: 4000
AF hash: sha256
Area offset:32768 [bytes]
Area length:1089536 [bytes]
Digest ID: 0
However we do not provide the --key-size here is the output.
sh-5.1# cryptsetup luksFormat /dev/dasdb1 --pbkdf pbkdf2 --volume-key-file '/etc/zkey/repository/secure_AESCipherKey5.skey' --cipher paes-xts-plain64
WARNING: Device /dev/dasdb1 already contains a 'crypto_LUKS' superblock signature.
WARNING!
========
This will overwrite data on /dev/dasdb1 irrevocably.
Are you sure? (Type 'yes' in capital letters): YES
Enter passphrase for /dev/dasdb1:
Verify passphrase:
sh-5.1# cryptsetup luksDump /dev/dasdb1
LUKS header information
Version: 2
Epoch: 3
Metadata area: 16384 [bytes]
Keyslots area: 16744448 [bytes]
UUID: 473b0977-45ff-4194-8b53-36dc4babdca1
Label: (no label)
Subsystem: (no subsystem)
Flags: (no flags)
Data segments:
0: crypt
offset: 16777216 [bytes]
length: (whole device)
cipher: paes-xts-plain64
sector: 4096 [bytes]
Keyslots:
0: luks2
Key: 512 bits
Priority: normal
Cipher: aes-xts-plain64
Cipher key: 512 bits
PBKDF: pbkdf2
Hash: sha256
Iterations: 1804776
Salt: f0 d2 15 e9 06 ad e6 7b a8 3b 28 38 7f 0b cb 99
1e 65 4f 49 58 2d 63 67 3a 5f 1e 2e 2d e6 d0 b3
AF stripes: 4000
AF hash: sha256
Area offset:32768 [bytes]
Area length:258048 [bytes]
Digest ID: 0
There was a problem hiding this comment.
@jlebon adding on above ^ , I had word with zcrypto team. Here is the explanation for that.
If you pass a key to paes that is a valid clear key size, then it uses it as clear key.
So in your case it reads the first 64 bytes from the secure key file and uses them as clear key value for AES-XTS. And for this it of course does not need any crypto cards.
The reason why this is done that way, is because all kernel ciphers need to run a selftest when they are loaded, and this self test can only be performed with clear keys. So paes must also accept clear key as key. Valid secure keys will never have a size that matches a valid clear key size.
So you must be careful and always specify the correct key size during luksFormat.
So the key-size must be verified and use that size as --key-size for luksFormat,
So in the ignition we could read the secure key file-size which guarantees the key-size.
Kindly suggest.
There was a problem hiding this comment.
Gotcha, thanks. That clears it up nicely.
I think one confusing thing here is that we're hardcoding key lengths instead of using the source of truth, which is the key that zkey generate generated. Isn't it just the size of the key file itself?
There was a problem hiding this comment.
Yes, it is the size of the key file. The following is key-type CCA-AESCIPHER which generates 272bytes size. CCA-AESDATA generates 128bytes size. Usual cases zkey command manages all argument including the luksFormat and verification. However zkey generates the command arg pbdkf algo as argon2i where FIPS does not support.
sh-5.1# cd /etc/zkey/repository/
sh-5.1# ls
secure_AESCipherKey5.info secure_AESCipherKey5.skey
sh-5.1# ls -l
total 8
-rw-r-----. 1 root zkeyadm 321 Mar 5 04:26 secure_AESCipherKey5.info
-rw-r-----. 1 root zkeyadm 272 Mar 5 04:26 secure_AESCipherKey5.skey
sh-5.1# KEYBITSIZE=$(stat --printf="%s" secure_AESCipherKey5.skey)
sh-5.1# KEYBITSIZE=$((${KEYBITSIZE}*8))
sh-5.1# echo $KEYBITSIZE
2176
There was a problem hiding this comment.
Then I'd say instead of hardcoding these values, let's just os.Stat the keyfile and use its length (* 8) as the argument.
|
What's the plan around CI testing this BTW on the CoreOS side? Do we have the required hardware in our pipeline s390x builders to make use of this? Ideally we have a kola test exercising this. |
The s390x comes with CEX cards, I will check whether LPAR has cex card or not. |
madhu-pillai
force-pushed
the
test_cex
branch
2 times, most recently
from
62d71da
to
98be9e1
Compare
|
Hi @jlebon , The unit test |
Could you post your error here ? |
|
|
Hi, Kindly help here. Did i miss anything here? I read the development doc and it says to add the translation if requires. The error logs as above. |
docs/release-notes.md
Outdated
| ### Bug fixes | ||
|
|
||
| ## Ignition 2.18.0 (2024-03-01) | ||
| ## Upcoming Ignition 2.18.0 (unreleased) |
There was a problem hiding this comment.
You might have a merge conflict that was resolved wrong?
There was a problem hiding this comment.
Will fix that..
dracut/30ignition/ignition-cex
Outdated
| @@ -0,0 +1 @@ | |||
| This is a dummy "keyfile" which explain the purpose of this file in the dracut module. A key used to encrypt and decrypt the user data on a volume formatted in luks2 format. A key slot in luks2 header stores a wrapped copy of this volume key, where the wrapping key is derived from the users passphrase or "keyfile". In the infrastructure for protected volume encryption, the luks2 volume key is secure key. The effective volume is twofold protected: it is encrypted by an AES master key from a CCA or EP11 coProcessor and by a wrapping key or KEK derived from a passphrase or "keyfile". Therefore to unlock a luks2 volume a passphrase - provided interactively or from a "keyfile" is required to decrypt the outer wrapping. The security provided by the passphrase or "keyfile" is typically much lower than that provided by the wrapping AES master key. Therefore the password or "keyfile" may be exposed without any loss of security. When a secure key for the PAES cipher is provide to dm-crypt inorder to open a volume, it automatically transforms this secure key into a protected key that can be interpreted by the CPACF. The actual effective key of the luks2 volume key is never exposed to the operating system. | |||
There was a problem hiding this comment.
Can we add any formatting for easier consumption?
dracut/30ignition/module-setup.sh
Outdated
| @@ -112,5 +114,8 @@ installkernel() { | |||
| instmods -c vsock | |||
| instmods -c vmw_vsock_virtio_transport_common | |||
| instmods -c vmw_vsock_virtio_transport | |||
|
|
|||
| # required for cex card early initialization. | |||
There was a problem hiding this comment.
Remove '.' for consistency with other comments similar to this.
dracut/30ignition/module-setup.sh
Outdated
| inst_multiple -o chccwdev vmur | ||
| inst_multiple -o chccwdev vmur zkey zkey-cryptsetup | ||
|
|
||
| inst_simple "$moddir/ignition-cex" "/etc/luks/cex.key" | ||
|
|
||
| # Required on system using SELinux |
There was a problem hiding this comment.
add a '.' for consistency.
internal/distro/distro.go
Outdated
| vmurCmd = "vmur" | ||
| chccwdevCmd = "chccwdev" | ||
| cioIgnoreCmd = "cio_ignore" | ||
| // zVM programs |
internal/exec/stages/disks/luks.go
Outdated
| @@ -531,3 +566,135 @@ func (d LuksDump) hasFlag(flag string) bool { | |||
| } | |||
| return false | |||
| } | |||
|
|
|||
| // collect the cex card domains xx.xxxx, | |||
| func getAllCexOnlineApqns() (string, error) { | |||
There was a problem hiding this comment.
is Apqns an acronym? if so can we cap it i.e APQNS
There was a problem hiding this comment.
it stands for Adjunct Processor Queue Number. An APQN designates the combination of a cryptographic coprocessor (adapter) and a domain. I'll change to Caps.
internal/exec/stages/disks/luks.go
Outdated
| @@ -531,3 +566,135 @@ func (d LuksDump) hasFlag(flag string) bool { | |||
| } | |||
| return false | |||
| } | |||
|
|
|||
| // collect the cex card domains xx.xxxx, | |||
There was a problem hiding this comment.
er, is the ',' part of the format?
There was a problem hiding this comment.
Sorry its err. Removed.
internal/exec/stages/disks/luks.go
Outdated
| // match the regexp for pattern xx.xxxx | ||
| for _, dir := range event { | ||
| match := cexRegx.FindStringSubmatch(dir.Name()) | ||
| // if the match found read the uevent from that apqn directory |
There was a problem hiding this comment.
Would it be correct to say
// If a match is found, read the uevent from that APQN directory
// to verify whether it corresponds to a CCA controller. This is
// intended for potential future use with an EP11 controller.
fd1446d
to
ce262c2
Compare
|
Hi, |
|
@madhu-pillai Looks like you still have some failing CI, have you had a chance to look at it ? |
|
Hi @prestist ,
|
|
Hi, |
|
Resolved the earlier failed test. Kindly review. |
There was a problem hiding this comment.
I think this is looking good overall, just lots of minor comments.
Can we follow up on
What's the plan around CI testing this BTW on the CoreOS side? Do we have the required hardware in our pipeline s390x builders to make use of this? Ideally we have a kola test exercising this.
The s390x comes with CEX cards, I will check whether LPAR has cex card or not.
?
We need to scope in a kola test as part of this feature work.
config/doc/ignition.yaml
Outdated
| - name: enabled | ||
| desc: whether or not to use a CEX secure key to encrypt luks device. | ||
| - name: keyType | ||
| desc: the type of the secure key (CCA-AESDATA or CCA-AESCIPHER). Defaults to CCA-AESCIPHER. |
There was a problem hiding this comment.
On this topic and following up on #1693 (comment), is there a reason one would want CCA-AESDATA? Trying to reduce config knobs that aren't necessary.
There was a problem hiding this comment.
The reason for the optional key-type as below.
By default if we do not give --key-type it uses CCA-AESDATA .
`CCA-AESDATA` -> support Linux upstream kernel version 4.12 or later use with < CEX6C model card
`CCA-AESCIPHER` -> support Linux upstream kernel version 5.14 (or older version where required modules have been backported)
The different secure key types have different sizes (in bytes):
• CCA AES DATA: 64, 128 for XTS
• CCA AES CIPHER: 136, 272 for XTS
• EP11 AES: 320, 640 for XTS
According to IBM allow an option to select key-type variant.
- AES CIPHER keys can be presented as default & recommended
- AES DATA keys are less secure and should only be used if at one point in time an export of those keys will be required. AES CIPHER does not support export of keys.
In terms of Config knob cex: true is prettiest and easy. But I think we need to provide an option to user to choose the key-type.
Kindly suggest.
There was a problem hiding this comment.
But I think we need to provide an option to user to choose the key-type.
Do we? I can imagine some situations where a user could want to export the volume key (e.g. for backup or migration), but I'm not sure how desirable they are in practice vs just reprovisioning the node since Ignition is usually used in situations where that's the preferred option. Important data is usually backed up at the userspace/filesystem level instead (or e.g. mounted from dedicated storage services). ISTM like a large part of the appeal with CEX is that the keys never leave the hardware.
We could also not support it to start and see if there's strong motivation to add support for it in the future. It wouldn't be hard to add it at that point. I would still structure it as
cex:
enabled: true
for extending it for this and anything else in the future.
There was a problem hiding this comment.
I understood, Will remove keyType option.
| if util.NilOrEmpty(luks.KeyFile.Source) { | ||
|
|
||
| if luks.Cex.IsPresent() { | ||
| keyFilePath = "/etc/luks/cex.key" |
There was a problem hiding this comment.
One note on this is that if multiple LUKS devices are encrypted via CEX secure keys, we're going to have a copy of this file in /etc/luks/ for each LUKS device. Not a big deal, and I don't think it's worth trying to complicate things to special-case it further, but just noting.
Maybe let's make a comment about that here?
There was a problem hiding this comment.
If i understood correctly.
One note on this is that if multiple LUKS devices are encrypted via CEX secure keys, we're going to have a copy of this file in /etc/luks/ for each LUKS device
No, Only one file for all devices, as these are --key-file for the cryptsetup to create and open. I think it overwrites for each devices. If i am not wrong.?
There was a problem hiding this comment.
Oh Sorry, I mis understood.. Yes each device name it creates new file....
There was a problem hiding this comment.
Can we add a comment about this as suggested?
internal/exec/stages/files/files.go
Outdated
| var ( | ||
| ErrFilesystemUndefined = errors.New("the referenced filesystem was not defined") | ||
| ) | ||
| var ErrFilesystemUndefined = errors.New("the referenced filesystem was not defined") |
There was a problem hiding this comment.
Will correct it.
internal/exec/stages/files/files.go
Outdated
| @@ -123,6 +121,11 @@ func (s stage) runImpl(config types.Config, isApply bool, applyIgnoreUnsupported | |||
| if err := s.relabelFiles(); err != nil { | |||
| return fmt.Errorf("failed to handle relabeling: %v", err) | |||
| } | |||
|
|
|||
| // !isApply: saves LUKS volume key | |||
There was a problem hiding this comment.
I'd put this right after the createCrypttabEntries() since it's related.
There was a problem hiding this comment.
Will add next to createCrypttabEntries()
| s.Logger.PushPrefix("createCexVolumeKeys") | ||
| defer s.Logger.PopPrefix() | ||
| for _, luks := range config.Storage.Luks { | ||
| if luks.Cex.IsPresent() { |
There was a problem hiding this comment.
Minor: instead of intending here, I'd reflow as:
if !luks.Cex.IsPresent() {
continue
}
...There was a problem hiding this comment.
Will realign the flow.
| fileExt := []string{".info", ".skey"} | ||
| entries := []filesystemEntry{} | ||
| for _, zfile := range fileExt { | ||
| zkeyFile := filepath.Join(distro.LuksRealVolumeKeyFilePath(), "ignition-luks-"+luks.Name+zfile) |
There was a problem hiding this comment.
That ignition-luks-"+luks.Name+ext happens enough times in both this file and luks.go` at this point that it probably makes sense to add tiny helpers somewhere that can be shared.
There was a problem hiding this comment.
Added helper function in util so that it can be shared across on luks.go and filesystementries.go.
Presently we do not have cex hardware for the CI pipeline test. I had discussion with Holger and for the time we mitigate the issues we are facing by supporting only the part that is currently impletement/working/tested and documented it as such. |
Is it a viable option like. We have a VM with CEX card and each rhcos build can be test run on that vm; whenever a new rhcos build the cronjob in the VM can sense and trigger a kola cex test? |
internal/exec/stages/disks/luks.go
Outdated
| "ignition-luks-" + luks.Name, | ||
| "--xts", | ||
| "--description", | ||
| "Secure Key for Root Volume", |
There was a problem hiding this comment.
Not necessarily root. Should probably use luks.Name instead.
That could be an option. The best would be if the builders we use can have access to CEX hardware. But otherwise, yes we could also trigger a separate test that e.g. provisions a VM in IBM Cloud (is that where the VM would be?) and runs tests there. |
having CEX in the cloud is not possible, this is a pure on-prem available feature. |
| if util.NilOrEmpty(luks.KeyFile.Source) { | ||
|
|
||
| if luks.Cex.IsPresent() { | ||
| keyFilePath = "/etc/luks/cex.key" |
There was a problem hiding this comment.
Can we add a comment about this as suggested?
internal/exec/stages/disks/luks.go
Outdated
| "CCA-AESCIPHER", | ||
| "--xts", | ||
| "--description", | ||
| "Secure Key for " + luks.Name + " Volume.", |
There was a problem hiding this comment.
Super minor: let's drop the final period here?
There was a problem hiding this comment.
Comment Added. and dropped the period. Done
internal/exec/stages/disks/luks.go
Outdated
| if luks.Cex.IsPresent() { | ||
| err := s.zkeyCryptSetvp(luks, keyFilePath) | ||
| if err != nil { | ||
| return fmt.Errorf("zkey cryptsetup setvp failed: %w", err) |
There was a problem hiding this comment.
I'd just return err here since the error context within s.zkeyCryptSetvp() seems sufficient.
internal/exec/util/util.go
Outdated
| @@ -131,3 +131,13 @@ func PathExists(path string) (bool, error) { | |||
| } | |||
| return true, nil | |||
| } | |||
|
|
|||
| // Helper function to concatenate the key files. | |||
| func ZKeyHelper(l string) map[string]string { | |||
There was a problem hiding this comment.
This really needs a more specific name. E.g. ZkeySecureKeyRepoFiles?
internal/exec/stages/disks/luks.go
Outdated
| zkeyfile := execUtil.ZKeyHelper(luks.Name) | ||
| zfilePath := distro.LuksRealVolumeKeyFilePath() | ||
| for _, zfile := range zkeyfile { | ||
| key, err := os.ReadFile(zfilePath + zfile) |
internal/state/state.go
Outdated
| @@ -41,6 +41,9 @@ type State struct { | |||
| // the filesystem during files stage. This is for special | |||
| // circumstances only. | |||
| ProviderOutputFiles []types.File `json:"providerOutputFiles"` | |||
| // Volume Key files generated during LUKS setup in disks stage, which | |||
| // need to be written out during files stage. | |||
| LuksPersistVolumeKeyFiles map[string]string `json:"luksPersistVolumeKeyFiles"` | |||
There was a problem hiding this comment.
Minor/optional: since this includes both secure keys and info files, we could make this more generic, e.g. LuksPersistSecureKeyRepoFiles?
There was a problem hiding this comment.
LGTM! Thanks a lot for working through all those rounds of review.
This code went through a lot of revisions; can you confirm that the latest version works well with the latest RHCOS 4.16 builds? Were you able to check both the rootfs and secondary filesystem cases? (I know there's still coreos/fedora-coreos-config#2986 that we need to work on, but apart from that.)
Hi @jlebon and @prestist , Thanks a lot for your support. I performed testing before every PR push on all the platform , zKvm, zVm-zfcp and zVm-eckd. <style> </style>
Here is the ignition config i used for testing. For each luks device and disk i changes accordingly. |
Extend the `luks` schema to support a new `cex` key. When enabled, the volume key of the LUKS device uses a secure key generated using a CEX card. The keyfile to unlock the volume is not considered confidential. Closes: coreos#1693 Co-authored-by: Jonathan Lebon <jonathan@jlebon.com>
|
I just added more details to the commit message and marked it as closing #1693. Auto-merge enabled. |
This kola test is crucial for verifying the security of CEX hardware-based LUKS encryption on root volume. It guarantees that the encrypted device employs protected keys to encrypt and decrypt the volume. This is essentially testing the enablement done in coreos/ignition#1820. To run this, it needs to be on a system with a CEX device with passthrough enabled and the device's UUID exposed via KOLA_CEX_UUID. See also coreos/fedora-coreos-pipeline#1010. Co-authored-by: Jonathan Lebon <jonathan@jlebon.com>
This kola test is crucial for verifying the security of CEX hardware-based LUKS encryption on root volume. It guarantees that the encrypted device employs protected keys to encrypt and decrypt the volume. This is essentially testing the enablement done in coreos/ignition#1820. To run this, it needs to be on a system with a CEX device with passthrough enabled and the device's UUID exposed via KOLA_CEX_UUID. See also coreos/fedora-coreos-pipeline#1010. Co-authored-by: Jonathan Lebon <jonathan@jlebon.com>
This PR is to enable CEX mode disk encryption for root volume.
The discussion on this feature has been updated in #1693)
During the unit test , the translation get failed.