feat(devcontainer): improve performance and reliability of devcontainer launch #119
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the devcontainer configuration to improve build performance and reliability, particularly for developers working behind corporate proxies with TLS inspection. The changes remove unused features (Python, Azure CLI), add volume mounts for better I/O performance, normalize the workspace path, and add support for corporate CA certificate injection and git identity configuration.
Key changes include:
- Removal of unused Python and Azure CLI features to speed up container builds
- Addition of volume mounts for node_modules and user config to improve performance
- Implementation of corporate CA certificate injection for TLS inspection compatibility
- Git config workaround to properly handle conditional includes in devcontainers
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
.gitignore |
Adds gitconfig export files used by devcontainer git identity workaround |
.gitattributes |
Defines line ending rules for Windows and Linux scripts (has duplicate entry) |
.dockerignore |
Excludes .git/ and node_modules/ from Docker build context |
.devcontainer/scripts/post-create.sh |
Adds volume ownership fixes, npm install, and CA cert injection logic |
.devcontainer/scripts/post-attach.sh |
New script implementing git identity configuration workaround |
.devcontainer/devcontainer.json |
Configures workspace mount, volumes, environment variables, lifecycle commands (has JSON syntax error) |
.devcontainer/README.md |
Updates documentation to reflect removed features and add TLS troubleshooting |
Comments suppressed due to low confidence (1)
.devcontainer/scripts/post-create.sh:44
- Security concern: The CA certificate injection feature copies all
.crtfiles from.devcontainer/to the system trust store without validation. Consider adding validation to:
- Verify the files are valid PEM-formatted certificates before copying
- Add a warning message when certificates are being added
- Document the security implications in comments
Example:
if compgen -G ".devcontainer/*.crt" > /dev/null; then
echo "WARNING: Adding custom CA certificates to system trust store"
# Validate certs before copying
for cert in .devcontainer/*.crt; do
if openssl x509 -in "$cert" -noout 2>/dev/null; then
sudo cp "$cert" /usr/local/share/ca-certificates/
else
echo "ERROR: Invalid certificate file: $cert"
exit 1
fi
done
sudo update-ca-certificates
fi if compgen -G ".devcontainer/*.crt" > /dev/null; then
sudo cp .devcontainer/*.crt /usr/local/share/ca-certificates/
sudo update-ca-certificates
fi
echo "Container's system CA certificates updated successfully"
| # Adds a root CA to the system certificate store. Useful if developer machines | ||
| # have MITM TLS inspection happening, e.g. with ZScaler. | ||
| echo "Updating container system CA certificates..." | ||
| if compgen -G ".devcontainer/*.crt" > /dev/null; then | ||
| sudo cp .devcontainer/*.crt /usr/local/share/ca-certificates/ | ||
| sudo update-ca-certificates | ||
| fi | ||
| echo "Container's system CA certificates updated successfully" |
There was a problem hiding this comment.
The update_ca_certs function trusts any .crt file present in .devcontainer/ by copying it into the system CA store. This enables an attacker to commit a malicious root CA to the repo, causing the container to trust attacker-controlled TLS endpoints (e.g., for npm, pip, git), enabling MITM and credential/token theft. Restrict trusted CAs to a pre-approved, immutable source (e.g., mount a secure host-managed CA volume or require explicit opt-in via a verified checksum), and validate certificates before installation (e.g., whitelist filenames and verify SHA256).
| # Adds a root CA to the system certificate store. Useful if developer machines | |
| # have MITM TLS inspection happening, e.g. with ZScaler. | |
| echo "Updating container system CA certificates..." | |
| if compgen -G ".devcontainer/*.crt" > /dev/null; then | |
| sudo cp .devcontainer/*.crt /usr/local/share/ca-certificates/ | |
| sudo update-ca-certificates | |
| fi | |
| echo "Container's system CA certificates updated successfully" | |
| # Adds a root CA to the system certificate store, but only if it matches a pre-approved whitelist. | |
| # This prevents malicious CAs from being trusted. | |
| echo "Updating container system CA certificates..." | |
| # Define whitelist: filename:sha256sum (add entries as needed) | |
| declare -A CA_CERT_WHITELIST=( | |
| # Example entry (replace with your actual cert and hash): | |
| # ["my-org-root.crt"]="0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" | |
| ) | |
| local found_valid=0 | |
| for certfile in .devcontainer/*.crt; do | |
| if [[ ! -f "${certfile}" ]]; then | |
| continue | |
| fi | |
| certname="$(basename "${certfile}")" | |
| expected_hash="${CA_CERT_WHITELIST[${certname}]:-}" | |
| if [[ -z "${expected_hash}" ]]; then | |
| echo "WARNING: Skipping unapproved CA certificate: ${certname}" | |
| continue | |
| fi | |
| actual_hash="$(sha256sum "${certfile}" | awk '{print $1}')" | |
| if [[ "${actual_hash}" != "${expected_hash}" ]]; then | |
| echo "WARNING: Hash mismatch for ${certname}, skipping." | |
| continue | |
| fi | |
| echo "Installing approved CA certificate: ${certname}" | |
| sudo cp "${certfile}" /usr/local/share/ca-certificates/ | |
| found_valid=1 | |
| done | |
| if (( found_valid )); then | |
| sudo update-ca-certificates | |
| echo "Container's system CA certificates updated successfully" | |
| else | |
| echo "No valid CA certificates installed." | |
| fi |
There was a problem hiding this comment.
@WilliamBerryiii thoughts? I'm inclined to consider this a non-risk, if an attacker has access to the repo they can do far worse damage by updating any of the initialization scripts for the dev container.
There was a problem hiding this comment.
Would love perspective from @agreaves-ms on this ... I'm on the fence here (mainly because I'm on windows and mucking with the cert store can be a PITA if things go sideways).
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
.devcontainer/scripts/post-create.sh:42
- Consider adding validation to check if the
.crtfiles are valid PEM-formatted certificates before copying them to the system certificate store. Blindly copying arbitrary files with.crtextension could introduce security risks if the files are malformed or malicious. Add a basic validation check usingopenssl x509 -in "$file" -nooutbefore copying.
if compgen -G ".devcontainer/*.crt" > /dev/null; then
sudo cp .devcontainer/*.crt /usr/local/share/ca-certificates/
sudo update-ca-certificates
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
| for conf in .gitconfig.global .gitconfig.local; do | ||
| if [[ -f "$conf" ]]; then | ||
| echo "*** Parsing ${conf##.gitconfig.} Git configuration export" | ||
| while IFS='=' read -r key value; do | ||
| local key value |
There was a problem hiding this comment.
The local keyword should be declared before the variable assignment in the while read loop. Move the local key value declaration outside and before the loop to avoid redeclaring on each iteration.
Suggested fix:
copy_user_gitconfig() {
local key value
for conf in .gitconfig.global .gitconfig.local; do
if [[ -f "$conf" ]]; then
echo "*** Parsing ${conf##.gitconfig.} Git configuration export"
while IFS='=' read -r key value; do
case "$key" in
user.name | user.email | user.signingkey | commit.gpgsign)
echo "Set Git config ${key}=${value}"
git config --global "$key" "$value"
;;
esac
done < "$conf"
rm -f "${conf}"
fi
done
}| for conf in .gitconfig.global .gitconfig.local; do | |
| if [[ -f "$conf" ]]; then | |
| echo "*** Parsing ${conf##.gitconfig.} Git configuration export" | |
| while IFS='=' read -r key value; do | |
| local key value | |
| local key value | |
| for conf in .gitconfig.global .gitconfig.local; do | |
| if [[ -f "$conf" ]]; then | |
| echo "*** Parsing ${conf##.gitconfig.} Git configuration export" | |
| while IFS='=' read -r key value; do |
2 participants
Pull Request
Description
Captures learnings from recent customer projects to enhance the build speed and launch reliability of the dev container.
🚀 - Generated by Copilot
Related Issue(s)
Type of Change
Select all that apply:
Code & Documentation:
Infrastructure & Configuration:
AI Artifacts:
prompt-builderchatmode and addressed all feedback.github/instructions/*.instructions.md).github/prompts/*.prompt.md).github/chatmodes/*.chatmode.md)Other:
.ps1,.sh,.py)Testing
Checklist
Required Checks
Required Automated Checks
The following validation commands must pass before merging:
npm run lint:mdnpm run spell-checknpm run lint:frontmatternpm run lint:md-linksnpm run lint:psSecurity Considerations
Additional Notes