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.

Sign up for GitHub

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

Jump to bottom

Add new insert and remove subcommand #23

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Add new insert and remove subcommand #23

wants to merge 14 commits into from

Conversation

nick-child-ibm
Copy link
Collaborator

In response to Nageswara's recommendation, this pull request adds a new secvarctl subcommand: insert. secvarctl insert allows for a new secvar update to be submitted without the old one being replaced. The use case is if the user would like to add an ESL without overwriting the previous secvar ESLs.

This PR comes complete with implementations, tests and documentation for the new secvarctl insert command.

Okay, so I sorta-kinda lied when I said "secvarctl insert allows for a new secvar update to be submitted without the old one being replaced" . In reality, the function gets the current ESLs and appends the new ESL to it. With the combined ESL, it is sent to be put into an auth file. With the auth file, secvarctl insert allows the user to use the -w flag to submit the update to secvar sysfs or to a file with -o <outFile>. So, to simplify, the function really takes in two ESL's-> appends them -> generate auth -> ouptut . Various flags were added for different use cases. For example, by default the current ESL is taken from secvar sysfs /sys/firmware/secvar/vars/<varName>/data but the user can use -e <eslFile> to specify the current ESL. Additionally, the -p PATH flag can be used to specify the sysfs where it writes (to PATH/<varName>/update) and reads (to PATH/<varName>/data

Special thanks to @nasastry for a great feature request! I promise secvarctl remove is coming soon...

@nick-child-ibm
Copy link
Collaborator Author

secvarctl insert implemented and has test suite. secvarctl remove implemented but holding off on writing tests until I get the go ahead on general approach and arrguments (using X509 serial number as ESL identifier). Also still have to implement insert and remove for dbx updates.

@nick-child-ibm nick-child-ibm changed the title Add new insert subcommand Add new insert and remove subcommand Jul 20, 2021
@daxtens
Copy link
Contributor

daxtens commented Aug 26, 2021

Do you think there's any value in teaching skiboot about EFI_VARIABLE_APPEND_WRITE? That would save you doing the current read-modify-write loop. (I'm also hoping to support it in libstb-secvar.)

I don't think there's any avoiding a RMW loop in removing an entry.

Is there a specific sort of review you're looking for on this PR? it sounds like you've still got a bit of development to go, did you want an eye cast over what you've got at the moment?

@nick-child-ibm
Copy link
Collaborator Author

Do you think there's any value in teaching skiboot about EFI_VARIABLE_APPEND_WRITE?

Surely! But I worry it would require too much testing and effort from everyone to change the firmware. From a user perspective, I don't know if it would look any different. It would certainly simplify things on secvarctl's side but I can't say that the skiboot code would be that open to new API's. I see what you are saying though, this solution is a bit hacky and is not a true "insert" / "deletion". BUT! I think these two commands could be the most useful for an average user so I definitely want to have them in some form.

Is there a specific sort of review you're looking for on this PR?

Just wanted to make sure you are not totally against the idea of RMW approach. You are right that I am not done with implementation yet. dbx insert/remove needs to be written still. This one is going to be more difficult since I think, for hostboot, the dbx has one ESL header with multiple entries (might be wrong on this). I also wanted to make sure removing an PK, KEK, db entry by the serial number in the x509 is not outrageous before I make a test suite for it.

As always, thanks for the review!

@nick-child-ibm
edk2-svc-generate.c: Add child arg parser for generating signed files
c25e51d 
This commit adds a child parser with argp's `struct argp_child` to `secvarctl generate`. All options specific to generating a Auth or PKCS7 have been moved here. The goal is to allow future secvarctl subcommands to use the same arg parsing functions without having duplicate code.

Signed-off-by: Nick Child <nick.child@ibm.com>
@nick-child-ibm
edk2-svc-read/generate: Further exposure of functions for future subc…
ccdc833 
…ommands

This commit exposes more functions from edk2-svc-generate.c by adding them to edk2-svc.h. This will allow future subcommands to use these funtions, these mainly revolve around the ability to generate an auth file.

Additionally, `readFileFromSecVar` from edk2-svc-read.c was broken down into two functions so that the data in a secvar could be returned easily with the new `getDataFromSecVar` function. This function was also declared globally so future subcommands can make use of it.

