Skip to content

Tdx common functions and adding PCCS cases#4437

Closed
zixi-chen wants to merge 2 commits intoautotest:masterfrom
zixi-chen:tdx_pccs
Closed

Tdx common functions and adding PCCS cases#4437
zixi-chen wants to merge 2 commits intoautotest:masterfrom
zixi-chen:tdx_pccs

Conversation

@zixi-chen
Copy link
Contributor

@zixi-chen zixi-chen commented Jan 29, 2026

Summary by CodeRabbit

  • New Features
    • Added comprehensive TDX PCCS/DCAP support and end-to-end attestation validation flow for host and guest.
  • Tests
    • Added automated PCCS admin test script and new TDX PCCS test that runs PCCS/DCAP checks, boots a secure VM, and verifies attestation.
    • New configuration stanza to support TDX PCCS test variants.
  • Refactor
    • Streamlined multi-VM TDX test flow with centralized host capability and DCAP service handling.

✏️ Tip: You can customize this high-level summary in your review settings.

Signed-off-by: Zixi Chen <zixchen@redhat.com>
@zixi-chen zixi-chen marked this pull request as draft January 29, 2026 09:32
@coderabbitai
Copy link

coderabbitai bot commented Jan 29, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

  • 🔍 Trigger a full review

Walkthrough

Adds comprehensive TDX/DCAP support: a new provider module implementing host capability checks, TDX verification, and DCAP management; a shell test script for PCCS admin operations; new and updated test modules that use the provider to prepare host DCAP/PCCS services, run a pccsadmin test, boot TDX VMs, perform guest TDX checks, and validate attestation; and a new configuration stanza with PCCS/DCAP parameters and SSL setup.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Tdx common functions and adding PCCS cases' accurately describes the main changes: introducing TDX common utility classes (TDXError, TDXHostCapability, TDXChecker, TDXDcap) and adding PCCS-related test cases (tdx_pccs.py and tdx_pccs.cfg).
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
qemu/tests/tdx_multi_vms.py (1)

78-90: Guard session.close() when login fails

If wait_for_login throws, session is None and the finally block raises, masking the original failure.

🛠️ Suggested fix
         finally:
-            session.close()
+            if session:
+                session.close()
             vm.destroy()
🤖 Fix all issues with AI agents
In `@provider/tdx.py`:
- Around line 154-166: The chmod is being run against guest_cmd which may
include arguments; instead compute the copied script path (e.g.,
guest_script_path = os.path.join(guest_dir, host_script) or use
self._params["guest_script"]) and call session.cmd_output("chmod 755 %s" %
guest_script_path) so chmod targets the script file only; keep using
session.cmd_status(guest_cmd, timeout=360) for execution (since it may include
args) and ensure the variables host_file, guest_dir, host_script, guest_cmd and
the copy call self._vm.copy_files_to(...) are left consistent.

In `@qemu/deps/tdx/pccsadmin_test.sh`:
- Around line 12-14: The assignments in pccsadmin_test.sh use spaces around '='
which makes them commands and aborts the script; update the three variables
(API_Key, UserTokenHash, AdminTokenHash) to use valid shell assignment syntax
(no spaces around '=') and quote the expansions (e.g.
API_Key="${PCCS_PRIMARY_API_KEY}") so empty or spaced values are handled safely.
- Around line 16-33: The expect script started with expect -c " never has a
terminating quote, causing a syntax error; close the string after the expect eof
(add a closing double-quote right after the expect eof line) so the expect -c "
... " block ends before the subsequent shell checks (leave the spawn pccsadmin
fetch / send / expect sequence intact), ensuring the following shell code that
checks for platform_collaterals.json runs outside the expect block.

In `@qemu/tests/cfg/tdx_pccs.cfg`:
- Line 1: The top-level stanza name "tdx_pccs" is malformed because it is
written as a list item with a leading "-" (e.g., "- tdx_pccs"); remove the
leading hyphen so the stanza reads simply "tdx_pccs:" at top level to ensure it
is parsed as a test entry rather than a list element.
- Line 31: The OpenSSL command in generate_ssl_cmd is writing the private key
and certificate to the wrong variables (using -keyout ${pccs_ssl_cert} and -out
${pccs_ssl_key}); update generate_ssl_cmd to swap the -keyout and -out targets
so -keyout writes to ${pccs_ssl_key} and -out writes to ${pccs_ssl_cert},
ensuring pccs_ssl_key receives the private key and pccs_ssl_cert receives the
certificate.

