RDKEMW-23527 : Inherit extract-sha-cve bbclass for CVE support - Addition of CVE middleware paths - #164
Open
jomothomas wants to merge 12 commits into
Open
RDKEMW-23527 : Inherit extract-sha-cve bbclass for CVE support - Addition of CVE middleware paths#164jomothomas wants to merge 12 commits into
jomothomas wants to merge 12 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the machine include configuration to add CVE feed artifact URLs alongside existing middleware IPK feed URIs, aiming to expose CVE tarballs for middleware artifacts.
Changes:
- Appends middleware CVE tarball entries to
CVE_LAYER_FEED_PATHfor the base middleware feed and the OSS middleware feed. - Adds an include of
conf/dynamic_sha.inc(presumably to provide per-artifact SHA256 values).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| conf/machine/include/oss_mw.inc | Adds an OSS middleware CVE feed entry and includes conf/dynamic_sha.inc. |
| conf/machine/include/middleware.inc | Adds a middleware CVE feed entry, includes OSS middleware include conditionally, and includes conf/dynamic_sha.inc. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (4)
conf/machine/include/middleware.inc:19
- This file includes
conf/dynamic_sha.inc, but that file is not present anywhere in the repository, so SHA population (and/or parsing) won’t work as intended. Either addconf/dynamic_sha.incin this PR (defining the referenced*_SHA256SUMvars), or update the include to point to the correct existing file.
include conf/dynamic_sha.inc
conf/machine/include/oss_mw.inc:11
- This file includes
conf/dynamic_sha.inc, but that file is not present anywhere in the repository. If this include is meant to defineMW_OSS_SHA256SUM, it needs to be added in this PR or the include path corrected.
include conf/dynamic_sha.inc
conf/machine/include/oss_mw.inc:8
CVE_LAYER_COMMON_PATH_MIDDLEWAREis already set inmiddleware.inc, so using?=here means the OSS CVE feed can end up using the non-OSS path. Also, the CVE tarball URL here doesn’t mirror the OSS IPK layout (missing/${MW_OSS}/), and the filename appends-middlewaretwice becauseMW_OSSalready includes that suffix.
CVE_LAYER_COMMON_PATH_MIDDLEWARE ?= "${MW_OSS_IPK_PATH}"
MW_OSS_IPK_SERVER_PATH = "${RDK_ARTIFACTS_BASE_URL}/${MW_OSS_IPK_PATH}/${RDK_MW_ARCH}/${MW_OSS}/ipks/${BUILD_VARIANT}"
CVE_LAYER_FEED_PATH += " ${MW_OSS}##${RDK_ARTIFACTS_BASE_URL}/${CVE_LAYER_COMMON_PATH_MIDDLEWARE}/${RDK_MW_ARCH}/tarball/cve/debug/${MW_OSS}-middleware.tgz;sha256sum=${MW_OSS_SHA256SUM}"
conf/machine/include/middleware.inc:13
MIDDLEWARE_ARCH_SHA256SUMis not defined anywhere in this layer (it only appears in this new reference), so the generated feed entry will likely contain an emptysha256sum=value. If the SHA is optional, it should be conditionally appended; if it’s required, it needs to be defined (e.g., via an included .inc that is actually present).
CVE_LAYER_FEED_PATH += " ${MIDDLEWARE_ARCH}##${RDK_ARTIFACTS_BASE_URL}/${CVE_LAYER_COMMON_PATH_MIDDLEWARE}/${RDK_MW_ARCH}/tarball/cve/debug/${MIDDLEWARE_ARCH}.tgz;sha256sum=${MIDDLEWARE_ARCH_SHA256SUM}"
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
conf/machine/include/middleware.inc:13
- This layer currently has no
conf/dynamic_sha.inc(and there are no other definitions of*_SHA256SUMvariables in this repo), but the new CVE feed URL appends;sha256sum=${MIDDLEWARE_ARCH_SHA256SUM}. Unless another layer in the build providesconf/dynamic_sha.incand defines these variables, the checksum parameter will expand empty, which can break CVE feed consumers. Ensure the checksum include file is actually provided (or define the SHA256SUM variables in this layer) and consider usingrequireif the file is mandatory.
CVE_LAYER_FEED_PATH += " ${MIDDLEWARE_ARCH}##${RDK_ARTIFACTS_BASE_URL}/${CVE_LAYER_COMMON_PATH_MIDDLEWARE}/${RDK_MW_ARCH}/tarball/cve/debug/${MIDDLEWARE_ARCH}.tgz;sha256sum=${MIDDLEWARE_ARCH_SHA256SUM}"
conf/machine/include/oss_mw.inc:8
CVE_LAYER_COMMON_PATH_MIDDLEWAREis also set inmiddleware.inc(line 11) with?=; whenoss_mw.incis included frommiddleware.inc, this?=will not override it. As a result, overridingMW_OSS_IPK_PATHwill not affect the OSS CVE feed URL here (it will keep using the non-OSS path). Use a distinct variable for the OSS CVE common path (or an override) and reference that inCVE_LAYER_FEED_PATH.
CVE_LAYER_COMMON_PATH_MIDDLEWARE ?= "${MW_OSS_IPK_PATH}"
MW_OSS_IPK_SERVER_PATH = "${RDK_ARTIFACTS_BASE_URL}/${MW_OSS_IPK_PATH}/${RDK_MW_ARCH}/${MW_OSS}/ipks/${BUILD_VARIANT}"
CVE_LAYER_FEED_PATH += " ${MW_OSS}##${RDK_ARTIFACTS_BASE_URL}/${CVE_LAYER_COMMON_PATH_MIDDLEWARE}/${RDK_MW_ARCH}/tarball/cve/debug/${MW_OSS}.tgz;sha256sum=${MW_OSS_SHA256SUM}"
conf/machine/include/oss_mw.inc:11
oss_mw.incis only included frommiddleware.incin this layer, andmiddleware.incalready includesconf/dynamic_sha.inc(line 19). Including it again here causes the file to be parsed twice and risks duplicate/overriding variable definitions; keep the include in one place.
include conf/dynamic_sha.inc
…tion of CVE middleware pathsv2
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The shared CVE_LAYER_COMMON_PATH_MIDDLEWARE defaulting is order-dependent between includes and can yield incorrect CVE feed URLs when middleware and OSS paths diverge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
conf/machine/include/oss_mw.inc:26
CVE_LAYER_COMMON_PATH_MIDDLEWAREis being defaulted here based onMW_OSS_IPK_PATH, but it is also defaulted inmiddleware.incbased onMW_IPK_PATH. Becauseoss_mw.incis included frommiddleware.incearlier in parse order, whichever file runs first will “win”, which can produce incorrect CVE feed URLs ifMW_IPK_PATHandMW_OSS_IPK_PATHare ever set differently. Consider setting this default in only one place, or using separate variables (e.g.,CVE_LAYER_COMMON_PATH_MIDDLEWAREandCVE_LAYER_COMMON_PATH_MWOSS).
if d.getVar("CVE_CHECK_ENABLED") == "1":
if not d.getVar("CVE_LAYER_COMMON_PATH_MIDDLEWARE"):
d.setVar("CVE_LAYER_COMMON_PATH_MIDDLEWARE", "${MW_OSS_IPK_PATH}")
d.setVar("CVE_FEED_URL_MWOSS", "${MW_OSS}##${RDK_ARTIFACTS_BASE_URL}/${CVE_LAYER_COMMON_PATH_MIDDLEWARE}/${RDK_MW_ARCH}/tarball/cve/${BUILD_VARIANT}/${MW_OSS}.tgz")
d.appendVar("CVE_LAYER_FEED_PATH", " ${CVE_FEED_URL_MWOSS};sha256sum=${MW_OSS_SHA256SUM}")
conf/machine/include/middleware.inc:34
CVE_LAYER_COMMON_PATH_MIDDLEWAREis defaulted here fromMW_IPK_PATH, but it is also defaulted inoss_mw.incfromMW_OSS_IPK_PATH. Sinceoss_mw.incis included earlier in this file, its defaulting can take precedence, leading to an unexpected CVE feed path for the non-OSS middleware feed when these paths diverge. Consider defaulting the shared path in a single place, or use distinct override variables per feed.
if d.getVar("CVE_CHECK_ENABLED") == "1":
if not d.getVar("CVE_LAYER_COMMON_PATH_MIDDLEWARE"):
d.setVar("CVE_LAYER_COMMON_PATH_MIDDLEWARE", "${MW_IPK_PATH}")
d.setVar("CVE_FEED_URL_MIDDLEWARE", "${MIDDLEWARE_ARCH}##${RDK_ARTIFACTS_BASE_URL}/${CVE_LAYER_COMMON_PATH_MIDDLEWARE}/${RDK_MW_ARCH}/tarball/cve/${BUILD_VARIANT}/${MIDDLEWARE_ARCH}.tgz")
d.appendVar("CVE_LAYER_FEED_PATH", " ${CVE_FEED_URL_MIDDLEWARE};sha256sum=${MIDDLEWARE_ARCH_SHA256SUM}")
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+18
to
+22
| #The following section will be only enabled if CVE_CHECK is set | ||
|
|
||
| # Check whether CVE-check is globally inherited | ||
| CVE_CHECK_ENABLED = "${@bb.utils.contains('INHERIT', 'cve-check', '1', '0', d)}" | ||
|
|
Comment on lines
+10
to
+13
| #The following section will be only enabled if CVE_CHECK is set | ||
|
|
||
| # Check whether CVE-check is globally inherited | ||
| CVE_CHECK_ENABLED = "${@bb.utils.contains('INHERIT', 'cve-check', '1', '0', d)}" |
…tion of CVE middleware pathsv2
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Moderate issues remain around duplicate inclusion and the undeclared dynamic SHA provider dependency.
Review details
Suppressed comments (3)
conf/machine/include/middleware.inc:25
- When
STACK_LAYER_OSS_SUPPORTis enabled,middleware.incincludesoss_mw.incat line 16, soconf/dynamic_sha.incis included once fromoss_mw.incand then included again here. BitBake does not deduplicateincludedirectives; any anonymous Python, task, or variable side effects in that externally supplied file can therefore be applied twice and duplicate the SHA/feed setup. Keep this include in one common location (or explicitly guard the second include) so OSS configurations parse it only once.
CVE_CONF_INCLUDE = "${@bb.utils.contains('CVE_CHECK_ENABLED', '1', 'conf/dynamic_sha.inc', '', d)}"
include ${CVE_CONF_INCLUDE}
conf/machine/include/middleware.inc:25
- When CVE checking is enabled, this include relies on
conf/dynamic_sha.incbeing supplied by the external auxiliary layer, but this layer'sconf/layer.confstill declares onlycoreinLAYERDEPENDS_middleware-release. A consumer can therefore enable this layer andcve-checkwithout the provider, leaving the checksum variables unavailable and the CVE feed configuration broken. Add the provider to the layer dependency (or otherwise make that dependency explicit) with this change.
CVE_CONF_INCLUDE = "${@bb.utils.contains('CVE_CHECK_ENABLED', '1', 'conf/dynamic_sha.inc', '', d)}"
include ${CVE_CONF_INCLUDE}
conf/machine/include/oss_mw.inc:17
- This CVE-enabled path also depends on
conf/dynamic_sha.incfrom the external auxiliary layer, whileconf/layer.confdeclares onlycoreinLAYERDEPENDS_middleware-release. Without the provider layer, the checksum variable used below is unavailable and the OSS CVE feed is invalid. Add the provider to the layer dependency or make that dependency explicit.
CVE_CONF_INCLUDE = "${@bb.utils.contains('CVE_CHECK_ENABLED', '1', 'conf/dynamic_sha.inc', '', d)}"
include ${CVE_CONF_INCLUDE}
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
…tion of CVE middleware pathsv3
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved dependency, variable-expansion, and architecture-path issues block approval.
Review details
Suppressed comments (5)
conf/machine/include/middleware.inc:25
- This new parse-time include depends on
conf/dynamic_sha.incbeing supplied by another layer, butconf/layer.conf:8still declares onlycoreas a dependency. A build that enables CVE checking without the auxiliary layer will fail while parsing this file; declare the provider layer inLAYERDEPENDSor keep the include in this layer.
CVE_CONF_INCLUDE = "${@bb.utils.contains('CVE_CHECK_ENABLED', '1', 'conf/dynamic_sha.inc', '', d)}"
include ${CVE_CONF_INCLUDE}
conf/machine/include/middleware.inc:29
CVE_CHECK_ENABLEDis assigned with a lazy${@...}expansion, butgetVaris called without the expansion flag here. In BitBake this compares the raw expression with"1", so even whencve-checkis inherited this block is skipped and the middleware CVE feed is never appended. Read the variable with expansion (or testINHERITdirectly).
if d.getVar("CVE_CHECK_ENABLED") == "1":
conf/machine/include/oss_mw.inc:17
- This OSS-specific include also requires the externally provided
conf/dynamic_sha.inc, whileconf/layer.conf:8declares onlycore. Without the auxiliary layer onBBPATH, enabling CVE checking makes OSS configurations fail at parse time; declare the provider layer as a dependency or vendor the include here.
CVE_CONF_INCLUDE = "${@bb.utils.contains('CVE_CHECK_ENABLED', '1', 'conf/dynamic_sha.inc', '', d)}"
include ${CVE_CONF_INCLUDE}
conf/machine/include/oss_mw.inc:21
CVE_CHECK_ENABLEDis assigned with a lazy${@...}expansion, butgetVaris called without the expansion flag here. In BitBake this compares the raw expression with"1", so even whencve-checkis inherited this block is skipped and the OSS middleware CVE feed is never appended. Read the variable with expansion (or testINHERITdirectly).
if d.getVar("CVE_CHECK_ENABLED") == "1":
conf/machine/include/oss_mw.inc:25
MW_OSSis derived fromget_oss_arch(d)and is used as the feed architecture, but this URL hard-codes therdk-arm7vetarball name. For any other supported OSS architecture, the feed key and artifact path will refer to different artifacts (or a nonexistent file), so build the filename from the same architecture variable instead.
d.setVar("CVE_FEED_URL_MWOSS", "${MW_OSS}##${RDK_ARTIFACTS_BASE_URL}/${CVE_LAYER_COMMON_PATH_MIDDLEWARE}/xione-uk/tarball/cve/${BUILD_VARIANT}/rdk-arm7ve-oss-middleware.tgz")
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
…tion of CVE middleware pathsv4
…tion of CVE middleware pathsv5
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds the CVE feed paths to Middleware which get SHA256 checksums
Changes:
Addition of the CVE feedback paths for middleware artifacts corresponding to IPK feeds
Added CVE_LAYER_COMMON_PATH_MIDDLEWARE for usage in local conf