add: Support LUKS encryption using IBM CEX secure keys on s390x #536

madhu-pillai · 2024-05-29T17:12:57Z

Added the feature of IBM CEX card based LUKS encryption. Kindly review..

travier · 2024-07-03T10:56:00Z

docs/config-fcos-v1_6-exp.md

@@ -219,6 +221,7 @@ The Fedora CoreOS configuration is a YAML document conforming to the following s
    * **_tpm2_** (boolean): whether or not to use a tpm2 device.
    * **_threshold_** (integer): sets the minimum number of pieces required to decrypt the device. Default is 1.
    * **_discard_** (boolean): whether to issue discard commands to the underlying block device when blocks are freed. Enabling this improves performance and device longevity on SSDs and space utilization on thinly provisioned SAN devices, but leaks information about which disk blocks contain data. If omitted, it defaults to false.
+    * **_enabled_** (boolean): UnSupported


This looks weird

Will fix this.

travier · 2024-07-03T10:56:53Z

docs/config-flatcar-v1_2-exp.md

+    * **_cex_** (object): describes the IBM Crypto Express (CEX) card configuration for the luks device.
+      * **_enabled_** (boolean): whether or not to use a CEX secure key to encrypt the luks device.


If it needs changes outside of Ignition and Butane then we should reach out to them before we add it here.

Hi @travier , Could you help here how can i get rid of this from flatcar? It was auto generated.

Probably something like: https://github.com/madhu-pillai/butane/blob/test_cex/internal/doc/butane.yaml#L49..L60

Ah, no, looks like it's just for a substring.

Hi @travier,
What i found is flatcar has luks feature too where it generates the document from the ignition/v2/config/doc. The flatcar schema is configured to obtain config from base/v0_6_exp . For the cex feature i've added the cex.enabled in the base/v0_6_exp. Adding conditions in the butane.yaml does not make any changes, as the docs get pulled according to the ignition/v2/config/doc.

It looks like there are field filters that can be used to ensure that trying to use some base Ignition fields result in errors on some variants. See e.g.

butane/config/openshift/v4_17_exp/translate.go

Lines 62 to 109 in d26d803

var (

// See also validateRHCOSSupport() and validateMCOSupport()

fieldFilters = cutil.NewFilters(result.MachineConfig{}, cutil.FilterMap{

// UNPARSABLE, REDUNDANT

"spec.config.kernelArguments": common.ErrKernelArgumentSupport,

// IMMUTABLE

"spec.config.passwd.groups": common.ErrGroupSupport,

// TRIPWIRE

"spec.config.passwd.users.gecos": common.ErrUserFieldSupport,

// TRIPWIRE

"spec.config.passwd.users.groups": common.ErrUserFieldSupport,

// TRIPWIRE

"spec.config.passwd.users.homeDir": common.ErrUserFieldSupport,

// TRIPWIRE

"spec.config.passwd.users.noCreateHome": common.ErrUserFieldSupport,

// TRIPWIRE

"spec.config.passwd.users.noLogInit": common.ErrUserFieldSupport,

// TRIPWIRE

"spec.config.passwd.users.noUserGroup": common.ErrUserFieldSupport,

// TRIPWIRE

"spec.config.passwd.users.primaryGroup": common.ErrUserFieldSupport,

// TRIPWIRE

"spec.config.passwd.users.shell": common.ErrUserFieldSupport,

// TRIPWIRE

"spec.config.passwd.users.shouldExist": common.ErrUserFieldSupport,

// TRIPWIRE

"spec.config.passwd.users.system": common.ErrUserFieldSupport,

// TRIPWIRE

"spec.config.passwd.users.uid": common.ErrUserFieldSupport,

// IMMUTABLE

"spec.config.storage.directories": common.ErrDirectorySupport,

// FORBIDDEN

"spec.config.storage.files.append": common.ErrFileAppendSupport,

// redundant with a check from Ignition validation, but ensures we

// exclude the section from docs

"spec.config.storage.files.contents.httpHeaders": common.ErrFileHeaderSupport,

// IMMUTABLE

// If you change this to be less restrictive without adding

// link support in the MCO, consider what should happen if

// the user specifies a storage.tree that includes symlinks.

"spec.config.storage.links": common.ErrLinkSupport,

})

)