In `@qemu/tests/tdx_pccs.py`:
- Around line 62-79: The current flow may leave the guest session open or VM
running on exceptions (during wait_for_login or
tdx_dcap.verify_dcap_attestation); wrap the verification steps in a try/finally
(or add a top-level try/except/finally) so that session.close() and vm.destroy()
are always executed: ensure you check session is not None before calling
session.close(), and always call vm.destroy() (or vm.cleanup equivalent) in the
finally block; update the block around vm.wait_for_login, session.cmd_output,
and TDXDcap.verify_dcap_attestation (references: session, vm,
TDXDcap.verify_dcap_attestation, tdx_dcap) to perform cleanup regardless of
errors.
🧹 Nitpick comments (1)
provider/tdx.py (1)

273-279: Drop the unused result variable

It’s assigned but never used.

🛠️ Suggested cleanup
-                    result = process.run("systemctl restart %s" % service, shell=True, timeout=60)
+                    process.run("systemctl restart %s" % service, shell=True, timeout=60)

Comment on lines +154 to +166
if not self._params.get("guest_script"):
self._test.cancel("Missing guest_script for attestation.")
guest_dir = self._params["guest_dir"]
host_script = self._params["host_script"]
guest_cmd = self._params["guest_cmd"]
host_file = os.path.join(deps_dir, host_script)
try:
self._vm.copy_files_to(host_file, guest_dir)
session.cmd_output("chmod 755 %s" % guest_cmd)
except Exception as e:
self._test.fail("Guest test preparation fail: %s" % str(e))
s = session.cmd_status(guest_cmd, timeout=360)
if s:
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

chmod should target the script path, not the full command

guest_cmd can include arguments (see cfg), so chmod 755 <cmd with args> fails. Use the copied script path.

🛠️ Suggested fix
         guest_dir = self._params["guest_dir"]
         host_script = self._params["host_script"]
         guest_cmd = self._params["guest_cmd"]
         host_file = os.path.join(deps_dir, host_script)
+        guest_script_path = os.path.join(guest_dir, os.path.basename(host_script))
         try:
             self._vm.copy_files_to(host_file, guest_dir)
-            session.cmd_output("chmod 755 %s" % guest_cmd)
+            session.cmd_output("chmod 755 %s" % guest_script_path)
🤖 Prompt for AI Agents
In `@provider/tdx.py` around lines 154 - 166, The chmod is being run against
guest_cmd which may include arguments; instead compute the copied script path
(e.g., guest_script_path = os.path.join(guest_dir, host_script) or use
self._params["guest_script"]) and call session.cmd_output("chmod 755 %s" %
guest_script_path) so chmod targets the script file only; keep using
session.cmd_status(guest_cmd, timeout=360) for execution (since it may include
args) and ensure the variables host_file, guest_dir, host_script, guest_cmd and
the copy call self._vm.copy_files_to(...) are left consistent.

Comment on lines 12 to 14
API_Key = ${PCCS_PRIMARY_API_KEY}
UserTokenHash = ${PCCS_USER_TOKEN_HASH}
AdminTokenHash = ${PCCS_ADMIN_TOKEN_HASH}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Invalid shell assignments will abort the script

Spaces around = turn these into commands, so the script exits before any PCCS actions run.

🛠️ Suggested fix
-API_Key = ${PCCS_PRIMARY_API_KEY}
-UserTokenHash = ${PCCS_USER_TOKEN_HASH}
-AdminTokenHash = ${PCCS_ADMIN_TOKEN_HASH}
+API_Key="${PCCS_PRIMARY_API_KEY}"
+UserTokenHash="${PCCS_USER_TOKEN_HASH}"
+AdminTokenHash="${PCCS_ADMIN_TOKEN_HASH}"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
API_Key = ${PCCS_PRIMARY_API_KEY}
UserTokenHash = ${PCCS_USER_TOKEN_HASH}
AdminTokenHash = ${PCCS_ADMIN_TOKEN_HASH}
API_Key="${PCCS_PRIMARY_API_KEY}"
UserTokenHash="${PCCS_USER_TOKEN_HASH}"
AdminTokenHash="${PCCS_ADMIN_TOKEN_HASH}"
🧰 Tools
🪛 Shellcheck (0.11.0)

