fix: use hmac.compare_digest() for timing-safe HMAC comparison

[benthomasson]

Summary

• WebsocketSecretAuthHelper.verify_secret() compared HMAC digests using != which is vulnerable to timing attacks
• Replace with hmac.compare_digest() for constant-time comparison
• hmac is already imported — this is a one-line fix

Files changed

File	Change
awx/main/consumers.py	!= → hmac.compare_digest()

Fixes: AAP-68043

Test plan

• Verify WebSocket relay connections still authenticate successfully
• Verify invalid secrets are still rejected

Change Type

• Bug, Docs Fix or other nominal change

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved secret verification security to protect against timing-based attacks.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a4a2b3cf-d644-42b7-ab2b-153f49e2e499

📥 Commits

Reviewing files that changed from the base of the PR and between 0d08a4d and 1c44df2.

📒 Files selected for processing (1)

• awx/main/consumers.py

---

📝 Walkthrough

Walkthrough

A single function in the WebSocket consumer replaces direct string equality checking with constant-time comparison using hmac.compare_digest to prevent timing-based attacks when verifying secrets. All existing logic, error handling, and parsing remain unchanged.

Changes

Cohort / File(s)	Summary
Security Improvement awx/main/consumers.py	Replaced direct string equality check with hmac.compare_digest() in the verify_secret function to use constant-time comparison, mitigating timing attack vulnerabilities while preserving all error handling and control flow.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely describes the main change: replacing a direct string comparison with hmac.compare_digest() for timing-safe HMAC verification.

_{✏️ 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[AlanCoding]

@coderabbitai do existing unit tests cover this method, giving validation that regressions did not occur?

[AlanCoding]

SonarCloud seems to think it is covered.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for test coverage of verify_secret and WebsocketSecretAuthHelper
echo "=== Searching for verify_secret in tests ==="
rg -n "verify_secret" --type python -C3

echo ""
echo "=== Searching for WebsocketSecretAuthHelper in tests ==="
rg -n "WebsocketSecretAuthHelper" --type python -C3

echo ""
echo "=== Searching for consumers tests ==="
fd -e py "test.*consumer" -i
fd -e py "consumer.*test" -i

echo ""
echo "=== Searching for hmac in tests ==="
rg -n "hmac" --type python -g "*test*" -C3

Length of output: 271

---

🏁 Script executed:

#!/bin/bash
echo "=== Searching for verify_secret ==="
rg -n "verify_secret" -g "*.py" -C3

echo ""
echo "=== Searching for WebsocketSecretAuthHelper ==="
rg -n "WebsocketSecretAuthHelper" -g "*.py" -C3

echo ""
echo "=== Finding consumer-related test files ==="
fd -e py . -g "*consumer*"

echo ""
echo "=== Searching for hmac in test files ==="
rg -n "hmac" -g "*.py" -g "*test*" -C3

echo ""
echo "=== Show the verify_secret method implementation ==="
rg -n "verify_secret\|WebsocketSecretAuthHelper" -g "*.py" awx/main/consumers.py -C10

Length of output: 9514

---

🏁 Script executed:

#!/bin/bash
echo "=== Full content of test_routing.py ==="
cat awx/main/tests/functional/test_routing.py

echo ""
echo "=== Search for any other test files that test verify_secret or consumers ==="
rg -rn "verify_secret\|construct_secret\|WebsocketSecretAuthHelper" -g "*.py" --with-filename

Length of output: 3466

---

@AlanCoding: There are existing functional/integration tests that exercise verify_secret() indirectly, providing regression coverage. They live in awx/main/tests/functional/test_routing.py under TestWebsocketRelay:

Test	What it validates
test_authorized	Valid secret → connection succeeds
test_not_authorized	Missing secret → connection refused
test_wrong_secret	Wrong secret → connection refused

These tests flow through RelayConsumer.connect() → WebsocketSecretAuthHelper.verify_secret(), so they will catch any regression where a valid secret is incorrectly rejected or an invalid secret is incorrectly accepted.

However, there are no direct unit tests that call verify_secret() in isolation. The hmac.compare_digest() behavior itself (constant-time comparison) cannot be meaningfully tested with a unit test since timing properties aren't observable that way.

In summary: the existing tests are sufficient to validate that the functional behavior hasn't regressed (valid secrets still work, invalid secrets are still rejected), but a dedicated unit test for verify_secret() is currently absent from the test suite. It may be worth adding one as a follow-up.

[AlanCoding]

@coderabbitai

What I need is a test that gives some input values where the keys match. And then the comparison finds they match. It's not at all obvious to me how the keys are produced in this indirect testing, and if the pivotal methods are not mocked. The coverage could be totally superficial.

So what I'm looking for is a happy path test, which test_authorized is the best patch for. And it is a gargantuan red flag.

    @pytest.mark.xfail(reason="Ran out of coding time.")
    async def test_authorized(self, websocket_server_generator, application, admin):
        server = websocket_server_generator('/websocket/')

        """
        I ran out of time. Here is what I was thinking ...
        Inject a valid session into the cookies in the header

        server.scope['headers'] = (
            (b'cookie', ...),
        )
        """
        connected, _ = await server.connect()
        assert connected is True, "User should be allowed in via cookies auth via a session key in the cookies"

Not okay with this. Nope no no.

[AlanCoding]

Need some testing confirmation that this is regression-free and research on existing tests isn't doing it for me.