-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add tests for the IAK and IDevID patches #514
Add tests for the IAK and IDevID patches #514
Conversation
Hello @Isaac-Matthews , thank you very much for the test. I will take a detailed look early next week. No need to care about Nitrate. Could you please clarify why the test requires Fedora 39? We are running tests with upstream keylime bits here so in theory the required keylime feature should be available everywhere. |
Btw, I can see there are some failures in the test. For details, please check service logs in the Cleanup phase or here. |
@@ -0,0 +1,157 @@ | |||
#!/bin/bash | |||
# vim: dict+=/usr/share/beakerlib/dictionary.vim cpt=.,w,b,u,t,i,k | |||
. /usr/share/beakerlib/beakerlib.sh || exit 1 |
Check notice
Code scanning / shellcheck
Not following: /usr/share/beakerlib/beakerlib.sh: openBinaryFile: does not exist (No such file or directory)
Hi @kkaarreell, no problem! Thanks I will be interested to hear your thoughts as I have not used any of this testing framework before so likely I have made some errors. Fedora-39/rawhide : these changes require a newer version of python-cryptography than is currently packaged for other distributions. This problem can be circumvented by using pip to install but unless I have misunderstood that is not how keylime is being installed in the testing framework. Maybe just installing a more recent version of crypto as part of the test would work though? For the failures, I will look at and fix the linting issue early next week. I am not sure why the centos failed, it looks unrelated to my changes but I might be misunderstanding. Fedora-39 will fail until PR 669 is merged as this is the test that is actually running my changes, which requires this PR to function. I have run the tests on the PR myself and the tests pass when the PR is used. Hopefully that all makes sense but feel free to let me know if clarification is needed or if you want any changes at all. Thanks. |
Yes, container failures seem unrelated but your test has failed on F39 too. |
Yes it will fail until the final rust agent pr is merged so this is expected for the moment. When the merge happens it will (should) pass. keylime/rust-keylime#669 |
Right, I am sorry, you have said that already. |
No worries! Hopefully we can get that merged soon as I think it should be all working now. |
It can be tested by temporarily redirecting agent CI to your test branch. See packit-ci.fmf in agent's repo and modify it in your PR for the agent, change url and ref and add new test to the test listing. I think devs would likes to see this test passing anyway. |
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.
Hi @Isaac-Matthews , I have reviewed the test and I do not have many requests regarding the code itself, it looks great. I will also ask few colleagues to review it since I am not very familiar with the associated feature yet. In some cases you are using some "magic" values and it is not clear where it comes from. Could you please add comments clarifying those so the test is easier to understand/maintain for us going forward?
Thank again.
-g sha256 \ | ||
-G rsa2048:null:null \ | ||
-a 'fixedtpm|fixedparent|sensitivedataorigin|userwithauth|adminwithpolicy|sign' \ | ||
-L 'ad6b3a2284fd698a0710bf5cc1b9bdf15e2532e3f601fa4b93a6a8fa8de579ea' \ |
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.
Where does this digest come from?
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.
Good point, I have added an explanation in this commit.
fi | ||
rlPhaseEnd | ||
|
||
rlPhaseStartSetup "Install tpm2-openssl to generate csrs with TPM keys" |
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.
Currently, Fedora does have tpm2-openssl provider packaged, just tpm2-openssl-engine. By any chance, would you know if it is possible to do the setup using tools that currently exist in Fedora (not the engine though as that is deprecated).
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.
Unfortunately the engine does not work here, I have tested and had to use as the provider in the end rather than the engine.
|
||
rlJournalStart | ||
|
||
rlPhaseStartSetup "Do the keylime setup" |
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.
Could you please create a temporary directory in the setup phase and do the execution inside this directory (leaving and removing it in the cleanup phase)? For example check https://github.com/RedHat-SP-Security/keylime-tests/blob/main/functional/tenant-runtime-policy-sanity/test.sh#L15
We do not have it in all tests but it is better to avoid creating additional artifacts in the test directory itself.
rlPhaseEnd | ||
|
||
rlPhaseStartSetup "Create CA" | ||
rlRun "CURRENTD=$(pwd)" |
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.
FYI, prior creating TMPDIR
, we also save and use TESTDIR
to access the original test location, in case there are artifacts that you are using.
# vim: dict+=/usr/share/beakerlib/dictionary.vim cpt=.,w,b,u,t,i,k | ||
. /usr/share/beakerlib/beakerlib.sh || exit 1 | ||
|
||
HTTP_SERVER_PORT=8080 |
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 you forgot to remove this line, it comes from the revocation webhook scenario.
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.
This line is removed now
authorityKeyIdentifier = keyid,issuer:always | ||
keyUsage = critical, digitalSignature, keyEncipherment | ||
extendedKeyUsage = serverAuth | ||
subjectAltName=DER:306FA06D06082B06010505070804A061305F0605678105010204565354000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 |
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.
Where does this value come from?
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.
As with above, explanation added in this commit
/packit retest-failed |
|
||
HTTP_SERVER_PORT=8080 | ||
AGENT_ID="d432fbb3-d2f1-4a97-9ef7-75bd81c00000" | ||
TPM2_OPENSSL="https://github.com/tpm2-software/tpm2-openssl/releases/download/1.2.0/tpm2-openssl-1.2.0.tar.gz" |
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.
From my point of view it would be better, to not have hardcoded version of release, but to take latest one.
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.
We have discussed it and decided to stick with the "known to be working" version for now. We can revisit the decision later.
/packit retest-failed |
1 similar comment
/packit retest-failed |
Signed-off-by: Isaac Matthews <[email protected]>
Tests looks good, please join commits and I will merge it. Thank you. |
302403e
to
23d153c
Compare
This is an end to end test for the IDevID and IAK patches:
1, 2, 3, 4
The test recreates the IAK and IDevID keys out of band of the agent (using tpm2_tools), and sets up a CA to create certs for the keys. The agent attempts to register with the keys (checking the keys match the internal keys in the process) and fails when the CA has not been added as trusted, then succeeds when the CA is added.
There may be missing parts or oversights (for example I was not sure how to generate the nitrate part of the fmf file)
The test currently should only be run for fedora-39