Issue 6753 - Port ticket 48194 test

[mirielka]

Description:
Porting ticket 48194 test to dirsrvtests/tests/suites/tls/cipher_test.py

Relates: #6753
Author: Lenka Doudova
Reviewer: ???

Summary by Sourcery

Port TLS cipher policy tests from the ticket-specific suite into the shared TLS cipher_test suite and remove the old ticket48194 test file.

New Features:

• Add parametrized TLS cipher policy tests validating weak/strong cipher handshake behavior under various nsSSL3Ciphers and allowWeakCipher configurations.
• Introduce helper and fixture logic to drive openssl-based TLS connections against the standalone instance for cipher validation.

Tests:

• Add multiple TLS cipher policy integration tests covering default, custom, and invalid cipher suite configurations, including failure cases when all ciphers are disabled or unsupported ciphers are requested.
• Remove the legacy ticket-specific test file ticket48194_test.py now that its coverage is provided in the tls cipher_test suite.

[mirielka]

This needs to be reviewed if the tests are even valid, also there are some failed assertions on handshake and I am not certain whether it's desired change in behavior or a bug.

[sourcery-ai]

Hey - I've found 4 issues, and left some high level feedback:

• The connectWithOpenssl helper is fairly brittle: it can hang indefinitely on readline() and the Popen error path references proc after a failed spawn; consider adding a timeout/communicate()-based read and ensuring the process is safely cleaned up only if it was successfully created.
• Instead of using assert True/assert False branches inside connectWithOpenssl, assert directly on the condition (e.g. assert (b'(NONE)' not in l) is expect or similar) to make the test logic clearer and failures easier to interpret.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `connectWithOpenssl` helper is fairly brittle: it can hang indefinitely on `readline()` and the `Popen` error path references `proc` after a failed spawn; consider adding a timeout/`communicate()`-based read and ensuring the process is safely cleaned up only if it was successfully created.
- Instead of using `assert True`/`assert False` branches inside `connectWithOpenssl`, assert directly on the condition (e.g. `assert (b'(NONE)' not in l) is expect` or similar) to make the test logic clearer and failures easier to interpret.

## Individual Comments

### Comment 1
<location path="dirsrvtests/tests/suites/tls/cipher_test.py" line_range="87-88" />
<code_context>
+    strcmdline = " ".join(cmdline)
+    log.info(f"Running cmdline: {strcmdline}")
+
+    try:
+        proc = subprocess.Popen(cmdline, stdout=subprocess.PIPE, stdin=subprocess.PIPE, stderr=subprocess.STDOUT)
+    except ValueError:
+        log.info(f"{cmdline} failed: {ValueError}")
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Exception handling around `Popen` is fragile and may reference an undefined process object

In `connectWithOpenssl`, the `except ValueError:` block logs the exception type instead of the actual error, and calls `proc.kill()` even though `proc` may not exist if `Popen` failed before assignment. This can cause an `UnboundLocalError` and hide the original failure. Please capture the exception and avoid using `proc` unless it’s guaranteed to be defined, e.g.:

```python
try:
    proc = subprocess.Popen(...)
except ValueError as e:
    log.info("%s failed: %s", cmdline, e)
    return pytest.fail("Failed to spawn openssl s_client")
```

Then only call `kill()`/`wait()` in code paths where `proc` is known to be initialized.

Suggested implementation:

```python
    try:
        proc = subprocess.Popen(cmdline, stdout=subprocess.PIPE, stdin=subprocess.PIPE, stderr=subprocess.STDOUT)
    except ValueError as e:
        log.info("%s failed: %s", cmdline, e)
        pytest.fail("Failed to spawn openssl s_client")

```

1. Ensure `import pytest` is present at the top of `dirsrvtests/tests/suites/tls/cipher_test.py`. If it is not already imported, add:
   `import pytest`
</issue_to_address>

### Comment 2
<location path="dirsrvtests/tests/suites/tls/cipher_test.py" line_range="113-122" />
<code_context>
+                    assert False
+
+
+@pytest.fixture(scope='function')
+def setup_cipher_test(request, topo):
+    topo.standalone.enable_tls()
+    topo.standalone.restart()
+    topo.standalone.simple_bind_s(DN_DM, PASSWORD)
+
+    def fin():
+        try:
+            topo.standalone.encryption.set('nsSSL3Ciphers', b'default')
+            topo.standalone.restart()
+        except ValueError:
+            dse = DSEldif(topo.standalone)
+            topo.standalone.stop()
+            dse.replace(ENC_DN, 'nsSSL3Ciphers', b'default')
+            topo.standalone.use_ldap_uri()
+            topo.standalone.start()
+    request.addfinalizer(fin)
+
+
</code_context>
<issue_to_address>
**issue (testing):** Fixture cleanup only restores `nsSSL3Ciphers`, leaving other TLS-related settings to leak between tests

Several tests modify additional TLS attributes (`allowWeakCipher`, `nsslapd-errorlog-level`, and sometimes `nsSSL3Ciphers`) without consistently restoring their original values (e.g. `test_cipher_policy_5`, `test_cipher_policy_8`). To avoid cross-test leakage and ordering dependence, consider extending this fixture’s finalizer to reset all TLS-related attributes touched by these tests, or ensure each test that changes them saves and restores the prior state in a `finally` block.
</issue_to_address>

