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

Create Secureboot_uefi_key_management.md #1451

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

sacnaik
Copy link

@sacnaik sacnaik commented Aug 28, 2023

Secure boot UEFI key management HLD

Secure boot UEFI key management HLD
@zhangyanzhao
Copy link
Collaborator


![image](https://github.com/sacnaik/SONiC/assets/25231205/a42cd039-0675-4a71-84e0-bce00eb2ca9d)

### 5.2 OVMF based platform can use Linux implementation to access UEFI variable
Copy link
Contributor

Choose a reason for hiding this comment

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

can you clarify the cisco code contribution here?

Copy link
Author

Choose a reason for hiding this comment

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

The HLD is referred to as OVMF since it is Open firmware and available for someone to experiment with. Cisco BIOS has implemented a similar approach to OVMF. When Cisco8000 implements SONiC UEFI key management for Cisco hardware similar code will be added under Cisco platform-specific code. That means the common contribution here is the SONiC CLI and Python module defining the base class that is mentioned in the document.

Copy link

@mheese mheese left a comment

Choose a reason for hiding this comment

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

Great to see some progress in SONiC around UEFI Secure Boot features!

Please see my inline comments, but I also have some general comments:

  • enrolling and managing custom keys for Secure Boot is probably the pinnacle of security in this area, and the hardest to achieve for average users, yet these commands are really only helpful for that specific use-case
  • using custom keys particularly in db can have dire consequences particularly if SecureBoot=1
  • the system should also provide access to the SecureBoot variable
  • if SecureBoot=1, there should be protection of the boot chain: if db or dbx get updated, the system should ensure that the current boot entries are actually still going to work. As you mentioned updating these databases requires a reboot, but the shim of a normal SONiC installation will need to be signed correctly as well for this to still work. So either there should be a check of at least the SONiC OS boot entry against the db/dbx before updating it, or maybe the system should refuse to reboot otherwise (?) --> this requires some more discussion IMHO
  • the same goes for SONiC updates, they should fail if an update leads to installing a shim which is not going to be able to boot any longer
  • I understand that the idea for specific SONiC CLI commands is to abstract potentially platform specific implementations away. However, would you mind providing examples where different implementations would be necessary? At least on x86 I would have yet to encounter hardware where the standard tooling would be insufficient.

2. platform security uefi variables update kek <file>
3. platform security uefi variables update db <file>
4. platform security uefi variables update dbx <file>
```
Copy link

Choose a reason for hiding this comment

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

a couple of comments here:

  • can we call the subcommand secureboot instead of uefi? This is more fitting as these commands are all specifically dealing with secureboot variables, and not other UEFI variables
  • You outlined really well all the steps which are necessary in order to create an authenticated variable file. Honestly, this is the hard part, while running a command to update the variable will on most platforms simply translate to an efi-updatevar command or by direct access through efivarfs. It would be nice to see an easy UX here
  • I'm missing access to the general SecureBoot variable which can get information if UEFI Secure Boot is enabled or not on the platform

cat db-orig-1.esl db-orig-3.esl > DB-new.esl
7. Create authenticated variable and sign it
sign-efi-sig-list -g "<GUID>" -k KEK.key -c KEK.crt db DB-new.esl DB-new.auth
8. Copy DB-new.auth to the system and use CLI to update UEFI database
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the command used here?
Can you copy the DB-new.auth without providing the KEK.key to the system?

Copy link

Choose a reason for hiding this comment

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

I believe the idea is to perform the signing outside of the system, and then in (8) only copy the new file to the system where you can then update it with the CLI

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand, but there might be UEFI cases where you cannot upload a new entry to the UEFI without providing the private KEK key (worth checking if this is true, when I played with these items a few years ago this was the case).
Or if you are entering a new KEK, you'll need the PK.

```

## 7 Test considerations
The sonic-mgmt can cover UEFI key management such as adding, updating, removing, and revoking UEFI keys.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add a note that this feature should enhance secure boot tests where we can install an image with a specific key, then move it to the dbx and therefor - the same image cannot be installed anymore (signed with the previous key).


````

### 4.4 Revoke keys
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth mentioning that key revocation is a potentially destructive action.
If we are removing a DB entry that is currently being used - this will cause the system to enter fatal mode (recoverable only via UEFI).

Copy link

Choose a reason for hiding this comment

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

totally agree. See also my comment above about protection of boot chain.

The platform plugin for this OVMF type UEFI can use either sysfs or efi-readvar and efi-updatevar to implement getVariable() and setVariable() methods.


## 6 SONiC CLI commands
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the potential return values here?
Updatevar might fail (it being immutable, specific platform constraints, etc').
What is the expected design for these type of commands? As I see that they only receive a file and perform an action.

@abdosi
Copy link
Contributor

abdosi commented Sep 28, 2023

@xumia for reference

@zhangyanzhao
Copy link
Collaborator

@sacnaik can you please add the code PRs to this HLD by referring to #806? Thanks.

@zhangyanzhao
Copy link
Collaborator

code PR is not ready, move to backlog for future release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: MovedToBacklog
Development

Successfully merging this pull request may close these issues.

6 participants