feat: add IDNA and password verification support - #926
niteshpurohit wants to merge 9 commits into
Conversation
- Introduced IDNA lookup functionality in the idna.cpp file to handle internationalized domain names. - Implemented password verification using bcrypt and SHA-512 in the password_auth.cpp file. - Updated core error handling to include new dependency operations for IDNA and password verification. - Added tests for IDNA and password authentication to ensure correctness and reliability. - Enhanced the contract definitions to support new dependency statuses and operations. - Updated CMake configuration to include new dependencies for IDNA and password authentication. closes: #57
There was a problem hiding this comment.
🟡 Changes recommended
Three moderate findings remain in cross-compilation configuration and password secret/error handling.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds optional IDNA and password-auth adapters backed by libidn2 and libxcrypt, with dependency integration, tests, and CI coverage.
Changes:
- Adds bounded IDNA conversion and bcrypt/SHA-512 password verification.
- Extends dependency contracts, acquisition, probing, and error normalization.
- Adds adapter tests, build fixtures, and CI coverage.
File summaries
| File | Reviewed changes |
|---|---|
tests/dependencies/probes/libxcrypt.cpp |
Adds libxcrypt capability probe. |
tests/dependencies/probes/libidn2.cpp |
Adds libidn2 capability probe. |
tests/dependencies/fixture/CMakeLists.txt |
Tests vendored dependency targets. |
tests/build-variants/fixture/CMakeLists.txt |
Tests new feature variants. |
tests/adapters/password_auth.cpp |
Adds password verification tests. |
tests/adapters/idna.cpp |
Adds IDNA vector and boundary tests. |
tests/adapters/dependency_error_logging.cpp |
Tests dependency error normalization. |
src/core/contract/laghu/core/views.hpp |
Adds secure buffer cleansing support. |
src/core/contract/laghu/core/contract.hpp |
Adds dependency statuses and operations. |
src/adapters/password_auth.cpp |
Implements password verification; findings remain for cleansing copied secrets and classifying crypt_r errors. |
src/adapters/idna.cpp |
Implements bounded IDNA conversion. |
src/adapters/dependency.cpp |
Registers dependency names and statuses. |
src/adapters/contract/laghu/adapters/password_auth.hpp |
Defines the password-auth contract. |
src/adapters/contract/laghu/adapters/idna.hpp |
Defines the IDNA contract. |
CMakeLists.txt |
Builds and tests the new adapters. |
cmake/LaghuToolchain.cmake |
Adds dependency validation scenarios. |
cmake/LaghuFeatures.cmake |
Registers new features. |
cmake/LaghuDependencies.cmake |
Adds dependency acquisition; cross-compiling Autoconf configuration needs host/toolchain settings. |
cmake/LaghuBuildIdentity.cmake |
Includes new adapter sources in build identity. |
cmake/ExpectFeatureMetadata.cmake |
Updates feature metadata checks. |
cmake/ExpectDependencyMetadata.cmake |
Updates dependency metadata checks. |
cmake/ExpectDependencyConfigure.cmake |
Supports new dependency fixtures. |
.github/workflows/toolchain.yml |
Adds adapter CI configurations. |
.github/workflows/codeql.yml |
Builds adapters for CodeQL analysis. |
Review details
Suppressed comments (1)
src/adapters/password_auth.cpp:247
- All
crypt_rfailures other thanENOMEMare classified as dependency I/O. This misclassifies documentedEINVAL(invalid setting),ENOSYS/ENOTSUP(unsupported algorithm), andERANGEfailures as retryable I/O, so malformed or unsupported bcrypt cases do not receive normalized dependency statuses. Map the native errno classes before callingnormalize_dependency_error.
[[nodiscard]] core::Error crypt_failure(int native_code,
const DependencyLogSink& log_sink) noexcept {
const core::DependencyStatus status = native_code == ENOMEM
? core::DependencyStatus::exhaustion
: core::DependencyStatus::io;
const core::Error error = normalize_dependency_error(
core::DependencyId::libxcrypt, core::DependencyOperation::password_verify,
status, static_cast<std::int32_t>(native_code));
- Files reviewed: 24/24 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Added cross-compilation support for password authentication dependencies. - Updated maximum password byte limit to align with libxcrypt's specifications. - Introduced new error statuses for better input validation. - Refactored password verification logic to improve error handling. - Added tests for native error normalization and password limits.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical static-linking and moderate correctness and secret-cleansing issues block approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
src/adapters/password_auth.cpp:325
crypt_datais libxcrypt's caller-owned work area for the password computation, but this stack object is never cleansed on any return path. The existing guard only wipes the copied password buffer, leaving plaintext-derived intermediate state and/or the computed hash in native scratch after verification. Add an RAII best-effort wipe for the native workspace before returning.
src/adapters/idna.cpp:125
- This converts the caller's UTF-8 input through a 254-byte
StaticCString, so any IDN whose UTF-8 representation exceeds 253 bytes is rejected before libidn2. The 253-byte hostname bound applies to the resulting ASCII/A-label form; valid internationalized hostnames can have a larger UTF-8 representation while producing an ASCII result within the bound. Use a separately bounded input buffer and retain the existing post-conversionAsciiHostname::from_asciicheck.
const auto input = hostname.to_c_string<AsciiHostname::storage_capacity>();
src/adapters/idna.cpp:62
- libidn2 returns
IDN2_TOO_BIG_DOMAINandIDN2_TOO_BIG_LABELfor inputs that exceed its domain/label bounds, but this fallback classifies every non-allocation/non-codeset error ascorrupt_data. A Unicode hostname that exceeds the bounded IDNA contract will therefore report corrupt data instead ofinvalid_range; map the two size return codes toDependencyStatus::invalid_rangeso callers can distinguish a range violation from malformed dependency output.
[[nodiscard]] core::DependencyStatus idn2_status(int status) noexcept {
if (status == IDN2_MALLOC) {
return core::DependencyStatus::exhaustion;
}
if (status == IDN2_NO_CODESET) {
return core::DependencyStatus::unavailable;
}
return core::DependencyStatus::corrupt_data;
- Files reviewed: 25/25 changed files
- Comments generated: 2
- Review effort level: Lite
- Added support for password authentication features in the build identity process. - Updated CMake scripts to include new source files related to password authentication. - Enhanced error handling for IDNA processing to manage range errors effectively. - Introduced new tests to validate UTF-8 input and native length boundaries for IDNA.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings affect cross-compilation, dependency linkage, IDNA normalization, credential scrubbing, and contract compatibility.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
cmake/LaghuDependencies.cmake:779
- The vendored libxcrypt build and install commands run from the archive root, but
libcrypt.laand the generatedinstall-*targets are defined by libxcrypt'slib/Makefile; unlike the libidn2 branch above, these commands never change into<SOURCE_DIR>/lib. The vendored IDNA/password fixture will therefore fail when ExternalProject runsmake libcrypt.la(and again during install). Invoke these targets from thelibsubdirectory or use the corresponding top-level recursive targets.
set(build_command "${laghu_make_program}" libcrypt.la)
set(install_command
"${laghu_make_program}" install-libLTLIBRARIES install-nodist_includeHEADERS)
cmake/LaghuDependencies.cmake:809
- The vendored libidn2 build is configured with
--with-included-libunistring, so the producedlibidn2.astill has a bundled libunistring archive dependency for static consumers. Exposing only this raw archive drops that dependency; the generated symbol probe (and later adapter link) will fail with unresolved libunistring symbols in the default STATIC/full configuration. Export the bundled archive as a transitive target or create a genuinely self-contained artifact before assigning onlyIMPORTED_LOCATIONhere.
add_library("${private_target}_artifact" "${library_type}" IMPORTED GLOBAL)
set_target_properties("${private_target}_artifact" PROPERTIES
IMPORTED_LOCATION "${library_path}")
add_dependencies("${private_target}_artifact" "laghu_vendor_${id}")
add_library("${private_target}" INTERFACE)
target_include_directories("${private_target}" SYSTEM INTERFACE "${install_directory}/include")
target_link_libraries("${private_target}" INTERFACE "${private_target}_artifact")
add_dependencies("${private_target}" "laghu_vendor_${id}")
src/adapters/idna.cpp:153
IDN2_NFC_INPUTtells libidn2 that the caller has already normalized the input; it does not perform NFC normalization. This contract accepts arbitrary UTF-8 and passes it straight through, so canonically equivalent non-NFC hostnames can be rejected or mapped inconsistently instead of receiving the required TR46 processing. Remove this flag (or normalize before calling) and add a non-NFC vector to the adapter test.
constexpr int idna_flags = IDN2_NFC_INPUT | IDN2_NONTRANSITIONAL | IDN2_USE_STD3_ASCII_RULES;
src/adapters/password_auth.cpp:325
crypt_datais libxcrypt's caller-provided scratch/output buffer and can retain password-derived state aftercrypt_rreturns. The existing guard only cleansesbounded_password, so both successful and failed verifications leave native credential material on the stack until that memory is reused. Add an RAII cleanse for the completecrypt_dataobject before every return from this scope.
crypt_data native_data{};
errno = 0;
const char* native_result = crypt_r(bounded_password->c_str(), bounded_hash->c_str(), &native_data);
- Files reviewed: 26/26 changed files
- Comments generated: 2
- Review effort level: Lite
- Added checks for missing cross-compilation tools (C compiler, ar, ranlib) to ensure proper error handling. - Enhanced the test suite to validate scenarios where cross-compilation tools are missing, preventing silent failures. - Updated the acquisition function to set appropriate variables for cross-compilation, improving robustness.
There was a problem hiding this comment.
🟡 Changes recommended
One critical build issue and four moderate correctness and contract issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
src/adapters/idna.cpp:153
IDN2_NFC_INPUTtells libidn2 to assume the input is already NFC-normalized; this API accepts arbitrary UTF-8 and does not normalize it first. A valid decomposed hostname such asu\u0308ber.examplecan therefore be rejected or encoded differently instead of receiving the required TR46/NFC processing. Remove this flag (or normalize the input before setting it) and add a decomposed Unicode golden vector.
constexpr int idna_flags = IDN2_NFC_INPUT | IDN2_NONTRANSITIONAL | IDN2_USE_STD3_ASCII_RULES;
src/adapters/idna.cpp:76
- This fallback maps caller-supplied hostname validation failures such as libidn2's
IDN2_LDH/IDN2_DISALLOWED(for example,-leading.example, which passes the local character filter) tocorrupt_data. That makes ordinary invalid input security-relevant corruption and produces the wrong normalized contract error; classify documented input-validation statuses asinvalid_inputand reservecorrupt_datafor malformed native data/encoding.
[[nodiscard]] core::DependencyStatus idn2_status(int status) noexcept {
if (status == IDN2_MALLOC) {
return core::DependencyStatus::exhaustion;
}
if (status == IDN2_NO_CODESET) {
return core::DependencyStatus::unavailable;
}
if (status == IDN2_TOO_BIG_DOMAIN || status == IDN2_TOO_BIG_LABEL) {
return core::DependencyStatus::invalid_range;
}
return core::DependencyStatus::corrupt_data;
src/adapters/password_auth.cpp:337
crypt_ris allowed to return a failure token beginning with*instead ofnullptr(the vendored libxcrypt build leaves failure tokens enabled).bounded_crypt_outputaccepts that token, so this return path reports unsupported algorithms and other native failures as a normal password mismatch and bypasses the promised dependency-error normalization. Detect a leading*and route it throughpassword_auth_native_errorbefore comparing hashes.
return constant_time_hash_equal(bounded_hash->view(), *output);
src/core/contract/laghu/core/contract.hpp:52
- These new enum members are inserted before all existing
DependencyStatusvalues, changing the underlying numeric value of every pre-existing status (for example,unavailableandio). Because this is a public contract and statuses are carried in dependency log records, that silently breaks consumers that persist or exchange the enum values; append the new statuses or assign explicit stable values instead.
invalid_input,
invalid_range,
unavailable,
exhaustion,
- Files reviewed: 26/26 changed files
- Comments generated: 1
- Review effort level: Lite
- Added a new test for password authentication failure token normalization to ensure proper error handling. - Enhanced IDNA error classification by adding checks for various encoding errors. - Updated existing tests to include new error handling scenarios for IDNA and password authentication.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved IDNA compilation and normalization defects, plus libidn2 vendored build and linkage issues, remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
cmake/LaghuDependencies.cmake:787
ExternalProject_Add(BUILD_COMMAND ...)accepts one command vector; theCOMMANDtokens embedded inbuild_commandare passed literally as arguments to the firstmake, not treated as separators for three commands. The vendored libidn2 build therefore invokes something equivalent tomake -C gl all COMMAND make ...and fails before producinglibidn2.la. Use separate external-project steps or a wrapper script/one command that sequences these sub-builds.
set(build_command
"${laghu_make_program}" -C "<SOURCE_DIR>/gl" all
COMMAND "${laghu_make_program}" -C "<SOURCE_DIR>/unistring" all
COMMAND "${laghu_make_program}" -C "<SOURCE_DIR>/lib" libidn2.la)
cmake/LaghuDependencies.cmake:827
- In STATIC mode this imported target exposes only libidn2.a, while
--with-included-libunistringbuilds libunistring as a separate archive that libidn2.a references. No bundled unistring archive or platform dependency is added to the target's link interface, so any real idna consumer will fail with unresolvedu8_*/unistring symbols.
add_library("${private_target}" INTERFACE)
target_include_directories("${private_target}" SYSTEM INTERFACE "${install_directory}/include")
target_link_libraries("${private_target}" INTERFACE "${private_target}_artifact")
src/adapters/idna.cpp:165
IDN2_NFC_INPUTtells libidn2 that the caller has already provided NFC input; it does not request NFC normalization. This contract accepts UTF-8 and claims TR46 processing, so decomposed-but-valid sequences (for exampleuplus combining diaeresis) will be rejected withIDN2_NOT_NFCinstead of being normalized to the same ASCII hostname as the precomposed form. Let libidn2 perform normalization (or normalize before setting this flag) and add a decomposed Unicode vector.
constexpr int idna_flags = IDN2_NFC_INPUT | IDN2_NONTRANSITIONAL | IDN2_USE_STD3_ASCII_RULES;
- Files reviewed: 27/27 changed files
- Comments generated: 2
- Review effort level: Lite
- Added private include directory for IDNA adapter. - Introduced internal IDNA status normalization function. - Updated IDNA test cases to include new status checks. - Improved error handling for Punycode outputs in tests.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings affect dependency builds, secret cleanup, IDNA behavior, and CI coverage.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (10)
Previously missed (1) — in code that hasn't changed since the last review.
src/adapters/idna.cpp:172
IDN2_NFC_INPUTtells libidn2 to assume the caller already supplied NFC and to reject non-NFC input. That makes valid UTF-8 hostnames such as a decomposed Unicode label fail withIDN2_NOT_NFC, whereas the public contract and the issue's IDNA2008/TR46 requirement call for normalization during lookup. Remove this flag (and cover a decomposed golden vector).
.github/workflows/toolchain.yml:49
- The new
laghu_password_auth_failure_token_testis registered, but this job builds onlylaghu_password_auth_testand its regex only matcheslaghu.adapters.password_auth. The native failure-token normalization regression therefore never runs in the full-adapter CI job; include that target andlaghu.adapters.password_auth.failure_tokenin the verification.
cmake --build "$RUNNER_TEMP/laghu-full" --target \
laghu_crypto_provider_test laghu_idna_test laghu_password_auth_test
ctest --test-dir "$RUNNER_TEMP/laghu-full" --output-on-failure \
-R '^laghu\.(crypto\.provider|adapters\.(idna|password_auth))$'
.github/workflows/toolchain.yml:62
- The new
laghu_password_auth_failure_token_testis registered, but this system-adapter job builds onlylaghu_password_auth_testand its regex only matcheslaghu.adapters.password_auth. The native failure-token normalization regression is not exercised here; include that target andlaghu.adapters.password_auth.failure_tokenin the verification.
cmake --build "$RUNNER_TEMP/laghu-system-adapters" --target \
laghu_idna_test laghu_password_auth_test
ctest --test-dir "$RUNNER_TEMP/laghu-system-adapters" --output-on-failure \
-R '^laghu\.adapters\.(idna|password_auth)$'
.github/workflows/toolchain.yml:86
- The new
laghu_password_auth_failure_token_testis registered, but this Clang adapter job builds onlylaghu_password_auth_testand its regex only matcheslaghu.adapters.password_auth. The native failure-token normalization regression is not exercised here; include that target andlaghu.adapters.password_auth.failure_tokenin the verification.
cmake --build "$RUNNER_TEMP/laghu-clang-adapters" --target \
laghu_idna_test laghu_password_auth_test
ctest --test-dir "$RUNNER_TEMP/laghu-clang-adapters" --output-on-failure \
-R '^laghu\.adapters\.(idna|password_auth)$'
.github/workflows/toolchain.yml:179
- The new
laghu_password_auth_failure_token_testis registered, but this FreeBSD adapter job builds onlylaghu_password_auth_testand its regex only matcheslaghu.adapters.password_auth. The native failure-token normalization regression is not exercised here; include that target andlaghu.adapters.password_auth.failure_tokenin the verification.
cmake --build /tmp/laghu-adapters --target \
laghu_idna_test laghu_password_auth_test
ctest --test-dir /tmp/laghu-adapters --output-on-failure \
-R '^laghu\.adapters\.(idna|password_auth)$'
.github/workflows/toolchain.yml:242
- The new
laghu_password_auth_failure_token_testis registered, but this sanitizer adapter job builds onlylaghu_password_auth_testand its regex only matcheslaghu.adapters.password_auth. The native failure-token normalization regression is not exercised under ASan/UBSan; include that target andlaghu.adapters.password_auth.failure_tokenin the verification.
cmake --build "$RUNNER_TEMP/laghu-asan-adapters" --target \
laghu_idna_test laghu_password_auth_test
ctest --test-dir "$RUNNER_TEMP/laghu-asan-adapters" --output-on-failure \
-R '^laghu\.adapters\.(idna|password_auth)$'
.github/workflows/toolchain.yml:145
- The new
laghu_password_auth_failure_token_testis registered, but this Apple Clang job builds onlylaghu_password_auth_testand its regex only matcheslaghu.adapters.password_auth. The native failure-token normalization regression is not exercised here; include that target andlaghu.adapters.password_auth.failure_tokenin the verification.
cmake --build "$RUNNER_TEMP/laghu-adapters" --target \
laghu_idna_test laghu_password_auth_test
ctest --test-dir "$RUNNER_TEMP/laghu-adapters" --output-on-failure \
-R '^laghu\.adapters\.(idna|password_auth)$'
cmake/LaghuDependencies.cmake:802
- The Autoconf environment forwards only
CC,AR, andRANLIB; it drops CMake toolchain flags such asCMAKE_C_FLAGS,CMAKE_SYSROOT, and linker flags. A cross toolchain that relies on a non-default sysroot or target ABI flags can therefore build these libraries against host headers/libraries or fail despite the--hostsetting. Propagate the relevant compile/link environment (or a generated wrapper) into configure and make.
set(configure_environment "${CMAKE_COMMAND}" -E env
"CC=${laghu_autoconf_c_compiler}" "AR=${laghu_autoconf_ar}" "RANLIB=${laghu_autoconf_ranlib}"
"MAKE=${laghu_make_program}")
cmake/LaghuToolchain.cmake:656
- These three new tests omit
LAGHU_EXPECT_CROSSCOMPILING, butExpectDependencyConfigure.cmakeunconditionally forwards-DLAGHU_EXPECT_CROSSCOMPILING=${LAGHU_EXPECT_CROSSCOMPILING}. An undefined value is still passed as a defined-but-empty cache variable; the fixture rejects that asexpected=native actual=crossafter it setsCMAKE_CROSSCOMPILINGfor these scenarios, so the tests never reach the missing-tool checks. Make the wrapper omit this argument when it is undefined (or otherwise adjust the fixture's cross-mode expectation).
-DSCENARIO=autoconf_cross_missing_${missing_tool}
-DEXPECT_FAIL=ON
"-DEXPECT_TEXT=rule=cross_tool_missing tool=${cmake_tool}"
src/adapters/idna.cpp:160
AsciiHostname::from_asciiestablishes that an empty hostname isinvalid_input, but this path never rejects an emptyTextViewbefore the native call. An empty input can therefore be surfaced as a libidn2corrupt_datafailure instead of the caller-input error. Reject an emptyinputat this boundary before invoking libidn2.
const auto input = hostname.to_c_string<utf8_input_storage_capacity>();
if (!input.has_value()) {
return std::unexpected{input.error()};
- Files reviewed: 28/28 changed files
- Comments generated: 2
- Review effort level: Lite
- Added validation for empty hostname input in IDNA conversion to prevent errors. - Updated test cases to include checks for decomposed German characters in IDNA conversion. - Enhanced CMake configuration for cross-compilation, ensuring proper environment setup for Autoconf. - Added new tests to verify the cross-toolchain environment and its dependencies.
There was a problem hiding this comment.
🟡 Changes recommended
Critical dependency-build and IDNA issues, plus unresolved password-buffer zeroization findings, block approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
cmake/LaghuDependencies.cmake:866
ExternalProject_AddtreatsBUILD_COMMANDas one command line; theCOMMANDtokens in this list are not separate build steps. The libidn2 build will therefore passCOMMANDto the firstmakeinvocation (or otherwise fail to execute the later commands), so the vendored IDNA dependency cannot be built. Use a wrapper command/script or separate external-project steps to run these threemakeinvocations sequentially.
set(build_command
"${laghu_make_program}" -C "<SOURCE_DIR>/gl" all
COMMAND "${laghu_make_program}" -C "<SOURCE_DIR>/unistring" all
COMMAND "${laghu_make_program}" -C "<SOURCE_DIR>/lib" libidn2.la)
cmake/LaghuDependencies.cmake:877
- The libxcrypt archive's
libcrypt.laandinstall-*Automake targets are generated in itslib/subdirectory, so invoking them from<SOURCE_DIR>makes every vendored password build fail with a missing-target error. Run both commands with-C <SOURCE_DIR>/lib(as is already done for libidn2) so the default vendored configuration can produce the imported artifact.
set(build_command "${laghu_make_program}" libcrypt.la)
set(install_command
"${laghu_make_program}" install-libLTLIBRARIES install-nodist_includeHEADERS)
src/adapters/idna.cpp:90
IDN2_ZERO_LENGTH_LABELis a normal input-validation result for values such as the addedtwo..labels.examplevector, but it is not included here. It therefore falls through tocorrupt_data, misclassifying malformed caller input and violating the dependency-error normalization contract. Map this status toinvalid_inputalong with the other IDNA validation statuses.
if (status == IDN2_ENCODING_ERROR || status == IDN2_PUNYCODE_BAD_INPUT ||
status == IDN2_PUNYCODE_OVERFLOW ||
status == IDN2_INVALID_ALABEL || status == IDN2_UALABEL_MISMATCH ||
status == IDN2_NOT_NFC || status == IDN2_2HYPHEN ||
status == IDN2_HYPHEN_STARTEND || status == IDN2_LEADING_COMBINING ||
status == IDN2_DISALLOWED || status == IDN2_CONTEXTJ ||
status == IDN2_CONTEXTJ_NO_RULE || status == IDN2_CONTEXTO ||
status == IDN2_CONTEXTO_NO_RULE || status == IDN2_UNASSIGNED ||
status == IDN2_BIDI || status == IDN2_DOT_IN_LABEL ||
status == IDN2_INVALID_TRANSITIONAL || status == IDN2_INVALID_NONTRANSITIONAL ||
status == IDN2_ALABEL_ROUNDTRIP_FAILED) {
src/adapters/password_auth.cpp:300
- The guard only cleanses the
StaticCStringheld insidebounded_password, butTextView::to_c_stringconstructs a separate localStaticCStringand moves/copies its array into the returnedResult; the default move leaves the source bytes intact when that local is destroyed. This leaves a plaintext password copy on the stack after verification, so use a secret-specific non-copying buffer or a move/destructor contract that scrubs every instance.
auto bounded_password = password.to_c_string<password_c_string_capacity>();
if (!bounded_password.has_value()) {
return std::unexpected{bounded_password.error()};
}
PasswordCStringGuard password_guard{*bounded_password};
src/adapters/password_auth.cpp:326
crypt_rreceives caller-ownedcrypt_datastorage, which libxcrypt can use for password-derived state and input buffering, but this object is never wiped on either the success or error paths. The existing password-buffer guard therefore does not remove all sensitive material from the stack; add an RAII zeroization guard fornative_databefore returning.
crypt_data native_data{};
errno = 0;
const char* native_result =
crypt_r(bounded_password->c_str(), bounded_hash->c_str(), &native_data);
- Files reviewed: 28/28 changed files
- Comments generated: 2
- Review effort level: Lite
- Added `validate_input_label_structure` to check for empty labels in hostnames. - Integrated input validation into `idna_to_ascii` to ensure proper hostname structure. - Introduced `SecretPassword` class to manage password storage securely, preventing copy operations. - Added tests for empty label handling in IDNA and for the cleansing behavior of `SecretPassword`.
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved findings affect libidn2 build/linking, IDNA compilation and normalization, password-buffer cleansing, and an IDNA boundary test.
Review details
Suppressed comments (7)
cmake/LaghuDependencies.cmake:866
ExternalProject_AddtreatsBUILD_COMMANDas one command vector;COMMANDis not a separator for multiple build commands here and will be passed to the firstmakeinvocation as a literal argument. Consequently the vendored libidn2 build cannot run the three required make steps. Use anExternalProject_Add_Stepchain or a wrapper script/CMake script that executes the commands separately.
set(build_command
"${laghu_make_program}" -C "<SOURCE_DIR>/gl" all
COMMAND "${laghu_make_program}" -C "<SOURCE_DIR>/unistring" all
COMMAND "${laghu_make_program}" -C "<SOURCE_DIR>/lib" libidn2.la)
cmake/LaghuDependencies.cmake:869
- The bundled libidn2 build creates a separate
unistringlibrary (--with-included-libunistring), but this install step installs only libidn2 and the imported target later links onlylibidn2. In STATIC mode the resulting archive has unresolved libunistring symbols; in DYNAMIC mode its bundled runtime dependency is not installed, so the new vendored IDNA fixture and real adapter link/runtime can fail. Install and expose the bundled libunistring artifact as a transitive dependency, or configure/build libidn2 as a truly self-contained artifact.
set(install_command
"${laghu_make_program}" -C "<SOURCE_DIR>/lib"
install-libLTLIBRARIES install-includeHEADERS)
src/adapters/idna.cpp:203
IDN2_NFC_INPUTmeans that the caller asserts the input is already NFC; it does not ask libidn2 to normalize arbitrary UTF-8. This adapter accepts decomposed input (the added golden vector passesu\u0308) and therefore this flag causes that vector to be rejected or processed without TR46 NFC mapping instead of producing the expected A-label. Omit this flag or normalize before the native call while retaining the non-transitional and STD3 flags.
constexpr int idna_flags = IDN2_NFC_INPUT | IDN2_NONTRANSITIONAL | IDN2_USE_STD3_ASCII_RULES;
src/adapters/idna.cpp:106
IDN2_NOT_NFCis not a libidn2 status exported byidn2.h; the public constant isIDN2_NFC. As written, this translation unit cannot compile against the declared system floor or the vendored header. Replace it with the exported constant.
status == IDN2_NOT_NFC || status == IDN2_2HYPHEN ||
src/adapters/password_auth.cpp:307
- Only
SecretPasswordis cleansed here.crypt_datais libxcrypt's caller-owned scratch/output buffer and can retain password-derived material aftercrypt_r; every return below leaves that stack object untouched. Wrap it in an RAII secure zeroizer over the completecrypt_dataobject before returning.
crypt_data native_data{};
errno = 0;
const char* native_result =
crypt_r(bounded_password->c_str(), bounded_hash->c_str(), &native_data);
src/adapters/password_auth.cpp:287
to_c_stringcreates a second stack copy of the caller's encoded credential, but no path cleanses thisStaticCString; the newly addedStaticCString::cleanse()is never used. Cleanse this copy via RAII so early parse/native-error returns do not leave the verifier hash in stack memory.
const auto bounded_hash = encoded_hash.to_c_string<encoded_password_capacity>();
if (!bounded_hash.has_value()) {
return std::unexpected{bounded_hash.error()};
}
const auto parsed_hash = parse_password_hash(bounded_hash->view());
tests/adapters/idna.cpp:125
- This 255-byte hostname is still at the inclusive 255-octet libidn2 bound (four 63-byte labels plus separators), so libidn2 can accept it and the adapter will then reject it at its separate 253-byte output contract. The assertion requiring a libidn2 range status is therefore not reliable. Use an input definitely over the native bound, such as 257 bytes with a separator after the fourth 63-byte label, or assert the adapter-level
invalid_rangeresult.
std::array<char, 255> too_big_domain{};
for (std::size_t index = 0; index < too_big_domain.size(); ++index) {
too_big_domain[index] = (index == 63 || index == 127 || index == 191) ? '.' : 'a';
}
- Files reviewed: 28/28 changed files
- Comments generated: 0 new
- Review effort level: Lite
- Changed the return statement in the from() method of SecretPassword to return the output directly instead of using std::move. - This improves code clarity and avoids unnecessary move semantics for the SecretPassword object.
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate issues remain in dependency builds/linking, IDNA normalization and bounds, secret wiping, hash bounds, and enum compatibility.
Review details
Suppressed comments (7)
cmake/LaghuDependencies.cmake:907
- In STATIC mode, the bundled libidn2 build uses the included libunistring archive, but this interface exposes only
libidn2.a. The symbol probe and any consumer therefore link the IDNA archive without itsu8_*dependency and fail with unresolved symbols. Export the bundled libunistring archive as a transitive link dependency (and make its build/install artifact available) when linking the static target.
target_link_libraries("${private_target}" INTERFACE "${private_target}_artifact")
cmake/LaghuDependencies.cmake:866
ExternalProject_AddtreatsBUILD_COMMANDas one command;COMMANDis not a separator here and will be passed to the firstmakeinvocation as a literal target/argument. Vendored libidn2 builds will therefore fail before the library is produced. Use a single wrapper/script (or otherwise model the three dependent build steps) rather than embedding multipleCOMMANDtokens in this value.
set(build_command
"${laghu_make_program}" -C "<SOURCE_DIR>/gl" all
COMMAND "${laghu_make_program}" -C "<SOURCE_DIR>/unistring" all
COMMAND "${laghu_make_program}" -C "<SOURCE_DIR>/lib" libidn2.la)
src/adapters/idna.cpp:203
IDN2_NFC_INPUTtells libidn2 that the caller has already supplied NFC input; it does not request normalization. As a result, the decomposedbu\xCC\x88cher.examplevector in this PR will be rejected or processed without the required NFC/TR46 mapping, contrary to the stated IDNA2008/TR46 behavior. Omit this flag so libidn2 performs normalization.
constexpr int idna_flags = IDN2_NFC_INPUT | IDN2_NONTRANSITIONAL | IDN2_USE_STD3_ASCII_RULES;
src/adapters/idna.cpp:188
- The same unbounded-scan problem exists for hostnames:
to_c_string()checks for embedded NUL before checking its capacity, so an oversized borrowedTextViewis scanned in full instead of being rejected at the adapter's 1020-byte input bound. Checkhostname.size()againstutf8_input_storage_capacityfirst.
const auto input = hostname.to_c_string<utf8_input_storage_capacity>();
if (!input.has_value()) {
return std::unexpected{input.error()};
src/adapters/password_auth.cpp:307
crypt_datacontains libxcrypt's native working state and output, including password-derived material. Every return aftercrypt_rleavesnative_datauncleansed on the stack, so the new password adapter can retain sensitive state for later stack reuse or crash/core-dump exposure. Wrap this object in an RAII cleanup guard that performs a volatile wipe before all exits.
crypt_data native_data{};
errno = 0;
const char* native_result =
crypt_r(bounded_password->c_str(), bounded_hash->c_str(), &native_data);
src/adapters/password_auth.cpp:285
- The hash length is not checked before
TextView::to_c_string()scans for embedded NULs. A caller can pass an arbitrarily large borrowed view and force an unbounded linear scan before the adapter rejects it, defeating the advertised bounded authentication path. Rejectencoded_hash.size() >= encoded_password_capacitybefore conversion.
const auto bounded_hash = encoded_hash.to_c_string<encoded_password_capacity>();
if (!bounded_hash.has_value()) {
return std::unexpected{bounded_hash.error()};
src/core/contract/laghu/core/contract.hpp:52
- These new statuses are inserted before every existing
DependencyStatusvalue. Because the enum is a fixed-width public contract and is stored inDependencyLogRecord, this renumbers existingunavailable,unsupported_version, and later statuses for consumers decoding the record. Append new values after the existing list or assign explicit numeric values to preserve the established encoding.
invalid_input,
invalid_range,
unavailable,
exhaustion,
- Files reviewed: 28/28 changed files
- Comments generated: 0 new
- Review effort level: Lite
closes: #57