// Return FieldFilters for this spec.

func (c Config) FieldFilters() *cutil.FieldFilters {

return &fieldFilters

}

. I think we can do the same here.

But also, since most of the enablement work was done in Ignition and Flatcar does support Clevis at least, CEX encryption also should mostly work in Flatcar. So they might be interested in testing it as well and if it works, remove the field filters in a follow up (cc @tormath1).

Thanks for the heads-up. For what is worth, I never heard about someone running Flatcar on IBM and we don't even provide IBM images with ibmcloud OEM id.
So yeah, I guess we can go ahead with a filter, similar to what we have done with Clevis before adding support on Flatcar: a8c311b

travier · 2024-07-03T10:58:28Z

internal/doc/butane.yaml

+            - name: enabled 
+              desc: whether or not to use a cex device.
+              transforms:
+                - regex: ".*"
+                  replacement: "UnSupported"
+                  if:
+                    - variant: fcos
+                      min: 1.6.0-experimental
+                    - variant: openshift
+                      min: 4.16.0-experimental


This was a hack for the GRUB support. Ideally we don't need this.

Will remove the transforms

madhu-pillai · 2024-07-11T04:55:14Z

Hi @travier ,
Can i've a review on this?

travier · 2024-07-12T07:40:53Z

config/fcos/v1_6_exp/schema.go

