Added verification of GPG key URIs against a list of trusted repositories for enhanced security #460

tsirlapu · 2024-01-11T18:50:46Z

Suggested PR Title - Implement GPG key URI verification against trusted repositories for enhanced security

RTC 537769

check if sourceApplication Gpg key URL is in trusted repo

The PR review is to check for sustainability and correctness. Sustainability is actually more business critical as correctness is largely tested into the code over time. Its useful to keep in mind that SW often outlives the HW it was written for and engineers move from job to job so it is critical that code developed for Intel be supportable across many years. It is up to the submitter and reviewer to look at the code from a perspective of what if we have to debug this 3 years from now after the author is no longer available and defect databases have been lost. Yes, that happens all the time when we are working with time scales of more than 2 years. When reviewing your code it is important to look at it from this perspective.

Author Mandatory (to be filled by PR Author/Submitter)

Developer who submits the Pull Request for merge is required to mark the checklist below as applicable for the PR changes submitted.
Those checklist items which are not marked are considered as not applicable for the PR change.
Items marked with an asterisk suffix are mandatory items to check and if not marked will be treated as non-compliant pull requests by the developers for Inner Source Development Model (ISDM) compliance

PULL DESCRIPTION

Provide a 1-2 line brief overview of the changes submitted through the Pull Request...

REFERENCES

Reference URL for issue tracking (JIRA/HSD/Github): <URL to be filled>

Added label to the Pull Request following the template: ISDM_<Complexity>*
Note-1: Depending on complexity of code changes, use the suitable word for complexity: Low/Medium/High
Example: PR for Slim boot loader project with medium complexity can have the label as: ISDM_Medium
Added label to the Pull Request for easier discoverability and search
RTC or HSD number will be included in final merge. HSD must always be included if available.
Changelogs are updated (or N/A if not customer visible)
inbm/log_changes.txt and inbm-vision/log_changes.txt are updated for potentially Validation-breaking log changes (or N/A if none)

CODE MAINTAINABILITY

APPLICATION SPECIFIC

Does PR change default config files under /etc? If so, will application still work after an upgrade that leaves /etc alone, like a Mender upgrade?
Is cloud UI changed? If so, are cloud definition files updated?

Maintainer Mandatory (to be filled by PR Reviewer/Approving Maintainer)

Maintainer who approves the Pull Request for merge is required to mark the checklist below as appropriate for the PR change reviewed as key proof of attestation indicating reasons for merge.
Those checklist items which are not marked are considered as not applicable for the PR change.
Items marked with an asterisk suffix are mandatory items to check and if not marked will be treated as non-compliant pull requests by the maintainers for ISDM compliance.

QUALITY CHECKS

CODE REVIEW IMPACT

Summary of Defects Detected in Code Review: <%P1xx,P2xx,P3xx,P4xx%>
Note P1/P2/P3/P4 denotes severity of defects found (Showstopper/High/Medium/Low) and xx denotes number of defects found

SECURITY CHECKS

Please check if your PR fulfills the following requirements:

Code must act as a teacher for future developers

${\color{red}DO \space NOT \space EDIT \space ANYTHING}$

Start of GenAI Co-Pilot generated PR Summary

Types of major changes in the pull request

enhancement, bug_fix

Pull request change summary

Added GPG key URI verification against a list of trusted repositories to enhance security.
Updated do_source_command and _handle_app_source_command to accept and pass a dispatcher_broker parameter.
Modified create_application_source_manager to require a dispatcher_broker parameter.
Enhanced UbuntuApplicationSourceManager to perform GPG key URI verification.
Updated unit tests to support changes in source command handling and GPG key URI verification.
Added documentation in README.md regarding the addition of GPG key URIs to trusted repositories.
Updated the Changelog to include the new security feature.

Details of Pull Request changes by File

Relevant files

Enhancement

4 files

dispatcher_class.py inbm/dispatcher-agent/dispatcher/dispatcher_class.py Added an additional parameter dispatcher_broker to the do_source_command function call within the do_install method to pass the MQTT broker instance.	+2/-1
source_command.py inbm/dispatcher-agent/dispatcher/source/source_command.py Modified do_source_command and _handle_app_source_command to accept a dispatcher_broker parameter and pass it to the application source manager creation function.	+7/-4
source_manager_factory.py inbm/dispatcher-agent/dispatcher/source/source_manager_factory.py Updated create_application_source_manager to accept a dispatcher_broker parameter and pass it to the UbuntuApplicationSourceManager constructor.	+4/-2
ubuntu_source_manager.py inbm/dispatcher-agent/dispatcher/source/ubuntu_source_manager.py Enhanced the UbuntuApplicationSourceManager class to include GPG key URI verification against a list of trusted repositories. Added dispatcher_broker to the constructor and used it for the verification process.	+15/-4

Tests

4 files

test_source_cmd_factory.py inbm/dispatcher-agent/tests/unit/source/test_source_cmd_factory.py Updated unit tests to create application source managers with a mock dispatcher broker.	+5/-4
test_source_command.py inbm/dispatcher-agent/tests/unit/source/test_source_command.py Updated unit tests for source commands to include the mock dispatcher broker as a parameter.	+9/-8
test_ubuntu_source_cmd.py inbm/dispatcher-agent/tests/unit/source/test_ubuntu_source_cmd.py Updated unit tests for the UbuntuApplicationSourceManager class to include the mock dispatcher broker. Added tests for the new GPG key URI verification feature.	+52/-11
SOURCE.sh inbm/integration-reloaded/test/source/SOURCE.sh Added a test step to append a trusted repository before performing OS source tests.	+4/-1

Documentation

2 files

README.md inbc-program/README.md Updated the README to include a note on adding the GPG key URI to the list of trusted repositories before using the source application add command.	+7/-1
Changelog.md inbm/Changelog.md Updated the Changelog to reflect the addition of GPG key URI verification against a list of trusted repositories.	+2/-0

End of Co-Pilot generated PR Summary

gblewis1 · 2024-01-11T20:34:40Z

/review

nex-maximus · 2024-01-11T20:36:21Z

Code Review Analysis

(review updated until commit `bb48264`)

🎯 Main theme: The PR introduces a security enhancement by verifying GPG key URIs against a list of trusted repositories before adding them to the system.
📝 PR summary: This PR adds a new security feature to the UbuntuApplicationSourceManager class, which checks if the GPG key URI provided for a source is in a list of trusted repositories. This is to ensure that only keys from trusted sources are added to the system, enhancing the security of the software update process.
📌 Type of PR: Enhancement
🏅 Score: 85
🧪 Relevant tests added: Yes
⏱️ Estimated effort to review [1-5]: 2, because the PR is focused on a single feature addition with a clear purpose, and the changes are not extensive. However, it touches on security, which requires careful consideration.
🔒 Security concerns: No

Code Review Feedback

💡 General suggestions: The PR is well-intentioned with a focus on security, which is commendable. However, it's important to ensure that the error handling is consistent and that the tests cover both success and failure scenarios adequately. Additionally, the PR should maintain the codebase's readability and follow the established coding conventions, such as consistent commenting and documentation.

✨ Usage tips:

Tag me in a comment '@nex-maximus' and add one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."