Skip to content

Conversation

@shjala
Copy link
Member

@shjala shjala commented Jan 2, 2025

Description

New TPM communication APIs have been changed to use protobuf and the vcomlink package has been refactored to accommodate these changes. The existing custom message handling/protocol has been removed, and its functionalities have been integrated into the mentioned protobuf based API. This restructuring aims to streamline TPM-related operations and improve code maintainability.

New functions have been introduced to handle various TPM tasks more effectively. These include:

  1. tpmReadNV: Reads data from a specified Non-Volatile (NV) index in the TPM
  2. tpmGetPub: Retrieves the public key associated with a given handle
  3. tpmSign: Signs data using a specified key index
  4. certifykey: Certify a key using AK.

In addition this update introduces stricter security measures, such as enforcing security policies before performing TPM operations (for now a simple but sufficient policy, based on the key/NV index).


Relates to lf-edge/eve-tools#13 (not dependent).

How to test and validate this PR

Go tests are provided to fully test the vcomlink TPM related functionalities, the following are results of testing on HW TPM:

➜  pillar git:(vcomlink.refactor) ✗ /usr/bin/go test github.com/lf-edge/eve/pkg/pillar/cmd/vcomlink -v
time="2025-07-22T16:23:49+03:00" level=info msg="Containerd Init"
time="2025-07-22T16:23:49+03:00" level=info msg="VSOCK is supported, using VSOCK transport" pid=3783013 source=vcomlink_test
time="2025-07-22T16:23:49+03:00" level=info msg="Listening on vsock CID 1, port 2000" pid=3783013 source=vcomlink_test
=== RUN   TestValidGetPublic
TPM EK: 0001000b000300720000000600800043...
TPM EK Algorithm: RSA
TPM EK Attributes: FlagFixedParent | FlagSensitiveDataOrigin | FlagUserWithAuth | FlagRestricted | FlagDecrypt | FlagFixedTPM
--- PASS: TestValidGetPublic (0.02s)
=== RUN   TestValidGetEkCert
TPM EK cert issuer: CN=Nuvoton TPM Root CA 2112,O=Nuvoton Technology Corporation,C=TW
TPM EK URL: [https://www.nuvoton.com/security/NTC-TPM-EK-Cert/Nuvoton TPM Root CA 2112.cer]
Downloading issuing CA cert from URL: https://www.nuvoton.com/security/NTC-TPM-EK-Cert/Nuvoton TPM Root CA 2112.cer
EK cert verified successfully against issuing CA cert
--- PASS: TestValidGetEkCert (0.93s)
=== RUN   TestValidSigner
Signature verified successfully using RSA public key
--- PASS: TestValidSigner (0.11s)
=== RUN   TestValidActivateCred
Credential:           f63e0c102742a64374740b78995834828e8484e07d70a2a455af3c50202f9b3c
Recovered credential: f63e0c102742a64374740b78995834828e8484e07d70a2a455af3c50202f9b3c
Credential activation successful, we can trust the AIK
--- PASS: TestValidActivateCred (0.11s)
=== RUN   TestValidCertifyKey
    vcomlink_test.go:665: Successfully certified key with AIK and verified signature.
--- PASS: TestValidCertifyKey (0.10s)
PASS
ok      github.com/lf-edge/eve/pkg/pillar/cmd/vcomlink  3.282s

Changelog notes

Refactored vcomlink TPM communication to use protobuf-based APIs with updated vcomlink support, replacing custom messaging, adding tpmReadNV, tpmGetPub, and tpmSign functions, and introducing basic security policy enforcement.

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

For backport PRs (remove it if it's not a backport):

  • I've added a reference link to the original PR
  • PR's title follows the template

And the last but not least:

  • I've checked the boxes above, or I've provided a good reason why I didn't
    check them.

Please, check the boxes above after submitting the PR in interactive mode.

@github-actions github-actions bot requested a review from rucoder January 2, 2025 12:13
@shjala shjala marked this pull request as draft January 2, 2025 12:15
@shjala shjala force-pushed the vcomlink.refactor branch 2 times, most recently from ba73204 to 3597b93 Compare February 5, 2025 13:21
@github-actions github-actions bot requested a review from zedi-pramodh February 5, 2025 13:22
@shjala shjala force-pushed the vcomlink.refactor branch from 3597b93 to ed98a85 Compare February 5, 2025 14:02
@shjala shjala force-pushed the vcomlink.refactor branch 4 times, most recently from 21fb09d to 10803cd Compare July 14, 2025 13:29
@shjala shjala added the enhancement New feature or request label Jul 14, 2025
@shjala shjala self-assigned this Jul 14, 2025
@shjala shjala marked this pull request as ready for review July 14, 2025 13:51
@shjala
Copy link
Member Author

shjala commented Jul 14, 2025

Code ready for review, I'm fixing the CI errors and updating the documents.

@shjala shjala changed the title [WIP] Vcomlink refactor vcomLink communication refactoring Jul 14, 2025
@shjala shjala force-pushed the vcomlink.refactor branch 2 times, most recently from db381cd to 251b958 Compare July 14, 2025 15:04
@shjala shjala added the main-quest The fate of the project rests on this PR. Prioritise review to advance the storyline! label Jul 15, 2025
@shjala shjala force-pushed the vcomlink.refactor branch 3 times, most recently from 6f1b902 to 30fbca9 Compare July 15, 2025 11:05
@OhmSpectator
Copy link
Member

I recall using code from this PR once... I had to modify it slightly to make it suitable... But I have no memory of the details... Will have to review again...

@OhmSpectator
Copy link
Member

@shjala could you, please, meanwhile, check the Docker hash warning?...

@shjala
Copy link
Member Author

shjala commented Jul 18, 2025

@shjala could you, please, meanwhile, check the Docker hash warning?...

Sure, I have some more changes coming today, I'll fix that docker too.

@shjala
Copy link
Member Author

shjala commented Jul 18, 2025

I recall using code from this PR once... I had to modify it slightly to make it suitable... But I have no memory of the details... Will have to review again...

Yeah that probably was for sending monitoring data.

@shjala shjala force-pushed the vcomlink.refactor branch from 30fbca9 to e1be23b Compare July 18, 2025 08:28
@OhmSpectator
Copy link
Member

@shjala, I think it's better to rebase the branch

@eriknordmark
Copy link
Contributor

There are failures in TestEdenScripts/eden_vcom, and also SPDX license headers to address.

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

There are failures in TestEdenScripts/eden_vcom, and also SPDX license headers to address.

@shjala
Copy link
Member Author

shjala commented Sep 25, 2025

There are failures in TestEdenScripts/eden_vcom, and also SPDX license headers to address.

@eriknordmark the test failure is expected, I can disable that test for now in Eden until this gets merged lf-edge/eden#1095, but @OhmSpectator is in favour of not doing that and letting the test to fail.

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

Note one more SPDX failure:
Checking tests/tpm/prep-and-run.sh

  • SPDX-License-Identifier: OK
  • Copyright: does not have the current year 2025 in the copyright notice!

Letting the test fail until Eden is fixed should be ok, but presumably we need a new version of eden so we can run the older one on 14.5.0?

@shjala
Copy link
Member Author

shjala commented Sep 25, 2025

Letting the test fail until Eden is fixed should be ok, but presumably we need a new version of eden so we can run the older one on 14.5.0?

But the older branches point to the old Eden right? so we need one for the master...

@shjala
Copy link
Member Author

shjala commented Sep 25, 2025

Yetus is picking up on unrelated files, my local run shows nothing :

shah in 🌐 shah-MS-7E12 in eve on  vcomlink.refactor [$?] via 🐍 v3.12.3 took 5s 
❯ make mini-yetus MYETUS_VERBOSE=y
shah in 🌐 shah-MS-7E12 in eve on  vcomlink.refactor [$?] via 🐍 v3.12.3 took 8s 
❯ cat /tmp/yetus.WZ5B3epSZ1/yetus-output/results-full.txt 
shah in 🌐 shah-MS-7E12 in eve on  vcomlink.refactor [$?] via 🐍 v3.12.3 
❯ 

@eriknordmark
Copy link
Contributor

Yetus is picking up on unrelated files, my local run shows nothing :

shah in 🌐 shah-MS-7E12 in eve on  vcomlink.refactor [$?] via 🐍 v3.12.3 took 5s 
❯ make mini-yetus MYETUS_VERBOSE=y
shah in 🌐 shah-MS-7E12 in eve on  vcomlink.refactor [$?] via 🐍 v3.12.3 took 8s 
❯ cat /tmp/yetus.WZ5B3epSZ1/yetus-output/results-full.txt 
shah in 🌐 shah-MS-7E12 in eve on  vcomlink.refactor [$?] via 🐍 v3.12.3 
❯ 

I did see a comment in the output that the pr is not on top of current master so it might pick up other files.

VSOCKListener implements the net.Conn and net.Listener
interfaces for vsock.

Signed-off-by: Shahriyar Jalayeri <[email protected]>
Add vcom link API protobufs definition and generated code
for Go and Python.

Signed-off-by: Shahriyar Jalayeri <[email protected]>
New TPM communication APIs have been changed to use protbuf and
the vcomlink package has been refactored to accommodate these changes.

Signed-off-by: Shahriyar Jalayeri <[email protected]>
Updated the vcomlink.md documentation to reflect recent changes in the
TPM service protobuf API.

Signed-off-by: Shahriyar Jalayeri <[email protected]>
Make sure read syscall does not return n < 0 which then panics the bufio.

Signed-off-by: Shahriyar Jalayeri <[email protected]>
Add a script to prepare and run SWTPM to test vcomlink locally.

Signed-off-by: Shahriyar Jalayeri <[email protected]>
@shjala
Copy link
Member Author

shjala commented Sep 25, 2025

I did see a comment in the output that the pr is not on top of current master so it might pick up other files.

rebased, let's see if Yetus is happy now...

@eriknordmark eriknordmark merged commit cbe9231 into lf-edge:master Sep 25, 2025
44 of 47 checks passed
shjala added a commit to shjala/eden that referenced this pull request Sep 30, 2025
Disable vComLink test temporarily until we have an EVE release that
includes the changes introduced in lf-edge/eve#4502.

Signed-off-by: Shahriyar Jalayeri <[email protected]>
uncleDecart pushed a commit to lf-edge/eden that referenced this pull request Sep 30, 2025
Disable vComLink test temporarily until we have an EVE release that
includes the changes introduced in lf-edge/eve#4502.

Signed-off-by: Shahriyar Jalayeri <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request main-quest The fate of the project rests on this PR. Prioritise review to advance the storyline!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants