RDKB-63135, RDKB-63664 : Handle Security Edge feature for business#241
RDKB-63135, RDKB-63664 : Handle Security Edge feature for business#241SanthoshGujulvajagadeesh wants to merge 2 commits intodevelopfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds support for two new XB10 device models (CGM601TCOM and SG417DBCT) to enable the Security Edge XDNS/DNSSec feature. According to the PR description, these features should only work in business device mode, but the implementation only adds box type checks without any device mode validation.
Changes:
- Added CGM601TCOM and SG417DBCT to the list of supported models for XDNS DNSSec functionality in both C and shell script implementations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| source/service_dhcp/service_dhcp_server.c | Adds CGM601TCOM and SG417DBCT to box type conditional for enabling DNSSec proxy feature in dnsmasq server start |
| source/scripts/init/service.d/service_dhcp_server.sh | Adds CGM601TCOM and SG417DBCT to model number conditional for enabling DNSSec proxy feature in shell script dnsmasq startup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fprintf(g_fArmConsoleLog, "\n%s Inside non XB3 block Box_Type=%s.......\n",__FUNCTION__, Box_Type); | ||
| syscfg_get(NULL, "XDNS_DNSSecEnable", DNSSEC_FLAG, sizeof(DNSSEC_FLAG)); | ||
| if ((!strncmp(Box_Type, "CGA4332COM", 10) || !strncmp(Box_Type, "CGA4131COM", 10)) && !strncasecmp(l_cXdnsEnable, "1", 1) && !strncasecmp(DNSSEC_FLAG, "1", 1)) | ||
| if ((!strncmp(Box_Type, "CGA4332COM", 10) || !strncmp(Box_Type, "CGA4131COM", 10) || !strncmp(Box_Type, "CGM601TCOM", 10) || !strncmp(Box_Type, "SG417DBCT", 10)) && !strncasecmp(l_cXdnsEnable, "1", 1) && !strncasecmp(DNSSEC_FLAG, "1", 1)) |
There was a problem hiding this comment.
The strncmp length parameter for "SG417DBCT" should be 9, not 10. The model name "SG417DBCT" has only 9 characters, while the other model names in this condition (CGA4332COM, CGA4131COM, CGM601TCOM) all have 10 characters. Using 10 for a 9-character string will read one byte past the string literal, which could lead to incorrect comparisons.
| fprintf(g_fArmConsoleLog, "\n%s Inside non XB3 block Box_Type=%s.......\n",__FUNCTION__, Box_Type); | ||
| syscfg_get(NULL, "XDNS_DNSSecEnable", DNSSEC_FLAG, sizeof(DNSSEC_FLAG)); | ||
| if ((!strncmp(Box_Type, "CGA4332COM", 10) || !strncmp(Box_Type, "CGA4131COM", 10)) && !strncasecmp(l_cXdnsEnable, "1", 1) && !strncasecmp(DNSSEC_FLAG, "1", 1)) | ||
| if ((!strncmp(Box_Type, "CGA4332COM", 10) || !strncmp(Box_Type, "CGA4131COM", 10) || !strncmp(Box_Type, "CGM601TCOM", 10) || !strncmp(Box_Type, "SG417DBCT", 10)) && !strncasecmp(l_cXdnsEnable, "1", 1) && !strncasecmp(DNSSEC_FLAG, "1", 1)) |
There was a problem hiding this comment.
The PR description states that the DML parameters "SHOULD work only if the devicemode is business," but the code changes only add box type checks without any device mode validation. The changes allow the XDNS/DNSSec features for the new box types (CGM601TCOM and SG417DBCT) regardless of device mode. If business mode restriction is indeed required as stated in the PR description, a device mode check should be added to this condition.
| SYSCFG_DNSSEC_FLAG=`syscfg get XDNS_DNSSecEnable` | ||
| SYSCFG_XDNSREFAC_FLAG=`syscfg get XDNS_RefacCodeEnable` | ||
| if ([ "$MODEL_NUM" = "CGA4131COM" ] || [ "$MODEL_NUM" = "CGA4332COM" ]) && [ -n "$SYSCFG_XDNS_FLAG" ] && [ "$SYSCFG_XDNS_FLAG" = "1" ] && [ "$SYSCFG_DNSSEC_FLAG" = "1" ] ; then | ||
| if ([ "$MODEL_NUM" = "CGA4131COM" ] || [ "$MODEL_NUM" = "CGA4332COM" ] || [ "$MODEL_NUM" = "CGM601TCOM" ] || [ "$MODEL_NUM" = "SG417DBCT" ]) && [ -n "$SYSCFG_XDNS_FLAG" ] && [ "$SYSCFG_XDNS_FLAG" = "1" ] && [ "$SYSCFG_DNSSEC_FLAG" = "1" ] ; then |
There was a problem hiding this comment.
The PR description states that the DML parameters "SHOULD work only if the devicemode is business," but the code changes only add model number checks without any device mode validation. The changes allow the XDNS/DNSSec features for the new models (CGM601TCOM and SG417DBCT) regardless of device mode. If business mode restriction is indeed required as stated in the PR description, a device mode check should be added to this condition.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| syscfg_get(NULL, "XDNS_DNSSecEnable", DNSSEC_FLAG, sizeof(DNSSEC_FLAG)); | ||
| if ((!strncmp(Box_Type, "CGA4332COM", 10) || !strncmp(Box_Type, "CGA4131COM", 10)) && !strncasecmp(l_cXdnsEnable, "1", 1) && !strncasecmp(DNSSEC_FLAG, "1", 1)) | ||
| if ((!strncmp(Box_Type, "CGA4332COM", 10) || !strncmp(Box_Type, "CGA4131COM", 10) || !strncmp(Box_Type, "CGM601TCOM", 10) || !strncmp(Box_Type, "SG417DBCT", 9)) && !strncasecmp(l_cXdnsEnable, "1", 1) && !strncasecmp(DNSSEC_FLAG, "1", 1)) | ||
| { |
There was a problem hiding this comment.
PR description says the XDNS/DNSSEC DMLs should only take effect when the device mode is business, but this gating logic only checks MODEL_NUM + syscfg flags. If the same model can operate in residential mode, this will still enable --proxy-dnssec when the flags are set. Please add an explicit business-mode check here (using the repo’s source of truth for business vs residential), or update the PR description if model-number gating is the intended criterion.
| SYSCFG_XDNS_FLAG=`syscfg get X_RDKCENTRAL-COM_XDNS` | ||
| SYSCFG_DNSSEC_FLAG=`syscfg get XDNS_DNSSecEnable` | ||
| SYSCFG_XDNSREFAC_FLAG=`syscfg get XDNS_RefacCodeEnable` | ||
| if ([ "$MODEL_NUM" = "CGA4131COM" ] || [ "$MODEL_NUM" = "CGA4332COM" ]) && [ -n "$SYSCFG_XDNS_FLAG" ] && [ "$SYSCFG_XDNS_FLAG" = "1" ] && [ "$SYSCFG_DNSSEC_FLAG" = "1" ] ; then | ||
| if ([ "$MODEL_NUM" = "CGA4131COM" ] || [ "$MODEL_NUM" = "CGA4332COM" ] || [ "$MODEL_NUM" = "CGM601TCOM" ] || [ "$MODEL_NUM" = "SG417DBCT" ]) && [ -n "$SYSCFG_XDNS_FLAG" ] && [ "$SYSCFG_XDNS_FLAG" = "1" ] && [ "$SYSCFG_DNSSEC_FLAG" = "1" ] ; then | ||
| if [ "$SYSCFG_XDNSREFAC_FLAG" = "1" ] && [ "$SYSCFG_XDNS_FLAG" = "1" ] ; then |
There was a problem hiding this comment.
PR description indicates the Security Edge XDNS/DNSSEC behavior should be enabled only in business device mode, but this condition currently gates solely on MODEL_NUM and syscfg flags. If business/residential is a runtime mode switch on the same model, this script will not enforce the requirement. Please include an explicit business-mode check (or clarify in the PR description that model-number gating is the intended business-only mechanism).
Reason for change: Handle Security Edge feature for business
Test Procedure:
Functionality of the following DMLs need to be tested:
Above mentioned DML parameters SHOULD work only if the devicemode is business.
Risks: Low
Priority: P1
Signed-off-by: Santhosh_GujulvaJagadeesh@comcast.com