[error] 12-12: Remove spaces around = to assign (or use [ ] to compare, or quote '=' if literal).

(SC2283)


[error] 13-13: Remove spaces around = to assign (or use [ ] to compare, or quote '=' if literal).

(SC2283)


[error] 14-14: Remove spaces around = to assign (or use [ ] to compare, or quote '=' if literal).

(SC2283)

🤖 Prompt for AI Agents
In `@qemu/deps/tdx/pccsadmin_test.sh` around lines 12 - 14, The assignments in
pccsadmin_test.sh use spaces around '=' which makes them commands and aborts the
script; update the three variables (API_Key, UserTokenHash, AdminTokenHash) to
use valid shell assignment syntax (no spaces around '=') and quote the
expansions (e.g. API_Key="${PCCS_PRIMARY_API_KEY}") so empty or spaced values
are handled safely.

Comment on lines 16 to 33
expect -c "
set timeout 60
spawn pccsadmin fetch
expect \"Please input ApiKey for Intel PCS:\"
send \"$PCCS_PRIMARY_API_KEY\r\"
expect \"Would you like to remember Intel PCS ApiKey in OS keyring? (y/n)\"
send \"n\r\"
expect \"Do you want to save the list?\"
send \"y\r\"
expect \"Please input file name (Press enter to use default name not_available.json):\"
send \"\r\"
expect eof

# Check if platform_collaterals.json was generated
if [ ! -f platform_collaterals.json ]; then
echo "Pccsadmin fetch failed, platform_collaterals.json was not generated."
exit 1
fi
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Missing closing quote in the first expect -c block

The string never terminates, which makes the script a syntax error.

🛠️ Suggested fix
 expect eof
-
+"
 
 # Check if platform_collaterals.json was generated
 if [ ! -f platform_collaterals.json ]; then
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
expect -c "
set timeout 60
spawn pccsadmin fetch
expect \"Please input ApiKey for Intel PCS:\"
send \"$PCCS_PRIMARY_API_KEY\r\"
expect \"Would you like to remember Intel PCS ApiKey in OS keyring? (y/n)\"
send \"n\r\"
expect \"Do you want to save the list?\"
send \"y\r\"
expect \"Please input file name (Press enter to use default name not_available.json):\"
send \"\r\"
expect eof
# Check if platform_collaterals.json was generated
if [ ! -f platform_collaterals.json ]; then
echo "Pccsadmin fetch failed, platform_collaterals.json was not generated."
exit 1
fi
expect -c "
set timeout 60
spawn pccsadmin fetch
expect \"Please input ApiKey for Intel PCS:\"
send \"$PCCS_PRIMARY_API_KEY\r\"
expect \"Would you like to remember Intel PCS ApiKey in OS keyring? (y/n)\"
send \"n\r\"
expect \"Do you want to save the list?\"
send \"y\r\"
expect eof
"
# Check if platform_collaterals.json was generated
if [ ! -f platform_collaterals.json ]; then
echo "Pccsadmin fetch failed, platform_collaterals.json was not generated."
exit 1
fi
🤖 Prompt for AI Agents
In `@qemu/deps/tdx/pccsadmin_test.sh` around lines 16 - 33, The expect script
started with expect -c " never has a terminating quote, causing a syntax error;
close the string after the expect eof (add a closing double-quote right after
the expect eof line) so the expect -c " ... " block ends before the subsequent
shell checks (leave the spawn pccsadmin fetch / send / expect sequence intact),
ensuring the following shell code that checks for platform_collaterals.json runs
outside the expect block.

@@ -0,0 +1,42 @@
- tdx_pccs:
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Top‑level stanza name is malformed

