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

config/internal: add clevis override support #1031

Merged
merged 3 commits into from
Sep 18, 2020

Conversation

arithx
Copy link
Contributor

@arithx arithx commented Jul 16, 2020

Adds new override options that allow for the direct specification of the
clevis pin & configuration JSON that will be passed to clevis luks bind.

Closes #1019

@arithx
Copy link
Contributor Author

arithx commented Jul 16, 2020

Open question: should validation fail if both overrides & regular clevis configuration options are specified? I'm leaning towards yes but wanted to ask. Conflicting configs can be written otherwise e.x.:

"clevis": {
    "tpm2": true,
    "threshold": 1,
    "override": {
        "pin": "tang",
        "config": "{\"url\": \"http://my.tang.server:8443\", \"thp\": \"ABCDEF123\"}",
        "netdev": true
    }
}

@jlebon
Copy link
Member

jlebon commented Jul 20, 2020

Hmm, I wonder if we should reframe #1019 as support for "custom" pins instead? So e.g.

"luks": [
    {
        "name": "luksroot",
        "device": "/dev/md/foobar",
        "clevis": {
            "tpm2": true,
            "custom": {
                "pin": "my-custom-pin",
                "config": "{\"custom-field\": \"custom-value\"}"
            }
        },
        "label": "root"
    }
],

would use sss with both tpm2 and my-custom-pin.

One thing we could do down the road then is that if threshold is 1 (or omitted) and there is only one pin type given, we don't use sss and just use the one pin directly. That way custom would effectively have full control over the inputs, instead of being nested within the "sss wrapper".

@jlebon
Copy link
Member

jlebon commented Jul 20, 2020

/cc @puiterwijk -- this is about the comment you initially raised here.

@arithx
Copy link
Contributor Author

arithx commented Jul 21, 2020

Hmm, I wonder if we should reframe #1019 as support for "custom" pins instead?

My personal (slight) leaning would be to keep it as a complete override.

@jlebon
Copy link
Member

jlebon commented Jul 22, 2020

My personal (slight) leaning would be to keep it as a complete override.

Sure, I also see the appeal in that. The reason I suggested it was so that users could still leverage Ignition's built-in tpm2 and tang support (and the validation that comes with that) and minimize the JSON in string in JSON bit.

In that case yes, I agree we should make it an error if any other pinning options are specified. (Though small bikeshed: I'd still call it e.g. custom instead of override since the latter slightly implies to me that it's legal to have it alongside e.g. tpm2 and tang and those will just be ignored.)

@arithx
Copy link
Contributor Author

arithx commented Jul 23, 2020

I've written up a few different potential paths, do either of 2 or 3 encapsulate your proposal?

  1. Complete override of the PIN & CFG (as is done in the current PR)

    • This is definitely the most straight forward approach code wise as we're just accepting string values and directly passing them to clevis
    • One drawback for this is that it makes the Ignition config potentially murky to read as even if other clevis fields are specified they are ignored
  2. A custom object that overrides individual CFGs for a given PIN

    • This would directly override Tang sections rather than appending & when an SSS config is given it will entirely override the config
  3. A custom list of objects that overrides TPM2 section, appends to Tang section, and merges the SSS CFG with the other custom objects & Ignition attrs

    • This might require Ignition to gain a greater understanding of all PIN CFG options to be able to properly merge SSS CFGs

@jlebon
Copy link
Member

jlebon commented Aug 19, 2020

Sorry for the delay on this.

I think just going with what you have here works. Two things:

  • I agree we should error out if tpm2 or tang is specified.
  • I think the key should be named e.g. custom instead of override. In my mind, override implies that tpm2 and tang are still allowed but just ignored. But it's clearer to say to users: "you can set up TPM2 and/or Tang or a completely custom config".

@arithx
Copy link
Contributor Author

arithx commented Aug 19, 2020

I agree we should error out if tpm2 or tang is specified.

If they're specified inside the config or if they're specified as the custom pin?

@jlebon
Copy link
Member

jlebon commented Aug 19, 2020

I agree we should error out if tpm2 or tang is specified.

If they're specified inside the config or if they're specified as the custom pin?

The former. In a custom pin users can then just do whatever they want.

@arithx arithx force-pushed the custom_clevis branch 5 times, most recently from b9dd472 to d2c982d Compare September 3, 2020 05:17
@arithx
Copy link
Contributor Author

arithx commented Sep 3, 2020

Ok, this is rebased, updated, and tested. Should be good for review.

Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM generally.

config/v3_2_experimental/types/luks.go Show resolved Hide resolved
doc/configuration-v3_2_experimental.md Outdated Show resolved Hide resolved
doc/configuration-v3_2_experimental.md Outdated Show resolved Hide resolved
doc/configuration-v3_2_experimental.md Outdated Show resolved Hide resolved
@arithx arithx force-pushed the custom_clevis branch 3 times, most recently from 7130bac to aecb0cc Compare September 17, 2020 07:02
@arithx
Copy link
Contributor Author

arithx commented Sep 17, 2020

Updated.

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice and simple! LGTM overall, just some minor nits.

config/shared/errors/errors.go Outdated Show resolved Hide resolved
config/shared/errors/errors.go Outdated Show resolved Hide resolved
@@ -144,6 +144,10 @@ The Ignition configuration is a JSON document conforming to the following specif
* **thumbprint** (string): thumbprint of a trusted signing key.
* **_tpm2_** (bool): whether or not to use a tpm2 device.
* **_threshold_** (int): sets the minimum number of pieces required to decrypt the device.
* **_custom_** (object): overrides the clevis configuration. The `pin` & `config` will be passed directly to `clevis luks bind`. If specified, all other clevis options must be omitted.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not new here so we can address this in a follow-up, but WDYT about using the proper noun "Clevis" (capitalized) in descriptions to match upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WFM

Adds new custom options that allow for the direct specification of the
clevis pin & configuration JSON that will be passed to `clevis luks
bind`.
@arithx
Copy link
Contributor Author

arithx commented Sep 18, 2020

Updated the error messages. Merging on green.

@arithx arithx merged commit 775433a into coreos:master Sep 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LUKS: support custom clevis configurations
3 participants