-
Notifications
You must be signed in to change notification settings - Fork 2
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdater daemon) #255
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
Open
mkadinti
wants to merge
79
commits into
develop
Choose a base branch
from
topic/RDKEMW-16405
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
79 commits
Select commit
Hold shift + click to select a range
90f9e3d
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download
mkadinti 0d87cea
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download
mkadinti 9957cd2
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download
mkadinti 3b82095
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download
mkadinti d1897aa
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download
mkadinti 930a0be
Update project.md
mkadinti 76aa171
Update project.md
mkadinti c2c5721
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download- Spec gen…
mkadinti da2a482
Merge branch 'topic/RDKEMW-9150' of https://github.com/rdkcentral/rdk…
mkadinti a4a81bd
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download- Adding R…
mkadinti 6f2d9ed
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download- XConf UR…
mkadinti 4efdd2a
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download- XConf UR…
mkadinti 6cf4911
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download- XConf UR…
mkadinti 7bf605a
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download- XConf UR…
mkadinti e1e9719
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download- XConf UR…
mkadinti debdb78
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download- Per-Arti…
mkadinti 8ef4765
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download- Per-Arti…
mkadinti c1aef77
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download- Per-arti…
mkadinti 797e538
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download - upated…
mkadinti 8493e75
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download - L1 fai…
mkadinti 463d3a5
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download - L1 fail…
mkadinti c21de97
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download - L1
mkadinti 4d5da4e
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download-CI build …
mkadinti 5770a0f
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download- addressi…
mkadinti 713f946
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download- addressi…
mkadinti 0578f74
Update rdkv_main.c
mkadinti 8fd3244
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download- addressi…
mkadinti 6112108
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download- after ad…
mkadinti f06cef6
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download-OpenSpec …
mkadinti dbdfe85
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download-OpenSpec …
mkadinti 9ac867b
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download- statered…
mkadinti ae59898
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download- statered…
mkadinti 64ed027
RDKEMW-9150:[SECVULN] - HTTPS support for firmware download- PR clean…
mkadinti de6dc08
Potential fix for pull request finding
mkadinti 077103e
Update rdkv_upgrade.c
mkadinti 725b07e
Potential fix for pull request finding
mkadinti a4df51e
Update rdkv_upgrade.c
mkadinti 953c334
Potential fix for pull request finding
mkadinti 0dc786a
Update rdkv_main.c
mkadinti 1d3cca5
Update rdkv_upgrade.c
mkadinti 49244ff
Potential fix for pull request finding
mkadinti 21a8a84
Merge branch 'topic/RDKEMW-16405' of https://github.com/rdkcentral/rd…
mkadinti 14d90ed
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti 6b6c9e6
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti ed7a8fb
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti 5a564b5
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti 5cd5a01
Merge branch 'develop' into topic/RDKEMW-16405
mkadinti a2b1904
Potential fix for pull request finding
mkadinti cf3860d
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti 06aef62
Merge branch 'develop' into topic/RDKEMW-16405
mkadinti fb05111
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti 004a558
Merge branch 'develop' into topic/RDKEMW-16405
mkadinti 7a16dba
Potential fix for pull request finding
mkadinti c692c25
Potential fix for pull request finding
mkadinti 414ffa5
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti af37202
Potential fix for pull request finding
mkadinti ef19bd3
Revert "Potential fix for pull request finding"
mkadinti 42aaec5
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti 1989b1c
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti 8ca5010
Merge branch 'develop' into topic/RDKEMW-16405
mkadinti 361a0dc
Merge branch 'develop' into topic/RDKEMW-16405
mkadinti ccb802a
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti a0416a2
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti 139f857
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti 440bd06
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti 1ac1e4f
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti 9869b90
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti caa38f3
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti 72918a6
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti 40f516b
Potential fix for pull request finding
mkadinti 4c6484c
Potential fix for pull request finding
mkadinti 3db5855
Potential fix for pull request finding
mkadinti 32b3022
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti b661eb5
Merge branch 'topic/RDKEMW-16405' of https://github.com/rdkcentral/rd…
mkadinti ff9033c
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti eaab3d2
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti db97e9b
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti 0cc9fd4
RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdat…
mkadinti e1fef50
Merge branch 'develop' into topic/RDKEMW-16405
mkadinti File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
2 changes: 2 additions & 0 deletions
2
openspec/changes/direct-cdn-token-and-mtls-fix/.openspec.yaml
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| schema: spec-driven | ||
| created: 2026-06-21 |
203 changes: 203 additions & 0 deletions
203
openspec/changes/direct-cdn-token-and-mtls-fix/design.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,203 @@ | ||
| ## Context | ||
|
|
||
| PR #249 (`topic/RDKEMW-9150`) refactored Direct CDN into the context-struct architecture (`RdkUpgradeContext_t` → `rdkv_upgrade_request()`). The `context->direct_cdn` flag correctly gates Codebig bypass and per-artifact orchestration, but two inner behaviors in `downloadFile()` and `retryDownload()` were not conditioned on this flag. | ||
|
|
||
| ### PR-249 Baseline (What Already Works) | ||
|
|
||
| | Feature | Status | Evidence | | ||
| |---------|--------|----------| | ||
| | RFC gate `SWDLDirect.Enable` | ✅ | `src/rfcInterface/rfcinterface.c` | | ||
| | XConf URL path branching | ✅ | `GetServURL()` in `src/deviceutils/device_api.c` | | ||
| | Per-artifact URL parsing | ✅ | `src/json_process.c` | | ||
| | Codebig bypass flag | ✅ | `src/rdkv_upgrade.c:525,714` | | ||
| | Per-artifact selective retry | ✅ | `src/directcdn.c` loop | | ||
| | 403 → DIRECT_CDN_RETRY_ERR at checkTriggerUpgrade | ✅ | `src/rdkv_main.c:671-676` | | ||
|
|
||
| ### Confirmed Gaps | ||
|
|
||
| | ID | Gap | Impact | Severity | | ||
| |----|-----|--------|----------| | ||
| | GAP-1 | `retryDownload()` retries 403 with stale token (120s wasted) | Slow token refresh, poor UX | HIGH | | ||
| | GAP-2 | `downloadFile()` fetches mTLS cert unconditionally | Unnecessary I/O, potential spurious state-red | HIGH | | ||
|
|
||
| ### Constraints | ||
|
|
||
| - All changes MUST be additive guards — no control-flow restructuring | ||
| - MUST NOT change any function signatures, struct layouts, or Makefile dependencies | ||
| - MUST NOT modify behavior when `direct_cdn == false` (legacy path unchanged) | ||
| - MUST NOT alter existing `direct-cdn-adoption` or `direct-cdn-parity-guards` designs | ||
| - Build flag `LIBRDKCERTSELECTOR` creates two compilation paths; both MUST be guarded | ||
|
|
||
| --- | ||
|
|
||
| ## Goals / Non-Goals | ||
|
|
||
| **Goals:** | ||
| - Restore behavioral parity with RDKV-reference for token expiry and mTLS bypass | ||
| - Zero behavioral change when `direct_cdn == false` | ||
| - Independently testable per gap | ||
| - Each fix independently reviewable and independently deployable | ||
|
|
||
| **Non-Goals:** | ||
| - Refactoring retry architecture or introducing new retry layers | ||
| - Modifying `DirectCDNDownload()` or `checkTriggerUpgrade()` logic | ||
| - Changing cert-selector library behavior or interfaces | ||
| - Adding Direct CDN-specific telemetry markers (not present in reference) | ||
| - Modifying `directcdn.c`, `rdkv_main.c`, or daemon code paths | ||
|
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
|
||
|
|
||
| --- | ||
|
|
||
| ## Decisions | ||
|
|
||
| ### D1: 403 short-circuit placement — inside `retryDownload()` while-loop break conditions | ||
|
|
||
| **Decision:** Add a break condition in the direct-path while loop of `retryDownload()` at ~line 1244, alongside existing 200/206/404/DWNL_BLOCK break conditions: | ||
|
|
||
| ```c | ||
| if (*httpCode == 403 && context->direct_cdn) { | ||
| SWLOG_INFO("%s: HTTP 403 with Direct CDN - token expired, breaking retry\n", __FUNCTION__); | ||
| break; | ||
| } | ||
| ``` | ||
|
mkadinti marked this conversation as resolved.
|
||
|
|
||
| **Rationale:** This is the minimal behavioral equivalent of RDKV-reference `rdkv_main.c:1114-1119` which returns immediately on 403 before ever calling a retry function. In the refactored architecture, the equivalent point is inside `retryDownload()` after the first `downloadFile()` call returns. | ||
|
|
||
| **Alternative considered:** Returning early from `rdkv_upgrade_request()` before calling `retryDownload()`. Rejected — `retryDownload()` is also the function that captures the first download attempt's result and performs the retry loop; restructuring it would violate the "no control-flow changes" constraint. | ||
|
|
||
|
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
|
||
| **Backward compatibility:** When `context->direct_cdn == false`, this condition is never true; existing 403 behavior (retry with delay) is preserved unchanged. | ||
|
|
||
| --- | ||
|
|
||
| ### D2: mTLS bypass placement — guard before `getMtlscert()` in `downloadFile()` | ||
|
|
||
| **Decision:** Add a guard before the `getMtlscert()` call in both `#ifdef LIBRDKCERTSELECTOR` and `#ifndef LIBRDKCERTSELECTOR` paths: | ||
|
|
||
| ```c | ||
| if (context->direct_cdn && state_red != 1) { | ||
| /* Direct CDN: token-authenticated URLs; skip mTLS cert fetch */ | ||
| mtls_enable = -1; | ||
| /* sec remains zero-initialized — NULL cert passed to download */ | ||
| } else { | ||
| /* Existing mTLS cert fetch logic */ | ||
| getMtlscert(&sec, &thisCertSel); | ||
| ... | ||
| } | ||
| ``` | ||
|
|
||
| **Rationale:** Direct CDN URLs contain embedded authentication tokens. Client certificates are neither required nor expected by the CDN. Fetching certs introduces: | ||
| - Unnecessary filesystem I/O (cert file reads) | ||
| - Risk of `MTLS_CERT_FETCH_FAILURE` → `RDKV_UPGRADE_ERROR_STATE_RED` on a path where certs are irrelevant | ||
|
|
||
| Reference behavior: `RDKV-reference/rdkv_main.c:702-714` — when `rfc_directcdn=="true"` AND `server_type==HTTP_SSR_DIRECT` AND `state_red_enable!=1`, NULL cert is passed to `doHttpFileDownload()`. | ||
|
|
||
| **Alternative considered:** Passing cert anyway and relying on CDN to ignore it. Rejected — cert fetch failure triggers state-red entry, which is a critical safety mechanism that MUST NOT fire due to an irrelevant cert lookup. | ||
|
|
||
| **State-red exception:** When `isInStateRed() == 1`, the device is in boot-time recovery. Recovery may use a different CDN path that requires certs. The guard preserves the existing recovery cert flow. | ||
|
|
||
| **Backward compatibility:** When `context->direct_cdn == false`, the guard is never active; existing mTLS behavior is preserved unchanged. | ||
|
|
||
| --- | ||
|
|
||
| ### D3: State-red interaction — recovery cert path preserved | ||
|
|
||
| **Decision:** The mTLS bypass guard SHALL be `context->direct_cdn && state_red != 1`. When state-red is active, existing cert-fetch logic executes regardless of `direct_cdn` flag. | ||
|
|
||
| **Rationale:** State-red recovery is a boot-time emergency path. The device may not have valid CDN tokens (tokens may have expired during the failed boot cycle). The recovery cert provides an alternative authentication mechanism that MUST remain available. | ||
|
|
||
| --- | ||
|
|
||
| ## Architecture & Control-Flow | ||
|
|
||
| ### GAP-1: Token Expiry Short-Circuit | ||
|
|
||
| ```mermaid | ||
| sequenceDiagram | ||
| participant DCL as DirectCDNDownload() | ||
| participant CTU as checkTriggerUpgrade() | ||
| participant RUR as rdkv_upgrade_request() | ||
| participant RD as retryDownload() | ||
| participant DF as downloadFile() | ||
| participant CDN as CDN Server | ||
|
|
||
| DCL->>CTU: per-artifact download | ||
| CTU->>RUR: rdkv_upgrade_request(context) | ||
| RUR->>RD: retryDownload(context, ...) | ||
| RD->>DF: downloadFile(context, ...) | ||
| DF->>CDN: GET /firmware.bin?token=expired | ||
| CDN-->>DF: HTTP 403 Forbidden | ||
| DF-->>RD: curl_ret=0, httpCode=403 | ||
| Note over RD: NEW: if httpCode==403 && direct_cdn → break | ||
| RD-->>RUR: return (httpCode=403) | ||
| RUR-->>CTU: curl=0, http=403 | ||
| Note over CTU: Existing: 403 → DIRECT_CDN_RETRY_ERR | ||
| CTU-->>DCL: DIRECT_CDN_RETRY_ERR | ||
| Note over DCL: Outer loop re-queries XConf for fresh URLs | ||
| ``` | ||
|
|
||
| ### GAP-2: mTLS Bypass | ||
|
|
||
| ```mermaid | ||
| flowchart TD | ||
| A[downloadFile called] --> B{context->direct_cdn?} | ||
| B -->|false| C[Existing mTLS path] | ||
| C --> D[getMtlscert] | ||
| D --> E[Download with cert] | ||
|
|
||
| B -->|true| F{isInStateRed?} | ||
| F -->|== 1| G[State-Red Recovery] | ||
| G --> H[getMtlscert with recovery group] | ||
| H --> I[Download with recovery cert] | ||
|
|
||
| F -->|!= 1| J[Direct CDN Normal] | ||
| J --> K[Skip getMtlscert] | ||
| K --> L[mtls_enable = -1] | ||
| L --> M[Download with NULL cert] | ||
|
|
||
| style J fill:#90EE90 | ||
| style K fill:#90EE90 | ||
| style L fill:#90EE90 | ||
| style M fill:#90EE90 | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Subtask-to-Design Mapping | ||
|
|
||
| | Subtask | Design Decision | Code Area | Spec Section | | ||
| |---------|----------------|-----------|--------------| | ||
| | 1: Update OpenSpec specs | — | — | `direct-cdn-download` §Token Expiry, §mTLS Bypass; `retry-recovery` §Inner Loop Short-Circuit | | ||
| | 2: 403 early-return | D1 | `src/rdkv_upgrade.c` `retryDownload()` ~L1244 | `retry-recovery` §Inner Loop Short-Circuit | | ||
| | 3: mTLS cert-skip | D2, D3 | `src/rdkv_upgrade.c` `downloadFile()` ~L1038 | `direct-cdn-download` §mTLS Bypass | | ||
| | 4: UT for 403 | D1 validation | `unittest/` | — | | ||
| | 5: UT for mTLS | D2, D3 validation | `unittest/` | — | | ||
|
|
||
| --- | ||
|
|
||
| ## Behavioral Requirements Traceability Matrix | ||
|
|
||
| | Requirement | Spec Section | Code Function | Lines (approx) | Test | | ||
| |-------------|-------------|---------------|-----------------|------| | ||
| | 403 → immediate break when direct_cdn | `retry-recovery` §Inner Loop | `retryDownload()` | ~1244-1260 | Subtask 4: mock 403 + direct_cdn=true → no sleep | | ||
| | 403 → normal retry when !direct_cdn | `retry-recovery` §unchanged | `retryDownload()` | ~1244-1260 | Subtask 4: mock 403 + direct_cdn=false → retries | | ||
| | Skip getMtlscert when direct_cdn && !state_red | `direct-cdn-download` §mTLS Bypass | `downloadFile()` | ~1038-1070 | Subtask 5: verify no cert fetch | | ||
| | Use recovery cert when direct_cdn && state_red | `direct-cdn-download` §mTLS Bypass | `downloadFile()` | ~1071-1095 | Subtask 5: verify RCVRY cert group | | ||
| | Legacy path unchanged when !direct_cdn | Both specs §backward compat | `downloadFile()`, `retryDownload()` | all | Subtask 4+5: verify no regression | | ||
|
|
||
| --- | ||
|
|
||
| ## Risks / Trade-offs | ||
|
|
||
| | Risk | Likelihood | Impact | Mitigation | | ||
| |------|-----------|--------|-----------| | ||
| | CDN rejects requests without mTLS headers | Low | Download failure | RFC kill-switch: `SWDLDirect.Enable=false` reverts to legacy path | | ||
| | Break condition accidentally triggers for non-CDN 403 | Very Low | Missed retry opportunity | Guard is AND-ed with `context->direct_cdn`; only true in DirectCDN path | | ||
| | Cert-selector `static` variable state issue | Low | Stale cert handle | Guard placed BEFORE cert-selector init; handle never created in CDN path | | ||
| | Regression in legacy (non-DirectCDN) download | Very Low | Production download failure | All changes gated by `context->direct_cdn == true`; legacy path untouched | | ||
|
|
||
| **Rollback**: Set RFC `Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.SWDLDirect.Enable` to `"false"`. This disables the entire Direct CDN code path including the new guards. | ||
|
|
||
| --- | ||
|
|
||
| ## Open Questions | ||
|
|
||
| (none — all design decisions resolved during gap analysis) | ||
33 changes: 33 additions & 0 deletions
33
openspec/changes/direct-cdn-token-and-mtls-fix/proposal.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| ## Why | ||
|
|
||
| PR #249 (RDKEMW-9150) delivered the structural foundation for Direct CDN — RFC gate, per-artifact URL parsing, Codebig bypass, and context-struct architecture — but two runtime behaviors present in the RDKV-reference implementation were not carried over into the refactored download engine: | ||
|
|
||
| 1. **Token expiry short-circuit**: When a per-artifact CDN download returns HTTP 403 (token expired), the RDKV-reference returns immediately to the outer XConf re-query loop. The refactored code enters `retryDownload()` unconditionally, wasting 120 seconds (2 retries × 60s delay) retrying the same expired-token URL before the outer loop can refresh. | ||
|
mkadinti marked this conversation as resolved.
|
||
|
|
||
| 2. **mTLS bypass**: The RDKV-reference skips mTLS certificate fetch entirely when downloading from token-authenticated CDN URLs (except during state-red recovery). The refactored `downloadFile()` always fetches mTLS certs regardless of `direct_cdn` flag, causing unnecessary I/O and potential spurious state-red entry if cert fetch fails. | ||
|
|
||
| These gaps are confirmed by line-for-line comparison between `RDKV-reference/rdkv_main.c` (lines 702-714, 1114-1119) and `src/rdkv_upgrade.c` (lines 1039-1067, 1244-1260). | ||
|
|
||
| ## What Changes | ||
|
|
||
| - `retryDownload()` in `src/rdkv_upgrade.c`: Add HTTP 403 as a break condition when `context->direct_cdn == true`. No change to behavior when `direct_cdn == false`. | ||
| - `downloadFile()` in `src/rdkv_upgrade.c`: Add guard to skip `getMtlscert()` when `context->direct_cdn == true` and `isInStateRed() != 1`. Pass `NULL` cert to `doHttpFileDownload()` / `chunkDownload()`. When state-red IS active, recovery cert path preserved. | ||
|
|
||
|
mkadinti marked this conversation as resolved.
|
||
| ## Capabilities | ||
|
|
||
| ### New Capabilities | ||
|
|
||
| (none — no new capabilities introduced) | ||
|
|
||
| ### Modified Capabilities | ||
|
|
||
| - `direct-cdn-download`: Add token expiry handling and mTLS bypass behavioral requirements | ||
| - `retry-recovery`: Refine inner retry loop behavior to short-circuit on HTTP 403 in Direct CDN mode | ||
|
|
||
| ## Impact | ||
|
|
||
| - **Files modified**: `src/rdkv_upgrade.c` (two functions: `retryDownload()`, `downloadFile()`) | ||
| - **API changes**: None — no signature, struct, or Makefile changes | ||
| - **Test impact**: New unit tests for both paths; existing tests unaffected | ||
| - **Risk**: Low — both changes are additive guards with RFC kill-switch (`SWDLDirect.Enable=false` disables entire Direct CDN path) | ||
|
mkadinti marked this conversation as resolved.
|
||
| - **Scope boundary**: Does NOT touch `directcdn.c`, `rdkv_main.c`, `json_process.c`, daemon handlers, or any other code paths | ||
|
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
Comment on lines
+29
to
+33
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
|
||
57 changes: 57 additions & 0 deletions
57
openspec/changes/direct-cdn-token-and-mtls-fix/specs/direct-cdn-download/spec.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| ## ADDED Requirements | ||
|
|
||
| ### Requirement: Token Expiry Inner-Retry Short-Circuit | ||
| When operating in Direct CDN mode, the download engine's inner retry loop (`retryDownload()`) SHALL NOT retry a request that received HTTP 403. The 403 response indicates token expiry; retrying with the same stale-token URL is futile. Control SHALL return immediately to the caller so the outer `DirectCDNDownload()` loop can re-query XConf for fresh token-bearing URLs. | ||
|
|
||
| #### Scenario: HTTP 403 breaks inner retry in Direct CDN mode | ||
| - **WHEN** `retryDownload()` is executing in direct-path mode (HTTP_SSR_DIRECT) | ||
| - **AND** `context->direct_cdn == true` | ||
| - **AND** `downloadFile()` returns with `*httpCode == 403` | ||
| - **THEN** `retryDownload()` SHALL break immediately without further iterations or delay | ||
|
mkadinti marked this conversation as resolved.
|
||
|
|
||
|
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
|
||
| #### Scenario: HTTP 403 retries normally in legacy mode | ||
| - **WHEN** `retryDownload()` is executing in direct-path mode (HTTP_SSR_DIRECT) | ||
| - **AND** `context->direct_cdn == false` | ||
| - **AND** `downloadFile()` returns with `*httpCode == 403` | ||
| - **THEN** `retryDownload()` SHALL continue its normal retry loop (existing behavior unchanged) | ||
|
|
||
| #### Scenario: Other break conditions preserved | ||
| - **WHEN** `retryDownload()` is executing with `context->direct_cdn == true` | ||
| - **AND** `downloadFile()` returns HTTP 200, 206, 404, or DWNL_BLOCK | ||
| - **THEN** existing break conditions SHALL remain unchanged | ||
|
|
||
| #### Scenario: Backward compatibility for non-Direct-CDN paths | ||
| - **WHEN** `context->direct_cdn == false` | ||
| - **THEN** all retry behavior in `retryDownload()` SHALL remain identical to pre-change behavior regardless of HTTP response code | ||
|
|
||
| --- | ||
|
|
||
| ### Requirement: mTLS Bypass for Direct CDN Downloads | ||
| When operating in Direct CDN mode and NOT in state-red recovery, `downloadFile()` SHALL skip mTLS certificate acquisition and pass NULL as the certificate parameter to the HTTP download function. Direct CDN URLs contain embedded authentication tokens; client certificates are neither required nor expected by the CDN. | ||
|
|
||
| #### Scenario: Certificate fetch skipped for Direct CDN (normal state) | ||
| - **WHEN** `downloadFile()` is called with `context->direct_cdn == true` | ||
| - **AND** `isInStateRed()` returns 0 (not in state-red) | ||
| - **THEN** `getMtlscert()` SHALL NOT be called | ||
| - **AND** `doHttpFileDownload()` / `chunkDownload()` SHALL receive NULL as the certificate parameter | ||
|
|
||
| #### Scenario: Recovery certificate used during state-red (even with Direct CDN) | ||
| - **WHEN** `downloadFile()` is called with `context->direct_cdn == true` | ||
| - **AND** `isInStateRed()` returns 1 (state-red active) | ||
| - **THEN** `getMtlscert()` SHALL be called with the recovery cert group | ||
| - **AND** the download function SHALL receive the populated certificate structure | ||
|
|
||
| #### Scenario: Legacy mTLS path unchanged | ||
| - **WHEN** `downloadFile()` is called with `context->direct_cdn == false` | ||
| - **THEN** existing mTLS certificate fetch and usage behavior SHALL remain unchanged regardless of state-red status | ||
|
|
||
| #### Scenario: Cert fetch failure cannot trigger spurious state-red in Direct CDN mode | ||
| - **WHEN** `context->direct_cdn == true` | ||
| - **AND** `isInStateRed() != 1` | ||
| - **THEN** `MTLS_CERT_FETCH_FAILURE` → `RDKV_UPGRADE_ERROR_STATE_RED` path SHALL NOT be reachable (since `getMtlscert()` is never called) | ||
|
|
||
| #### Scenario: Both ifdef/ifndef LIBRDKCERTSELECTOR paths guarded | ||
| - **WHEN** `downloadFile()` is compiled with `LIBRDKCERTSELECTOR` defined | ||
| - **THEN** the mTLS bypass guard SHALL apply to the `getMtlscert()` call in the `#ifdef` path | ||
| - **WHEN** `downloadFile()` is compiled without `LIBRDKCERTSELECTOR` defined | ||
| - **THEN** the mTLS bypass guard SHALL apply to the `getMtlscert()` call in the `#ifndef` path | ||
31 changes: 31 additions & 0 deletions
31
openspec/changes/direct-cdn-token-and-mtls-fix/specs/retry-recovery/spec.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| ## ADDED Requirements | ||
|
|
||
| ### Requirement: Inner Retry Loop Short-Circuit for Direct CDN HTTP 403 | ||
| The inner retry loop (`retryDownload()`) SHALL short-circuit on HTTP 403 when the download context indicates Direct CDN mode. This prevents the retry loop from wasting time re-attempting a download with an expired token. The outer orchestration loop in `DirectCDNDownload()` is responsible for obtaining fresh URLs via XConf re-query. | ||
|
|
||
| #### Scenario: Direct CDN 403 causes immediate break | ||
| - **WHEN** `retryDownload()` receives control after `downloadFile()` returns HTTP 403 | ||
| - **AND** `context->direct_cdn == true` | ||
| - **THEN** the retry loop SHALL break immediately | ||
| - **AND** no `sleep(delay)` SHALL be executed for this iteration | ||
|
mkadinti marked this conversation as resolved.
|
||
| - **AND** the 403 HTTP code SHALL be preserved in `*httpCode` for the caller | ||
|
mkadinti marked this conversation as resolved.
Comment on lines
+7
to
+11
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
|
||
|
|
||
| #### Scenario: Non-Direct-CDN 403 retries normally | ||
| - **WHEN** `retryDownload()` receives control after `downloadFile()` returns HTTP 403 | ||
| - **AND** `context->direct_cdn == false` | ||
| - **THEN** existing retry behavior SHALL apply (retry with delay, up to retry_cnt iterations) | ||
|
|
||
| #### Scenario: Short-circuit does not affect connectivity-based fallback | ||
| - **WHEN** `retryDownload()` returns after 403 short-circuit | ||
| - **AND** the caller (`rdkv_upgrade_request()`) evaluates fallback conditions | ||
| - **THEN** the Codebig fallback SHALL NOT trigger (since `*httpCode == 403`, not 0, and `curl_ret_code != CURL_CONNECTIVITY_ISSUE`) | ||
| - **AND** the Direct CDN Codebig-skip guard at `rdkv_upgrade_request()` line 525 provides defense-in-depth | ||
|
|
||
| #### Scenario: Time savings verification | ||
| - **WHEN** Direct CDN download receives HTTP 403 on first attempt | ||
| - **THEN** `retryDownload()` SHALL return to caller in less than 1 second (no 60-second sleep delays) | ||
| - **AND** the outer `DirectCDNDownload()` loop can re-query XConf immediately | ||
|
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
mkadinti marked this conversation as resolved.
|
||
|
|
||
| #### Scenario: Backward compatibility for all non-Direct-CDN modes | ||
| - **WHEN** `context->direct_cdn == false` | ||
| - **THEN** all existing break conditions (HTTP 200, 206, 404, DWNL_BLOCK) and retry timing in `retryDownload()` SHALL remain unchanged | ||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.