The leading “-” makes this a list item, so the tdx_pccs stanza may not be parsed as a test entry.

🛠️ Suggested fix
-- tdx_pccs:
+tdx_pccs:
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- tdx_pccs:
tdx_pccs:
🤖 Prompt for AI Agents
In `@qemu/tests/cfg/tdx_pccs.cfg` at line 1, The top-level stanza name "tdx_pccs"
is malformed because it is written as a list item with a leading "-" (e.g., "-
tdx_pccs"); remove the leading hyphen so the stanza reads simply "tdx_pccs:" at
top level to ensure it is parsed as a test entry rather than a list element.

pccs_ssl_dir = "/etc/pccs/ssl"
pccs_ssl_cert = "/etc/pccs/ssl/server-cert.pem"
pccs_ssl_key = "/etc/pccs/ssl/server-key.pem"
generate_ssl_cmd = 'openssl req -x509 -nodes -days 365 -newkey rsa:2048 -keyout ${pccs_ssl_cert} -out ${pccs_ssl_key} -subj "/O=Intel/OU=TDX/CN=localhost"'
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🌐 Web query:

OpenSSL req -keyout -out options private key certificate

💡 Result:

In openssl req, -keyout is for the private key file (only when a new key is being generated), and -out is for the request/certificate output. (docs.openssl.org)

What each option produces

  • -keyout <file>
    Writes the newly generated private key to <file>. This is used with -newkey ... (or when -new causes a key to be generated). (docs.openssl.org)

  • -out <file>
    Writes the main output to <file>:

    • normally a CSR (PKCS#10)
    • if you also pass -x509, then it writes a self-signed certificate instead of a CSR (docs.openssl.org)
  • -key <file> (related, often confused with -keyout)
    Uses an existing private key from <file> (so you would not generate a new one, and typically wouldn’t use -keyout). (docs.openssl.org)

Common command patterns

1) Generate a new private key and a CSR

openssl req -new -newkey rsa:2048 -keyout private.key -out request.csr

(-keyout = private key, -out = CSR) (docs.openssl.org)

2) Generate a CSR from an existing private key

openssl req -new -key private.key -out request.csr

(-out = CSR) (docs.openssl.org)

3) Generate a new private key and a self-signed certificate

openssl req -x509 -newkey rsa:2048 -keyout private.key -out cert.pem -days 365

(-out = certificate because -x509 was used) (docs.openssl.org)

Citations:


Swap cert/key outputs in the OpenSSL command

-keyout writes the private key and -out writes the certificate (when using -x509). The current command assigns them to swapped variables, causing the private key to be written to ${pccs_ssl_cert} and the certificate to ${pccs_ssl_key}, which will break PCCS TLS.

🛠️ Suggested fix
-generate_ssl_cmd = 'openssl req -x509 -nodes -days 365 -newkey rsa:2048  -keyout ${pccs_ssl_cert}  -out ${pccs_ssl_key}  -subj "/O=Intel/OU=TDX/CN=localhost"'
+generate_ssl_cmd = 'openssl req -x509 -nodes -days 365 -newkey rsa:2048  -keyout ${pccs_ssl_key}  -out ${pccs_ssl_cert}  -subj "/O=Intel/OU=TDX/CN=localhost"'
🤖 Prompt for AI Agents
In `@qemu/tests/cfg/tdx_pccs.cfg` at line 31, The OpenSSL command in
generate_ssl_cmd is writing the private key and certificate to the wrong
variables (using -keyout ${pccs_ssl_cert} and -out ${pccs_ssl_key}); update
generate_ssl_cmd to swap the -keyout and -out targets so -keyout writes to
${pccs_ssl_key} and -out writes to ${pccs_ssl_cert}, ensuring pccs_ssl_key
receives the private key and pccs_ssl_cert receives the certificate.

Comment on lines 62 to 79
# Verify TDX enabled in guest
error_context.context("Verify TDX enabled in guest", test.log.info)
guest_check_cmd = params["tdx_guest_check"]
session = None
try:
session = vm.wait_for_login(timeout=timeout)
session.cmd_output(guest_check_cmd, timeout=240)
except Exception as e:
test.fail("Guest tdx verify fail: %s" % str(e))

