tpm : more verbose error reporting when policy check fails #3315
Conversation
shjala
force-pushed
the
tpm-error-handling
branch
from
5eef527 to
038d6f8
Compare
shjala
force-pushed
the
tpm-error-handling
branch
4 times, most recently
from
c567553 to
473f9e3
Compare
shjala
changed the title
shjala
force-pushed
the
tpm-error-handling
branch
2 times, most recently
from
65b126e to
473f9e3
Compare
When EVE fails to unseal the vault due to policy check failure, it reports a generic error. This changes makes the error more verbose by finding the mismatching PCR(s) and report it. Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
shjala
force-pushed
the
tpm-error-handling
branch
5 times, most recently
from
5cfd364 to
d3d58eb
Compare
| // TPM is enabled. Check if defaultVault directory exists, if not set vaultconfig | ||
| tpmKeyOnlyMode := checkAndPublishVaultConfig(&ctx) | ||
| handler.SetHandlerOptions(vault.HandlerOptions{TpmKeyOnlyMode: tpmKeyOnlyMode}) | ||
| } | ||
|
|
||
| if tpmEnabled { |
There was a problem hiding this comment.
what about integrate these messages to the if above, so we don't have another redundant check here....
There was a problem hiding this comment.
I didn't get rid of it intentionally because these are two separate tasks, one is configuring and the other is setup, I thought there might be added some intermediary stages in between in the two in future, and having the "SetupDefaultVault" log in the configuration part and then having the log for intermediary stage after it (which should go before setup) might cause confusion when reading logs.
what do you think?
There was a problem hiding this comment.
ok, I got it, thanks, just let as it is...
Use WriteRename to be resilient against sudden crash/poweroff, it will create a tmp file, writes to and does a atomic rename. This commit also fixes some mindless yetus complains on using %w instead of %v. Signed-off-by: Shahriyar Jalayeri <shahriyar@zededa.com>
shjala
force-pushed
the
tpm-error-handling
branch
from
d3d58eb to
feeb41a
Compare
| t.Errorf("OpenTPM failed with err: %v", err) | ||
| return |
There was a problem hiding this comment.
isn't this the same as:
| t.Errorf("OpenTPM failed with err: %v", err) | |
| return | |
| t.Fatalf("OpenTPM failed with err: %v", err) |
There was a problem hiding this comment.
it is, but all the are tests in this file are written like that, I thought it might be intentional (catching/parsing errors in build or something), so I just followed the pattern.
|
@eriknordmark @rouming should I backport this to lts? |
|
@shjala we try to port only bugfixes, but I know how that can be painful to debug the PCR mismatch issues, so I would say yes. Does it cleanly apply to the 8.12 and 9.4? |
When EVE fails to unseal the vault due to policy check failure, it reports a generic error. This changes makes the error more verbose by finding the mismatching PCR(s) and report it.
Before this changes we would get :
UnsealWithSession failed: session 1, error code 0x1d : a policy check failedNow for example if indexes 1, 7 and 8 are not matching the original value, it reports :
UnsealWithSession failed: session 1, error code 0x1d : a policy check failed, possibly mismatching PCR indexes [1 7 8]second commit, makes the errors more informative and easier to follow in the logs.