Signed-off-by: Nick Child <nick.child@ibm.com>
@nick-child-ibm
Add new command insert
0c1e9cf 
This commit adds the initial implementation of the 6th secvarctl command,
`insert`. This general idea is that this command can add a new ESL to the
current ESL chain in a secure variable. Since the data is just appended
single-entry ESL's, all we really need to do is append the new data to the
current data and generate an auth file over the combined data buffers.

The command requires a new esl (with '-i'), a varname (with '-n') and auth
generation info for signing (at least one pair of signers with '-k'/'-x',
'-c'). It also requires either an '-w' or '-o' flag for either writing the
output data to the secvar sysfs `update` file or to a user defined output
file, respectively. Similarly, the command can read the current esl (the one
that is being appended to) from the secvar sysfs `data` file (default) or
from a user defined input file (with '-e'). The path to the sysfs is /sys/
firmware/secvar/vars/ by default but can be changed with '-p' option. A few
other flags like '-f', '-t', '-h' have also been added. Still to be added is
documentation and tests.

Signed-off-by: Nick Child <nick.child@ibm.com>
@nick-child-ibm
edk2-svc-insert.c: remove -h flag
118f927 
This commit removes the option to specify a hash algorithm. Originally, I thought it would be useful for validating an ESL that countained a hash but after looking at validateESL(), the hash funtion is not needed. In effect this commit is just removing dead code.

Signed-off-by: Nick Child <nick.child@ibm.com>
@nick-child-ibm
runSvcGenerateTests.py: Add tests for edk2-svc-insert
248be25 
This commit adds test cases for `secvarctl insert` to runSvcGenerateTests.py. After running coverage tests, these tests result in 96% coverage of edk2-svc-insert.c

Signed-off-by: Nick Child <nick.child@ibm.com>
@nick-child-ibm
update README and man page with insert info
fa7b543 
This commit adds documentation for the new `secvarctl insert` command. Additionally, the version and date of the man page has been updated. The version changed from 0.1 to 0.3 since 0.2 has alraeady been released and SOMEONE (me) forgot to update it.

Signed-off-by: Nick Child <nick.child@ibm.com>
@nick-child-ibm
edk2-svc-insert.c: make argp child parser for future remove command
d8ded3e 
This commit factors out some argp parsing for future implementations of the `remove` command. Since, `remove` should essentially do the opposite as `insert`, it will take some of the same command line options. To avoid reusing code, we use `arg_child` structs again. This strategy was previously used with sharing flags between `secvarctl generate` and `secvarctl insert`.

Admitedly, this is extra confusing since the new shared parser for `insert` and `remove` has its own child parser for giving auth flags for signing. It makes use of the ability for a child parser to have a child parser. Think of these child parsers as the union of mulitple sets of command line options.

Signed-off-by: Nick Child <nick.child@ibm.com>
@nick-child-ibm
edk2-svc-insert.c: Expose required options in help message
2a52764 
Previously, commmand line options like '-o', '-i' and '-w' were hidden from the `secvactl insert --help` message. This was because they weren't exactly an option (since they were mandatory), so we marked them with the flag `OPTION_HIDDEN`. But this flag has been deemed unnessary because some description of the command line argument should be given and it should not be restrained in the help message. We do not want to treat them as normal options because then they will appear twice in `secvarctl insert --usage`, once in the list of options and again in the requirments. We will now use the flag OPTION_NO_USAGE, which will display the flag in the help message but not in the list of options when using `secvarctl insert --usage`.

Note: It would be nice to also label the required flags for auth signing {-n, -s/-k, -c} as OPTION_NO_USAGE. But since `secvarctl generate` argp parsers also using these flags through argp child parsers, we will leave them as regular options. This conclusion was because it is likely better to have those flags appear twice in `secvarctl insert --usage` as a usage and a requirement than to have them not appear in `secvarctl generate --usage` at all.

