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

crypto: Improve error handling and move to library #736

Merged
merged 8 commits into from
Feb 22, 2024
Merged

crypto: Improve error handling and move to library #736

merged 8 commits into from
Feb 22, 2024

Conversation

ansasaki
Copy link
Contributor

@ansasaki ansasaki commented Feb 6, 2024 •
edited
Loading

Wrap every possible error in crypto.rs in a single error type CryptoError.
Moved AES_128_KEY_LEN, AES_256_KEY_LEN, and AES_BLOCK_SIZE definitions from common.rs to crypto.rs
Marked the source of the errors in tpm.rs
Move the crypto module from keylime-agent to the keylime library

Also add some auxiliary functions and tests for them:

  • x509_get_pubkey()
  • x509_to_pem()
  • x509_to_der()
  • tss_pubkey_to_pem()
  • hash()

Rename:

  • kdf() to pbkdf2()
  • load_x509() to load_x509_pem()

Bump ahash to version 0.8.7 to avoid compilation error.

Fixes #648

@ansasaki ansasaki marked this pull request as draft February 6, 2024 20:33
@ansasaki ansasaki force-pushed the improve_errors branch 4 times, most recently from fcd4674 to 663192f Compare February 7, 2024 13:00
@codecov Codecov
Copy link

codecov bot commented Feb 7, 2024 •
edited
Loading

Codecov Report

Attention: 337 lines in your changes are missing coverage. Please review.

Comparison is base (409d2e8) 65.13% compared to head (a895c85) 61.57%.
Report is 1 commits behind head on master.

Additional details and impacted files
Flag Coverage Δ
e2e-testsuite 58.09% <51.51%> (-0.88%) ⬇️
upstream-unit-tests 50.62% <44.67%> (-1.93%) ⬇️

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

Files Coverage Δ
keylime-agent/src/common.rs 67.94% <100.00%> (+0.24%) ⬆️
keylime-agent/src/keys_handler.rs 65.43% <ø> (+0.84%) ⬆️
keylime-agent/src/payloads.rs 82.84% <ø> (+0.49%) ⬆️
keylime-agent/src/quotes_handler.rs 50.86% <ø> (ø)
keylime-agent/src/registrar_agent.rs 98.11% <100.00%> (ø)
keylime-agent/src/revocation.rs 71.13% <100.00%> (+4.13%) ⬆️
keylime/src/list_parser.rs 83.33% <100.00%> (ø)
keylime/src/algorithms.rs 72.52% <80.00%> (+14.63%) ⬆️
keylime-agent/src/error.rs 15.25% <33.33%> (ø)
keylime-agent/src/main.rs 26.01% <25.00%> (-34.65%) ⬇️
... and 2 more

... and 5 files with indirect coverage changes

@ansasaki ansasaki changed the title WIP: Improve error handling crypto: Improve error handling Feb 7, 2024
@ansasaki ansasaki marked this pull request as ready for review February 7, 2024 14:23
@ansasaki
Copy link
Contributor Author

ansasaki commented Feb 7, 2024

I'm seeing unrelated failures on CI that I hope to be fixed by: RedHat-SP-Security/keylime-tests#550

@ansasaki ansasaki added the dependencies Pull requests that update a dependency file label Feb 7, 2024
@ansasaki ansasaki changed the title crypto: Improve error handling crypto: Improve error handling and move to library Feb 7, 2024
@ansasaki ansasaki force-pushed the improve_errors branch 3 times, most recently from 205c14f to 6610d39 Compare February 8, 2024 09:33
@ansasaki
Copy link
Contributor Author

ansasaki commented Feb 8, 2024

/packit retest-failed

1 similar comment
@ansasaki
Copy link
Contributor Author

ansasaki commented Feb 8, 2024

/packit retest-failed

@ansasaki ansasaki force-pushed the improve_errors branch 3 times, most recently from 4a40ff4 to 99c862e Compare February 13, 2024 15:58
ueno
ueno reviewed Feb 14, 2024
View reviewed changes
Copy link
Contributor

@ueno ueno left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It feels to me a bit overwhelming to have this level of granularity. For example, SignerNewError exactly maps to openssl::sign::Signer::new function. I can understand that this would be useful for diagnosing any issues at production environment, but perhaps this kind of matter could be better handled with structured logging (for example, maybe switching to the tracing crate from pretty_env_logger)?

keylime-agent/src/crypto.rs Outdated Show resolved Hide resolved
@ansasaki
Copy link
Contributor Author

