test: add basic encrypted key management tests #38
Conversation
dlrobertson
force-pushed
the
add_encrypted_tests
branch
from
c3282d1
to
9385117
Compare
| keyring.unlink_key(&enc_key).unwrap(); | ||
|
|
||
| // This should not panic but currently does due to the use | ||
| // of ByteBuf when encoding the load payload. |
There was a problem hiding this comment.
Is there a reason ByteBufs LowerHex is used for the Load payload type? AFAIK that is what is causing this test to fail.
There was a problem hiding this comment.
https://github.com/torvalds/linux/blob/master/Documentation/security/keys/trusted-encrypted.rst
All user level blobs, are displayed and loaded in hex ascii for convenience, and are integrity verified.
In code, it is read here and parsed here by this function which only supports lowercase ASCII hex.
There was a problem hiding this comment.
Sorry, I didn't do a good job of explaining the issue I hit.
https://github.com/torvalds/linux/blob/master/security/keys/encrypted-keys/encrypted.c#L162
* load [<format>] <master-key name> <decrypted data length> * <encrypted iv + data>
The key format, master key description, and the decrypted data length are parsed before the hex encoded data.
The current payload encoding for the load payload encodes everything but the key command as lower-case hex. This includes the format, master key, and decrypted data length, which AIUI should not be encoded as lower-case hex (why the should_panic test fails when it AFAIK should pass).
There was a problem hiding this comment.
Oh. Let's fix that in the Payload enum representation. I may have mixed up which docs/code I was looking at when making the payload type here.
Tests for the various keytypes was on my list after finishing #34, but the CI issues burned me out for a while on that :/ .
There was a problem hiding this comment.
I suppose a patch to fix the documentation for the load format should also be made:
keyctl add encrypted name "load hex_blob" ring
Yes, testing this stuff has been…painful on public CI infrastructure. They have this fetish with running as root inside containers which really messes with the permission checking done here. Quotas are also wacky and cause…interesting failures. See #34 for some fun. |
|
|
||
| #[test] | ||
| #[should_panic( | ||
| expected = "called `Result::unwrap()` on an `Err` value: Errno { code: 22, description: Some(\"Invalid argument\") }" |
There was a problem hiding this comment.
See #34 for the preferred method for checking for expected errors. The main issue with this strategy is that it doesn't check which unwrap caused this problem.
| @@ -129,3 +132,103 @@ fn add_key_to_session() { | |||
| assert_eq!(new_payload, new_expected); | |||
| keyring.unlink_key(&key).unwrap(); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn add_encrypted_key_to_session() { | |||
There was a problem hiding this comment.
I'd rather that keytype-specific tests go into src/keytypes/TYPE.rs than at the operation level.
| keyring.unlink_key(&enc_key).unwrap(); | ||
|
|
||
| // This should not panic but currently does due to the use | ||
| // of ByteBuf when encoding the load payload. |
There was a problem hiding this comment.
https://github.com/torvalds/linux/blob/master/Documentation/security/keys/trusted-encrypted.rst
All user level blobs, are displayed and loaded in hex ascii for convenience, and are integrity verified.
In code, it is read here and parsed here by this function which only supports lowercase ASCII hex.
#34 also has code to make tests into no-ops if a feature is not supported. I wish there were a way to say "this test is skipped" in Rust, but that is tied up in rust-lang/rust#50297. |
|
Awesome! Will update soon |
I added some basic tests for the
EncryptedKeyType. Note that this change now requiresCONFIG_ENCRYPTED_KEYSto be enabled for unit tests to pass.