-
Notifications
You must be signed in to change notification settings - Fork 2
Topic/rdk 57502 #65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Topic/rdk 57502 #65
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a new upload utilities module (uploadutils/) to the RDK common utilities library, providing mTLS-enabled file upload capabilities with certificate rotation, CodeBig authorization support, and enhanced status reporting. The implementation mirrors the existing download utilities structure and integrates with the RDK certificate selector library for secure authentication.
Key Changes:
- Added comprehensive upload API with three upload methods: two-stage mTLS flow, CodeBig flow, and S3 PUT operations
- Implemented thread-local status tracking to capture detailed HTTP/CURL error codes without modifying existing function signatures
- Integrated certificate rotation support through the rdkcertselector library for automatic retry with alternate certificates
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| uploadutils/upload_status.h | Public API for enhanced upload functions with detailed status reporting structure |
| uploadutils/upload_status.c | Implementation of status tracking using thread-local storage and wrapper functions for existing upload APIs |
| uploadutils/uploadUtil.h | Core upload utility functions including HTTP POST and S3 PUT operations |
| uploadutils/uploadUtil.c | Implementation of HTTP metadata POST and S3 PUT upload with mTLS support |
| uploadutils/mtls_upload.h | mTLS upload API with certificate rotation support |
| uploadutils/mtls_upload.c | Two-stage upload workflow (POST + PUT) with automatic certificate retry mechanism |
| uploadutils/codebig_upload.h | CodeBig authorization-based upload API definitions |
| uploadutils/codebig_upload.c | CodeBig upload implementation with signed URL generation and authorization headers |
| uploadutils/main_upload_test.c | Test program demonstrating upload functionality usage |
| uploadutils/Makefile.am | Build configuration for upload utilities library with conditional CPC code support |
| configure.ac | Autotools configuration additions for CPC code support and uploadutils module |
| Makefile.am | Updated to include uploadutils subdirectory in build process |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
uploadutils/Makefile.am
Outdated
| #bin_PROGRAMS = uploadutil_test | ||
|
|
||
| #uploadutil_test_SOURCES = main_upload_test.c | ||
|
|
||
| #uploadutil_test_CFLAGS = -I${top_srcdir}/dwnlutils -I$(top_srcdir)/utils -I${top_srcdir}/uploadutils -DLIBRDKCERTSELECTOR | ||
| #uploadutil_test_LDADD = -lrdkloggers -lpthread $(curl_LIBS) -L$(PKG_CONFIG_SYSROOT_DIR)/usr/lib -lRdkCertSelector -L$(PKG_CONFIG_SYSROOT_DIR)/usr/lib64 -lrdkconfig | ||
| #uploadutil_test_LDADD += ${top_builddir}/dwnlutils/libdwnlutil.la ${top_builddir}/utils/libfwutils.la ${top_builddir}/uploadutils/libuploadutil.la | ||
|
|
||
|
|
||
|
|
||
| # Debug binary for testing | ||
| #bin_PROGRAMS = uploadutil_test | ||
|
|
||
| #uploadutil_test_SOURCES = uploadUtil.c \ | ||
| # mtls_upload.c \ | ||
| # main_upload_test.c \ | ||
| # codebig_upload.c | ||
| #if USE_CPC_CODE | ||
| #uploadutil_test_SOURCES += \ | ||
| # ${top_srcdir}/src/upload_util-cpc/upload_util/codebigUtils.c | ||
| #endif | ||
|
|
||
| #uploadutil_test_CFLAGS = -I${top_srcdir}/dwnlutils -I$(top_srcdir)/utils -I${top_srcdir}/uploadutils -I${top_srcdir}/parsejson -DLIBRDKCERTSELECTOR | ||
| #if USE_CPC_CODE | ||
| #uploadutil_test_CFLAGS += -I${top_srcdir}/src/upload_util-cpc/upload_util | ||
| #endif | ||
| #uploadutil_test_LDADD = -lrfcapi -lrdkloggers -lpthread $(curl_LIBS) -L$(PKG_CONFIG_SYSROOT_DIR)/usr/lib -lRdkCertSelector -L$(PKG_CONFIG_SYSROOT_DIR)/usr/lib64 -lrdkconfig | ||
| #uploadutil_test_LDADD += ${top_builddir}/dwnlutils/libdwnlutil.la ${top_builddir}/utils/libfwutils.la | ||
|
|
||
|
|
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Large sections of commented-out build configuration (lines 53-81) should be removed. If these test build targets are intended for future use, they should either be enabled with proper conditional compilation or removed entirely. Maintaining extensive commented-out configuration makes the build system harder to understand and maintain.
| #bin_PROGRAMS = uploadutil_test | |
| #uploadutil_test_SOURCES = main_upload_test.c | |
| #uploadutil_test_CFLAGS = -I${top_srcdir}/dwnlutils -I$(top_srcdir)/utils -I${top_srcdir}/uploadutils -DLIBRDKCERTSELECTOR | |
| #uploadutil_test_LDADD = -lrdkloggers -lpthread $(curl_LIBS) -L$(PKG_CONFIG_SYSROOT_DIR)/usr/lib -lRdkCertSelector -L$(PKG_CONFIG_SYSROOT_DIR)/usr/lib64 -lrdkconfig | |
| #uploadutil_test_LDADD += ${top_builddir}/dwnlutils/libdwnlutil.la ${top_builddir}/utils/libfwutils.la ${top_builddir}/uploadutils/libuploadutil.la | |
| # Debug binary for testing | |
| #bin_PROGRAMS = uploadutil_test | |
| #uploadutil_test_SOURCES = uploadUtil.c \ | |
| # mtls_upload.c \ | |
| # main_upload_test.c \ | |
| # codebig_upload.c | |
| #if USE_CPC_CODE | |
| #uploadutil_test_SOURCES += \ | |
| # ${top_srcdir}/src/upload_util-cpc/upload_util/codebigUtils.c | |
| #endif | |
| #uploadutil_test_CFLAGS = -I${top_srcdir}/dwnlutils -I$(top_srcdir)/utils -I${top_srcdir}/uploadutils -I${top_srcdir}/parsejson -DLIBRDKCERTSELECTOR | |
| #if USE_CPC_CODE | |
| #uploadutil_test_CFLAGS += -I${top_srcdir}/src/upload_util-cpc/upload_util | |
| #endif | |
| #uploadutil_test_LDADD = -lrfcapi -lrdkloggers -lpthread $(curl_LIBS) -L$(PKG_CONFIG_SYSROOT_DIR)/usr/lib -lRdkCertSelector -L$(PKG_CONFIG_SYSROOT_DIR)/usr/lib64 -lrdkconfig | |
| #uploadutil_test_LDADD += ${top_builddir}/dwnlutils/libdwnlutil.la ${top_builddir}/utils/libfwutils.la |
uploadutils/upload_status.c
Outdated
|
|
||
| if (!upload_url || !src_file) { | ||
| snprintf(status->error_message, sizeof(status->error_message), | ||
| "Invalid parameters: upload_url=%p, src_file=%p", upload_url, src_file); |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using %p format specifier to print pointer addresses in error messages could leak memory addresses, which is a security concern in production environments as it could aid in exploiting memory corruption vulnerabilities. Consider using a generic error message like "Invalid parameters: missing upload_url or src_file" instead of exposing pointer values.
| "Invalid parameters: upload_url=%p, src_file=%p", upload_url, src_file); | |
| "Invalid parameters: missing upload_url or src_file"); |
uploadutils/upload_status.c
Outdated
|
|
||
| if (!upload_url || !src_file) { | ||
| snprintf(status->error_message, sizeof(status->error_message), | ||
| "Invalid parameters: upload_url=%p, src_file=%p", upload_url, src_file); |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using %p format specifier to print pointer addresses in error messages could leak memory addresses. Use a generic error message instead.
| "Invalid parameters: upload_url=%p, src_file=%p", upload_url, src_file); | |
| "Invalid parameters: upload_url or src_file is NULL"); |
uploadutils/mtls_upload.c
Outdated
| snprintf(postfields, sizeof(postfields), "md5=%s", md5); | ||
| file_upload.pPostFields = postfields; |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The buffer size for postfields is fixed at 256 bytes. If the MD5 hash is 44 characters (base64 encoded), this leaves limited room, but it should be sufficient. However, consider validating the MD5 string length to ensure it doesn't cause truncation, which would result in an invalid MD5 value being sent to the server.
| snprintf(postfields, sizeof(postfields), "md5=%s", md5); | |
| file_upload.pPostFields = postfields; | |
| size_t md5_len = strlen(md5); | |
| /* "md5=" is 4 chars, plus null terminator */ | |
| if (md5_len > (sizeof(postfields) - 5)) { | |
| COMMONUTILITIES_ERROR("%s: MD5 string too long (%zu bytes), not sending\n", __FUNCTION__, md5_len); | |
| file_upload.pPostFields = NULL; | |
| } else { | |
| snprintf(postfields, sizeof(postfields), "md5=%s", md5); | |
| file_upload.pPostFields = postfields; | |
| } |
uploadutils/upload_status.c
Outdated
| snprintf(status->error_message, sizeof(status->error_message), | ||
| "Authentication failed (HTTP %ld)", http_code); | ||
| } else if (http_code >= 400) { | ||
| status->auth_success = true; // Auth OK, but other error |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for setting auth_success is incorrect. When HTTP code is 401 or 403, auth_success is set to false (lines 198-201), which is correct. However, when HTTP code is >= 400 (but not 401/403), auth_success is set to true with the comment "Auth OK, but other error" (lines 202-205). This is misleading because a 4xx error doesn't necessarily mean authentication succeeded - it could be that the authentication was never attempted. For example, if there's a 400 Bad Request error before authentication occurs, setting auth_success = true would be incorrect. Consider setting auth_success based on whether the upload progressed past the authentication stage, not just on the HTTP status code.
| status->auth_success = true; // Auth OK, but other error | |
| status->auth_success = false; // Could be pre-auth error, do not assume auth succeeded |
uploadutils/uploadUtil.c
Outdated
| } | ||
|
|
||
| /* Capture response body */ | ||
| resp_fp = fopen("/tmp/httpresult.txt", "wb"); |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded path /tmp/httpresult.txt is used for storing HTTP response data. This creates potential issues: (1) the filename is not unique, which could cause conflicts if multiple upload operations run concurrently in different processes, (2) no cleanup mechanism is visible for this temporary file. Consider using mkstemp() or a similar function to generate unique temporary filenames, and ensure proper cleanup after the file is no longer needed.
uploadutils/codebig_upload.c
Outdated
|
|
||
| /* Step 4: Extract S3 URL and perform S3 PUT */ | ||
| char s3_url[S3_URL_BUF]; | ||
| if (extractS3PresignedUrl("/tmp/httpresult.txt", s3_url, sizeof(s3_url)) != 0) { |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded path /tmp/httpresult.txt is used here as well. This is a repeated issue across the codebase that could cause race conditions and data corruption in concurrent scenarios.
uploadutils/upload_status.c
Outdated
|
|
||
| // Copy hostname to output buffer | ||
| size_t hostname_len = hostname_end - hostname_start; | ||
| if (hostname_len > 0 && hostname_len < fqdn_size) { |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition hostname_len < fqdn_size should be hostname_len < fqdn_size - 1 or changed to <= to ensure there's always room for the null terminator. Currently, if hostname_len equals fqdn_size - 1, the code will work correctly because line 129 explicitly null-terminates. However, if hostname_len equals fqdn_size, the hostname won't be copied at all (silently fails). This edge case should either be handled with a proper size check or documented. Consider changing to hostname_len <= fqdn_size - 1 to be more explicit about the space requirement.
| if (hostname_len > 0 && hostname_len < fqdn_size) { | |
| if (hostname_len > 0 && hostname_len <= fqdn_size - 1) { |
| AM_CFLAGS = -D_ANSC_LINUX | ||
| AM_CFLAGS += -D_ANSC_USER | ||
| AM_CFLAGS += -D_ANSC_LITTLE_ENDIAN_ | ||
| AM_CFLAGS += -Wall -Werror $(curl_CFLAGS) $(CFLAGS) -DLIBRDKCERTSELECTOR |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flag -DLIBRDKCERTSELECTOR is hardcoded in the AM_CFLAGS at line 21, but there's also a conditional block at lines 48-51 that checks IS_LIBRDKCERTSEL_ENABLED. This creates inconsistency - the define is always set regardless of whether the feature is enabled. Consider removing the hardcoded -DLIBRDKCERTSELECTOR from line 21 and letting it be controlled by the conditional block, or clarify why it needs to be always defined.
| AM_CFLAGS += -Wall -Werror $(curl_CFLAGS) $(CFLAGS) -DLIBRDKCERTSELECTOR | |
| AM_CFLAGS += -Wall -Werror $(curl_CFLAGS) $(CFLAGS) |
uploadutils/upload_status.h
Outdated
| * @brief Enhanced CodeBig upload with detailed status return | ||
| * | ||
| * @param src_file Local file path to upload | ||
| * @param server_type CodeBig server type |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter documentation for server_type lacks detail. It should specify what values are valid (e.g., HTTP_SSR_CODEBIG=1, HTTP_XCONF_CODEBIG=3 as defined in codebig_upload.h). Consider adding: "@param server_type CodeBig server type (HTTP_SSR_CODEBIG or HTTP_XCONF_CODEBIG)" or referencing the constants defined in the related header.
| * @param server_type CodeBig server type | |
| * @param server_type CodeBig server type. Valid values are HTTP_SSR_CODEBIG or HTTP_XCONF_CODEBIG as defined in codebig_upload.h. |
uploadutils/uploadUtil.c
Outdated
| curl_off_t filesize = ftell(fp); | ||
| fseek(fp, 0, SEEK_SET); | ||
|
|
||
| curl_easy_setopt(curl, CURLOPT_PUT, 1L); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Unchecked return value from library
Calling "curl_easy_setopt(curl, _curl_opt, 1L)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
uploadutils/uploadUtil.c
Outdated
| fseek(fp, 0, SEEK_SET); | ||
|
|
||
| curl_easy_setopt(curl, CURLOPT_PUT, 1L); | ||
| curl_easy_setopt(curl, CURLOPT_READDATA, fp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Unchecked return value from library
Calling "curl_easy_setopt(curl, _curl_opt, fp)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
uploadutils/uploadUtil.c
Outdated
|
|
||
| curl_easy_setopt(curl, CURLOPT_PUT, 1L); | ||
| curl_easy_setopt(curl, CURLOPT_READDATA, fp); | ||
| curl_easy_setopt(curl, CURLOPT_INFILESIZE_LARGE, filesize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Unchecked return value from library
Calling "curl_easy_setopt(curl, _curl_opt, filesize)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
uploadutils/uploadUtil.c
Outdated
| curl_easy_setopt(curl, CURLOPT_PUT, 1L); | ||
| curl_easy_setopt(curl, CURLOPT_READDATA, fp); | ||
| curl_easy_setopt(curl, CURLOPT_INFILESIZE_LARGE, filesize); | ||
| curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Unchecked return value from library
Calling "curl_easy_setopt(curl, _curl_opt, 1L)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
uploadutils/uploadUtil.c
Outdated
| snprintf(postfields, sizeof(postfields), "filename=%s", | ||
| pfile_upload->pathname); | ||
| } | ||
| curl_easy_setopt(curl, CURLOPT_POSTFIELDS, postfields); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Unchecked return value from library
Calling "curl_easy_setopt(curl, _curl_opt, postfields)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
uploadutils/uploadUtil.c
Outdated
| if (headers) curl_slist_free_all(headers); | ||
| return (int)UPLOAD_FAIL; | ||
| } | ||
| curl_easy_setopt(curl, CURLOPT_WRITEDATA, resp_fp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Unchecked return value from library
Calling "curl_easy_setopt(curl, _curl_opt, resp_fp)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
uploadutils/uploadUtil.c
Outdated
| return (int)UPLOAD_FAIL; | ||
| } | ||
| curl_easy_setopt(curl, CURLOPT_WRITEDATA, resp_fp); | ||
| curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Unchecked return value from library
Calling "curl_easy_setopt(curl, _curl_opt, 1L)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
Hi @Abhinavpv28 : If these new files are really from RDK, then the RDK headers are fine, but please append a credit to the end of top-level NOTICE like this: Thank you, |
| name: Execute L1 test suite in test container environment | ||
| runs-on: ubuntu-latest | ||
| container: | ||
| image: ghcr.io/rdkcentral/docker-rdk-ci:latest | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Log in to GitHub Container Registry | ||
| uses: docker/login-action@v2 | ||
| with: | ||
| registry: ghcr.io | ||
| username: ${{ github.actor }} | ||
| password: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| - name: Pull test container image | ||
| run: docker pull ghcr.io/rdkcentral/docker-device-mgt-service-test/native-platform:latest | ||
|
|
||
| - name: Start test container | ||
| run: | | ||
| docker run -d --name native-platform -v ${{ github.workspace }}:/mnt/L1_CONTAINER_SHARED_VOLUME ghcr.io/rdkcentral/docker-device-mgt-service-test/native-platform:latest | ||
| - name: Run unit tests | ||
| run: sh unit_test.sh | ||
| - name: Run L1 Unit Tests inside container | ||
| run: docker exec -i native-platform /bin/bash -c "cd /mnt/L1_CONTAINER_SHARED_VOLUME/ && sh unit_test.sh" | ||
|
|
||
| - name: Upload test results to automatic test result management system | ||
| - name: Copy L1 test results to runner | ||
| run: | | ||
| docker cp native-platform:/tmp/Gtest_Report /tmp/Gtest_Report | ||
| ls -l /tmp/Gtest_Report | ||
| upload-test-results: |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 5 hours ago
To fix this problem, an explicit permissions block should be added to the workflow at the root or job level to restrict the GitHub Actions token to the minimum required privileges. In this workflow, only reading repository contents is necessary (for actions/checkout), so contents: read is sufficient. This permissions block can be placed at the top level of the workflow so that it applies to all jobs, unless a specific job requires additional permissions. Edit the .github/workflows/L1-Test.yaml file to add:
permissions:
contents: readafter the name: block and before the on: block. No additional dependencies or code changes outside the workflow file are needed.
-
Copy modified lines R3-R5
| @@ -1,5 +1,8 @@ | ||
| name: L1 Unit Tests | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| on: | ||
| push: | ||
| branches: [ topic/RDK-57502 ] |
No description provided.