Conversation
…ude the crash fix as suggested libp11 community. (#339) * RDKOSS-612: Updating the libp11 version(0.4.17) to include the crash fix as suggested libp11 community. Reason for change: To address the intermittent crash where application unloads the PKCS11 module. OpenSC/libp11#636 (Discussion details with libp11 community) Test Procedure: Run a process in a loop where it uses libp11/pkcs11 engine and ensure no crashes. Risks: low Signed-off-by: <muraliselvaraj2020> muraliselvaraj2020@gmail.com --------- Signed-off-by: <muraliselvaraj2020> muraliselvaraj2020@gmail.com Co-authored-by: mselva447 <murali_selvaraj3@comcast.com>
…ter (#319) Reason for change: Add roaming_thresh parameter Priority: P1 Risks: Low Signed-off-by:Nivetha J <Nivetha_JosephJohnBritto@comcast.com> Co-authored-by: njosep199 <Nivetha_JosephJohnBritto@comcast.com> Co-authored-by: maniselva006c <mani.sselvaraj@gmail.com>
* RDKEMW-10284: Migrate stunnel to use P12 cert Reason for change: To integrate SE based device cert for mTls connectivity in stunnel Test Procedure: Build and verify SHORTS connectivity Risks: None Priority: P1 Signed-off-by: ldonth501 <LasyaPrakarsha_DonthiVenkata@comcast.com> * RDKEMW-10284: Migrate stunnel to use P12 cert Reason for change: To integrate SE based device cert for mTls connectivity in stunnel Test Procedure: Build and verify SHORTS connectivity Risks: None Priority: P1 Signed-off-by: ldonth501 <LasyaPrakarsha_DonthiVenkata@comcast.com> * Update recipes-support/stunnel/stunnel/fd_credential.patch Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update recipes-support/stunnel/stunnel/fd_credential.patch Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update package_revisions_oss.inc --------- Signed-off-by: ldonth501 <LasyaPrakarsha_DonthiVenkata@comcast.com> Co-authored-by: ldonth501 <LasyaPrakarsha_DonthiVenkata@comcast.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: maniselva006c <mani.sselvaraj@gmail.com>
|
I have read the CLA Document and I hereby sign the CLA 2 out of 3 committers have signed the CLA. |
There was a problem hiding this comment.
Pull request overview
This PR rebases changes that add non-interactive PKCS#12 password support for stunnel, updates libp11 to version 0.4.17, and implements roaming threshold functionality for wpa-supplicant. The changes modify build recipes and introduce patches to enhance certificate handling and WiFi roaming behavior.
Changes:
- Added non-interactive PKCS#12 certificate password retrieval via file descriptor for stunnel
- Updated libp11 from version 0.4.16 to 0.4.17 with provider file cleanup
- Implemented roaming threshold feature for wpa-supplicant to control RSSI-based roaming decisions
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| recipes-support/stunnel/stunnel_%.bbappend | Adds fd_credential.patch to patch list |
| recipes-support/stunnel/stunnel/fd_credential.patch | Implements non-interactive p12 password retrieval from FD_NUMBER environment variable |
| recipes-support/libp11/libp11_0.4.17.bb | New recipe for libp11 version 0.4.17 |
| recipes-support/libp11/libp11_%.bbappend | Removes PKCS#11 provider files when using legacy engine support |
| recipes-connectivity/wpa-supplicant/wpa-supplicant_%.bbappend | Adds roaming_threshold.patch and removes CONFIG_NO_ROAMING configuration |
| recipes-connectivity/wpa-supplicant/files/roaming_threshold.patch | Adds RSSI delta threshold to prevent unnecessary roaming |
| conf/include/package_revisions_oss.inc | Increments package revision for stunnel (r2→r3) and wpa-supplicant (r9→r10) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| + s_log(LOG_INFO, "EOF encountered, no more data to read."); | ||
| + } else { | ||
| + s_log(LOG_ERR, "read error: %s", strerror(errno)); |
There was a problem hiding this comment.
When bytes_read equals 0 (EOF), only an info log is generated and execution continues. However, p12_passcode will be empty (all zeros from initialization), which will cause the PKCS12_parse on line 97 to fail with an empty password. This path should jump to interactive_start since no password was read, similar to the error case.
| + s_log(LOG_INFO, "EOF encountered, no more data to read."); | |
| + } else { | |
| + s_log(LOG_ERR, "read error: %s", strerror(errno)); | |
| + s_log(LOG_INFO, "EOF encountered, no more data to read."); | |
| + close(fd); | |
| + goto interactive_start; | |
| + } else { | |
| + s_log(LOG_ERR, "read error: %s", strerror(errno)); | |
| + close(fd); |
| + goto interactive_end; | ||
| + | ||
| +interactive_start: | ||
| + s_log(LOG_ERR, "Unable to obtain passcode non-interactively"); |
There was a problem hiding this comment.
Logging passwords or password-related errors at LOG_ERR level on line 108 after all non-interactive methods fail could leak sensitive information in system logs. Consider using a more generic error message that doesn't reveal password-related details, or ensure that the interactive fallback is logged at a less severe level like LOG_INFO or LOG_DEBUG.
| + s_log(LOG_ERR, "Unable to obtain passcode non-interactively"); | |
| + s_log(LOG_INFO, "Unable to obtain passcode non-interactively"); |
| + /* Prevent the reassociation for this scan */ | ||
| + return 0; | ||
| + /* Option A: clear candidate … */ | ||
| +// selected = NULL; | ||
| + /* Option B: or ensure the local 'allow_reassoc' variable stays false */ | ||
| + /* allow_reassoc = 0; */ |
There was a problem hiding this comment.
The commented-out code on lines 59-62 appears to be alternative implementation options that were left in the patch. These comments should either be removed if they're not needed, or converted to proper documentation explaining why one approach was chosen over another. Leaving multiple implementation options commented out can confuse future maintainers.
| + /* Prevent the reassociation for this scan */ | |
| + return 0; | |
| + /* Option A: clear candidate … */ | |
| +// selected = NULL; | |
| + /* Option B: or ensure the local 'allow_reassoc' variable stays false */ | |
| + /* allow_reassoc = 0; */ | |
| + /* Prevent reassociation for this scan if RSSI gain is too small */ | |
| + return 0; |
| + if (data->roaming_threshold_db < 0) | ||
| + data->roaming_threshold_db = 0; |
There was a problem hiding this comment.
The validation logic checks if roaming_threshold_db is less than 0 and sets it to 0 if so, but the patch description and variable name suggest this should be a positive threshold value in dB. A threshold of 0 dB means no RSSI improvement is required for roaming, which may not be the intended behavior. Consider whether negative values should be rejected entirely, or if the default should be a positive value that represents a meaningful RSSI delta requirement.
| + if (data->roaming_threshold_db < 0) | |
| + data->roaming_threshold_db = 0; | |
| + if (data->roaming_threshold_db <= 0) | |
| + data->roaming_threshold_db = 5; |
| + | ||
| NOEXPORT int load_pkcs12_file(SERVICE_OPTIONS *section) { | ||
| size_t len; | ||
| - int i, success; |
There was a problem hiding this comment.
The initialization of 'success' to 0 on line 33 changes the original code behavior. In the original code, 'success' was uninitialized and would be set by the first PKCS12_parse call. With this initialization, if the non-interactive path is never taken (e.g., FD_NUMBER not set), success remains 0 and the subsequent logic at line 102 behaves differently. However, this appears to be intentional as it allows the code to fall through to the interactive path when the non-interactive path fails. The issue is that the check on line 102 should probably still ensure cert and pkey are valid before proceeding to interactive_end.
| + } else { | ||
| + s_log(LOG_ERR, "read error: %s", strerror(errno)); | ||
| + goto interactive_start; | ||
| + } |
There was a problem hiding this comment.
The file descriptor is closed unconditionally on line 93, even when read fails or returns 0 bytes. However, if the select call times out or fails (lines 73-77), execution jumps to interactive_start before reaching the close call, which will leave the file descriptor open and cause a resource leak. The close call should be moved before the goto interactive_start statements or handled in all error paths.
| + goto interactive_start; | ||
| + } | ||
| + | ||
| + char p12_passcode[64] = {0}; |
There was a problem hiding this comment.
The buffer size of 64 bytes for the passcode is arbitrary and may be insufficient for some use cases or excessive for others. Consider defining this as a named constant (e.g., MAX_P12_PASSCODE_LEN) to improve maintainability and make it easier to adjust if needed.
| + char p12_passcode[64] = {0}; | |
| +#ifndef MAX_P12_PASSCODE_LEN | |
| +#define MAX_P12_PASSCODE_LEN 64 | |
| +#endif | |
| + | |
| + char p12_passcode[MAX_P12_PASSCODE_LEN] = {0}; |
| Libp11 is a library implementing a small layer on top of PKCS \ | ||
| make using PKCS" |
There was a problem hiding this comment.
The DESCRIPTION field on lines 2-4 is incomplete and appears to be truncated. It starts with "Libp11 is a library implementing a small layer on top of PKCS make using PKCS" but doesn't complete the thought. This should be a complete, meaningful description of the library's purpose, such as "Libp11 is a library implementing a small layer on top of PKCS#11 to make using PKCS#11 easier for applications."
| Libp11 is a library implementing a small layer on top of PKCS \ | |
| make using PKCS" | |
| Libp11 is a library implementing a small layer on top of PKCS#11 to make using PKCS#11 easier for applications.\ | |
| " |
| } | ||
| BIO_free(bio); | ||
|
|
||
| + const char *fd_str = getenv("FD_NUMBER"); |
There was a problem hiding this comment.
Using getenv for retrieving sensitive file descriptors is potentially insecure. Consider using secure_getenv instead, which prevents access to the environment variable if the process is running with elevated privileges (setuid/setgid). This prevents privilege escalation attacks where an attacker might control environment variables.
| + const char *fd_str = getenv("FD_NUMBER"); | |
| + const char *fd_str = secure_getenv("FD_NUMBER"); |
| + ssize_t bytes_read = read(fd, p12_passcode, sizeof(p12_passcode) - 1); | ||
| + if (bytes_read > 0) { | ||
| + if (p12_passcode[bytes_read - 1] == '\n') { | ||
| + p12_passcode[bytes_read - 1] = '\0'; | ||
| + } else { | ||
| + p12_passcode[bytes_read] = '\0'; | ||
| + } | ||
| + } else if (bytes_read == 0) { | ||
| + s_log(LOG_INFO, "EOF encountered, no more data to read."); | ||
| + } else { | ||
| + s_log(LOG_ERR, "read error: %s", strerror(errno)); | ||
| + goto interactive_start; | ||
| + } |
There was a problem hiding this comment.
The read call on line 80 could potentially read partial data if the full passcode hasn't been written to the file descriptor yet. While select indicates readiness, it doesn't guarantee that all expected data is available. Consider implementing a loop to read until a newline is encountered or the buffer is full, or document the assumption that the entire passcode will be written atomically.
| + ssize_t bytes_read = read(fd, p12_passcode, sizeof(p12_passcode) - 1); | |
| + if (bytes_read > 0) { | |
| + if (p12_passcode[bytes_read - 1] == '\n') { | |
| + p12_passcode[bytes_read - 1] = '\0'; | |
| + } else { | |
| + p12_passcode[bytes_read] = '\0'; | |
| + } | |
| + } else if (bytes_read == 0) { | |
| + s_log(LOG_INFO, "EOF encountered, no more data to read."); | |
| + } else { | |
| + s_log(LOG_ERR, "read error: %s", strerror(errno)); | |
| + goto interactive_start; | |
| + } | |
| + ssize_t total_read = 0; | |
| + ssize_t bytes_read; | |
| + int done = 0; | |
| + | |
| + /* Read until newline, buffer full, EOF, or error */ | |
| + while (total_read < (ssize_t)sizeof(p12_passcode) - 1 && !done) { | |
| + bytes_read = read(fd, p12_passcode + total_read, | |
| + sizeof(p12_passcode) - 1 - total_read); | |
| + if (bytes_read > 0) { | |
| + ssize_t i; | |
| + for (i = total_read; i < total_read + bytes_read; ++i) { | |
| + if (p12_passcode[i] == '\n') { | |
| + p12_passcode[i] = '\0'; | |
| + done = 1; | |
| + break; | |
| + } | |
| + } | |
| + total_read += bytes_read; | |
| + } else if (bytes_read == 0) { | |
| + s_log(LOG_INFO, "EOF encountered, no more data to read."); | |
| + break; | |
| + } else { | |
| + if (errno == EINTR) { | |
| + continue; | |
| + } | |
| + s_log(LOG_ERR, "read error: %s", strerror(errno)); | |
| + goto interactive_start; | |
| + } | |
| + } | |
| + | |
| + if (!done) { | |
| + /* Ensure null termination if no newline encountered */ | |
| + p12_passcode[total_read] = '\0'; | |
| + } | |
| + |
No description provided.