@@ -33,6 +33,7 @@ type BootDevice struct {
 type BootDeviceLuks struct {
 	Discard   *bool       `yaml:"discard"`
 	Device    *string     `yaml:"device"`
+	Enabled   *bool       `yaml:"enabled"`


Shouldn't this field be named cex? Enabled is ambiguous here.

There were few s390x specific parameter that can be selected under cex type. But later those parameter added as default and hardcoded in the ignition and decided that we can add in future if there is a requirement eg CIPHER. That's why the cex.enabled. The types is added under v0_6_exp as cex.enabled.

madhu@Madhus-MacBook-Pro arm64 % ./butane test.bu -o test.ign --pretty madhu@Madhus-MacBook-Pro arm64 % cat test.bu variant: fcos version: 1.6.0-experimental boot_device: layout: s390x-zfcp luks: device: /dev/sda enabled: true madhu@Madhus-MacBook-Pro arm64 % cat test.ign { "ignition": { "version": "3.5.0-experimental" }, "storage": { "filesystems": [ { "device": "/dev/mapper/root", "format": "xfs", "label": "root", "wipeFilesystem": true } ], "luks": [ { "cex": { "enabled": true }, "device": "/dev/sda4", "label": "luks-root", "name": "root", "wipeVolume": true } ] } }

Why is this not cex.enabled like in the 0.6-exp scema?

or the luks one

Okay, I thought the boot-device sugar we only need enabled because it works only with s390x and looks short. Let me try same in fcos too.

has this been resolved?

madhu-pillai · 2024-07-14T10:41:19Z

Hi @travier ,
I've changed the schema part to Cex. Kindly review it.

madhu-pillai · 2024-07-14T10:48:36Z

$ butane % cat bin/arm64/openshift.bu 
variant: openshift
version: 4.17.0-experimental
metadata:
  name: MachineConfig
  labels:
    machineconfiguration.openshift.io/role: master1
boot_device:
  layout: s390x-eckd
  luks:
    device: /dev/dasda
    cex:
      enabled: true

 $ ./butane openshift.bu -o openshift.ign --pretty && cat openshift.ign
# Generated by Butane; do not edit
apiVersion: machineconfiguration.openshift.io/v1
kind: MachineConfig
metadata:
  labels:
    machineconfiguration.openshift.io/role: master1
  name: MachineConfig
spec:
  config:
    ignition:
      version: 3.5.0-experimental
    storage:
      filesystems:
        - device: /dev/mapper/root
          format: xfs
          label: root
          wipeFilesystem: true
      luks:
        - cex:
            enabled: true
          device: /dev/dasda2
          label: luks-root
          name: root
          wipeVolume: true

travier · 2024-07-15T15:53:37Z

internal/doc/butane.yaml

+            - name: cex 
+              desc: whether or not to use cex device.
+              children:
+                - name: enabled


Missing the description here

HI @travier , Oh, My Bad, I verified only from the docs side. Now i've added the description.

prestist

It's coming along! a few nits;

Thank you for doing this.

prestist · 2024-07-15T21:32:37Z

config/common/errors.go

@@ -59,6 +59,7 @@ var (
 	ErrNoLuksBootDevice        = errors.New("device is required for layouts: s390x-eckd, s390x-zfcp")
 	ErrMirrorNotSupport        = errors.New("mirroring not supported on layouts: s390x-eckd, s390x-zfcp, s390x-virt")
 	ErrLuksBootDeviceBadName   = errors.New("device name must start with /dev/dasd on s390x-eckd layout or /dev/sd on s390x-zfcp layout")
+	ErrCexUnSupportArch        = errors.New("cex does not supported architectures other than s390x")


Hmm, lets try
ErrCexArchitectureMissmatch
"when using cex the targeted architecture must match s390x"

Looking at the change it looks like it has not been updated?

prestist · 2024-07-15T21:50:10Z

config/fcos/v1_6_exp/translate.go

@@ -114,7 +114,7 @@ func (c Config) processBootDevice(config *types.Config, ts *translate.Translatio
 	var r report.Report

 	// check for high-level features
-	wantLuks := util.IsTrue(c.BootDevice.Luks.Tpm2) || len(c.BootDevice.Luks.Tang) > 0
+	wantLuks := util.IsTrue(c.BootDevice.Luks.Tpm2) || len(c.BootDevice.Luks.Tang) > 0 || util.IsTrue(c.BootDevice.Luks.Cex.Enabled)


This is getting long, not sure if there is anything to do to fix this.. but yeah.

Added this feature similar to the other two luks method.

config/flatcar/v1_2_exp/translate.go

internal/doc/butane.yaml

prestist · 2024-07-15T22:14:04Z

docs/examples.md

+  layout: s390x-eckd
+  luks:
+    device: /dev/dasda
+    cex:


So we have a 3d object, with only two dementions ? (on and off)

Are there plans for adding other fields to the cex object?

if not we can we make it 2d? i.e cex: bool

There were few s390x specific parameter that can be selected under cex type. But later those parameter added as default and hardcoded in the ignition and decided that we can add in future if there is a requirement eg CIPHER. That's why the cex.enabled.

#536 (comment)

Ah, ok, nothing to push on if everyone else is okay with it.

madhu-pillai · 2024-07-16T15:22:24Z

Hi @prestist ,
Could you please review the updated comment?.

prestist

Gennerally lgtm I have like one nit; otherwise I think its gtg.

prestist · 2024-08-02T21:06:05Z

config/common/errors.go

@@ -59,6 +59,8 @@ var (
 	ErrNoLuksBootDevice        = errors.New("device is required for layouts: s390x-eckd, s390x-zfcp")
 	ErrMirrorNotSupport        = errors.New("mirroring not supported on layouts: s390x-eckd, s390x-zfcp, s390x-virt")
 	ErrLuksBootDeviceBadName   = errors.New("device name must start with /dev/dasd on s390x-eckd layout or /dev/sd on s390x-zfcp layout")
+	ErrCexArchitectureMismatch = errors.New("when using cex the targeted architecture must match s390x")
+	ErrUnSupportedHardware     = errors.New("the hardware is not supported")


Since we are adding this error for cex I expect it to have some indication that its related to it. Also I might have used the wrong verbiage before I am sorry about that.

Wdyt

ErrCexNotSupported: "cex is not currently supported on the target platform"

@madhu-pillai have you had a chance to update this error name?

@prestist , Oops sorry, I thought we need to hold till the ignition get stablized. Done

No worries; I think here were talks about keeping this in "experimental" so stabilization happing first would make sense.

prestist · 2024-08-02T21:10:58Z

docs/examples.md

+  layout: s390x-eckd
+  luks:
+    device: /dev/dasda
+    cex:


Ah, ok, nothing to push on if everyone else is okay with it.

discussion: coreos/butane#536 (comment)

Docs rephrase in luks.cex.enabled description. coreos/butane#536 (comment)

Bumps [github.com/coreos/ignition/v2](https://github.com/coreos/ignition) from 2.18.0 to 2.19.0. - [Release notes](https://github.com/coreos/ignition/releases) - [Changelog](https://github.com/coreos/ignition/blob/main/docs/release-notes.md) - [Commits](coreos/ignition@v2.18.0...v2.19.0) --- updated-dependencies: - dependency-name: github.com/coreos/ignition/v2 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>

madhu-pillai · 2024-10-22T06:56:47Z

[root@a3elp53 s390x]# ./butane boot.bu -o boot.ign --pretty
[root@a3elp53 s390x]# cat boot.bu
variant: fcos
version: 1.6.0-experimental
boot_device:
  layout: s390x-zfcp
  luks:
    device: /dev/sdb
    cex:
      enabled: true
[root@a3elp53 s390x]# cat boot.ign
{
  "ignition": {
    "version": "3.5.0-experimental"
  },
  "storage": {
    "filesystems": [
      {
        "device": "/dev/mapper/root",
        "format": "xfs",
        "label": "root",
        "wipeFilesystem": true
      }
    ],
    "luks": [
      {
        "cex": {
          "enabled": true
        },
        "device": "/dev/sdb4",
        "label": "luks-root",
        "name": "root",
        "wipeVolume": true
      }
    ]
  }
}

madhu-pillai · 2024-10-28T15:24:03Z

Hi, Is this pr ok to approve? or anything pending from my side?

prestist · 2024-11-11T19:23:01Z

@madhu-pillai , I am going to run through and review from my perspective, but will not be merging until I hear from @travier since he had a lot of feedback, and I want to ensure his concerns are resolved.

Note: the now latest version of ignition is 2.20.0 which has the functionality this sugar uses in stable, I would prefer to get this in before we update butane with that version and its new specs.

prestist

Please make sure that you have resolved, and fixed any review comments, there seems to be one which has gramar issues still here, and more importantly a question about structure here

madhu-pillai · 2024-11-12T09:49:33Z

Please make sure that you have resolved, and fixed any review comments, there seems to be one which has gramar issues still here, and more importantly a question about structure here

Hi @prestist ,
These reviews were already updated in the last commit. Sorry i missed to update the review message. Now done.

madhu-pillai force-pushed the test_cex branch from 43fddf7 to 23b740a Compare May 29, 2024 17:19

madhu-pillai force-pushed the test_cex branch from 23b740a to 9d9959e Compare July 2, 2024 15:25

travier reviewed Jul 3, 2024

View reviewed changes

prestist mentioned this pull request Jul 3, 2024

build(deps): bump github.com/coreos/ignition/v2 from 2.18.0 to 2.19.0 #539

Open

madhu-pillai force-pushed the test_cex branch from 9d9959e to d26c2ad Compare July 9, 2024 09:56

travier reviewed Jul 12, 2024

View reviewed changes

madhu-pillai force-pushed the test_cex branch from d26c2ad to 3b2e899 Compare July 14, 2024 10:34

travier reviewed Jul 15, 2024

View reviewed changes

madhu-pillai force-pushed the test_cex branch from 3b2e899 to 166ca24 Compare July 15, 2024 16:53

prestist reviewed Jul 15, 2024

View reviewed changes

madhu-pillai force-pushed the test_cex branch from 166ca24 to 0551402 Compare July 16, 2024 06:18

prestist reviewed Aug 5, 2024

View reviewed changes

madhu-pillai force-pushed the test_cex branch from 0551402 to 3ec0f5c Compare August 29, 2024 06:22

madhu-pillai added a commit to madhu-pillai/ignition that referenced this pull request Aug 30, 2024

config/doc/ignition.yaml: correction in description

570383f

discussion: coreos/butane#536 (comment)

madhu-pillai mentioned this pull request Aug 30, 2024

config/doc/ignition.yaml: correction in description coreos/ignition#1930

Merged

madhu-pillai added a commit to madhu-pillai/ignition that referenced this pull request Aug 30, 2024

config/doc/ignition.yaml: correction in description

0d91393

Docs rephrase in luks.cex.enabled description. coreos/butane#536 (comment)

madhu-pillai added a commit to madhu-pillai/ignition that referenced this pull request Sep 16, 2024

config/doc/ignition.yaml: correction in description

d11012b

Docs rephrase in luks.cex.enabled description. coreos/butane#536 (comment)

madhu-pillai and others added 2 commits October 22, 2024 11:31

add: Support LUKS encryption using IBM CEX secure keys on s390x

cdd101e

madhu-pillai force-pushed the test_cex branch from 3ec0f5c to f6f2964 Compare October 22, 2024 06:46

prestist requested changes Nov 11, 2024

View reviewed changes

prestist mentioned this pull request Nov 11, 2024

Release Butane 0.23.0 #550

Open

40 tasks

madhu-pillai requested a review from prestist November 13, 2024 10:24

		* _cex_ (object): describes the IBM Crypto Express (CEX) card configuration for the luks device.
		* _enabled_ (boolean): whether or not to use a CEX secure key to encrypt the luks device.

	var (
	// See also validateRHCOSSupport() and validateMCOSupport()
	fieldFilters = cutil.NewFilters(result.MachineConfig{}, cutil.FilterMap{
	// UNPARSABLE, REDUNDANT
	"spec.config.kernelArguments": common.ErrKernelArgumentSupport,
	// IMMUTABLE
	"spec.config.passwd.groups": common.ErrGroupSupport,
	// TRIPWIRE
	"spec.config.passwd.users.gecos": common.ErrUserFieldSupport,
	// TRIPWIRE
	"spec.config.passwd.users.groups": common.ErrUserFieldSupport,
	// TRIPWIRE
	"spec.config.passwd.users.homeDir": common.ErrUserFieldSupport,
	// TRIPWIRE
	"spec.config.passwd.users.noCreateHome": common.ErrUserFieldSupport,
	// TRIPWIRE
	"spec.config.passwd.users.noLogInit": common.ErrUserFieldSupport,
	// TRIPWIRE
	"spec.config.passwd.users.noUserGroup": common.ErrUserFieldSupport,
	// TRIPWIRE
	"spec.config.passwd.users.primaryGroup": common.ErrUserFieldSupport,
	// TRIPWIRE
	"spec.config.passwd.users.shell": common.ErrUserFieldSupport,
	// TRIPWIRE
	"spec.config.passwd.users.shouldExist": common.ErrUserFieldSupport,
	// TRIPWIRE
	"spec.config.passwd.users.system": common.ErrUserFieldSupport,
	// TRIPWIRE
	"spec.config.passwd.users.uid": common.ErrUserFieldSupport,
	// IMMUTABLE
	"spec.config.storage.directories": common.ErrDirectorySupport,
	// FORBIDDEN
	"spec.config.storage.files.append": common.ErrFileAppendSupport,
	// redundant with a check from Ignition validation, but ensures we
	// exclude the section from docs
	"spec.config.storage.files.contents.httpHeaders": common.ErrFileHeaderSupport,
	// IMMUTABLE
	// If you change this to be less restrictive without adding
	// link support in the MCO, consider what should happen if
	// the user specifies a storage.tree that includes symlinks.
	"spec.config.storage.links": common.ErrLinkSupport,
	})
	)

	// Return FieldFilters for this spec.
	func (c Config) FieldFilters() *cutil.FieldFilters {
	return &fieldFilters
	}

add: Support LUKS encryption using IBM CEX secure keys on s390x #536

Are you sure you want to change the base?

add: Support LUKS encryption using IBM CEX secure keys on s390x #536

Conversation

madhu-pillai commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madhu-pillai Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madhu-pillai commented Jul 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madhu-pillai commented Jul 14, 2024

madhu-pillai commented Jul 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestist Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madhu-pillai commented Jul 16, 2024

prestist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madhu-pillai commented Oct 22, 2024

madhu-pillai commented Oct 28, 2024

prestist commented Nov 11, 2024 • edited Loading

prestist left a comment

Choose a reason for hiding this comment

madhu-pillai commented Nov 12, 2024

madhu-pillai Jul 4, 2024 •

edited

Loading

prestist Jul 15, 2024 •

edited

Loading

prestist commented Nov 11, 2024 •

edited

Loading