-
Notifications
You must be signed in to change notification settings - Fork 52
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
tcti: Adds support for libtpms backend #535
Conversation
Sadly this isn't enough to get the tests going with libtpms. There seems to be an error initializing.
|
It doesn't work because the context needs to be |
Oh, is this because there's no option for |
2eae78b
to
30deecf
Compare
This makes it possible to run almost all the tests here. Only 4 will fail to run: error output of 4 integration tests
|
30deecf
to
b48a43a
Compare
related, the package changes required to get that working within nix: NixOS/nixpkgs#331676 in the mean time: tpm2-tss-libtpms = pkgs.tpm2-tss.overrideAttrs(old: {
buildInputs = old.buildInputs ++ [ pkgs.libtpms ];
postPatch = old.postPatch + ''
for src in src/tss2-tcti/tcti-libtpms.c test/unit/tcti-libtpms.c; do
substituteInPlace "$src" \
--replace '"libtpms.so"' '"${ pkgs.libtpms.out}/lib/libtpms.so"' \
--replace '"libtpms.so.0"' '"${pkgs.libtpms.out}/lib/libtpms.so.0"'
done
'';
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the failures were fixed in other PRs, should be fixed on rebase
b48a43a
to
aed6ccf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks good in general. I'd like to try it out locally in a couple of hours but don't mind me if you think it's good to go :)
e631c83
to
bc89ecf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay folks, I've modified my tpm-box crate locally to use libtpms
and it works really well 🎉
(it took me a while since it seems tss in Arch had libtpms disabled for some reason... will resolve this asynchronously).
I've added just a couple of nitpicks... nothing big :)
Thanks for your dedicated work @baloo 🙇
@@ -247,6 +256,16 @@ impl FromStr for TctiNameConf { | |||
)?)); | |||
} | |||
|
|||
let libtpms_pattern = Regex::new(r"^libtpms(:(.*))?$").unwrap(); //should not fail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too bad we've got parsing all these regexes here, instead of some static variable... but the same happens with others so it's 👍
Btw, while my local tests of tpm-box went just fine running
I wonder if that's the limitation of libtpms (which claims: "Libtpms provides a very narrow Do tests of this crate pass for your @baloo? |
I have a different set of tests that fail here: |
This is useful for running tests without having to spawn a simulator in another process. Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>
bc89ecf
to
bc7c440
Compare
Interesting. I'm using version 0.9.6 of libtpms which seems to be the latest stable. Just out of curiosity what version are you using? (Just to be clear it seems libtpms integration will be quite nice for testing and I'm planning to use it for my projects. I'm just trying to gauge the feature-completeness of it :) ). Thanks for your work on this! |
Can you really run the tests in parallel like that? Is there some kind of built in resource manager in that lib? Otherwise you need to force to just run the tests sequentially. |
Same libtpms 0.9.6 and tpm2-tss 4.1.3
Yeah, my only intent is to simplify my CI and remove the need for a full simulator. |
I have to force it to run on a single thread. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you really run the tests in parallel like that? Is there some kind of built in resource manager in that lib? Otherwise you need to force to just run the tests sequentially.
D'oh, right! 🤦 with -- --test-threads=1
I get the same exact failures as @baloo.
So, I'm happy to see this merged.
The failures indicate which parameter, handle or session looks bad but I guess digging further into that would consume too much time while this change is already providing benefits. Let's get this in and if we have someone interested in libtpms
and/or correctness of these failing functions we can always revisit.
Just for completeness, I've got in touch with libtpms' @stefanberger and he ran the tests to completion which altered me to the remark that @Superhepper mentioned previously. Running the tests with Maybe it'd be a good idea to run the test suite with libtmps too, since that seems to be relatively easy now :) |
FYI: TPMLIB_Process is not thread-safe since the TPM itself is never accessed by multiple threads. libtpms assumes that there's a TPM interface, like either one of the TPM CRB, TIS, or SPAPR interfaces, on top of it that strictly send one request , then wait for the response, and then only send the next request. Any locking to prevent concurrent accesses would have to be done on higher layers. |
This is useful for running tests without having to spawn a simulator in another process.
https://github.com/tpm2-software/tpm2-tss/blob/master/doc/tcti.md#tcti-libtpms