Signed-off-by: Nick Child <nick.child@ibm.com>
@nick-child-ibm
edk2-svc-insert: MAYBE SQUASH Don't allow insert or remove of dbx
2b8b7dd 
This commit removes the ability to insert dbx entries. Currently the dbx is being read as multiple ESL's each w/ one entry. I have heard about the possibility of the dbx being one ESL with multiple entries. It has been awhile since I checked in on this. I need to get confirmation of what the dbx format is. It is very possible this commit will be squashed. For now, it is muche easier to implement these functions without having to worry about the dbx so this commit just throws an error if the variable veing edited is the dbx

Signed-off-by: Nick Child <nick.child@ibm.com>
@nick-child-ibm
esk2-svc-insert.c: Implement secvarctl remove command
87202c8 
This commit is the initial implementation of `secvarctl remove` the purpose of this command is to remove specific entries to an esl chain and generate
a new auth file with the remaining data. This is very similar to the `insert` command. The only difference in arguments is that `insert` accepts a new ESL file with `-i <file>` and `remove` accepts a serial number with `-x <sn>`. The serial number is from an X509 contained in the ESL chain.

Signed-off-by: Nick Child <nick.child@ibm.com>
@nick-child-ibm
runSvcGenerateTests.py: Add tests for remove command
ff09187 
This commit adds tests to the python script for the new `secvarctl remove
command`. The test works by making a KEK with many ESL entries. We then
generate a db auth file signed with an entry in the KEK and use `secvarctl
verify` to prove that an update would be successful. After using `secvarctl
remove` to remove the KEK entry, we see that the same db update would now
fail (since it's signer has been removed from the KEK). We continue through
this process until the KEK is empty.
@nick-child-ibm
Allow insert command on dbx and add tests
352be4c 
Previously, the dbx was not allowed to be used in a `secvarctl
insert` command. After reconsidering, since a dbx is still just
appended ESL's the insert opperation on a dbx is no different
than with the other secure variables.
To prove this point, tests were added to the test script.
NOTE: dbx `remove` opperations will not work

Signed-off-by: Nick Child <nick.child@ibm.com>
@nick-child-ibm
Allow remove to work with dbx and add tests
d6433bd 
This commit allows for `secvarctl remove` to work properly
with the dbx. Since the dbx contains hashes instead of x509's,
we will use the hash values to identify the ESL that the user
would like to remove. For this reason, in the secvarctl command,
if the `-n <varName>` flag is the dbx, then the `-x XX:XX:...XX`
flag should contain the hash. If not dbx, then `-x` should still
be the serial number.

To prove the functionality, dbx test cases have been added to the
remove tests.

Signed-off-by: Nick Child <nick.child@ibm.com>
@nick-child-ibm
Update documentation for insert/remove commands
33f1834 
This commit updates README and the man page for proper usage
of the newly implemented commands `secvarctl insert` and
`secvarctl remove`

Signed-off-by: Nick Child <nick.child@ibm.com>
@nick-child-ibm
Copy link
Collaborator Author

Okay, I have finally got back to this. Thanks for all your patience. Updates are as follows:

  1. rebased on main and fix conflicts
  2. added test cases for secvarctl remove
  3. implemented insert/remove for the dbx (insert works the same as the other variables, remove not so much)
  4. add test cases for secvarctl insert/remove when working with dbx
  5. update documentation

The way remove is currently working is to use the -x <serial_number> flag to identify an ESL that is to be removed from a chain of ESL's. The <serial_number> must be in the format XX:XX:XX...:X with 20 bytes of hex data and be contained in the X509 of an ESL. This value can be taken from reading and ESL with secarctl read or secvarctl -v -e <esl_file>. I could not think of an easier way to identify the ESL to remove from the chain, besides indexing them but that feels like a recipe for index-at-0 vs index-at-1 user issues. Anways, I had figured the serial number would be sufficient BUT the RFC says that it technically does not need to be 20 bytes (it just has to have a max of 20 bytes) AND it is only a unique value between it and it's CA's. But who is really going to generate there own X509's (openssl generates with 20 bytes and does not allow for custom values) ? right? right? Looking for more thoughts on this. Also, since the dbx does not contain x509's, the -x <serial_number> is interpreted as -x <hash_value> and uses the same XX:XX...X format to identify a hash contained in an ESL of the dbx's ESL chain.

Looking forward to more feedback!
Thanks!
Nick

@nick-child-ibm nick-child-ibm mentioned this pull request Sep 20, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nick-child-ibm @daxtens