Skip to content
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

Update builder.py for Secure Package Verification #802

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dev-KartikSharma
Copy link

@dev-KartikSharma dev-KartikSharma commented Jan 4, 2025

Fixes #540

This pull request includes updates to the builder.py file to enhance the package verification process by integrating support for GPG verification. The following changes have been made:

Updated builder.py: The builder.py file was modified to include logic for verifying package signatures using GPG. This ensures that only verified packages are processed during the build and deployment.

Reason for Change: The original builder.py did not include proper handling of GPG signatures, which could potentially allow unverified or tampered packages to be processed. The update ensures that all packages are securely verified before being used.

Impact: This change improves the security of the build process by incorporating GPG signature verification. It guarantees that the packages used are legitimate and have not been altered in any way.

Additional Notes: The modifications to builder.py rely on the existing GPG keys and assume the presence of the appropriate GPG tarball for verification. The updated logic will prompt users to verify the GPG signatures of packages before they proceed with the build.

Testing:

Verified that the updated builder.py correctly checks GPG signatures for packages during the build process.
No issues were encountered during testing, and the package verification was successfully integrated into the build flow.

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks like a draft and, frankly, I don't see how it can be converted to an acceptable one. By reading the title & description I was under the impression that the change is about verifying signatures, however it is rather about signing release tarballs. After rereading the description I conclude that it is not merely ambiguous, it is rather plain misleading.

Comment on lines +37 to +38
build_output_dir = "/path/to/build/output" # Replace with your actual output directory
tarball_path = "/path/to/output/kiwix.tar.gz" # Desired tarball location
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoded paths shouldn't be present in the final version of a PR

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll fix that

Comment on lines +39 to +43
# Create the tarball
print(f"Creating tarball at {tarball_path}")
with tarfile.open(tarball_path, "w:gz") as tar:
tar.add(build_output_dir, arcname=os.path.basename(build_output_dir))
print(f"Tarball created successfully: {tarball_path}")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this should be delegated to the subclasses of Builder (look at the make_dist() and _make_dist() methods in kiwix-build/kiwixbuild/dependencies/base.py). However the big problem here is that those methods don't return the paths of their artifacts.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i didn't look at the base.py file this being my first draft i was focusing on signing the tarballs that were being released, i'll move on focusing the verification of the tarballs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sign source tarball
2 participants