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

Parse lists from configuration file #604

Merged
merged 6 commits into from
Jun 26, 2023
Merged

Parse lists from configuration file #604

merged 6 commits into from
Jun 26, 2023

Conversation

ansasaki
Copy link
Contributor

@ansasaki ansasaki commented Jun 13, 2023 •
edited
Loading

Support lists for trusted_client_ca and revocation_actions in more flexible formats, including the format described in the configuration specification.

The lists in trusted_client_ca and revocation_actions can be:

  • comma separated
  • space separated
  • new line separated
  • within square brackets or not
  • with quoted strings or not

Fixes: #440, #455, #489

NOTE: this adds pest as a new dependency

@keylime-bot keylime-bot added feature_request enhancement configuration Involves changes to configuration file format labels Jun 13, 2023
@ansasaki ansasaki force-pushed the parse_list branch 2 times, most recently from c716b3d to 0765f9b Compare June 13, 2023 13:34
aplanas
aplanas approved these changes Jun 15, 2023
View reviewed changes
keylime-agent/src/config.rs Outdated Show resolved Hide resolved
ueno
ueno reviewed Jun 15, 2023
View reviewed changes
keylime-agent/src/config.rs Outdated Show resolved Hide resolved
@kkaarreell
Copy link
Contributor

@ansasaki Would it be reasonable to update our test so it doesn't use bundled CA certificates?
https://github.com/RedHat-SP-Security/keylime-tests/blob/main/functional/basic-attestation-with-custom-certificates/test.sh#L94

@ansasaki
Copy link
Contributor Author

@ansasaki Would it be reasonable to update our test so it doesn't use bundled CA certificates? https://github.com/RedHat-SP-Security/keylime-tests/blob/main/functional/basic-attestation-with-custom-certificates/test.sh#L94

Yes, it makes sense, I'll update the test and use a temporary commit to use a separate branch for tests

@ansasaki ansasaki marked this pull request as draft June 16, 2023 12:05
@codecov
Copy link

codecov bot commented Jun 19, 2023 •
edited
Loading

Codecov Report

Merging #604 (14823c2) into master (4a59942) will decrease coverage by 0.01%.
The diff coverage is 72.61%.

❗ Current head 14823c2 differs from pull request most recent head ec8a188. Consider uploading reports for the commit ec8a188 to get more accurate results

Additional details and impacted files
Flag Coverage Δ
e2e-testsuite 59.48% <72.22%> (-0.17%) ⬇️
upstream-unit-tests 60.05% <75.92%> (+1.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
keylime-agent/src/error.rs 9.28% <0.00%> (-0.19%) ⬇️
keylime-agent/src/main.rs 35.80% <0.00%> (ø)
keylime-agent/src/registrar_agent.rs 97.67% <ø> (ø)
keylime-agent/src/crypto.rs 80.34% <82.60%> (-0.25%) ⬇️
keylime/src/list_parser.rs 83.33% <83.33%> (ø)
keylime-agent/src/config.rs 88.58% <100.00%> (+0.23%) ⬆️
keylime-agent/src/revocation.rs 71.47% <100.00%> (-1.04%) ⬇️

@ansasaki ansasaki marked this pull request as ready for review June 20, 2023 09:57
This was referenced Jun 20, 2023
Merged
Closed
ueno
ueno reviewed Jun 21, 2023
View reviewed changes
keylime/src/list_parser.rs Outdated Show resolved Hide resolved
keylime/src/list_parser.rs Outdated Show resolved Hide resolved
keylime/src/list_parser.rs Outdated Show resolved Hide resolved
@ansasaki ansasaki force-pushed the parse_list branch 2 times, most recently from 775607e to 03e7cd9 Compare June 23, 2023 13:48
ueno
ueno approved these changes Jun 23, 2023
View reviewed changes
keylime/src/list_parser.rs
/// * `'a', 'b', 'c'`
/// * `[a b c]`
/// * `['a', "b", c]`
///
Copy link
Contributor

Choose a reason for hiding this comment

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

This interface looks a bit too generic to be exposed from the keylime lib crate. Not necessarily in this PR, but we might eventually want to move the entire configuration handling in the lib crate with access methods to retrieve list of certs, actions, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed a bug in the way I parsed the strings: I cannot remove the quotes from quoted strings. I made the changes to keep the quotes.

@ansasaki ansasaki added the dependencies Pull requests that update a dependency file label Jun 23, 2023
ansasaki added 6 commits June 23, 2023 18:47
@ansasaki
lib: Implement parser for lists from config file
c2a4973 
The parser accepts more flexible formats for the lists read from the
configuration file. The lists can:

* be inside square brackets or not ("[]")
* be separated by comma, spaces, new line, tabs, or carriage-return
* have elements single-quoted, double-quoted, or unquoted

If the string is single or double quoted, the quotes are kept in the
output Vec<&str>.

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
@ansasaki
config: Make trusted_client_ca to accept lists
6e9984d 
Make the 'trusted_client_ca' option to accept a list.  The list is
parsed with the keylime::list_parser.

Fixes: keylime#455

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
@ansasaki
config: Add unit test for the list to files mapping
1df41c3 
Emulate what is performed when parsing a list of files from the
configuration file.

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
@ansasaki
crypto: Make internal functions private
9086791 
For functions used for testing, expose on a testing submodule.

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
@ansasaki
crypto: Add unit tests for x509 functions
6d8019c 
Add unit tests for functions that manipulate certificates.

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
@ansasaki
revocation: Parse revocation actions flexibly
ec8a188 
Make the accepted list format more flexible by parsing with the
keylime::list_parser.

Fixes: keylime#489, keylime#440

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
@ansasaki ansasaki force-pushed the parse_list branch 2 times, most recently from 14823c2 to ec8a188 Compare June 26, 2023 08:01
@ansasaki
Copy link
Contributor Author

ansasaki commented Jun 26, 2023 •
edited
Loading

/packit retest

@ansasaki
Copy link
Contributor Author

I removed the temporary commit that changed the source of tests. There is an ongoing outage on packit/testing farm which made the tests to not be executed after that, but I'll merge based on the previous test results (which included the test with the workaround removed).

@ansasaki ansasaki merged commit a780c6d into keylime:master Jun 26, 2023
@ansasaki ansasaki deleted the parse_list branch July 5, 2023 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aplanas aplanas aplanas approved these changes

@ueno ueno ueno approved these changes

@maugustosilva maugustosilva Awaiting requested review from maugustosilva

@lukehinds lukehinds Awaiting requested review from lukehinds

@THS-on THS-on Awaiting requested review from THS-on

@ashcrow ashcrow Awaiting requested review from ashcrow

@lkatalin lkatalin Awaiting requested review from lkatalin

@sergio-correia sergio-correia Awaiting requested review from sergio-correia

Assignees

@ashcrow ashcrow

@maugustosilva maugustosilva

@lukehinds lukehinds

@THS-on THS-on

@lkatalin lkatalin

@sergio-correia sergio-correia

Labels
configuration Involves changes to configuration file format dependencies Pull requests that update a dependency file enhancement feature_request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rust agent and python agent differently parse revocation scripts in action list