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

Implemented proc/crypto parsing #296

Merged
merged 12 commits into from
May 1, 2024
Merged

Conversation

Hwatwasthat
Copy link
Contributor

Hi, I have implemented parsing for the /proc/crypto file, as I noticed it wasn't implemented in the support file. I have tried to add in types where it felt relevant so it can restrict the output slightly and make handling the output a bit easier than throwing strings around. I've put in tests for various possibilities but admittedly they're far from exhaustive! I've tested on my local machine and a raspberry pi but nothing further.

Copy link
Owner

@eminence eminence left a comment

Choose a reason for hiding this comment

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

Hi, thanks for working on this! This is a good start

I made a few in-line comments. A few additional general comments:

  • Can you add a test that will actually parse a live /proc/crypto file?
  • I don't see an obvious way to construct a CryptoTable
  • There's a few very minor things that cargo clippy will report

procfs-core/src/crypto.rs Outdated Show resolved Hide resolved
procfs-core/src/crypto.rs Outdated Show resolved Hide resolved
procfs-core/src/crypto.rs Outdated Show resolved Hide resolved
@Hwatwasthat
Copy link
Contributor Author

Hi, thanks for working on this! This is a good start

I made a few in-line comments. A few additional general comments:

* Can you add a test that will actually parse a live `/proc/crypto` file?

* I don't see an obvious way to construct a `CryptoTable`

* There's a few very minor things that `cargo clippy` will report
  • For the test, you mean as in load a local file or just paste in the contents of an existing file?
  • At the moment there is only a way to create one from an iterator. It's not something you'd normally find in another location, but I could create a from Bufreader implementation.
  • Ah yeah, some imports I missed. I'll pop those out.

@eminence
Copy link
Owner

eminence commented Feb 3, 2024

For the test, you mean as in load a local file or just paste in the contents of an existing file?

I mean open /proc/crypto and make sure it can be parsed. This is relatively common in this crate, and it's useful because I can clone this repo to any computer and just run cargo test to make sure that everything can be parsed.

At the moment there is only a way to create one from an iterator

Sorry, I didn't quite follow this. Can you give an example about how one would create a CryptoTable? I think probably you can impl Current for CrypotoTable which means that you'd be able to run CryptoTable::current() to get an instance of CryptoTable

@Hwatwasthat
Copy link
Contributor Author

I can implement those, I just normally try and keep it all in test and not touch the system. I can implement current for a table, currently it's only implemented that if you pass an iterator of bytes that contains a crypto table it will parse from that. I'll get a push to you this weekend that will implement these.

@Hwatwasthat
Copy link
Contributor Author

I think I've got everything, if you could give it a look over :) There are some clippy lints outside of the crypto sections that I have left untouched, as I don't want to mess with other folks code!

@eminence
Copy link
Owner

I'm looking at the /proc/crypto file on my computer, and I see several blocks with the same name. For example:

name         : stdrng
driver       : drbg_nopr_hmac_sha512
module       : drbg
priority     : 221
refcnt       : 2
selftest     : passed
internal     : no
type         : rng
seedsize     : 0

name         : stdrng
driver       : drbg_nopr_hmac_sha256
module       : drbg
priority     : 220
refcnt       : 1
selftest     : passed
internal     : no
type         : rng
seedsize     : 0

Since the crypto table is a HashMap, only the last stdrng gets saved. Is that what you intended with this code?

procfs-core/src/crypto.rs Outdated Show resolved Hide resolved
procfs-core/src/crypto.rs Outdated Show resolved Hide resolved
@Hwatwasthat
Copy link
Contributor Author

That was not the intention. My reckoning is that the easiest way to resolve this is to change from a Hashmap<String, CryptoBlock to a Hashmap<String, Vec<CryptoBlock>>. This removes the loss of duplicates but maintains the ease of the interface, having to know the exact name of a driver is probably not the most desired outcome when using this interface, and it still allows for more fine grained searching later, without the overhead of a double hashmap (I doubt there are that many duplicates).

…toBlock>>` to account for duplicate names.

Minor changes to tests, added test for two blocks that share a name but are distinct drivers.
@Hwatwasthat
Copy link
Contributor Author

Added in a version that has a Hashmap<String, Vec<CryptoBlock>>. This adds some extra overhead to the type sadly but probably won't be that hurtful. Duplicates are just pushed to the vec when it happens, and that can be further broken down by the caller.

@eminence
Copy link
Owner

eminence commented Mar 5, 2024

Looks pretty good on a quick skim, will do a more detailed review later. Thanks!

@Hwatwasthat
Copy link
Contributor Author

Hey, just checking in if you've had a chance to look and see if anything else needs an update?

@eminence
Copy link
Owner

Thanks for the ping, I'll take a look Very Soon Now

#[cfg_attr(feature = "serde1", derive(Serialize, Deserialize))]
pub enum Type {
/// Symmetric Key Cipher
Skcipher(Skcipher),
Copy link
Owner

Choose a reason for hiding this comment

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

I think I might prefer using a struct variant, like:

pub enum Type {
    /// Symmetric Key Cipher
    Skcipher{async_capable: bool, block_size: bool, min_key_size: bool, ...}
     ...
}

It makes the whole enum a bit more complicated, but also you have less types overall (because you can get rid of the Skcipher struct).

But this is a minor point, no need to change this now

@eminence eminence merged commit 210c321 into eminence:master May 1, 2024
6 checks passed
@eminence
Copy link
Owner

eminence commented May 1, 2024

Thank you for the contribution, and your patience with the long review!

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

Successfully merging this pull request may close these issues.

2 participants