# Verify TDX attestation
error_context.context("Verify TDX attestation", test.log.info)
deps_dir = virttest_data_dir.get_deps_dir()
tdx_dcap = TDXDcap(test, params, vm)
tdx_dcap.verify_dcap_attestation(session, deps_dir)

session.close()
vm.destroy()
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Ensure session/VM cleanup on failures

If guest login or attestation fails, the VM can be left running and the session unclosed.

🛠️ Suggested fix
     session = None
-    try:
-        session = vm.wait_for_login(timeout=timeout)
-        session.cmd_output(guest_check_cmd, timeout=240)
-    except Exception as e:
-        test.fail("Guest tdx verify fail: %s" % str(e))
-
-    # Verify TDX attestation
-    error_context.context("Verify TDX attestation", test.log.info)
-    deps_dir = virttest_data_dir.get_deps_dir()
-    tdx_dcap = TDXDcap(test, params, vm)
-    tdx_dcap.verify_dcap_attestation(session, deps_dir)
-
-    session.close()
-    vm.destroy()
+    try:
+        session = vm.wait_for_login(timeout=timeout)
+        session.cmd_output(guest_check_cmd, timeout=240)
+
+        # Verify TDX attestation
+        error_context.context("Verify TDX attestation", test.log.info)
+        deps_dir = virttest_data_dir.get_deps_dir()
+        tdx_dcap = TDXDcap(test, params, vm)
+        tdx_dcap.verify_dcap_attestation(session, deps_dir)
+    except Exception as e:
+        test.fail("Guest tdx verify fail: %s" % str(e))
+    finally:
+        if session:
+            session.close()
+        vm.destroy()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Verify TDX enabled in guest
error_context.context("Verify TDX enabled in guest", test.log.info)
guest_check_cmd = params["tdx_guest_check"]
session = None
try:
session = vm.wait_for_login(timeout=timeout)
session.cmd_output(guest_check_cmd, timeout=240)
except Exception as e:
test.fail("Guest tdx verify fail: %s" % str(e))
# Verify TDX attestation
error_context.context("Verify TDX attestation", test.log.info)
deps_dir = virttest_data_dir.get_deps_dir()
tdx_dcap = TDXDcap(test, params, vm)
tdx_dcap.verify_dcap_attestation(session, deps_dir)
session.close()
vm.destroy()
# Verify TDX enabled in guest
error_context.context("Verify TDX enabled in guest", test.log.info)
guest_check_cmd = params["tdx_guest_check"]
session = None
try:
session = vm.wait_for_login(timeout=timeout)
session.cmd_output(guest_check_cmd, timeout=240)
# Verify TDX attestation
error_context.context("Verify TDX attestation", test.log.info)
deps_dir = virttest_data_dir.get_deps_dir()
tdx_dcap = TDXDcap(test, params, vm)
tdx_dcap.verify_dcap_attestation(session, deps_dir)
except Exception as e:
test.fail("Guest tdx verify fail: %s" % str(e))
finally:
if session:
session.close()
vm.destroy()
🤖 Prompt for AI Agents
In `@qemu/tests/tdx_pccs.py` around lines 62 - 79, The current flow may leave the
guest session open or VM running on exceptions (during wait_for_login or
tdx_dcap.verify_dcap_attestation); wrap the verification steps in a try/finally
(or add a top-level try/except/finally) so that session.close() and vm.destroy()
are always executed: ensure you check session is not None before calling
session.close(), and always call vm.destroy() (or vm.cleanup equivalent) in the
finally block; update the block around vm.wait_for_login, session.cmd_output,
and TDXDcap.verify_dcap_attestation (references: session, vm,
TDXDcap.verify_dcap_attestation, tdx_dcap) to perform cleanup regardless of
errors.

