feat(xtest): adds hybrid PQ/T KEM round trip testing#427
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR introduces a comprehensive post-quantum cryptography (X-Wing hybrid) support system across multiple repositories. Changes include a new platform feature override configuration system, Keycloak TLS key/certificate/keystore generation, enhanced CLI commands for platform configuration, new KAS algorithms and fixtures for X-Wing testing, SDK variant building capabilities, and a complete PQC test matrix infrastructure with roundtrip validation tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as otdf-local CLI
participant Keys as utils/keys.py
participant Docker as DockerService
participant Config as config/platform.py
participant Overrides as config/overrides
User->>CLI: otdf-local up
CLI->>Keys: ensure_all_temp_keys(platform_dir, keys_dir)
Keys->>Keys: ensure_keycloak_keys (CA, certs, PKCS12, JKS)
Keys-->>CLI: ✓ Keys generated
CLI->>Config: _generate_config()
Config->>Overrides: load_overrides(xtest_root)
Overrides-->>Config: override dict
Config->>Config: apply_overrides(config_data)
Config-->>CLI: ✓ Config with overrides applied
CLI->>Docker: DockerService.start(keys_dir=...)
Docker->>Docker: Set KEYS_DIR env variable
Docker->>Docker: docker compose up -d
Docker-->>CLI: ✓ Services started
CLI-->>User: Platform ready with X-Wing support
sequenceDiagram
participant Test as test_pqc.py
participant Fixture as fixtures/keys.py
participant Client as TDF Client
participant KAS as KAS Service
participant Platform as Platform Service
Test->>Test: require('key_management/autoconfigure/mechanism-xwing')
Test->>Fixture: key_xwing fixture
Fixture->>KAS: Register X-Wing public key
KAS-->>Fixture: ✓ Key registered
Fixture-->>Test: X-Wing managed key
Test->>Client: encrypt(plaintext, attribute_policy)
Client->>KAS: Request key access object (X-Wing)
KAS->>KAS: Generate wrapped key + ephemeral public key
KAS-->>Client: Return KAO with X-Wing data
Client-->>Test: TDF ciphertext (cached)
Test->>Test: assert_xwing_kao_sizes()
Test->>Test: assert_xwing_public_key_size()
Test->>Client: decrypt(ciphertext)
Client->>KAS: Unwrap key
KAS-->>Client: Return plaintext key
Client-->>Test: Decrypted plaintext
Test->>Test: Verify plaintext matches original
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 7 minutes and 43 seconds.Comment |
![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 support for X-Wing hybrid post-quantum/traditional KEM (Key Encapsulation Mechanism). This includes updating ABAC definitions for X-Wing, adding new key management fixtures for X-Wing keys and attributes, and extending SDK CLI support checks to include the mechanism-xwing feature. A new test file test_pqc_xwing.py has been added to verify X-Wing encryption and decryption, including hybrid scenarios with EC keys. The review comments highlight a problematic global caching mechanism in the new X-Wing tests that could lead to test isolation issues and a PEP 8 violation. Additionally, there is a duplicate code block for platform feature caching and a redundant require method in tdfs.py that should be addressed for code cleanliness and API consistency.
| from abac import Attribute, KasKey | ||
| from tdfs import KeyAccessObject | ||
|
|
||
| cipherTexts: dict[str, Path] = {} |
There was a problem hiding this comment.
The global cipherTexts dictionary used for caching encrypted TDF files is problematic. Since tmp_dir is typically a function-scoped fixture (like the standard tmp_path), the directory and its contents are deleted or rotated after each test iteration. Subsequent tests (e.g., when parametrized across different SDKs) will attempt to use a path from a previous test's tmp_dir that may no longer exist or is not associated with the current test execution.
Additionally, the variable name cipherTexts violates PEP 8 naming conventions (should be cipher_texts).
It is recommended to remove this caching logic and re-encrypt for each test case to ensure test isolation and reliability, or use a session-scoped fixture if caching is strictly necessary for performance.
| _cached_pfs: PlatformFeatureSet | None = None | ||
|
|
||
|
|
||
| def get_platform_features() -> PlatformFeatureSet: | ||
| """Return a cached PlatformFeatureSet singleton.""" | ||
| global _cached_pfs | ||
| if _cached_pfs is None: | ||
| _cached_pfs = PlatformFeatureSet() | ||
| return _cached_pfs | ||
|
|
||
|
|
||
| _cached_pfs: PlatformFeatureSet | None = None | ||
|
|
||
|
|
There was a problem hiding this comment.
This entire block (lines 155-168) is a duplicate of the implementation provided immediately below (lines 169-174). It appears that _cached_pfs and get_platform_features were added twice in this file, likely due to a merge or copy-paste error. Please remove the redundant first definition.
| def require(self, *features: feature_type): | ||
| """Skip the current test if any of the given features are unsupported.""" | ||
| for feature in features: | ||
| if feature not in self.features: | ||
| pytest.skip( | ||
| f"platform service {self.version} doesn't yet support [{feature}]" | ||
| ) |
There was a problem hiding this comment.
The require method is redundant as it provides nearly identical functionality to the existing skip_if_unsupported method (lines 146-152). To maintain a clean and consistent API, consider removing require and using skip_if_unsupported instead. skip_if_unsupported is slightly more informative as it identifies all missing features in a single skip message rather than stopping at the first one.
There was a problem hiding this comment.
junit used to call this method assume. Maybe I should do that...
X-Test Results✅ java-v0.13.0 |
dmihalcik-virtru
force-pushed
the
DSPX-2791-xwing
branch
from
8b4fcc1 to
64c22c4
Compare
X-Test Results✅ java-main |
X-Test Results✅ java-main |
dmihalcik-virtru
force-pushed
the
DSPX-2791-xwing
branch
from
2d6aedb to
610f3d4
Compare
X-Test Failure Report |
X-Test Results✅ go-main |
X-Test Results✅ go-main |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (9)
xtest/tdfs.py (1)
160-169: Remove duplicated singleton declarations for platform features.
_cached_pfsandget_platform_features()are defined twice in the module; keep a single definition to avoid drift and confusion.♻️ Suggested cleanup
-_cached_pfs: PlatformFeatureSet | None = None - - -def get_platform_features() -> PlatformFeatureSet: - """Return a cached PlatformFeatureSet singleton.""" - global _cached_pfs - if _cached_pfs is None: - _cached_pfs = PlatformFeatureSet() - return _cached_pfs - - _cached_pfs: PlatformFeatureSet | None = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtest/tdfs.py` around lines 160 - 169, There are duplicate definitions of the singleton variable and accessor — remove the redundant copy so only one _cached_pfs variable and one get_platform_features() function remain; locate both symbol occurrences (get_platform_features and _cached_pfs), keep the intended implementation (ensure _cached_pfs: PlatformFeatureSet | None is declared and initialized once and get_platform_features() uses it to lazily construct PlatformFeatureSet), and delete the other duplicate declarations to avoid drift and confusion.xtest/fixtures/keys.py (1)
229-233: Extraneous blank lines after docstrings.There are blank lines between the docstrings and the
returnstatements inkey_secpmlkem_3andkey_secpmlkem_5. While not a functional issue, it's inconsistent with the other fixture definitions in this file.♻️ Proposed fix
def key_secpmlkem_3( ... ) -> abac.KasKey: """Get or create X-Wing hybrid hpqt:secp256r1-mlkem768 managed key on km1.""" - return _get_or_create_key(def key_secpmlkem_5( ... ) -> abac.KasKey: """Get or create X-Wing hybrid hpqt:secp384r1-mlkem1024 managed key on km1.""" - return _get_or_create_key(Also applies to: 241-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtest/fixtures/keys.py` around lines 229 - 233, Remove the extra blank line between the docstring and the return statement in the fixtures key_secpmlkem_3 and key_secpmlkem_5 so they match the other fixtures; locate the functions by name and simply place the return _get_or_create_key(...) immediately after the closing triple-quoted docstring (no empty line) to make formatting consistent.xtest/test_pqc.py (1)
17-17: Module-level mutable state may cause issues with parallel test execution.The
cipherTextsdictionary caches encrypted files at module level. While this optimization avoids re-encrypting for each decrypt SDK, it can cause test pollution if tests run in parallel (e.g.,pytest-xdist). If parallel execution isn't planned, this is acceptable; otherwise, consider using pytest's caching mechanisms or session-scoped fixtures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtest/test_pqc.py` at line 17, The module-level mutable dict cipherTexts can cause test pollution under parallel runs; replace it with a session-scoped pytest fixture (or use pytest's cache) that returns the dict so encryption artifacts are shared safely across tests; specifically, remove the top-level cipherTexts and implement a fixture (e.g., cipher_texts or session_cipher_texts) with scope="session" which initializes and caches encrypted files and is injected into tests that previously referenced cipherTexts.xtest/run-pqc-matrix.sh (4)
44-46: Use[[instead of[for conditional tests.As flagged by static analysis,
[[is safer and more feature-rich in Bash (proper handling of empty strings, no word splitting issues, pattern matching support).♻️ Proposed fix
- if [ ! -d "$platform_dir" ]; then + if [[ ! -d "$platform_dir" ]]; then- if [ ! -d "$backend_dir" ]; then + if [[ ! -d "$backend_dir" ]]; then- if $DIAGONAL_ONLY && [ "$si" != "$bi" ]; then + if $DIAGONAL_ONLY && [[ "$si" != "$bi" ]]; thenAlso applies to: 73-77, 95-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtest/run-pqc-matrix.sh` around lines 44 - 46, Replace the POSIX `[` test expressions with Bash `[[` in the conditional checks that verify directories/paths so they handle empty strings and word splitting safely; specifically update the `if [ ! -d "$platform_dir" ]; then` check (and the similar checks around the other occurrences referenced) to use `[[ ! -d "$platform_dir" ]]` (and the corresponding `[[ ... ]]` forms) while keeping the same negation and `continue` behavior for the `variant` loop and other blocks that check existence.
20-24: Hardcoded default paths reduce portability.The default paths reference a specific developer's local directory structure (
$HOME/Documents/GitHub/post-quantum-*). This will fail for other developers who don't have these exact paths. Consider either requiring the environment variables to be set or documenting the expected setup more prominently.♻️ Proposed approach
Either fail fast if the env vars aren't set:
VARIANTS=(gemini enhanced codex) -PLATFORM_DIRS=( - "${PQC_GEMINI_DIR:-$HOME/Documents/GitHub/post-quantum-hybrid-gemini-2026-03-dm/platform}" - "${PQC_ENHANCED_DIR:-$HOME/Documents/GitHub/post-quantum-enhanced-2026-03-dm/platform}" - "${PQC_CODEX_DIR:-$HOME/Documents/GitHub/post-quantum-hybrid-codex-2026-03-dm/platform}" -) +: "${PQC_GEMINI_DIR:?PQC_GEMINI_DIR must be set to the gemini platform directory}" +: "${PQC_ENHANCED_DIR:?PQC_ENHANCED_DIR must be set to the enhanced platform directory}" +: "${PQC_CODEX_DIR:?PQC_CODEX_DIR must be set to the codex platform directory}" +PLATFORM_DIRS=("$PQC_GEMINI_DIR" "$PQC_ENHANCED_DIR" "$PQC_CODEX_DIR")Or use a relative/discoverable default path pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtest/run-pqc-matrix.sh` around lines 20 - 24, The PLATFORM_DIRS array uses hardcoded defaults pointing to a specific developer's $HOME/Documents/GitHub paths (see PLATFORM_DIRS, PQC_GEMINI_DIR, PQC_ENHANCED_DIR, PQC_CODEX_DIR); change this to either (a) require the environment variables and exit with a clear error if any of PQC_GEMINI_DIR, PQC_ENHANCED_DIR or PQC_CODEX_DIR are unset, or (b) compute a discoverable relative/default path (e.g., relative to the repository root or a configurable BASE_DIR) instead of embedding user-specific $HOME/Documents/GitHub paths, and update the script to validate each resolved path exists before proceeding.
85-86: Redirect error message to stderr.Error messages should go to stderr for proper stream separation, allowing stdout to be piped/redirected independently.
♻️ Proposed fix
- echo "ERROR: Failed to start backend $backend" + echo "ERROR: Failed to start backend $backend" >&2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtest/run-pqc-matrix.sh` around lines 85 - 86, The script prints the error message with echo which goes to stdout; change the echo that prints "ERROR: Failed to start backend $backend" so it writes to stderr instead (redirect the echo/printf to stderr) while leaving the SUMMARY+=" BACKEND-FAIL $backend"$'\n' update unchanged; update the echo invocation in run-pqc-matrix.sh (the line referencing $backend and the SUMMARY variable) to send the error text to stderr.
105-109: EmptyPYTEST_ARGSarray causes issues when no extra arguments are passed.When
PYTEST_ARGSis empty,"${PYTEST_ARGS[@]}"expands to nothing which is fine, but ifset -uis active (which it is viaset -euo pipefail), some older bash versions may error. Additionally, ensure proper quoting is maintained.♻️ Proposed defensive fix
- "${PYTEST_ARGS[@]}" \ + ${PYTEST_ARGS[@]+"${PYTEST_ARGS[@]}"} \This syntax only expands the array if it's set and non-empty.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtest/run-pqc-matrix.sh` around lines 105 - 109, The if-line fails under set -u when PYTEST_ARGS is unset/empty; update the pytest invocation to defensively expand the array by replacing "${PYTEST_ARGS[@]}" with a safe expansion like "${PYTEST_ARGS[@]+"${PYTEST_ARGS[@]}"}" (keep the existing quoting and placement in the if condition that runs pytest so no other parts change).otdf-local/src/otdf_local/utils/keys.py (2)
387-399: Subprocess errors are captured but not logged, making debugging difficult.When
subprocess.run(..., check=True, capture_output=True)fails, it raisesCalledProcessErrorbut the actual stderr output (which often contains the reason for failure) is captured and not displayed. Consider logging the error output on failure.♻️ Proposed approach
Either use a helper that logs stderr on failure:
def _run_subprocess(cmd: list[str], **kwargs) -> subprocess.CompletedProcess: """Run subprocess, logging stderr on failure.""" try: return subprocess.run(cmd, check=True, capture_output=True, **kwargs) except subprocess.CalledProcessError as e: logger.error("Command failed: %s\nstderr: %s", " ".join(cmd), e.stderr.decode()) raiseOr log at the call sites in
generate_ca_stores:- subprocess.run(cmd, check=True, capture_output=True) + result = subprocess.run(cmd, capture_output=True) + if result.returncode != 0: + logger.error("keytool failed: %s", result.stderr.decode()) + result.check_returncode() # raises CalledProcessErrorAlso applies to: 406-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdf-local/src/otdf_local/utils/keys.py` around lines 387 - 399, The subprocess.run calls in generate_ca_stores (and similar calls around the PKCS12 and other openssl invocations) currently use check=True and capture_output=True but do not log stderr on failure; wrap these calls in a try/except for subprocess.CalledProcessError (or centralize into a helper like _run_subprocess) and log the failing command and e.stderr.decode() via the module logger before re-raising so you get the openssl error output; update every occurrence (e.g., the PKCS12 export block and the other openssl invocations around lines ~406-423) to use the helper or the try/except pattern referencing the exact command list passed to subprocess.run.
353-366: Image parsing assumes specific YAML format.The
_get_keycloak_imagefunction uses simple string parsing to extract the image from a docker-compose file. This works for common formats but may fail on edge cases (e.g., quoted values, anchors, multi-line). Since this is for local dev tooling and has a sensible fallback, it's acceptable, but consider noting this limitation.For more robust parsing, you could use PyYAML:
import yaml data = yaml.safe_load(compose_file.read_text()) return data.get("services", {}).get("keycloak", {}).get("image", default)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdf-local/src/otdf_local/utils/keys.py` around lines 353 - 366, The _get_keycloak_image function currently uses line-based string parsing which can miss quoted values, anchors or multi-line YAML in docker-compose; replace the heuristic with proper YAML parsing (e.g., use yaml.safe_load on compose_file.read_text()) and read services -> keycloak -> image, returning the default ("keycloak/keycloak:25.0") if the key path is missing or parsing fails; ensure you catch and log OSError/YAML parsing exceptions and preserve the existing fallback behavior in _get_keycloak_image.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@otdf-local/src/otdf_local/services/kas.py`:
- Around line 77-83: When self.is_key_management is true, the code currently
unconditionally sets updates["services.kas.preview.hybrid_tdf_enabled"], which
breaks on platforms lacking that field; change the block so you always set
updates["services.kas.preview.key_management"] and
updates["services.kas.preview.ec_tdf_enabled"] but only assign
updates["services.kas.preview.hybrid_tdf_enabled"] after verifying the platform
supports that config key (e.g., check the existing config/schema or a helper
like a platform feature list before setting), keep the registered_kas_uri
assignment as-is using self.port.
In `@otdf-local/src/otdf_local/utils/console.py`:
- Around line 11-13: You changed the default Console to write to stderr (console
= Console(stderr=True)) which will redirect any remaining JSON outputs that call
console.print(...) to stderr and break consumers; update code paths that emit
machine-readable data to use the dedicated _stdout_console instead (or change
those specific calls to _stdout_console.print(...))—search for uses of
console.print in places like the CLI JSON output (e.g., status --json /
functions invoking console.print) and replace them with _stdout_console.print to
ensure JSON goes to stdout while keeping diagnostics on stderr.
In `@xtest/abac.py`:
- Around line 185-197: Update the enum value for
KAS_PUBLIC_KEY_ALG_ENUM_HPQT_XWING from 10 to 30 and ensure the mapping in
_KAS_ALG_TO_STR_MAP remains correct (the key KAS_PUBLIC_KEY_ALG_ENUM_HPQT_XWING
should reference "hpqt:xwing"); locate the constant definition
KAS_PUBLIC_KEY_ALG_ENUM_HPQT_XWING and change its numeric value to 30 so
serialization/deserialization uses the intended contract.
In `@xtest/conftest.py`:
- Around line 68-71: The validator currently accepts entries like "go:" because
it only extracts sdk_name and doesn't check for an empty version suffix; update
the loop that iterates over inputs (look for variables i, sdk_name, valid_sdks
and the function returning v) to split with two parts when ":" in i (e.g.,
sdk_name, version = i.split(":", 1)) and raise a ValueError if version is an
empty string (include the offending string and valid_sdks in the message); keep
the existing invalid-SDK check for sdk_name not in valid_sdks.
In `@xtest/fixtures/keys.py`:
- Around line 336-351: The docstrings for the fixtures
attribute_with_secpmlkem_3_key and attribute_with_secpmlkem_5_key incorrectly
say "assign an X-Wing key"; update each fixture's triple-quoted docstring to
accurately describe the key being assigned (for attribute_with_secpmlkem_3_key
mention secp256r1-mlkem768 / secpmlkem-3 and for attribute_with_secpmlkem_5_key
mention secp384r1-mlkem1024 / secpmlkem-5) so the description matches the keys
created by _create_keyed_attribute and the key fixtures key_secpmlkem_3 /
key_secpmlkem_5.
In `@xtest/sdk/go/cli.sh`:
- Around line 108-112: Update the case label in xtest/sdk/go/cli.sh from
"mechanism-secpmkem" to the correct feature name "mechanism-secpmlkem" so the
case branch matches the test framework; leave the body (set -o pipefail and the
"${cmd[@]}" help ... | grep ... and exit $?) unchanged so the existing behavior
executes when the correct feature string is encountered.
In `@xtest/sdk/go/Makefile`:
- Around line 54-58: The build step hardcodes the workspace path to
"$(MAKEFILE_DIR)/src/main" (used in the cd and GOWORK assignment) which prevents
branch-generated workspaces from being built; update the build-variant rules to
use a variable representing the generated workspace directory (e.g., replace
"$(MAKEFILE_DIR)/src/main" with a parameterized path like
"$(MAKEFILE_DIR)/src/$(VARIANT)" or a dedicated variable such as "$(GOWORK_DIR)"
and ensure the cd and GOWORK assignments reference that variable (the cd command
and the GOWORK=$(abspath ...) assignment) so branch-specific go.work files are
used instead of the hardcoded main folder.
In `@xtest/sdk/js/cli.sh`:
- Around line 94-97: The mechanism-xwing probe currently runs a global help
check using "npx $CTL help | grep -i xwing" which can miss mechanism-specific
flags; change the probe to call the CLI's mechanism-specific help command (use
"npx $CTL encrypt-help" or the CLI's encrypt-help subcommand) and grep that
output for "xwing" so mechanism-xwing support is detected reliably (update the
mechanism-xwing block where "npx $CTL help | grep -i xwing" appears).
In `@xtest/test_pqc.py`:
- Around line 172-177: The test currently iterates
manifest.encryptionInformation.keyAccess comparing kao.kid to xwing_kid but
never fails if no match is found; modify the loop in test_pqc.py to record
whether a matching X-Wing KAO was found (e.g., set a boolean found_xwing_kAO
when calling assert_xwing_kao_sizes(kao)) and after the loop add an assertion
that found_xwing_kAO is True so the test fails when no X-Wing KAO is present;
keep existing calls to assert_xwing_kao_sizes and
assert_xwing_public_key_size(key_xwing).
---
Nitpick comments:
In `@otdf-local/src/otdf_local/utils/keys.py`:
- Around line 387-399: The subprocess.run calls in generate_ca_stores (and
similar calls around the PKCS12 and other openssl invocations) currently use
check=True and capture_output=True but do not log stderr on failure; wrap these
calls in a try/except for subprocess.CalledProcessError (or centralize into a
helper like _run_subprocess) and log the failing command and e.stderr.decode()
via the module logger before re-raising so you get the openssl error output;
update every occurrence (e.g., the PKCS12 export block and the other openssl
invocations around lines ~406-423) to use the helper or the try/except pattern
referencing the exact command list passed to subprocess.run.
- Around line 353-366: The _get_keycloak_image function currently uses
line-based string parsing which can miss quoted values, anchors or multi-line
YAML in docker-compose; replace the heuristic with proper YAML parsing (e.g.,
use yaml.safe_load on compose_file.read_text()) and read services -> keycloak ->
image, returning the default ("keycloak/keycloak:25.0") if the key path is
missing or parsing fails; ensure you catch and log OSError/YAML parsing
exceptions and preserve the existing fallback behavior in _get_keycloak_image.
In `@xtest/fixtures/keys.py`:
- Around line 229-233: Remove the extra blank line between the docstring and the
return statement in the fixtures key_secpmlkem_3 and key_secpmlkem_5 so they
match the other fixtures; locate the functions by name and simply place the
return _get_or_create_key(...) immediately after the closing triple-quoted
docstring (no empty line) to make formatting consistent.
In `@xtest/run-pqc-matrix.sh`:
- Around line 44-46: Replace the POSIX `[` test expressions with Bash `[[` in
the conditional checks that verify directories/paths so they handle empty
strings and word splitting safely; specifically update the `if [ ! -d
"$platform_dir" ]; then` check (and the similar checks around the other
occurrences referenced) to use `[[ ! -d "$platform_dir" ]]` (and the
corresponding `[[ ... ]]` forms) while keeping the same negation and `continue`
behavior for the `variant` loop and other blocks that check existence.
- Around line 20-24: The PLATFORM_DIRS array uses hardcoded defaults pointing to
a specific developer's $HOME/Documents/GitHub paths (see PLATFORM_DIRS,
PQC_GEMINI_DIR, PQC_ENHANCED_DIR, PQC_CODEX_DIR); change this to either (a)
require the environment variables and exit with a clear error if any of
PQC_GEMINI_DIR, PQC_ENHANCED_DIR or PQC_CODEX_DIR are unset, or (b) compute a
discoverable relative/default path (e.g., relative to the repository root or a
configurable BASE_DIR) instead of embedding user-specific $HOME/Documents/GitHub
paths, and update the script to validate each resolved path exists before
proceeding.
- Around line 85-86: The script prints the error message with echo which goes to
stdout; change the echo that prints "ERROR: Failed to start backend $backend" so
it writes to stderr instead (redirect the echo/printf to stderr) while leaving
the SUMMARY+=" BACKEND-FAIL $backend"$'\n' update unchanged; update the echo
invocation in run-pqc-matrix.sh (the line referencing $backend and the SUMMARY
variable) to send the error text to stderr.
- Around line 105-109: The if-line fails under set -u when PYTEST_ARGS is
unset/empty; update the pytest invocation to defensively expand the array by
replacing "${PYTEST_ARGS[@]}" with a safe expansion like
"${PYTEST_ARGS[@]+"${PYTEST_ARGS[@]}"}" (keep the existing quoting and placement
in the if condition that runs pytest so no other parts change).
In `@xtest/tdfs.py`:
- Around line 160-169: There are duplicate definitions of the singleton variable
and accessor — remove the redundant copy so only one _cached_pfs variable and
one get_platform_features() function remain; locate both symbol occurrences
(get_platform_features and _cached_pfs), keep the intended implementation
(ensure _cached_pfs: PlatformFeatureSet | None is declared and initialized once
and get_platform_features() uses it to lazily construct PlatformFeatureSet), and
delete the other duplicate declarations to avoid drift and confusion.
In `@xtest/test_pqc.py`:
- Line 17: The module-level mutable dict cipherTexts can cause test pollution
under parallel runs; replace it with a session-scoped pytest fixture (or use
pytest's cache) that returns the dict so encryption artifacts are shared safely
across tests; specifically, remove the top-level cipherTexts and implement a
fixture (e.g., cipher_texts or session_cipher_texts) with scope="session" which
initializes and caches encrypted files and is injected into tests that
previously referenced cipherTexts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ba500fb-83b7-4afc-97d1-9210dff2a70c
⛔ Files ignored due to path filters (9)
xtest/results/codex-on-codex.logis excluded by!**/*.logxtest/results/codex-on-enhanced.logis excluded by!**/*.logxtest/results/codex-on-gemini.logis excluded by!**/*.logxtest/results/enhanced-on-codex.logis excluded by!**/*.logxtest/results/enhanced-on-enhanced.logis excluded by!**/*.logxtest/results/enhanced-on-gemini.logis excluded by!**/*.logxtest/results/gemini-on-codex.logis excluded by!**/*.logxtest/results/gemini-on-enhanced.logis excluded by!**/*.logxtest/results/gemini-on-gemini.logis excluded by!**/*.log
📒 Files selected for processing (20)
otdf-local/src/otdf_local/cli.pyotdf-local/src/otdf_local/config/overrides.pyotdf-local/src/otdf_local/config/settings.pyotdf-local/src/otdf_local/services/docker.pyotdf-local/src/otdf_local/services/kas.pyotdf-local/src/otdf_local/services/platform.pyotdf-local/src/otdf_local/utils/console.pyotdf-local/src/otdf_local/utils/keys.pyotdf-sdk-mgr/src/otdf_sdk_mgr/cli_install.pyotdf-sdk-mgr/src/otdf_sdk_mgr/installers.pyxtest/abac.pyxtest/conftest.pyxtest/fixtures/keys.pyxtest/run-pqc-matrix.shxtest/sdk/go/Makefilextest/sdk/go/cli.shxtest/sdk/java/cli.shxtest/sdk/js/cli.shxtest/tdfs.pyxtest/test_pqc.py
| if self.is_key_management: | ||
| updates["services.kas.preview.key_management"] = True | ||
| updates["services.kas.preview.ec_tdf_enabled"] = True | ||
| updates["services.kas.preview.hybrid_tdf_enabled"] = True | ||
| # registered_kas_uri should NOT have /kas suffix | ||
| updates["services.kas.registered_kas_uri"] = f"http://localhost:{self.port}" | ||
|
|
There was a problem hiding this comment.
Gate hybrid_tdf_enabled behind platform feature support.
Line 80 enables the HPQT preview flag for every KM KAS config. On platform versions without that field, this can cause config incompatibility during service startup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@otdf-local/src/otdf_local/services/kas.py` around lines 77 - 83, When
self.is_key_management is true, the code currently unconditionally sets
updates["services.kas.preview.hybrid_tdf_enabled"], which breaks on platforms
lacking that field; change the block so you always set
updates["services.kas.preview.key_management"] and
updates["services.kas.preview.ec_tdf_enabled"] but only assign
updates["services.kas.preview.hybrid_tdf_enabled"] after verifying the platform
supports that config key (e.g., check the existing config/schema or a helper
like a platform feature list before setting), keep the registered_kas_uri
assignment as-is using self.port.
| # Global console instance (stderr for all status/diagnostic output) | ||
| console = Console(stderr=True) | ||
| _stdout_console = Console() # stdout for machine-readable data output |
There was a problem hiding this comment.
Potential JSON output regression after moving default console to stderr.
This shift is good for diagnostics, but any remaining JSON paths using console.print(...) will now emit on stderr (e.g., status --json path in otdf-local/src/otdf_local/cli.py around Line 357), which can break consumers expecting stdout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@otdf-local/src/otdf_local/utils/console.py` around lines 11 - 13, You changed
the default Console to write to stderr (console = Console(stderr=True)) which
will redirect any remaining JSON outputs that call console.print(...) to stderr
and break consumers; update code paths that emit machine-readable data to use
the dedicated _stdout_console instead (or change those specific calls to
_stdout_console.print(...))—search for uses of console.print in places like the
CLI JSON output (e.g., status --json / functions invoking console.print) and
replace them with _stdout_console.print to ensure JSON goes to stdout while
keeping diagnostics on stderr.
| sdk_name = i.split(":")[0] if ":" in i else i | ||
| if sdk_name not in valid_sdks: | ||
| raise ValueError(f"Invalid SDK '{sdk_name}' in '{i}', must be one of {valid_sdks}") | ||
| return v |
There was a problem hiding this comment.
Reject malformed qualified SDK specs like go: at option-parse time.
Right now empty version suffixes pass validation and fail later during SDK path resolution with a less actionable error.
✅ Suggested validation hardening
def is_a(v: str) -> typing.Any:
for i in v.split():
- sdk_name = i.split(":")[0] if ":" in i else i
+ if ":" in i:
+ sdk_name, version = i.split(":", 1)
+ if not version:
+ raise ValueError(f"Invalid SDK spec '{i}': missing version after ':'")
+ else:
+ sdk_name = i
if sdk_name not in valid_sdks:
raise ValueError(f"Invalid SDK '{sdk_name}' in '{i}', must be one of {valid_sdks}")
return v🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xtest/conftest.py` around lines 68 - 71, The validator currently accepts
entries like "go:" because it only extracts sdk_name and doesn't check for an
empty version suffix; update the loop that iterates over inputs (look for
variables i, sdk_name, valid_sdks and the function returning v) to split with
two parts when ":" in i (e.g., sdk_name, version = i.split(":", 1)) and raise a
ValueError if version is an empty string (include the offending string and
valid_sdks in the message); keep the existing invalid-SDK check for sdk_name not
in valid_sdks.
| @test -f "$(MAKEFILE_DIR)/src/main/go.work.$(VARIANT)" || { echo "Error: $(MAKEFILE_DIR)/src/main/go.work.$(VARIANT) not found"; exit 1; } | ||
| @echo "Building variant $(VARIANT) with GOWORK=go.work.$(VARIANT)" | ||
| cd $(MAKEFILE_DIR)/src/main && \ | ||
| GOWORK=$(abspath $(MAKEFILE_DIR)/src/main/go.work.$(VARIANT)) \ | ||
| go build -o $(MAKEFILE_DIR)/binary-$(VARIANT) . || { \ |
There was a problem hiding this comment.
build-variant is hardcoded to src/main, breaking branch-based variant builds.
Line 54 and Line 56 force src/main, so install variant --branch <non-main> cannot build the generated workspace for that branch.
Proposed fix
# Build a variant using a specific go.work file from src/main/
-# Usage: make build-variant VARIANT=gemini
-# Requires: src/main/go.work.$(VARIANT) must exist
+# Usage: make build-variant VARIANT=gemini [BRANCH=main]
+# Requires: src/$(BRANCH)/go.work.$(VARIANT) must exist
build-variant:
+ `@BRANCH_DIR`="$${BRANCH:-main}"; \
`@test` -n "$(VARIANT)" || { echo "Error: VARIANT is required (e.g., make build-variant VARIANT=gemini)"; exit 1; }
- `@test` -f "$(MAKEFILE_DIR)/src/main/go.work.$(VARIANT)" || { echo "Error: $(MAKEFILE_DIR)/src/main/go.work.$(VARIANT) not found"; exit 1; }
+ `@test` -f "$(MAKEFILE_DIR)/src/$${BRANCH_DIR}/go.work.$(VARIANT)" || { echo "Error: $(MAKEFILE_DIR)/src/$${BRANCH_DIR}/go.work.$(VARIANT) not found"; exit 1; }
`@echo` "Building variant $(VARIANT) with GOWORK=go.work.$(VARIANT)"
- cd $(MAKEFILE_DIR)/src/main && \
- GOWORK=$(abspath $(MAKEFILE_DIR)/src/main/go.work.$(VARIANT)) \
+ cd "$(MAKEFILE_DIR)/src/$${BRANCH_DIR}" && \
+ GOWORK="$(abspath $(MAKEFILE_DIR)/src/$${BRANCH_DIR}/go.work.$(VARIANT))" \
go build -o $(MAKEFILE_DIR)/binary-$(VARIANT) . || { \
echo "Error: Go build failed for variant $(VARIANT)"; \
exit 1; \
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xtest/sdk/go/Makefile` around lines 54 - 58, The build step hardcodes the
workspace path to "$(MAKEFILE_DIR)/src/main" (used in the cd and GOWORK
assignment) which prevents branch-generated workspaces from being built; update
the build-variant rules to use a variable representing the generated workspace
directory (e.g., replace "$(MAKEFILE_DIR)/src/main" with a parameterized path
like "$(MAKEFILE_DIR)/src/$(VARIANT)" or a dedicated variable such as
"$(GOWORK_DIR)" and ensure the cd and GOWORK assignments reference that variable
(the cd command and the GOWORK=$(abspath ...) assignment) so branch-specific
go.work files are used instead of the hardcoded main folder.
…pport Add integration test infrastructure for the X-Wing post-quantum/traditional hybrid KEM algorithm (draft-connolly-cfrg-xwing-kem-10) to prepare for Q-Day readiness testing. - Register hpqt:xwing algorithm (enum 30) in abac.py - Add "mechanism-xwing" feature type with platform version gating (>= 0.14.0) - Add SDK feature detection in go/java/js cli.sh via encrypt help grep - Create fixtures/pqc.py with key_xwing, attribute_with_xwing_key, and attribute_with_xwing_and_ec_keys fixtures - Create test_pqc_xwing.py with roundtrip tests for X-Wing-only and mixed X-Wing + EC multi-mechanism encryption - Assert X-Wing KEM-specific sizes (1216-byte encapsulation key, 1120-byte ciphertext) on KAO and registered public key Tests will gracefully skip until platform and SDKs ship X-Wing support. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Support building and testing multiple post-quantum platform variants side by side. This enables comparing different X-Wing implementations (e.g., from separate branches) by building variant-specific otdfctl binaries and switching the platform backend at runtime. - otdf-sdk-mgr: add `install variant` command that generates per-variant go.work files and builds otdfctl against a platform variant's modules - otdf-local: enable OTDF_LOCAL_PLATFORM_DIR env var to override the auto-discovered platform directory - Go SDK Makefile: add `build-variant` target using GOWORK env var - xtest: extend --sdks to accept sdk:version qualifiers (e.g., go:gemini) for filtering specific SDK versions during test runs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When using OTDF_LOCAL_PLATFORM_DIR to point at a fresh platform checkout (e.g. PQC variant branches), the required KAS and Keycloak TLS keys may not exist yet. This adds automatic key generation during `otdf-local up` so variant backends work out of the box. KAS keys are per-variant (in platform_dir), while Keycloak CA/TLS keys are shared in xtest/tmp/keys/ and passed via KEYS_DIR env var to docker compose. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nv) works cleanly All diagnostic/status console output now goes to stderr. Machine-readable data (JSON output, shell export lines) goes to stdout. Adds print_json() helper using a stdout Console for use in env and ls --json commands. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dmihalcik-virtru
force-pushed
the
DSPX-2791-xwing
branch
from
1a70f96 to
b0298b5
Compare
X-Test Failure Report |
dmihalcik-virtru
changed the title
X-Test Failure Report |
X-Test Failure Report |
X-Test Failure Report |
1 similar comment
X-Test Failure Report |
X-Test Failure Report |
|
X-Test Failure Report |
X-Test Results✅ java-main |
2 participants
Add integration test infrastructure for the X-Wing post-quantum/traditional
hybrid KEM algorithm (draft-connolly-cfrg-xwing-kem-10) to prepare for Q-Day
readiness testing.
hpqt:*algorithms in abac.py"mechanism-xwing"and"mechanism-secpmlkem"feature type with platform version gating (>= 0.14.0)mixed X-Wing + EC multi-mechanism encryption
1120-byte ciphertext) on KAO and registered public key
Tests will gracefully skip until platform and SDKs ship X-Wing support.
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Summary by CodeRabbit
New Features
otdf-local configurecommand for managing platform featuresinstall variantcommand for building custom SDK distributionsImprovements