Tests: Smart card authentication tests for SSSD's CKF_PROTECTED_AUTH… - #8744
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of system tests for SSSD smart card pinpad authentication, covering scenarios such as successful login via su, correct external keypad prompting, local user authentication, and keyboard-less PIN entry. The review feedback correctly identifies a security vulnerability where sensitive private key files are written to /tmp with default permissions and are not cleaned up after execution. It is recommended to restrict these files to 0600 permissions and ensure they are deleted in a finally block to prevent credential leakage.
| client.fs.write(pem_cert, cert_content) | ||
| client.fs.write(pem_key, key_content) | ||
|
|
||
| x509_args: CLIBuilderArgs = { | ||
| "in": (client.host.cli.option.VALUE, pem_cert), | ||
| "outform": (client.host.cli.option.VALUE, "DER"), | ||
| "out": (client.host.cli.option.VALUE, der_cert), | ||
| } | ||
| client.host.conn.run(client.host.cli.command("openssl x509", x509_args)) | ||
|
|
||
| rsa_args: CLIBuilderArgs = { | ||
| "in": (client.host.cli.option.VALUE, pem_key), | ||
| "outform": (client.host.cli.option.VALUE, "DER"), | ||
| "out": (client.host.cli.option.VALUE, der_key), | ||
| } | ||
| client.host.conn.run(client.host.cli.command("openssl rsa", rsa_args)) | ||
|
|
||
| for obj_path, obj_type in [(der_key, "privkey"), (der_cert, "cert")]: | ||
| args: CLIBuilderArgs = { | ||
| "module": (client.host.cli.option.VALUE, OPENSC_MODULE), | ||
| "login": (client.host.cli.option.SWITCH, True), | ||
| "pin": (client.host.cli.option.VALUE, pin), | ||
| "write-object": (client.host.cli.option.VALUE, obj_path), | ||
| "type": (client.host.cli.option.VALUE, obj_type), | ||
| "id": (client.host.cli.option.VALUE, cert_id), | ||
| "label": (client.host.cli.option.VALUE, username), | ||
| } | ||
| client.host.conn.run(client.host.cli.command("pkcs11-tool", args)) |
There was a problem hiding this comment.
The private key files are written to /tmp with default permissions, which can make them readable by other local users on the system. Additionally, these temporary files are never cleaned up from the filesystem after the test completes, leading to sensitive credential leakage.
To resolve this, restrict the permissions of the private key files to 0600 immediately after creation, and ensure all temporary files are cleaned up in a finally block.
try:
client.fs.write(pem_cert, cert_content)
client.fs.write(pem_key, key_content)
client.host.conn.run(f"chmod 600 {pem_key}", raise_on_error=False)
x509_args: CLIBuilderArgs = {
"in": (client.host.cli.option.VALUE, pem_cert),
"outform": (client.host.cli.option.VALUE, "DER"),
"out": (client.host.cli.option.VALUE, der_cert),
}
client.host.conn.run(client.host.cli.command("openssl x509", x509_args))
rsa_args: CLIBuilderArgs = {
"in": (client.host.cli.option.VALUE, pem_key),
"outform": (client.host.cli.option.VALUE, "DER"),
"out": (client.host.cli.option.VALUE, der_key),
}
client.host.conn.run(client.host.cli.command("openssl rsa", rsa_args))
client.host.conn.run(f"chmod 600 {der_key}", raise_on_error=False)
for obj_path, obj_type in [(der_key, "privkey"), (der_cert, "cert")]:
args: CLIBuilderArgs = {
"module": (client.host.cli.option.VALUE, OPENSC_MODULE),
"login": (client.host.cli.option.SWITCH, True),
"pin": (client.host.cli.option.VALUE, pin),
"write-object": (client.host.cli.option.VALUE, obj_path),
"type": (client.host.cli.option.VALUE, obj_type),
"id": (client.host.cli.option.VALUE, cert_id),
"label": (client.host.cli.option.VALUE, username),
}
client.host.conn.run(client.host.cli.command("pkcs11-tool", args))
finally:
client.host.conn.run(f"rm -f {pem_cert} {pem_key} {der_cert} {der_key}", raise_on_error=False)| try: | ||
| req_args: CLIBuilderArgs = { | ||
| "x509": (client.host.cli.option.SWITCH, True), | ||
| "nodes": (client.host.cli.option.SWITCH, True), | ||
| "sha256": (client.host.cli.option.SWITCH, True), | ||
| "days": (client.host.cli.option.VALUE, "365"), | ||
| "newkey": (client.host.cli.option.VALUE, "rsa:2048"), | ||
| "keyout": (client.host.cli.option.VALUE, key_pem), | ||
| "out": (client.host.cli.option.VALUE, cert_pem), | ||
| "subj": (client.host.cli.option.VALUE, "/CN=Pinpad Test Cert"), | ||
| } | ||
| client.host.conn.run(client.host.cli.command("openssl req", req_args)) | ||
|
|
||
| x509_args: CLIBuilderArgs = { | ||
| "in": (client.host.cli.option.VALUE, cert_pem), | ||
| "outform": (client.host.cli.option.VALUE, "DER"), | ||
| "out": (client.host.cli.option.VALUE, cert_der), | ||
| } | ||
| client.host.conn.run(client.host.cli.command("openssl x509", x509_args)) | ||
|
|
||
| rsa_args: CLIBuilderArgs = { | ||
| "in": (client.host.cli.option.VALUE, key_pem), | ||
| "outform": (client.host.cli.option.VALUE, "DER"), | ||
| "out": (client.host.cli.option.VALUE, key_der), | ||
| } | ||
| client.host.conn.run(client.host.cli.command("openssl rsa", rsa_args)) | ||
|
|
||
| for obj_path, obj_type in [(key_der, "privkey"), (cert_der, "cert")]: | ||
| args: CLIBuilderArgs = { | ||
| "module": (client.host.cli.option.VALUE, OPENSC_MODULE), | ||
| "login": (client.host.cli.option.SWITCH, True), | ||
| "pin": (client.host.cli.option.VALUE, TOKEN_PIN), | ||
| "write-object": (client.host.cli.option.VALUE, obj_path), | ||
| "type": (client.host.cli.option.VALUE, obj_type), | ||
| "id": (client.host.cli.option.VALUE, cert_id), | ||
| "label": (client.host.cli.option.VALUE, username), | ||
| } | ||
| client.host.conn.run(client.host.cli.command("pkcs11-tool", args)) | ||
|
|
||
| ensure_pcscd_accessible(client) | ||
|
|
||
| client.host.fs.rm("/etc/sssd/pki/sssd_auth_ca_db.pem") | ||
| cert_data = client.host.fs.read(cert_pem) | ||
| client.host.fs.append("/etc/sssd/pki/sssd_auth_ca_db.pem", cert_data) | ||
|
|
||
| client.authselect.select("sssd", ["with-smartcard"]) | ||
| client.sssd.common.local() | ||
| client.sssd.dom("local")["local_auth_policy"] = "only" | ||
| client.sssd.section(f"certmap/local/{username}")["matchrule"] = ( | ||
| "<SUBJECT>.*CN=Pinpad Test Cert.*" | ||
| ) | ||
| client.sssd.pam["pam_cert_auth"] = "True" | ||
| client.sssd.pam["p11_child_timeout"] = "60" | ||
| client.sssd.start(debug_level="0xFFF0", check_config=False) | ||
|
|
||
| result = client.host.conn.run( | ||
| f"su - {username} -c 'su - {username} -c whoami'", | ||
| raise_on_error=False, | ||
| ) | ||
|
|
||
| output = result.stdout + result.stderr | ||
| assert "Use external keypad" in output, ( | ||
| f"Expected 'Use external keypad' prompt but got: " | ||
| f"stdout={result.stdout!r}, stderr={result.stderr!r}" | ||
| ) | ||
| assert result.rc == 0, ( | ||
| f"Authentication failed with rc={result.rc}, " | ||
| f"stdout={result.stdout!r}, stderr={result.stderr!r}" | ||
| ) | ||
| assert username in result.stdout, ( | ||
| f"'{username}' not found in whoami output: {result.stdout}" | ||
| ) | ||
| finally: | ||
| cleanup_hw_card(client, cert_id=cert_id) |
There was a problem hiding this comment.
The generated self-signed private key files are written to /tmp with default permissions and are never cleaned up from the filesystem after the test completes, leading to sensitive credential leakage.
To resolve this, restrict the permissions of the private key files to 0600 immediately after creation, and ensure all temporary files are cleaned up in the finally block.
try:
req_args: CLIBuilderArgs = {
"x509": (client.host.cli.option.SWITCH, True),
"nodes": (client.host.cli.option.SWITCH, True),
"sha256": (client.host.cli.option.SWITCH, True),
"days": (client.host.cli.option.VALUE, "365"),
"newkey": (client.host.cli.option.VALUE, "rsa:2048"),
"keyout": (client.host.cli.option.VALUE, key_pem),
"out": (client.host.cli.option.VALUE, cert_pem),
"subj": (client.host.cli.option.VALUE, "/CN=Pinpad Test Cert"),
}
client.host.conn.run(client.host.cli.command("openssl req", req_args))
client.host.conn.run(f"chmod 600 {key_pem}", raise_on_error=False)
x509_args: CLIBuilderArgs = {
"in": (client.host.cli.option.VALUE, cert_pem),
"outform": (client.host.cli.option.VALUE, "DER"),
"out": (client.host.cli.option.VALUE, cert_der),
}
client.host.conn.run(client.host.cli.command("openssl x509", x509_args))
rsa_args: CLIBuilderArgs = {
"in": (client.host.cli.option.VALUE, key_pem),
"outform": (client.host.cli.option.VALUE, "DER"),
"out": (client.host.cli.option.VALUE, key_der),
}
client.host.conn.run(client.host.cli.command("openssl rsa", rsa_args))
client.host.conn.run(f"chmod 600 {key_der}", raise_on_error=False)
for obj_path, obj_type in [(key_der, "privkey"), (cert_der, "cert")]:
args: CLIBuilderArgs = {
"module": (client.host.cli.option.VALUE, OPENSC_MODULE),
"login": (client.host.cli.option.SWITCH, True),
"pin": (client.host.cli.option.VALUE, TOKEN_PIN),
"write-object": (client.host.cli.option.VALUE, obj_path),
"type": (client.host.cli.option.VALUE, obj_type),
"id": (client.host.cli.option.VALUE, cert_id),
"label": (client.host.cli.option.VALUE, username),
}
client.host.conn.run(client.host.cli.command("pkcs11-tool", args))
ensure_pcscd_accessible(client)
client.host.fs.rm("/etc/sssd/pki/sssd_auth_ca_db.pem")
cert_data = client.host.fs.read(cert_pem)
client.host.fs.append("/etc/sssd/pki/sssd_auth_ca_db.pem", cert_data)
client.authselect.select("sssd", ["with-smartcard"])
client.sssd.common.local()
client.sssd.dom("local")["local_auth_policy"] = "only"
client.sssd.section(f"certmap/local/{username}")["matchrule"] = (
"<SUBJECT>.*CN=Pinpad Test Cert.*"
)
client.sssd.pam["pam_cert_auth"] = "True"
client.sssd.pam["p11_child_timeout"] = "60"
client.sssd.start(debug_level="0xFFF0", check_config=False)
result = client.host.conn.run(
f"su - {username} -c 'su - {username} -c whoami'",
raise_on_error=False,
)
output = result.stdout + result.stderr
assert "Use external keypad" in output, (
f"Expected 'Use external keypad' prompt but got: "
f"stdout={result.stdout!r}, stderr={result.stderr!r}"
)
assert result.rc == 0, (
f"Authentication failed with rc={result.rc}, "
f
3d8ac0b to
d0a8fd2
Compare
|
Removed the Test result: |
32b9f1b to
7231e69
Compare
spoore1
left a comment
spoore1
left a comment
There was a problem hiding this comment.
Overall looks good. I have a few questions/suggestions in-line.
sssd-bot
left a comment
sssd-bot
left a comment
There was a problem hiding this comment.
Review done using Claude Code / claude-opus-4-6
Functional Issues
-
sucommand may hang or time out without stdin input (test_smartcard_pinpad.py:207-210): Thesucommand is invoked without aninput=parameter, relying on the physical pinpad for PIN entry. In a CI environment where no human is pressing buttons on a hardware keypad, this command will block indefinitely waiting for PIN entry, eventually timing out. If these tests are intended for manual-only execution on a machine with a physical pinpad reader, this should be clearly documented with a marker or skip condition beyonddetect_pinpad_reader(). If CI execution is intended, the test needs a mechanism to simulate pinpad entry or a timeout with a clear skip/failure message. -
detect_pinpad_readerusesresultafter the loop without guaranteed assignment (test_smartcard_pinpad.py:90-95): After thefor attempt in range(2)loop, the code accessesresult.stdoutat line 91. Ifattempt == 0succeeds (thebreakat line 78),resultis correctly set. But the variableresultis only assigned inside the loop body (line 73-76). If Python somehow skips the loop (e.g.,range(2)were changed torange(0)),resultwould be unbound. While currently safe withrange(2), the logic is fragile -- consider assigningresult = Nonebefore the loop or restructuring to make the data flow clearer. -
chmod 666on all USB devices is overly broad and not reverted (test_smartcard_pinpad.py:44): The_reset_usb_devicesfunction sets world-read-write permissions on every USB device node (/dev/bus/usb/*/*). This is a security concern as flagged by reviewer spoore1 -- it grants any user on the system access to all USB devices (keyboards, storage, etc.), and the permissions are never restored. Even in a test environment, this could cause unexpected interactions. At minimum, save and restore the original permissions, or target only the specific smart card reader device. -
Package installation in test code (
test_smartcard_pinpad.py:60):detect_pinpad_readerinstallsccidviadnf install -y ccidat runtime. This was flagged by reviewer spoore1 -- tests should not install packages. Package dependencies should be declared in the CI container configuration (sssd-ci-containers packages role). Ifccidis missing, the test should skip rather than attempt installation. -
No
input=forsumeans authentication will fail for the outersu(test_smartcard_pinpad.py:207-209): The nestedsu - username -c 'su - username -c whoami'command requires authentication twice. The outersuneeds the user's password/PIN, and the innersualso needs it. With a real pinpad reader, the user physically enters the PIN. But with noinput=parameter, ifsufalls back to expecting stdin input for any reason (e.g., pinpad not detected by PAM), the command will hang. There should be a timeout on thisconn.runcall.
Nits & Non-functional Issues
-
Hardcoded PAM log path instead of using framework accessor (
test_smartcard_pinpad.py:230): The test reads the PAM log viaclient.fs.read("/var/log/sssd/sssd_pam.log"). Other tests in the codebase use the framework-providedclient.sssd.logs.pamaccessor (seetest_passkey.py:521,test_ipa.py:384). Using the accessor is more robust against path changes. -
Missing
@pytest.mark.builtwithmarker (test_smartcard_pinpad.py:172-173): The existingtest_smartcard.pytests that useKnownTopology.Clientinclude@pytest.mark.builtwith(client="virtualsmartcard")(or a similar marker). This test lacks anybuiltwithmarker. While pinpad tests may need a different marker (since they require real hardware rather than virtual smart cards), the absence should be intentional and documented. A custom marker or topology requirement would help the CI system skip these tests when no pinpad hardware is available. -
sedcommands via rawconn.runinstead of framework utilities (test_smartcard_pinpad.py:110-115): Reviewer spoore1 suggested usinghost.fs.sed()if available. The rawsed -icommands are functional but bypass the test framework's abstraction layer. Using framework utilities is preferred for consistency and better error handling. -
Temporary files in
/tmpnot cleaned up (test_smartcard_pinpad.py:127-128): Theenroll_card()function writes a private key to/tmp/pinpad_test.keyand a certificate to/tmp/pinpad_test.crtbut never removes them. While the existingsetup_local_card()in the framework has a similar pattern, explicitly cleaning up sensitive files (especially the private key) after use is good practice. -
Inconsistent use of
client.host.fsvsclient.fs(test_smartcard_pinpad.py:167-169vs230,237): Inenroll_card(), the code usesclient.host.fs.rm(),client.host.fs.read(), andclient.host.fs.append(). In the test function, it usesclient.fs.read(). While both may resolve to the same underlying filesystem, the inconsistency is confusing. The existing framework code insetup_local_card()usesclient.host.fs.*, which suggests that should be the pattern in helper functions, while test functions commonly useclient.fs.*. -
USB_RESET_SCRIPTbareexcept(test_smartcard_pinpad.py:32): The inline Python script uses a bareexcept: passwhich silently swallows all exceptions includingKeyboardInterruptandSystemExit. While this is intentional (best-effort reset), usingexcept Exception: passwould be more correct Python style. -
Single test case provides limited coverage (
test_smartcard_pinpad.py:174): The file contains only one test (test_smartcard_pinpad__su_as_local_user). Consider adding negative test cases, such as: pinpad disabled (should fall back to keyboard PIN), wrong certificate on card, or testing withlocal_auth_policyset to values other than"only". -
client.sssd.dom("local")["local_auth_policy"] = "only"repeats setup already incommon.local()(test_smartcard_pinpad.py:200-201):client.sssd.common.local()already configures thelocaldomain with proxy provider. Then line 201 setslocal_auth_policy. The existingsetup_local_card()in the framework does the same pattern, so this is consistent but worth noting that the test largely reimplementssetup_local_card()logic inline.
Review of Existing Review Comments
-
sumit-bose on token label (line 16): Addressed -- the token label was changed from
"MyEID"to"SSSD Test Token"as suggested. -
sumit-bose on token URI matching (line 42): Addressed --
detect_pinpad_readernow checkstoken_uri = f"token={TOKEN_LABEL.replace(' ', '%20')}"against the output (line 90-95), which is a more specific match than just checking for "token" in stdout. -
sumit-bose on dropping
ensure_pcscd_accessible(line 49): Addressed -- the function was removed and its checks are now done withindetect_pinpad_reader()running as thesssduser viarunuser -u sssd. -
sumit-bose on
make up-smartcard-pinpad(line 69): The comment asked about CI container support. The PR author acknowledged the oversight. This remains unresolved -- there is no corresponding sssd-ci-containers PR to provide the test infrastructure. The test will silently skip in CI viadetect_pinpad_reader(), but the missing container support means the test is effectively untestable in CI. -
sumit-bose on re-adding
enable_pinpadmethod and cert enrollment (lines 64, 91): Partially addressed --enroll_card()andenable_opensc_pinpad()functions are present in the current version. However, sumit-bose's suggestion to "assume that everything has to be setup before the test is run" has not been adopted -- the test still performs card enrollment and OpenSC configuration inline. -
spoore1 on USB device permission changes (line 44): Not addressed --
chmod 666 /dev/bus/usb/*/*is still present without saving/restoring original permissions. -
spoore1 on package installation (line 60): Not addressed --
dnf install -y ccidis still performed inline in the test code. -
spoore1 on using
host.fs.sed()(line 115): Not addressed -- rawsedcommands viaconn.run()are still used. -
spoore1 on using
client.auth.su.smartcard_with_output()(lines 207-210): Not directly applicable -- the existingsmartcard_with_output()utility passes a PIN viainput=, which defeats the purpose of testing pinpad authentication where no stdin PIN input should occur. However, the test could benefit from a new framework utility method for pinpad authentication to avoid rawconn.runcalls. -
gemini-code-assist on temp file permissions and cleanup (lines 134-161, 456-529): Not addressed -- private key files are still written to
/tmpwithout restricted permissions and without cleanup. -
spoore1's CHANGES_REQUESTED status: Multiple suggestions remain unaddressed.
9cd42a5 to
a5a6650
Compare
|
Hi, thank you for the updates, the test is working for me. May I ask how you make the Smartcard reader available in the SSSD-CI client container? It looks like you make the USB bus available. I just forward the PCSCD socket I tend to ACK the patch. I'm just wondering, after seeing the Smartcard related variable declaration at the beginning of the file if e.g. bye, |
I tried by mounting the USB bus directly into the container (/dev/bus/usb:/dev/bus/usb:rw) with cgroup device rules (c 189:* rmw) in yml, and run pcscd inside the container to communicate with the reader. |
spoore1
left a comment
spoore1
left a comment
There was a problem hiding this comment.
It looks like my comment/suggestion about using the auth.su.smartcard_with_output() was incorrect. sssd-bot report points out that it passes the PIN via keyboard not necessarily testing the keypad entry. I believe you should put back the original host.conn.run() that didn't pass the PIN. That is unless you are adding updates to sssd-test-framework as well. Besides that. LGTM.
a5a6650 to
20b0165
Compare
@sumit-bose @krishnavema |
spoore1
left a comment
spoore1
left a comment
There was a problem hiding this comment.
I don't have a device to test with but, it LGTM.
Hi, yes, I think this can be done in a later PR by using the current values as defaults. bye, |
Hi, ok, I tested with the variant as well and it is working as expected. bye, |
sumit-bose
left a comment
sumit-bose
left a comment
There was a problem hiding this comment.
Hi,
thank you for the test and all the updates, ACK.
bye,
Sumit
|
Hi, I think the CI failures are all known issues, setting 'Accepted'. bye, |
…ENTICATION_PATH (hardware pinpad) support Reviewed-by: Scott Poore <spoore@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
5 participants
Smart card authentication tests for SSSD's CKF_PROTECTED_AUTHENTICATION_PATH (hardware pinpad) support, verifying that p11_child detects the flag, passes NULL PIN to C_Login, and PAM prompts for external keypad
entry instead of keyboard PIN input.