@zixi-chen zixi-chen force-pushed the tdx_pccs branch 3 times, most recently from 7d42ca0 to 00c5906 Compare January 30, 2026 08:36
@zixi-chen zixi-chen marked this pull request as ready for review January 30, 2026 08:59
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@qemu/deps/tdx/pccsadmin_test.sh`:
- Around line 21-41: The script uses AdminTokenHash in the expect block but
never defines or validates it; update pccsadmin_test.sh to derive/assign
AdminTokenHash before any expect calls (e.g., from an environment variable or
computed value), and add explicit checks that required env vars like
PCCS_PRIMARY_API_KEY and AdminTokenHash are set (exit with a clear error if
missing) so the pccsadmin put step cannot run with an empty password; reference
AdminTokenHash and the earlier PCCS_PRIMARY_API_KEY usage to locate where to
insert the validation and assignment.
🧹 Nitpick comments (1)
provider/tdx.py (1)

270-276: Avoid failing when the target PCCS port is already in use.

If PCCS is already running on the configured port, this pre-check will fail the test even though the port is valid. Consider warning (or skipping the check) and relying on service restart failures to catch real conflicts.

💡 One possible adjustment
-            if sock.connect_ex(('localhost', int(pccs_port))) == 0:
-                sock.close()
-                self._test.fail("Port %s is already in use" % pccs_port)
+            if sock.connect_ex(('localhost', int(pccs_port))) == 0:
+                sock.close()
+                self._test.log.info(
+                    "Port %s already in use; assuming PCCS is running", pccs_port
+                )
+                return

Comment on lines +21 to +41
spawn pccsadmin fetch
expect \"Please input ApiKey for Intel PCS:\"
send \"$PCCS_PRIMARY_API_KEY\r\"
expect \"Would you like to remember Intel PCS ApiKey in OS keyring? (y/n)\"
send \"n\r\"
expect eof

# Check if platform_collaterals.json was generated
if [ ! -f platform_collaterals.json ]; then
echo "Pccsadmin fetch failed, platform_collaterals.json was not generated."
exit 1
fi

# pccsadmin put
expect -c "
set timeout 60
spawn pccsadmin put
expect \"Please input your administrator password for PCCS service:\"
send \"$AdminTokenHash\r\"
expect \"Would you like to remember password in OS keyring? (y/n)\"
send \"n\r\"
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Define AdminTokenHash (and validate required env vars) before use.

AdminTokenHash is never assigned in this script, so the expect input is empty unless the caller sets it. Add explicit env validation and assignment to prevent silent auth failures.

🔧 Proposed fix
 cd "$TEST_DIR"
 
+# Required env vars
+: "${PCCS_PRIMARY_API_KEY:?Missing PCCS_PRIMARY_API_KEY}"
+: "${PCCS_ADMIN_TOKEN_HASH:?Missing PCCS_ADMIN_TOKEN_HASH}"
+AdminTokenHash="${PCCS_ADMIN_TOKEN_HASH}"
+
 # Start tests
 PCKIDRetrievalTool
 pccsadmin collect
🧰 Tools
🪛 Shellcheck (0.11.0)

[warning] 36-36: To assign a variable, use just 'var=value', no 'set ..'.

(SC2121)


[warning] 39-39: AdminTokenHash is referenced but not assigned.

(SC2154)


[warning] 39-39: \r is just literal 'r' here. For carriage return, use "$(printf '\r')" instead.

(SC1012)


[error] 40-40: '(' is invalid here. Did you forget to escape it?

(SC1036)


[error] 40-40: Parsing stopped here. Invalid use of parentheses?

(SC1088)

🤖 Prompt for AI Agents
In `@qemu/deps/tdx/pccsadmin_test.sh` around lines 21 - 41, The script uses
AdminTokenHash in the expect block but never defines or validates it; update
pccsadmin_test.sh to derive/assign AdminTokenHash before any expect calls (e.g.,
from an environment variable or computed value), and add explicit checks that
required env vars like PCCS_PRIMARY_API_KEY and AdminTokenHash are set (exit
with a clear error if missing) so the pccsadmin put step cannot run with an
empty password; reference AdminTokenHash and the earlier PCCS_PRIMARY_API_KEY
usage to locate where to insert the validation and assignment.

@zixi-chen zixi-chen marked this pull request as draft January 30, 2026 09:03
Signed-off-by: Zixi Chen <zixchen@redhat.com>
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.

1 participant