### Comment 3
<location path="dirsrvtests/tests/suites/tls/cipher_test.py" line_range="198" />
<code_context>
+                         ('AES256-SHA256', False),
+                         ('AES128-SHA', True),
+                         ('AES256-SHA', True)])
+def test_cipher_policy_2(topo, cipher, expect):
+    """Verify TLS handshakes when only rsa_aes_128_sha and rsa_aes_256_sha ciphers are enabled
+
+    :id: 0694da90-f67c-4243-abe7-fc9ee063c1a3
+    :setup: Standalone Instance
+    :steps:
+        1. Enable only rsa_aes_128_sha and rsa_aes_256_sha via change_ciphers, then restart
+        2. Use openssl s_client with each parameterized cipher and expected outcome
+    :expectedresults:
+        1. Success
+        2. Handshake outcome matches the expect parameter for each cipher
+    """
+    topo.standalone.simple_bind_s(DN_DM, PASSWORD)
+
+    topo.standalone.encryption.change_ciphers('+', ['rsa_aes_128_sha','rsa_aes_256_sha'])
+    topo.standalone.restart()
+
+    connectWithOpenssl(topo, cipher, expect)
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** `test_cipher_policy_2` does not use the TLS setup fixture, which may lead to state leakage across tests

Here the test modifies `nsSSL3Ciphers` via `change_ciphers` and restart, but unlike the other TLS tests it doesn’t use the `setup_cipher_test` fixture to reset configuration. That means its cipher changes may leak into later tests depending on execution order.

Please either:
- Add `setup_cipher_test` as a fixture argument so TLS state is automatically restored, or
- Manually save and restore `nsSSL3Ciphers` in a `try/finally` block.

This keeps the test suite order-independent and avoids hidden coupling between tests.

```suggestion
def test_cipher_policy_2(topo, setup_cipher_test, cipher, expect):
```
</issue_to_address>

### Comment 4
<location path="dirsrvtests/tests/suites/tls/cipher_test.py" line_range="132-141" />
<code_context>
+@pytest.mark.parametrize('cipher, expect',
</code_context>
<issue_to_address>
**issue (testing):** `test_cipher_policy_11`'s parametrization is unused and can be removed for clarity

The decorator parametrizes `cipher` and `expect`, but the test body never uses them, which makes the test look like it covers multiple scenarios when it doesn’t. Please either remove the parametrization (and unused parameters from the signature) or update the test to actually assert behavior based on these values.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[packit-as-a-service]

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

• sudo dnf install -y 'dnf*-command(copr)'
• dnf copr enable packit/389ds-389-ds-base-7414
• And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• The connectWithOpenssl helper has fragile process handling (e.g., no timeout, no wait()/cleanup, proc may be undefined in the except, and asserts embedded in the loop) – consider returning a boolean/raising a clear error from the helper and using a bounded read with proper process teardown to avoid hangs and undefined behavior.
• connectWithOpenssl takes a topology_st argument that is never used and relies on the global LDAPSPORT; either use the passed topology to derive the port or drop the unused parameter/global to keep the interface consistent and easier to understand.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `connectWithOpenssl` helper has fragile process handling (e.g., no timeout, no `wait()`/cleanup, `proc` may be undefined in the `except`, and asserts embedded in the loop) – consider returning a boolean/raising a clear error from the helper and using a bounded read with proper process teardown to avoid hangs and undefined behavior.
- `connectWithOpenssl` takes a `topology_st` argument that is never used and relies on the global `LDAPSPORT`; either use the passed topology to derive the port or drop the unused parameter/global to keep the interface consistent and easier to understand.

## Individual Comments

### Comment 1
<location path="dirsrvtests/tests/suites/tls/cipher_test.py" line_range="70-79" />
<code_context>
+def connectWithOpenssl(topology_st, cipher, expect):
</code_context>
<issue_to_address>
**issue (testing):** Make `connectWithOpenssl` deterministic and failure-safe (timeout, error handling, and explicit assertions)

This helper has a few issues that can cause flaky or hanging tests:

* In the `except ValueError` block, `proc.kill()` is called even though `proc` is undefined there, which will raise a new exception and hide the original problem. For test code, it would be clearer to `pytest.skip` or `pytest.fail` if `openssl` cannot be started.
* The `while True` loop has no timeout, so if `openssl` hangs or never prints `"Cipher is"`, the test can block indefinitely. Please add a timeout (e.g., via `communicate()` or elapsed-time tracking) and fail if the expected line is not seen.
* If the loop finishes without seeing `"Cipher is"`, the test currently passes. It should explicitly fail in that case.
* Instead of `assert True` / `assert False` inside the loop, use explicit conditions or `pytest.fail("expected handshake to succeed/fail")` so failures are easier to understand.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[jchapma]

Some tests are failing:
FAILED dirsrvtests/tests/suites/tls/cipher_test.py::test_cipher_policy_2[AES256-SHA256-False] - assert False
FAILED dirsrvtests/tests/suites/tls/cipher_test.py::test_cipher_policy_6[AES256-SHA256-False] - assert False

It seems to me that server cipher restriction is one issue, topo.standalone.encryption.change_ciphers('+', ['rsa_aes_128_sha','rsa_aes_256_sha']) adds these to the existing ciphers but the server still accepts AES256-SHA256

It seems some ciphers are not accepted by the server:
WARNING:Encryption:Cipher rsa_aes_128_sha is not supported.
WARNING:Encryption:Cipher rsa_aes_256_sha is not supported.

[mirielka]

Test was redone after consult since some functionalities are no longer supported with TLS1.3. The test currently mainly verifies that setting attributes works as expected.

[jchapma]

Tests all pass, LGTM