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

Add tests for the IAK and IDevID patches #514

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

Isaac-Matthews
Copy link
Contributor

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

@kkaarreell
Copy link
Collaborator

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.

@kkaarreell
Copy link
Collaborator

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)

Not following: /usr/share/beakerlib/beakerlib.sh: openBinaryFile: does not exist (No such file or directory)
@Isaac-Matthews
Copy link
Contributor Author

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.

@kkaarreell
Copy link
Collaborator

Yes, container failures seem unrelated but your test has failed on F39 too.

@Isaac-Matthews
Copy link
Contributor Author

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

@kkaarreell
Copy link
Collaborator

Right, I am sorry, you have said that already.

@Isaac-Matthews
Copy link
Contributor Author

No worries! Hopefully we can get that merged soon as I think it should be all working now.

@kkaarreell
Copy link
Collaborator

kkaarreell commented Nov 10, 2023

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.

@kkaarreell kkaarreell requested a review from ansasaki November 13, 2023 08:46
Copy link
Collaborator

@kkaarreell kkaarreell left a 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' \
Copy link
Collaborator

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?

Copy link
Contributor Author

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"
Copy link
Collaborator

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).

Copy link
Contributor Author

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"
Copy link
Collaborator

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)"
Copy link
Collaborator

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
Copy link
Collaborator

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.

Copy link
Contributor Author

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
Copy link
Collaborator

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?

Copy link
Contributor Author

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

@kkaarreell
Copy link
Collaborator

/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"
Copy link
Collaborator

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.

Copy link
Collaborator

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.

@Isaac-Matthews
Copy link
Contributor Author

/packit retest-failed

1 similar comment
@Isaac-Matthews
Copy link
Contributor Author

/packit retest-failed

@kkaarreell
Copy link
Collaborator

Tests looks good, please join commits and I will merge it. Thank you.

@kkaarreell kkaarreell merged commit 8318feb into RedHat-SP-Security:main Nov 17, 2023
4 of 5 checks passed
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.

3 participants