It feels to me a bit overwhelming to have this level of granularity. For example, SignerNewError exactly maps to openssl::sign::Signer::new function. I can understand that this would be useful for diagnosing any issues at production environment, but perhaps this kind of matter could be better handled with structured logging (for example, maybe switching to the tracing crate from pretty_env_logger)?

For me, these are complementary approaches. My end goal is to use the anyhow crate to simplify the error handling in the application (keylime_agent), providing human readable context with backtrace on what caused the issue. I plan to add tracing support in future to make it possible to extract more insights like timing to diagnose bottlenecks.

@ansasaki ansasaki force-pushed the improve_errors branch 2 times, most recently from 55346a3 to 02fac6a Compare February 14, 2024 09:23
@ansasaki
Copy link
Contributor Author

My end goal is to use the anyhow crate to simplify the error handling in the application (keylime_agent), providing human readable context with backtrace on what caused the issue.

Couldn't that goal be achieved without introducing dedicated error codes for each call to fallible functions, e.g., SignNewError, SignUpdateError, etc., instead of a single SignError with a descriptive message?

Sure, we can use more generic error types with different error messages to achieve that.

I fail to see how having that is an improvement over having unique error types, but I can change it.

@ansasaki ansasaki force-pushed the improve_errors branch 2 times, most recently from 1448e9e to f1f7561 Compare February 15, 2024 10:30
@ansasaki
algorithms: Make errors more specific
9f1ddd9 
Also add unit tests for error cases

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
@ansasaki
list_parser: Add source for error for backtrace
e8d3ee4 
Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
@ansasaki
Copy link
Contributor Author

/packit retest-failed

@ansasaki
Copy link
Contributor Author

Hopefully, the issue with the Fedora 39 tests will be solved with RedHat-SP-Security/keylime-tests#553

@ansasaki
Copy link
Contributor Author

@ueno Could you please review this again?

@ansasaki ansasaki requested a review from aplanas February 21, 2024 14:18
aplanas
aplanas approved these changes Feb 21, 2024
View reviewed changes
Copy link
Contributor

@aplanas aplanas left a comment

Choose a reason for hiding this comment

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

For me this change makes sense.

I understand @ueno comments about that the errors are very specific, but I do not see any issue with this.

keylime-agent/src/payloads.rs Show resolved Hide resolved
keylime/src/tpm.rs Outdated Show resolved Hide resolved
@ansasaki
Copy link
Contributor Author

/packit retest-failed

ueno
ueno approved these changes Feb 22, 2024
View reviewed changes
Copy link
Contributor

@ueno ueno left a comment

Choose a reason for hiding this comment

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

Looks good to me! Sorry for the delay; I was away for the last couple of days.

keylime-agent/src/revocation.rs Outdated Show resolved Hide resolved
@Koncpa
Copy link
Contributor

Koncpa commented Feb 22, 2024

/packit retest-failed

@packit-as-a-service Packit-as-a-Service
Copy link

Account Koncpa has no write access nor is author of PR!

@sergio-correia
Copy link
Contributor

/packit retest-failed

@ansasaki
tpm: Rename origin of error as source in structures
dc8464f 
This takes advantage of thiserror crate to create a backtrace for the
errors.

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
@ansasaki
crypto: Add specific type for every possible error
da73919 
Also move crypto-related definitions from common.rs to crypto.rs.

Adjust the code to handle the new error types.

Add few auxiliary functions and tests for them.

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
@ansasaki
crypto: Move to keylime library
1562b4c 
Move the crypto module from keylime-agent to the keylime library.

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
@ansasaki
payloads: Remove explicit drop of channel transmitter
b259659 
Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
@ansasaki
tests/run.sh: Run tarpaulin with a single thread
50a3f7f 
This is to avoid frequent deadlocks on CI.

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
@ansasaki
crypto: Make error types less specific
a895c85 
Consolidate errors with same origin and add context specific messages.

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
@ansasaki
Copy link
Contributor Author

/packit retest-failed

@ansasaki ansasaki merged commit ad57804 into keylime:master Feb 22, 2024
10 of 11 checks passed
@ansasaki ansasaki deleted the improve_errors branch February 22, 2024 17:03
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

@ashcrow ashcrow Awaiting requested review from ashcrow

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

Assignees

@ashcrow ashcrow

@sergio-correia sergio-correia

Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move crypto module from keylime-agent to keylime library
6 participants
@ansasaki @ueno @Koncpa @sergio-correia @aplanas @ashcrow