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

Custom UEFI certificates - API change proposal #5245

Open
stormi opened this issue Nov 15, 2023 · 6 comments
Open

Custom UEFI certificates - API change proposal #5245

stormi opened this issue Nov 15, 2023 · 6 comments

Comments

@stormi
Copy link
Contributor

stormi commented Nov 15, 2023

Requirements

In XCP-ng, we would like to offer the ability to install custom UEFI certificates directly through XAPI, as was the case in 8.2, without requiring to modify xapi.conf on all hosts. This is for user convenience on one hand, but also because we are not sure yet whether we can distribute the UEFI certificates in our RPMs without adding restrictions of use and/or of distribution to the XCP-ng as a whole, which would make XCP-ng non-free by FLOSS standards.

When no custom certificates are set, then the default certificates should be used, exactly as what would happen if support for custom certificates were not enabled. Users would be able to set custom certificates, but also to remove them, which would go back to using the defaults.

The current implementation almost fulfills these requirements, but has a few drawbacks for us:

  • When a user switches from override-uefi-certs=false to override-uefi-certs=true in xapi.conf, the Pool.uefi_certificates field begins with a non-empty value (written there by XAPI based on the master host's disk contents). Thus, XAPI believes that there already are custom certificates in the DB, and won't sync again with the defaults, were them to be updated in the future. However, as we intend to enable override-uefi-certs at installation time, we will likely avoid this issue, unless a user changes the value to false and then back to true. And we will have to tell existing testers of XCP-ng 8.3 that they must clear the custom certificates, and it's almost guaranteed that some will not see the message.
  • When no custom certificates are set but overriding is supported, /var/lib/varstored contains a copy of the default certificates, not a direct symlink to them. Thus, if the default certificates would get updated, the contents of /var/lib/varstored would get out of sync.
  • Once override-uefi-certs is true (and this was the default value we intended for XCP-ng 8.3), the contents of Pool.uefi_certificates doesn't not always describe the actual certificates in use. When the default certificates are used, that is no custom certificates are set, the getter will return an empty value, which means "no custom certificates", but doesn't allow to check the value of the default certificates that are used instead.

These issues all stem from the fact that there is only one DB field (Pool.uefi_certificates) for several different needs.

Here's a proposed update to the API which builds on the existing, but separates the values for custom certificates and default certificates in the database. I believe it would not change the behaviour in XenServer's context.

xapi.conf

  • rename the override-uefi-certs parameter to allow-custom-uefi-certs, which better describes what we need it to do when it's true.
  • similarly, rename override_uefi_certs to allow_custom_uefi_certs in the source code.

Proposed API changes

On the Pool object:

  • New field: Pool.custom_uefi_certificates. Same structure as the existing Pool.uefi_certificates (contains a tarball or is empty).
    • Messages:
      • Pool.get_custom_uefi_certificates.
      • Pool.set_custom_uefi_certificates. Returns an error if allow_custom_uefi_certs is false.
  • Ideally, we would also rename Pool.uefi_certificates to Pool.default_uefi_certificates. However, renaming database fields is not a simple operation in XAPI, so we'll keep the original field.
    • Messages
      • Pool.get_uefi_certificates:
        • when allow_custom_uefi_certs is false OR Pool.custom_uefi_certificates is empty: return the contents of Pool.uefi_certificates
        • when allow_custom_uefi_certs is true AND Pool.custom_uefi_certificates is not empty
      • Pool.set_uefi_certificates: always returns an error (or removed if doable. We don't need to keep it for backward compatibility in XCP-ng)

As far as XenServer is concerned, where allow_custom_uefi_certs is false, the API doesn't change:

  • Pool.get_uefi_certificates keeps the same signature and returns the same data.
  • Pool.set_uefi_certificates can be kept if necessary (and return an error like it does today), or completely dropped.

Proposed relation between XAPI and the filesystem

  1. When XAPI starts on the master host, Pool.uefi_certificates is updated based on the contents of /usr/share/varstored, regardless of the value of allow_custom_uefi_certs. This is simplified when compared with the previous behaviour, because there now is a dedicated field in the database to contain the custom certificates, if any.
  2. On XAPI startup, on pool join for a new host, and when (Pool.set_custom_uefi_certificates is called AND allow_custom_uefi_certs is true):
  • If allow_custom_uefi_certs is false:
    • Ensure /var/lib/varstored is a symlink to /usr/share/varstored (unchanged from previous behaviour)
  • If allow_custom_uefi_certs is true AND Pool.custom_uefi_certificates is empty or is not a valid value:
    • Ensure /var/lib/varstored is a symlink to /usr/share/varstored (no custom certs: keep/restore the symlink)
  • If allow_custom_uefi_certs is true AND Pool.custom_uefi_certificates is a valid value:
    • Create the /var/lib/varstored directory if needed
    • Remove any existing files in /var/lib/varstored
    • Extract the tarball to /var/lib/varstored

Database upgrade

Just creation of the new field.

@psafont
Copy link
Member

psafont commented Nov 15, 2023

This looks like the design we discussed last week. It's adding a new field, which is not used when the per-host setting is false, and it even simplifies the case. This looks good to me, and I'd be glad to review PRs implementing this, @mg12 might want to add comments before implementation starts

@edwintorok
Copy link
Contributor

If you add a new field then is the xapi.conf entry still needed, can it be completely removed to reduce the combinatorial matrix of codepaths to test?
By default that new field would be empty, and as long as the user doesn't override it, they'll be fine.

If they make an API call (or 'xe' cli call) then we'll apply the semantics of that field (maybe for a short while we need to do something on upgrades to still recognize the old xapi.conf field).
i.e. treat 'allow_custom_uefi_certs' as always true even in XenServer, it is just that in xenserver we'd leave the field to be empty.

That seems to match the semantics you wanted? i.e. these 2 options are the same

If allow_custom_uefi_certs is false:
Ensure /var/lib/varstored is a symlink to /usr/share/varstored (unchanged from previous behaviour)
If allow_custom_uefi_certs is true AND Pool.custom_uefi_certificates is empty or is not a valid value:
Ensure /var/lib/varstored is a symlink to /usr/share/varstored (no custom certs: keep/restore the symlink)

@stormi
Copy link
Contributor Author

stormi commented Nov 15, 2023

@edwintorok Yes. The reason why this setting exist is because XenServer didn't want to allow installing custom certificates and XCP-ng needed to. I'm fine with either keeping or removing the setting.

@edwintorok
Copy link
Contributor

edwintorok commented Nov 15, 2023

I thought the difference was that we installed some certificates by default (and the method that we used to install them changed over time), whereas xcp-ng got 0 certificates by default and the user had to install them.

I don't think that having the ability to install custom certificates in XenServer would be bad, it might be a useful feature, but it isn't something we'd advertise in the UI, or (initially) support customers to do, and because we don't strictly need it, we likely wouldn't test it.
But just like with all the other experimental and unsupported features in XAPI if someone from the CLI does it then if they break it is their responsibility to fix.

If we can keep the code simpler by having just 2 code paths (XenServer default and Xcp-ng default) instead of 3 or 4, then that might be better

@edwintorok
Copy link
Contributor

cc @mg12 who worked on the original flag in XS for this.

@stormi
Copy link
Contributor Author

stormi commented Nov 15, 2023

maybe for a short while we need to do something on upgrades to still recognize the old xapi.conf field

If it's just ignored when present, without causing XAPI to fail, that's enough for me. Only beta-testers of XCP-ng 8.3 are concerned on our side, and we override xapi.conf when the defaults change in it (this is because defaults in XAPI code are tailored for XenServer, so xapi.conf is our way to set our XCP-ng settings. Users are redirected to xapi.conf.d if they need to add or change a setting).

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

No branches or pull requests

3 participants