Skip to content

vtpm: fix unseal failure when SHA256 PCR bank is disabled#5975

Open
shjala wants to merge 2 commits into
lf-edge:masterfrom
shjala:fix.vtpm.unseal
Open

vtpm: fix unseal failure when SHA256 PCR bank is disabled#5975
shjala wants to merge 2 commits into
lf-edge:masterfrom
shjala:fix.vtpm.unseal

Conversation

@shjala
Copy link
Copy Markdown
Member

@shjala shjala commented May 20, 2026

Description

When the SHA256 PCR bank is not available in the TPM (e.g. the device was initialized without it), UnsealDiskKeyWithRecovery would fail because it requires the SHA256 bank. The vtpm unseal path now checks for the bank's availability first and falls back to FetchVaultKey when it is missing.

A new SHA256_BANK=N make option was also added to make it easy to reproduce this condition locally. When set, it initializes the swtpm state using swtpm_setup with only the SHA1 PCR bank before starting the emulator.

How to test and validate this PR

  1. Build and run EVE with a vTPM that has the SHA256 PCR bank disabled:

    make TPM=y SHA256_BANK=N run
    
  2. Confirm that the vault unseals successfully and no error is logged
    related to the SHA256 bank.

  3. Deploy and app and make sure vTPM is available.

Changelog notes

No user-facing changes. Internal fix to the vtpm vault unseal path to
handle TPM configurations where the SHA256 PCR bank is not active.

PR Backports

  • 16.0-stable: Yes
  • 14.5-stable: Yes.
  • 13.4-stable: Yes.

Checklist

  • I've provided a proper description
  • I've added the proper documentation
  • I've tested my PR on amd64 device
  • I've tested my PR on arm64 device
  • I've written the test verification instructions
  • I've set the proper labels to this PR
  • I've checked the boxes above, or I've provided a good reason why I didn't
    check them.

When the SHA256 PCR bank is not available, unseal was failing because
UnsealDiskKeyWithRecovery requires it. Now the code checks for the
bank first and falls back to FetchVaultKey.

Signed-off-by: Shahriyar Jalayeri <shahriyar@posteo.de>
@shjala shjala requested a review from eriknordmark as a code owner May 20, 2026 10:39
@shjala shjala added bug Something isn't working stable Should be backported to stable release(s) labels May 20, 2026
@rene
Copy link
Copy Markdown
Contributor

rene commented May 20, 2026

@claude

@claude
Copy link
Copy Markdown

claude Bot commented May 20, 2026

Claude finished @rene's task in 2m 7s —— View job


Code Review

  • Gather context
  • Read CLAUDE.md for repo guidelines
  • Review PR diff
  • Post inline comments
  • Final summary

The fix is logically correct — falling back to FetchVaultKey when the SHA256 PCR bank is absent mirrors the existing behavior in FetchSealedVaultKey. Three inline comments were posted:

  1. main.go line 73 — the comment block (lines 69–72) contains a typo ("eve" → "even") and inaccurate reasoning: FetchSealedVaultKey already bypasses key-creation when SHA256 is disabled, so the stated concern doesn't apply to that code path.
  2. Makefile line 644rm -rf $(CURRENT_SWTPM)/* silently destroys existing swtpm state without warning; accidental use with a populated state directory is irreversible.
  3. Makefile line 645--not-overwrite is a no-op after the preceding unconditional wipe; should either be removed or the wipe made conditional.

Comment thread pkg/vtpm/swtpm-vtpm/src/main.go
Comment thread Makefile Outdated
Comment thread Makefile Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented May 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 21.07%. Comparing base (aa6798d) to head (932435d).
⚠️ Report is 25 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5975      +/-   ##
==========================================
+ Coverage   20.64%   21.07%   +0.42%     
==========================================
  Files         489      499      +10     
  Lines       90373    92071    +1698     
==========================================
+ Hits        18656    19402     +746     
- Misses      70138    70911     +773     
- Partials     1579     1758     +179     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run tests while code review comments are reviewed

@eriknordmark
Copy link
Copy Markdown
Contributor

@shjala are there any things to change based on the Claude review comments?

Note that the CI upgrade failures are due to a know eden test regression (fix in review)

@shjala
Copy link
Copy Markdown
Member Author

shjala commented May 21, 2026

@shjala are there any things to change based on the Claude review comments?

Note that the CI upgrade failures are due to a know eden test regression (fix in review)

Yes, I'll push the changes today.

Added a SHA256_BANK variable that, when set to N, initializes the swtpm
state using swtpm_setup with only the SHA1 PCR bank before starting the
emulator. This is useful for testing the vtpm unseal fallback path that
activates when the SHA256 bank is not available.

Signed-off-by: Shahriyar Jalayeri <shahriyar@posteo.de>
@shjala shjala force-pushed the fix.vtpm.unseal branch from a17dbe2 to 932435d Compare May 21, 2026 09:51
@github-actions github-actions Bot requested a review from eriknordmark May 21, 2026 09:51
Copy link
Copy Markdown
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kick off tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working stable Should be backported to stable release(s)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants