RFC: https_proxy extension: initial commit #8298
Draft
Conversation
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
WalkthroughA new Bash script, ✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
size/medium
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
extensions/https_proxy.sh (1)
47-59: Remove or properly document disabled host cleanup
Commented-out code should either be deleted or accompanied by a clear TODO and issue reference. Leaving large blocks of dead code harms readability.Consider grouping this in a single TODO with rationale or tracking issue instead of full commented function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
extensions/https_proxy.sh(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
extensions/https_proxy.sh (1)
lib/functions/logging/runners.sh (2)
run_host_command_logged(204-206)chroot_sdcard(133-136)
Comment on lines
+39
to
+43
| function pre_umount_final_image__unprepare_https_proxy_inside_chroot() { | ||
| chroot_sdcard rm /usr/share/ca-certificates/extra/squid-self-signed.crt | ||
| run_host_command_logged sed -i "'/extra\/squid-self-signed.crt/d'" "${SDCARD}/etc/ca-certificates.conf" | ||
| chroot_sdcard update-ca-certificates | ||
| } |
There was a problem hiding this comment.
Fix sed quoting and guard certificate removal inside chroot
The sed -i expression is currently quoted incorrectly and may not run. Remove the file only if it exists and correctly strip the entry.
function pre_umount_final_image__unprepare_https_proxy_inside_chroot() {
- chroot_sdcard rm /usr/share/ca-certificates/extra/squid-self-signed.crt
- run_host_command_logged sed -i "'/extra\/squid-self-signed.crt/d'" "${SDCARD}/etc/ca-certificates.conf"
- chroot_sdcard update-ca-certificates
+ # Remove certificate file if present
+ chroot_sdcard bash -c '[[ -f /usr/share/ca-certificates/extra/squid-self-signed.crt ]] && rm /usr/share/ca-certificates/extra/squid-self-signed.crt'
+ # Remove entry from conf
+ chroot_sdcard sed -i '/extra\/squid-self-signed.crt/d' /etc/ca-certificates.conf
+ chroot_sdcard update-ca-certificates
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function pre_umount_final_image__unprepare_https_proxy_inside_chroot() { | |
| chroot_sdcard rm /usr/share/ca-certificates/extra/squid-self-signed.crt | |
| run_host_command_logged sed -i "'/extra\/squid-self-signed.crt/d'" "${SDCARD}/etc/ca-certificates.conf" | |
| chroot_sdcard update-ca-certificates | |
| } | |
| function pre_umount_final_image__unprepare_https_proxy_inside_chroot() { | |
| # Remove certificate file if present | |
| chroot_sdcard bash -c '[[ -f /usr/share/ca-certificates/extra/squid-self-signed.crt ]] && rm /usr/share/ca-certificates/extra/squid-self-signed.crt' | |
| # Remove entry from conf | |
| chroot_sdcard sed -i '/extra\/squid-self-signed.crt/d' /etc/ca-certificates.conf | |
| chroot_sdcard update-ca-certificates | |
| } |
🤖 Prompt for AI Agents
In extensions/https_proxy.sh around lines 39 to 43, fix the sed command by
correcting its quoting so the expression runs properly. Also, add a check to
remove the certificate file inside the chroot only if it exists, and ensure the
sed command correctly removes the certificate entry from the configuration file.
Comment on lines
+28
to
+35
| function pre_customize_image__prepare_https_proxy_inside_chroot() { | ||
| display_alert "Found cert file" "${EXTENSION}" "info" | ||
| chroot_sdcard mkdir -p /usr/share/ca-certificates/extra/ | ||
| run_host_command_logged cp ${USERPATCHES_PATH}/squid-self-signed.crt "${SDCARD}"/usr/share/ca-certificates/extra/squid-self-signed.crt | ||
| run_host_command_logged echo "extra/squid-self-signed.crt" >> "${SDCARD}"/etc/ca-certificates.conf | ||
| chroot_sdcard cat /etc/ca-certificates.conf | ||
| chroot_sdcard update-ca-certificates | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify chroot operations and properly quote paths
Mixing chroot_sdcard and run_host_command_logged for file edits complicates context. Use chroot_sdcard for all in-chroot manipulations, quote the chroot path, and remove the debugging cat call if not needed.
function pre_customize_image__prepare_https_proxy_inside_chroot() {
- display_alert "Found cert file" "${EXTENSION}" "info"
- chroot_sdcard mkdir -p /usr/share/ca-certificates/extra/
- run_host_command_logged cp ${USERPATCHES_PATH}/squid-self-signed.crt "${SDCARD}"/usr/share/ca-certificates/extra/squid-self-signed.crt
- run_host_command_logged echo "extra/squid-self-signed.crt" >> "${SDCARD}"/etc/ca-certificates.conf
- chroot_sdcard cat /etc/ca-certificates.conf
- chroot_sdcard update-ca-certificates
+ display_alert "Preparing chroot for HTTPS proxy" "${EXTENSION}" "info"
+ # Ensure cert directory in chroot
+ chroot_sdcard mkdir -p /usr/share/ca-certificates/extra/
+ # Copy certificate into chroot
+ chroot_sdcard bash -c "cp /host${USERPATCHES_PATH}/squid-self-signed.crt /usr/share/ca-certificates/extra/"
+ # Append to ca-certificates.conf if missing
+ chroot_sdcard bash -c 'grep -Fxq "extra/squid-self-signed.crt" /etc/ca-certificates.conf || echo "extra/squid-self-signed.crt" >> /etc/ca-certificates.conf'
+ chroot_sdcard update-ca-certificates
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function pre_customize_image__prepare_https_proxy_inside_chroot() { | |
| display_alert "Found cert file" "${EXTENSION}" "info" | |
| chroot_sdcard mkdir -p /usr/share/ca-certificates/extra/ | |
| run_host_command_logged cp ${USERPATCHES_PATH}/squid-self-signed.crt "${SDCARD}"/usr/share/ca-certificates/extra/squid-self-signed.crt | |
| run_host_command_logged echo "extra/squid-self-signed.crt" >> "${SDCARD}"/etc/ca-certificates.conf | |
| chroot_sdcard cat /etc/ca-certificates.conf | |
| chroot_sdcard update-ca-certificates | |
| } | |
| function pre_customize_image__prepare_https_proxy_inside_chroot() { | |
| display_alert "Preparing chroot for HTTPS proxy" "${EXTENSION}" "info" | |
| # Ensure cert directory in chroot | |
| chroot_sdcard mkdir -p /usr/share/ca-certificates/extra/ | |
| # Copy certificate into chroot | |
| chroot_sdcard bash -c "cp /host${USERPATCHES_PATH}/squid-self-signed.crt /usr/share/ca-certificates/extra/" | |
| # Append to ca-certificates.conf if missing | |
| chroot_sdcard bash -c 'grep -Fxq "extra/squid-self-signed.crt" /etc/ca-certificates.conf || echo "extra/squid-self-signed.crt" >> /etc/ca-certificates.conf' | |
| chroot_sdcard update-ca-certificates | |
| } |
🤖 Prompt for AI Agents
In extensions/https_proxy.sh around lines 28 to 35, unify all in-chroot file
operations by replacing run_host_command_logged calls with chroot_sdcard to
maintain consistent context. Properly quote all path variables used inside
chroot_sdcard to handle spaces or special characters safely. Also, remove the
chroot_sdcard cat command used for debugging if it is not necessary for the
final script.
Comment on lines
+12
to
+24
| function post_family_config__prepare_host_for_https_proxy() { | ||
| if [ -f ${USERPATCHES_PATH}/squid-self-signed.crt ]; then | ||
| display_alert "Found cert file: ${USERPATCHES_PATH}/squid-self-signed.crt" "${EXTENSION}" "info" | ||
| run_host_command_logged mkdir -p /usr/share/ca-certificates/extra/ | ||
| run_host_command_logged cp ${USERPATCHES_PATH}/squid-self-signed.crt /usr/share/ca-certificates/extra/squid-self-signed.crt | ||
| run_host_command_logged echo "extra/squid-self-signed.crt" >> /etc/ca-certificates.conf | ||
| run_host_command_logged update-ca-certificates | ||
| display_alert "Host/Docker prepared for https proxy" "${EXTENSION}" "info" | ||
| else | ||
| display_alert "Cert file not found" "${EXTENSION}" "error" | ||
| exit 1 | ||
| fi | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Quote variables and ensure idempotent CA config on host
Unquoted ${USERPATCHES_PATH} can break on spaces; appending to /etc/ca-certificates.conf may duplicate entries on re-runs. Guard against duplicates and wrap paths in quotes.
function post_family_config__prepare_host_for_https_proxy() {
- if [ -f ${USERPATCHES_PATH}/squid-self-signed.crt ]; then
- display_alert "Found cert file: ${USERPATCHES_PATH}/squid-self-signed.crt" "${EXTENSION}" "info"
- run_host_command_logged mkdir -p /usr/share/ca-certificates/extra/
- run_host_command_logged cp ${USERPATCHES_PATH}/squid-self-signed.crt /usr/share/ca-certificates/extra/squid-self-signed.crt
- run_host_command_logged echo "extra/squid-self-signed.crt" >> /etc/ca-certificates.conf
- run_host_command_logged update-ca-certificates
+ cert_host="${USERPATCHES_PATH}/squid-self-signed.crt"
+ if [[ -f "$cert_host" ]]; then
+ display_alert "Found cert file: $cert_host" "${EXTENSION}" "info"
+ run_host_command_logged mkdir -p "/usr/share/ca-certificates/extra/"
+ run_host_command_logged cp "$cert_host" "/usr/share/ca-certificates/extra/squid-self-signed.crt"
+ # Append only if not already present
+ if ! grep -Fxq "extra/squid-self-signed.crt" /etc/ca-certificates.conf; then
+ run_host_command_logged bash -c 'echo "extra/squid-self-signed.crt" >> /etc/ca-certificates.conf'
+ fi
+ run_host_command_logged update-ca-certificates
display_alert "Host/Docker prepared for https proxy" "${EXTENSION}" "info"
else
display_alert "Cert file not found" "${EXTENSION}" "error"
exit 1
fi
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function post_family_config__prepare_host_for_https_proxy() { | |
| if [ -f ${USERPATCHES_PATH}/squid-self-signed.crt ]; then | |
| display_alert "Found cert file: ${USERPATCHES_PATH}/squid-self-signed.crt" "${EXTENSION}" "info" | |
| run_host_command_logged mkdir -p /usr/share/ca-certificates/extra/ | |
| run_host_command_logged cp ${USERPATCHES_PATH}/squid-self-signed.crt /usr/share/ca-certificates/extra/squid-self-signed.crt | |
| run_host_command_logged echo "extra/squid-self-signed.crt" >> /etc/ca-certificates.conf | |
| run_host_command_logged update-ca-certificates | |
| display_alert "Host/Docker prepared for https proxy" "${EXTENSION}" "info" | |
| else | |
| display_alert "Cert file not found" "${EXTENSION}" "error" | |
| exit 1 | |
| fi | |
| } | |
| function post_family_config__prepare_host_for_https_proxy() { | |
| cert_host="${USERPATCHES_PATH}/squid-self-signed.crt" | |
| if [[ -f "$cert_host" ]]; then | |
| display_alert "Found cert file: $cert_host" "${EXTENSION}" "info" | |
| run_host_command_logged mkdir -p "/usr/share/ca-certificates/extra/" | |
| run_host_command_logged cp "$cert_host" "/usr/share/ca-certificates/extra/squid-self-signed.crt" | |
| # Append only if not already present | |
| if ! grep -Fxq "extra/squid-self-signed.crt" /etc/ca-certificates.conf; then | |
| run_host_command_logged bash -c 'echo "extra/squid-self-signed.crt" >> /etc/ca-certificates.conf' | |
| fi | |
| run_host_command_logged update-ca-certificates | |
| display_alert "Host/Docker prepared for https proxy" "${EXTENSION}" "info" | |
| else | |
| display_alert "Cert file not found" "${EXTENSION}" "error" | |
| exit 1 | |
| fi | |
| } |
🤖 Prompt for AI Agents
In extensions/https_proxy.sh between lines 12 and 24, the variable
USERPATCHES_PATH is used without quotes, which can cause issues if the path
contains spaces. Also, appending the certificate path to
/etc/ca-certificates.conf without checking can lead to duplicate entries on
multiple runs. To fix this, wrap all instances of ${USERPATCHES_PATH} in double
quotes to handle spaces properly, and before appending the certificate path to
/etc/ca-certificates.conf, check if the entry already exists to avoid
duplicates.
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.
Description
This is a draft/wip for adding https proxy caching. This is especially designed for machines hosting lots of github runners to reduce their bandwidth usage by sacrificing disk space. http caching has been merged already and only needed minor changes.
Caching https however is way more complicated. Encryption needs to be broken and rewrapped and then obviously downloads will fail due to cert issues. Classic MITM. So a new certificate authority (CA) needs to be put in place to allow these "fake" certs.
Why is this even necessary?
Well one major part is downloads from upstream apt repositories. The other major part is downloading 3rd party repositories and artifacts from OCI/ORAS/whatever the correct name for that system is. They do not allow plain http downloads but redirect to https.
So in order to cache these encryption must be broken on the fly.
On custom runs this works already with the mentioned extension. However when the build framework is used within a GH Actions Runner environment it would need to detect this and enable the extension automatically. No clue how to do that or overall if it is worth diving even deeper into this topic. Therefore this RFC to collect feedback.
Also lots of fail-safe checks are missing.
Sometimes I had to mix
run_host_command_loggedandchroot_sdcardsince pipes don't work using latter.Documentation summary for feature / change
Yes, this would need documentation.
How Has This Been Tested?
ENABLE_EXTENSIONChecklist: