Add TLS support for Registrar communication #1139
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sarroutbi
force-pushed
the
202510011436-include-registrar-tls-communication
branch
6 times, most recently
from
db59f9f to
236743f
Compare
|
Codecov Report❌ Patch coverage is Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
sarroutbi
force-pushed
the
202510011436-include-registrar-tls-communication
branch
6 times, most recently
from
da1002b to
28f0598
Compare
This change enables TLS-based communication with the Registrar service in the push model agent, providing secure registration and activation. Key changes: - Added registrar_tls_enabled, registrar_tls_ca_cert, registrar_tls_client_cert, and registrar_tls_client_key configuration options with empty defaults for backwards compatibility - Updated RegistrarClientBuilder to accept TLS configuration parameters (ca_certificate, certificate, key, insecure, timeout) - Modified RegistrarClient to use HTTPS client when TLS is configured, falling back to plain HTTP when TLS parameters are not provided - Refactored to use single ResilientClient for all HTTP/HTTPS requests instead of maintaining separate client instances - Added RegistrarTlsConfig struct in push model agent to manage TLS configuration from config file - Updated StateMachine to accept and pass registrar_tls_config to registration functions Backwards compatibility: - Defaults to plain HTTP when registrar_tls_enabled is false (default) - Defaults to plain HTTP when TLS certificate paths are empty (default) - TLS only enabled when all three certificate paths are provided AND registrar_tls_enabled is true - Pull model agent unchanged - maintains existing behavior with None values for all new TLS fields The implementation separates Registrar TLS configuration from Verifier TLS configuration, allowing each service to be secured independently based on deployment requirements. Co-Authored-By: Claude <[email protected]> Signed-off-by: Sergio Arroutbi <[email protected]>
This commit adds extensive unit tests to increase coverage for the Registrar TLS communication feature, focusing on actual execution of production code paths rather than just configuration validation. Tests added in https_client.rs (10 tests, 330 lines): - test_get_https_client_with_valid_certs: Validates successful HTTPS client creation with real generated TLS certificates - test_get_https_client_insecure_mode: Tests insecure mode bypassing certificate validation - test_get_https_client_missing_ca_cert: Tests error handling when CA certificate file is missing - test_get_https_client_missing_client_cert: Tests error handling when client certificate file is missing - test_get_https_client_missing_client_key: Tests error handling when client key file is missing - test_get_https_client_invalid_ca_cert: Tests error handling with malformed CA certificate content - test_get_https_client_invalid_client_identity: Tests error handling with invalid client certificate/key pair - test_get_https_client_with_different_timeouts: Validates timeout configuration with various values (0, 1000, 5000, 30000, 300000ms) - test_get_https_client_insecure_default: Tests default behavior when insecure flag is None - test_get_https_client_empty_ca_cert_path: Tests error handling with empty certificate path strings Tests added in registration.rs (17 tests, 581 lines): - test_get_retry_config_all_none: Tests retry config when all values are None (should return None) - test_get_retry_config_with_max_retries: Tests retry config with only max_retries set - test_get_retry_config_with_initial_delay: Tests retry config with only initial_delay set - test_get_retry_config_with_max_delay: Tests retry config with only max_delay set - test_get_retry_config_with_all_values: Tests retry config with all values configured - test_check_registration_with_none_context: Tests registration check with no context (early return path) - test_register_agent_creates_agent_registration_config: Tests full registration flow with TLS config and retry config - Plus 10 additional tests for TLS config validation, partial configs, insecure mode, and integration with real certificates Tests added in registrar_client.rs (21 tests, 421 lines): - Builder pattern tests for TLS configuration fields (ca_certificate, certificate, key, insecure, timeout) - Tests for partial TLS configurations (missing CA, cert, or key) - Tests for empty string paths and various timeout values - test_builder_with_real_tls_certificates: Validates builder with actual generated certificates written to temp files - test_builder_build_with_invalid_tls_cert_files: Tests build failure with non-existent certificate files - test_tls_enabled_when_all_certs_provided: Tests TLS activation when all certificate paths are provided - test_tls_disabled_when_insecure_true: Tests TLS bypass with insecure flag - test_http_fallback_when_partial_tls_config: Tests HTTP fallback when only some TLS parameters are provided Test infrastructure: - Added generate_test_certificates() helper in registrar_client.rs that creates real CA, client, and server certificates using crypto::x509::CertificateBuilder - Added generate_test_tls_certificates() helper in registration.rs for creating TLS test certificates - Added generate_test_certificates() helper in https_client.rs for certificate generation - All certificate generation uses crypto::testing::rsa_generate(2048) with proper PKCS8 PEM encoding - Certificates written to temporary directories that are automatically cleaned up after tests Coverage improvements: - https_client.rs: Executes production TLS certificate loading code (lines 16-67), including CA cert parsing, client identity creation, and error handling paths - registration.rs: Covers get_retry_config() function logic (lines 33-52) and TLS config extraction (lines 60-71) - registrar_client.rs: Tests builder configuration but not build() execution paths (requires running server) All tests require --features testing flag as they depend on crypto::testing::rsa_generate() which is only available with the testing feature enabled. Tests validated with: cargo test --features testing --lib All 305+ tests passing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Sergio Arroutbi <[email protected]>
This commit introduces a suite of integration tests for the registrar
client using Mockoon to simulate a registrar server. These tests provide
coverage for HTTP and HTTPS interactions, including agent registration,
activation, and API version retrieval.
Key changes include:
* New GitHub Actions Workflow: A dedicated workflow,
mockoon-registrar-tests, has been added to mockoon.yaml to run the new
registrar tests in CI. [cite_start]This workflow installs and runs the
Mockoon CLI with a new data file.
* Mockoon Registrar Configuration: A new JSON file, registrar.json
[cite_start], defines the mock registrar's API endpoints, including
/version
/v1.2/agents/{uuid} for registration and activation, and
their TLS-secured counterparts.
* Rust Integration Tests: New tests have been added to registrar_client.rs
to cover:
** HTTP agent registration and activation.
** API version endpoint retrieval.
** Client behavior with retry configurations.
** Verification of the TLS code path during client build.
* Test Execution Script: The mockoon_registrar_tests.sh script manages the
setup and teardown of the testing environment. It starts the
Mockoon server on port 3001, handles certificate generation
and executes the Cargo tests. It also includes
cleanup logic to stop the Mockoon server after the test run.
Assisted-by: Gemini <[email protected]>
Signed-off-by: Sergio Arroutbi <[email protected]>
The CI job for mockoon-registrar-tests was failing because both the GitHub Actions mockoon/cli-action and the test script were trying to start Mockoon on port 3001, causing a "Port 3001 is already in use" error. Changes: - Enhanced port detection logic in tests/mockoon_registrar_tests.sh to use multiple methods (lsof, netstat, ss, and curl) for detecting if port 3001 is already in use - Improved cleanup logic to properly terminate Mockoon processes using multiple methods (kill by PID, lsof, pkill by process name) - Made the script more robust in CI environments where lsof might not be available or behave differently The test script now properly detects when Mockoon is already running (started by the GitHub Actions workflow) and skips starting a duplicate instance, avoiding the port conflict. Co-Authored-By: Claude <[email protected]> Signed-off-by: Sergio Arroutbi <[email protected]>
Signed-off-by: Sergio Arroutbi <[email protected]>
Enhanced the Mockoon CI testing infrastructure with extensive debugging capabilities to diagnose port conflicts and process issues. Changes to tests/mockoon_registrar_tests.sh: - Added log_port_3001_info() function that captures detailed information: * Process IDs, commands, users, parent PIDs, start times * Working directories and environment variables * Network socket information (lsof, netstat, ss) * Docker container status and systemd services * HTTP connectivity tests and response headers - Enhanced cleanup logic with before/after logging - Made the script resilient to missing system tools Changes to .github/workflows/mockoon.yaml: - Added pre-Mockoon debugging step to capture baseline system state - Added post-Mockoon debugging step to verify Mockoon startup - Logs available system tools, network state, processes, and environment - Provides comprehensive visibility into CI environment This will help diagnose any future port conflicts or process management issues in the CI environment, making debugging much more efficient. Co-Authored-By: Claude <[email protected]> Signed-off-by: Sergio Arroutbi <[email protected]>
Fixed all shellcheck and yamllint issues in the Mockoon testing infrastructure: Shellcheck fixes in tests/mockoon_registrar_tests.sh: - Added proper quoting for /proc/$pid paths to prevent word splitting - Replaced 'cat file | cmd' with 'cmd < file' to avoid useless cat - Replaced 'ps aux | grep' with 'pgrep' for better process detection - All variables now properly quoted to prevent globbing Yamllint fixes in .github/workflows/mockoon.yaml: - Split long lines (>80 chars) using YAML line continuation - Improved readability while maintaining functionality - All debugging commands now properly formatted The code now passes both shellcheck (only expected SC1091 source warnings remain) and yamllint with no errors, improving code quality and maintainability. Co-Authored-By: Claude <[email protected]> Signed-off-by: Sergio Arroutbi <[email protected]>
This change consolidates duplicate certificate generation code and improves
test infrastructure reliability based on code review feedback.
Key improvements:
1. Consolidate Certificate Generation Utilities:
- Removed three separate and nearly identical certificate generation
helper functions from https_client.rs, registration.rs, and
registrar_client.rs (261 lines of duplicate code eliminated)
- Introduced a single, comprehensive utility function
crypto::testing::generate_tls_certs_for_test that creates CA,
server, and client certificate sets for testing purposes
- All relevant tests refactored to use this new shared utility,
reducing code duplication and improving maintainability
2. Strengthen Mockoon TLS Test Assertion:
- Fixed weak assertion assert!(result.is_err() || result.is_ok()) in
test_mockoon_registrar_https_registration test
- Changed to assert!(result.is_err()) to specifically verify that
connection fails as expected when mock server lacks TLS configuration
3. Replace Brittle Sleep with Polling Mechanism:
- Replaced sleep 3 command in tests/mockoon_registrar_tests.sh with
robust polling loop using curl to check server responsiveness
- Added 15-second timeout with proper error handling
- Makes tests less prone to failures on slower or busier CI runners
Co-Authored-By: Gemini <[email protected]>
Co-Authored-By: Claude <[email protected]>
Signed-off-by: Sergio Arroutbi <[email protected]>
sarroutbi
force-pushed
the
202510011436-include-registrar-tls-communication
branch
3 times, most recently
from
953f87f to
1bd9081
Compare
This change updates the default key generation algorithm for mTLS keys from RSA 2048 to ECC P-256 (secp256r1) for improved security and performance characteristics. Key changes: - Updated keylime-agent/src/main.rs to use Ecc256 instead of Rsa2048 for mTLS key generation via load_or_generate_key() - Updated keylime/src/cert.rs cert_from_server_key() function to generate ECC P-256 keys instead of RSA 2048 when creating new keys Benefits: - Smaller key sizes (256-bit vs 2048-bit) with equivalent security - Faster key generation and cryptographic operations - Lower memory and storage footprint - Better performance in embedded and resource-constrained environments Backward compatibility: - Existing RSA keys will continue to work due to load_or_generate_key logic that loads existing keys regardless of algorithm - Algorithm validation is disabled for mTLS keys to maintain compatibility - Only affects new key generation when no existing key file is found The ECC P-256 curve (X9_62_PRIME256V1/secp256r1) is widely supported, FIPS 186-4 approved, and provides 128-bit security level equivalent to RSA 3072. Co-Authored-By: Claude <[email protected]> Signed-off-by: Sergio Arroutbi <[email protected]>
sarroutbi
force-pushed
the
202510011436-include-registrar-tls-communication
branch
from
1bd9081 to
ac8c5a9
Compare
Signed-off-by: Sergio Arroutbi <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change enables TLS-based communication with the Registrar service in the push model agent, providing secure registration and activation. The implementation separates Registrar TLS configuration from Verifier TLS configuration, allowing each service to be secured independently based on deployment requirements.
Key changes:
Backwards compatibility:
Add Mockoon-based registration tests:
client using Mockoon to simulate a registrar server. These tests provide
coverage for HTTP and HTTPS interactions, including agent registration,
activation, and API version retrieval.
mockoon-registrar-tests, has been added to mockoon.yaml to run the new
registrar tests in CI. [cite_start]This workflow installs and runs the
Mockoon CLI with a new data file.
defines the mock registrar's API endpoints, including /version, /v1.2/agents/{uuid}
for registration and activation, and their TLS-secured counterparts.
to cover:
-- HTTP agent registration and activation.
-- API version endpoint retrieval.
-- Client behavior with retry configurations.
-- Verification of the TLS code path during client build.
-- Test Execution Script: The mockoon_registrar_tests.sh script manages the
setup and teardown of the testing environment. It starts the
Mockoon server on port 3001, handles certificate generation
and executes the Cargo tests. It also includes
cleanup logic to stop the Mockoon server after the test run.
Assisted-by: Gemini [email protected]
Co-Authored-By: Claude [email protected]
Signed-off-by: Sergio Arroutbi [email protected]