fix: CI for signing jars #1059
fix: CI for signing jars #1059
Conversation
|
Warning Rate limit exceeded@kolipakakondal has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughA new GitHub Actions workflow file named Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
.github/workflows/signjars.yml (3)
1-7: Suggest renaming the workflow for clarityThe current name "Java CI with Maven" is misleading as the workflow doesn't appear to use Maven. Consider renaming it to better reflect its purpose, such as "Sign JAR Files".
Apply this diff to update the workflow name:
-name: Java CI with Maven +name: Sign JAR Files
10-21: Improve job name for better clarityThe current job name "build" doesn't accurately describe its purpose. Consider renaming it to "sign-jars" or something similar to better reflect its function.
Apply this diff to update the job name:
- build: + sign-jars:🧰 Tools
🪛 yamllint
[error] 16-16: trailing spaces
(trailing-spaces)
16-16: Fix minor YAML formatting issuesThere are two minor formatting issues in the YAML file:
- There are trailing spaces on line 16.
- The file is missing a newline character at the end.
While these don't affect functionality, fixing them improves code consistency and adheres to YAML best practices.
Please remove the trailing spaces on line 16 and add a newline character at the end of the file.
Also applies to: 56-56
🧰 Tools
🪛 yamllint
[error] 16-16: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/signjars.yml (1 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/signjars.yml
[error] 16-16: trailing spaces
(trailing-spaces)
[error] 56-56: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.github/workflows/signjars.yml (1)
51-56: Artifact upload step looks goodThe artifact upload step is well-configured. It correctly uploads the signed JAR files and includes a condition to only upload if the job hasn't been cancelled, which is a good practice.
🧰 Tools
🪛 yamllint
[error] 56-56: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/signjars.yml
Outdated
| - name: Sign JARs | ||
| run: | | ||
| # Export secrets as environment variables | ||
| export JARSIGNER_KEYSTORE_B64=${{ secrets.JARSIGNER_REL_KEYSTORE_B64 }} | ||
| export JARSIGNER_STOREPASS=${{ secrets.JARSIGNER_REL_STOREPASS }} | ||
| export JARSIGNER_ALIAS=${{ secrets.JARSIGNER_REL_ALIAS }} | ||
|
|
||
| # Set up the keystore file path | ||
| KEYSTORE_FILE="${PWD}/{{secrets.JARSIGNER_KEYSTORE}}" | ||
| echo "Keystore file: ${KEYSTORE_FILE}" | ||
|
|
||
| # Decode and save the base64-encoded keystore to the file | ||
| printf "%s" "${JARSIGNER_KEYSTORE_B64}" | base64 -d > "${KEYSTORE_FILE}" | ||
|
|
||
| # Sign all JAR files located in the specified directory | ||
| LIB_DIR="${PWD}/BUNDLES/com.espressif.idf.serial.monitor/lib" | ||
| echo "Signing JAR files in ${LIB_DIR}" | ||
| for jar in "${LIB_DIR}"/*.jar; do | ||
| echo "Signing JAR file: ${jar}" | ||
| jarsigner -keystore "${KEYSTORE_FILE}" \ | ||
| -storepass "${JARSIGNER_STOREPASS}" \ | ||
| -signedjar "${jar}" \ | ||
| "${jar}" "${JARSIGNER_ALIAS}" | ||
| done | ||
|
|
||
| # Clean up the keystore file | ||
| rm -v "${KEYSTORE_FILE}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the JAR directory configurable
The current implementation uses a hardcoded path for the JAR files directory. To improve flexibility and reusability, consider making this path configurable through an environment variable or workflow input.
Here's an example of how you could modify the script to use an environment variable:
- LIB_DIR="${PWD}/BUNDLES/com.espressif.idf.serial.monitor/lib"
+ LIB_DIR="${JAR_DIRECTORY:-${PWD}/BUNDLES/com.espressif.idf.serial.monitor/lib}"Then, you can set the JAR_DIRECTORY environment variable in the workflow or allow it to fall back to the default path.
📝 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.
| - name: Sign JARs | |
| run: | | |
| # Export secrets as environment variables | |
| export JARSIGNER_KEYSTORE_B64=${{ secrets.JARSIGNER_REL_KEYSTORE_B64 }} | |
| export JARSIGNER_STOREPASS=${{ secrets.JARSIGNER_REL_STOREPASS }} | |
| export JARSIGNER_ALIAS=${{ secrets.JARSIGNER_REL_ALIAS }} | |
| # Set up the keystore file path | |
| KEYSTORE_FILE="${PWD}/{{secrets.JARSIGNER_KEYSTORE}}" | |
| echo "Keystore file: ${KEYSTORE_FILE}" | |
| # Decode and save the base64-encoded keystore to the file | |
| printf "%s" "${JARSIGNER_KEYSTORE_B64}" | base64 -d > "${KEYSTORE_FILE}" | |
| # Sign all JAR files located in the specified directory | |
| LIB_DIR="${PWD}/BUNDLES/com.espressif.idf.serial.monitor/lib" | |
| echo "Signing JAR files in ${LIB_DIR}" | |
| for jar in "${LIB_DIR}"/*.jar; do | |
| echo "Signing JAR file: ${jar}" | |
| jarsigner -keystore "${KEYSTORE_FILE}" \ | |
| -storepass "${JARSIGNER_STOREPASS}" \ | |
| -signedjar "${jar}" \ | |
| "${jar}" "${JARSIGNER_ALIAS}" | |
| done | |
| # Clean up the keystore file | |
| rm -v "${KEYSTORE_FILE}" | |
| - name: Sign JARs | |
| run: | | |
| # Export secrets as environment variables | |
| export JARSIGNER_KEYSTORE_B64=${{ secrets.JARSIGNER_REL_KEYSTORE_B64 }} | |
| export JARSIGNER_STOREPASS=${{ secrets.JARSIGNER_REL_STOREPASS }} | |
| export JARSIGNER_ALIAS=${{ secrets.JARSIGNER_REL_ALIAS }} | |
| # Set up the keystore file path | |
| KEYSTORE_FILE="${PWD}/{{secrets.JARSIGNER_KEYSTORE}}" | |
| echo "Keystore file: ${KEYSTORE_FILE}" | |
| # Decode and save the base64-encoded keystore to the file | |
| printf "%s" "${JARSIGNER_KEYSTORE_B64}" | base64 -d > "${KEYSTORE_FILE}" | |
| # Sign all JAR files located in the specified directory | |
| LIB_DIR="${JAR_DIRECTORY:-${PWD}/BUNDLES/com.espressif.idf.serial.monitor/lib}" | |
| echo "Signing JAR files in ${LIB_DIR}" | |
| for jar in "${LIB_DIR}"/*.jar; do | |
| echo "Signing JAR file: ${jar}" | |
| jarsigner -keystore "${KEYSTORE_FILE}" \ | |
| -storepass "${JARSIGNER_STOREPASS}" \ | |
| -signedjar "${jar}" \ | |
| "${jar}" "${JARSIGNER_ALIAS}" | |
| done | |
| # Clean up the keystore file | |
| rm -v "${KEYSTORE_FILE}" |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
.github/workflows/signjars.yml (2)
9-21: Consider renaming the job for clarity.The current job name "build" is generic. Consider renaming it to something more specific like "sign-jars" to better reflect its purpose.
The rest of the setup looks good:
- Using macOS-latest is appropriate for signing, especially if related to Apple's notarization process.
- The checkout and Java setup actions are up-to-date and correctly configured.
Here's a suggested change:
jobs: - build: + sign-jars:🧰 Tools
🪛 yamllint
[error] 16-16: trailing spaces
(trailing-spaces)
16-16: Remove trailing spaces.There are trailing spaces on this line. While they don't affect functionality, it's good practice to remove them for consistency and to avoid potential issues with some YAML parsers.
Please remove the trailing spaces from this line.
🧰 Tools
🪛 yamllint
[error] 16-16: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/signjars.yml (1 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/signjars.yml
[error] 16-16: trailing spaces
(trailing-spaces)
🔇 Additional comments (4)
.github/workflows/signjars.yml (4)
1-7: LGTM: Workflow name and trigger conditions are well-defined.The workflow name "Sign jars" clearly describes its purpose, and the trigger conditions ensure that the signing process is applied to all changes in the master branch, both for direct pushes and pull requests.
23-36: LGTM: Secure handling of secrets and keystore.The workflow correctly uses GitHub secrets for sensitive information and securely handles the keystore file:
- Secrets are exported as environment variables, keeping them protected.
- The base64-encoded keystore is properly decoded and saved to a file.
- The keystore file is removed after use, ensuring it's not left on the runner.
This approach maintains good security practices for handling sensitive signing information.
51-56: LGTM: Artifact upload is well-configured.The artifact upload step is correctly implemented:
- It uses the latest version (v4) of the upload-artifact action.
- The condition
if: ${{ !cancelled() }}ensures artifacts are only uploaded if the job completes successfully.- The path for the signed JAR files matches the directory where they were signed.
This configuration ensures that the signed JAR files are properly preserved and made available for subsequent steps or manual download.
38-38: 🛠️ Refactor suggestionConsider making the JAR directory configurable.
The current implementation uses a hardcoded path for the JAR files directory. To improve flexibility and reusability, consider making this path configurable through an environment variable or workflow input.
Here's an example of how you could modify the script to use an environment variable:
-LIB_DIR="${PWD}/BUNDLES/com.espressif.idf.serial.monitor/lib" +LIB_DIR="${JAR_DIRECTORY:-${PWD}/BUNDLES/com.espressif.idf.serial.monitor/lib}"Then, you can set the
JAR_DIRECTORYenvironment variable in the workflow or allow it to fall back to the default path.
.github/workflows/signjars.yml
Outdated
| echo "Signing JAR files in ${LIB_DIR}" | ||
| for jar in "${LIB_DIR}"/*.jar; do | ||
| echo "Signing JAR file: ${jar}" | ||
| jarsigner -keystore "${KEYSTORE_FILE}" \ | ||
| -storepass "${JARSIGNER_STOREPASS}" \ | ||
| -signedjar "${jar}" \ | ||
| "${jar}" "${JARSIGNER_ALIAS}" | ||
| done | ||
|
|
||
| # Clean up the keystore file | ||
| rm -v "${KEYSTORE_FILE}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling and logging.
While the current implementation works, it could benefit from improved error handling and logging:
- Check if the JAR directory exists before attempting to sign files.
- Add error handling for the jarsigner command.
- Implement more verbose logging to aid in troubleshooting.
Here's an example of how you could enhance the script:
echo "Signing JAR files in ${LIB_DIR}"
if [ ! -d "${LIB_DIR}" ]; then
echo "Error: Directory ${LIB_DIR} does not exist."
exit 1
fi
for jar in "${LIB_DIR}"/*.jar; do
if [ -f "$jar" ]; then
echo "Signing JAR file: ${jar}"
if jarsigner -keystore "${KEYSTORE_FILE}" \
-storepass "${JARSIGNER_STOREPASS}" \
-signedjar "${jar}" \
"${jar}" "${JARSIGNER_ALIAS}"; then
echo "Successfully signed ${jar}"
else
echo "Error: Failed to sign ${jar}"
exit 1
fi
fi
done
echo "All JAR files signed successfully"This enhancement adds checks for the directory and file existence, error handling for the jarsigner command, and more detailed logging.
kolipakakondal
changed the title
kolipakakondal
force-pushed
the
signjars
branch
from
8468f59
to
873cfff
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
.github/workflows/signjars.yml (1)
1-67: Summary: Workflow achieves its goal with room for improvement.The workflow successfully implements JAR and native library signing. However, several improvements can be made:
- Enhance job naming and update the checkout action.
- Improve certificate handling and keychain management for better security.
- Optimize JAR processing and signing for better performance.
- Address static analysis issues for better code quality.
These changes will make the workflow more secure, efficient, and maintainable.
Would you like assistance in implementing these improvements? I can help draft the updated workflow incorporating all the suggested changes.
🧰 Tools
🪛 actionlint
27-27: shellcheck reported issue in this script: SC2086:info:2:6: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:6:63: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2162:info:21:64: read without -r will mangle backslashes
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:23:38: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2035:info:28:17: Use ./glob or -- glob so names with dashes won't become options
(shellcheck)
🪛 yamllint
[error] 24-24: trailing spaces
(trailing-spaces)
[error] 67-67: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/signjars.yml (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/signjars.yml
27-27: shellcheck reported issue in this script: SC2086:info:2:6: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:6:63: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2162:info:21:64: read without -r will mangle backslashes
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:23:38: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2035:info:28:17: Use ./glob or -- glob so names with dashes won't become options
(shellcheck)
🪛 yamllint
.github/workflows/signjars.yml
[error] 24-24: trailing spaces
(trailing-spaces)
[error] 67-67: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (2)
.github/workflows/signjars.yml (2)
1-7: LGTM: Workflow name and trigger configuration are appropriate.The workflow name clearly describes its purpose, and the trigger configuration is suitable for signing JARs on pushes and pull requests to the master branch.
62-67: LGTM: Artifact upload step is well-configured.The artifact upload step is correctly implemented:
- It uses the latest version of the upload-artifact action (v4).
- The condition to run only if the job hasn't been cancelled is a good practice.
- The artifact name and path are appropriately set.
🧰 Tools
🪛 yamllint
[error] 67-67: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/signjars.yml
Outdated
| - name: Codesign Internal Native Libraries | ||
| env: | ||
| MACOS_CERTIFICATE: ${{ secrets.MACOS_CERTIFICATE }} | ||
| MACOS_CERTIFICATE_PWD: ${{ secrets.MACOS_CERTIFICATE_PWD }} | ||
| run: | |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure secure handling of sensitive data.
The use of GitHub secrets for storing sensitive data is good practice. However, consider the following security enhancements:
- Use
GITHUB_OUTPUTinstead of environment variables for sensitive data. - Clear sensitive environment variables after use.
Here's an example of how to use GITHUB_OUTPUT:
- name: Set up signing environment
id: signing_setup
run: |
echo "MACOS_CERTIFICATE=$(echo ${{ secrets.MACOS_CERTIFICATE }} | base64 --decode)" >> $GITHUB_OUTPUT
echo "MACOS_CERTIFICATE_PWD=${{ secrets.MACOS_CERTIFICATE_PWD }}" >> $GITHUB_OUTPUT
shell: bashThen, in subsequent steps, use ${{ steps.signing_setup.outputs.MACOS_CERTIFICATE }} instead of $MACOS_CERTIFICATE.
🧰 Tools
🪛 actionlint
27-27: shellcheck reported issue in this script: SC2086:info:2:6: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:6:63: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2162:info:21:64: read without -r will mangle backslashes
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:23:38: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2035:info:28:17: Use ./glob or -- glob so names with dashes won't become options
(shellcheck)
🪛 yamllint
[error] 24-24: trailing spaces
(trailing-spaces)
.github/workflows/signjars.yml
Outdated
| # Step 2: Define the directory containing the JARs and native libraries | ||
| LIB_DIR="${PWD}/BUNDLES/com.espressif.idf.serial.monitor/lib" | ||
|
|
||
| # Step 3: Extract, sign native libraries, and repackage JARs | ||
| for jar in "${LIB_DIR}"/*.jar; do | ||
| echo "Processing JAR file: ${jar}" | ||
|
|
||
| # Create a temporary directory to extract the JAR contents | ||
| TEMP_DIR=$(mktemp -d) | ||
| unzip -q "$jar" -d "$TEMP_DIR" | ||
|
|
||
| # Find and sign all .jnilib and .dylib files in the extracted JAR directory | ||
| find "$TEMP_DIR" -name "*.jnilib" -o -name "*.dylib" | while read lib; do | ||
| echo "Signing native library: ${lib}" | ||
| /usr/bin/codesign --entitlements $PWD/releng/com.espressif.idf.product/entitlements/espressif-ide.entitlement --options runtime --force -s "ESPRESSIF SYSTEMS (SHANGHAI) CO., LTD. (QWXF6GB4AV)" --timestamp --deep "$lib" | ||
| done | ||
|
|
||
| # Repackage the signed JAR | ||
| pushd "$TEMP_DIR" | ||
| zip -r "$jar" * | ||
| popd | ||
|
|
||
| # Clean up temporary directory | ||
| rm -rf "$TEMP_DIR" | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize JAR processing and signing.
The JAR processing and signing logic can be improved for better performance and reliability:
- Use parallel processing for signing multiple JARs.
- Add error checking and logging for each step.
- Use more efficient commands for JAR manipulation.
Here's an optimized version of the JAR processing logic:
LIB_DIR="${PWD}/BUNDLES/com.espressif.idf.serial.monitor/lib"
find "$LIB_DIR" -name "*.jar" | parallel --jobs 0 '
jar_file={}
echo "Processing JAR file: $jar_file"
temp_dir=$(mktemp -d)
if ! unzip -q "$jar_file" -d "$temp_dir"; then
echo "Error extracting $jar_file"
exit 1
fi
find "$temp_dir" \( -name "*.jnilib" -o -name "*.dylib" \) -print0 | xargs -0 -I {} /usr/bin/codesign --entitlements $PWD/releng/com.espressif.idf.product/entitlements/espressif-ide.entitlement --options runtime --force -s "ESPRESSIF SYSTEMS (SHANGHAI) CO., LTD. (QWXF6GB4AV)" --timestamp --deep {} || exit 1
if ! (cd "$temp_dir" && zip -qr "$jar_file" .); then
echo "Error repackaging $jar_file"
exit 1
fi
rm -rf "$temp_dir"
'This version uses GNU Parallel for concurrent processing, adds error checking, and uses more efficient commands for JAR manipulation.
.github/workflows/signjars.yml
Outdated
| name: Sign jars and internal native libraries | ||
|
|
||
| on: | ||
| push: | ||
| branches: [ master ] | ||
| pull_request: | ||
| branches: [ master ] | ||
|
|
||
| jobs: | ||
| build: | ||
| runs-on: macos-latest | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Set up JDK 17 | ||
| uses: actions/setup-java@v3 | ||
| with: | ||
| java-version: '17' | ||
| distribution: 'temurin' | ||
|
|
||
| - name: Codesign Internal Native Libraries | ||
| env: | ||
| MACOS_CERTIFICATE: ${{ secrets.MACOS_CERTIFICATE }} | ||
| MACOS_CERTIFICATE_PWD: ${{ secrets.MACOS_CERTIFICATE_PWD }} | ||
| run: | | ||
| # Step 1: Decode and import the certificate into a keychain | ||
| echo $MACOS_CERTIFICATE | base64 --decode > certificate.p12 | ||
| /usr/bin/security create-keychain -p espressif build.keychain | ||
| /usr/bin/security default-keychain -s build.keychain | ||
| /usr/bin/security unlock-keychain -p espressif build.keychain | ||
| /usr/bin/security import certificate.p12 -k build.keychain -P $MACOS_CERTIFICATE_PWD -T /usr/bin/codesign | ||
| /usr/bin/security set-key-partition-list -S apple-tool:,apple:,codesign: -s -k espressif build.keychain | ||
|
|
||
| # Step 2: Define the directory containing the JARs and native libraries | ||
| LIB_DIR="${PWD}/BUNDLES/com.espressif.idf.serial.monitor/lib" | ||
|
|
||
| # Step 3: Extract, sign native libraries, and repackage JARs | ||
| for jar in "${LIB_DIR}"/*.jar; do | ||
| echo "Processing JAR file: ${jar}" | ||
|
|
||
| # Create a temporary directory to extract the JAR contents | ||
| TEMP_DIR=$(mktemp -d) | ||
| unzip -q "$jar" -d "$TEMP_DIR" | ||
|
|
||
| # Find and sign all .jnilib and .dylib files in the extracted JAR directory | ||
| find "$TEMP_DIR" -name "*.jnilib" -o -name "*.dylib" | while read lib; do | ||
| echo "Signing native library: ${lib}" | ||
| /usr/bin/codesign --entitlements $PWD/releng/com.espressif.idf.product/entitlements/espressif-ide.entitlement --options runtime --force -s "ESPRESSIF SYSTEMS (SHANGHAI) CO., LTD. (QWXF6GB4AV)" --timestamp --deep "$lib" | ||
| done | ||
|
|
||
| # Repackage the signed JAR | ||
| pushd "$TEMP_DIR" | ||
| zip -r "$jar" * | ||
| popd | ||
|
|
||
| # Clean up temporary directory | ||
| rm -rf "$TEMP_DIR" | ||
| done | ||
|
|
||
| - name: Upload Signed JAR Files | ||
| if: ${{ !cancelled() }} | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: signed-jar-files | ||
| path: BUNDLES/com.espressif.idf.serial.monitor/lib/*.jar |
There was a problem hiding this comment.
Address static analysis issues.
Please address the following issues reported by static analysis tools:
-
Double quote variables and command substitutions to prevent globbing and word splitting. For example:
find "$TEMP_DIR" -name "*.jnilib" -o -name "*.dylib" | while read -r lib; do
-
Use
read -rinstead ofreadto prevent backslash mangling. -
Remove trailing spaces (line 24) and add a newline at the end of the file.
To automatically fix these issues, consider using tools like shfmt for shell script formatting and a YAML linter with auto-fix capabilities.
🧰 Tools
🪛 actionlint
27-27: shellcheck reported issue in this script: SC2086:info:2:6: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:6:63: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2162:info:21:64: read without -r will mangle backslashes
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:23:38: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2035:info:28:17: Use ./glob or -- glob so names with dashes won't become options
(shellcheck)
🪛 yamllint
[error] 24-24: trailing spaces
(trailing-spaces)
[error] 67-67: no new line character at the end of file
(new-line-at-end-of-file)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
.github/workflows/signjars.yml (2)
78-83: Add a newline at the end of the fileThe artifact upload step is well-implemented. However, to adhere to YAML best practices and resolve a linting issue, please add a newline at the end of the file.
with: name: signed-jar-files path: BUNDLES/com.espressif.idf.serial.monitor/lib/*.jar +This small change will improve the file's compliance with YAML standards.
🧰 Tools
🪛 yamllint
[error] 83-83: no new line character at the end of file
(new-line-at-end-of-file)
1-83: Address remaining static analysis issuesTo further improve the script's robustness, consider addressing the following minor issues reported by static analysis tools:
Double quote variables and command substitutions to prevent globbing and word splitting. For example:
echo "$MACOS_CERTIFICATE" | base64 --decode > certificate.p12Use
find ... -print0 | xargs -0 ...orwhile IFS= read -r -d '' file; do ...instead offind ... | while read file; do ...to correctly handle filenames with spaces or special characters.Remove trailing spaces (e.g., on line 24).
These changes will help prevent potential issues with unusual filenames or inputs and improve overall script quality.
🧰 Tools
🪛 actionlint
30-30: shellcheck reported issue in this script: SC2086:info:2:6: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2086:info:6:63: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2086:info:10:6: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2162:info:24:64: read without -r will mangle backslashes
(shellcheck)
30-30: shellcheck reported issue in this script: SC2086:info:26:38: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2035:info:31:17: Use ./glob or -- glob so names with dashes won't become options
(shellcheck)
🪛 yamllint
[error] 24-24: trailing spaces
(trailing-spaces)
[error] 83-83: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/signjars.yml (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/signjars.yml
30-30: shellcheck reported issue in this script: SC2086:info:2:6: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2086:info:6:63: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2086:info:10:6: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2162:info:24:64: read without -r will mangle backslashes
(shellcheck)
30-30: shellcheck reported issue in this script: SC2086:info:26:38: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2035:info:31:17: Use ./glob or -- glob so names with dashes won't become options
(shellcheck)
🪛 yamllint
.github/workflows/signjars.yml
[error] 24-24: trailing spaces
(trailing-spaces)
[error] 83-83: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.github/workflows/signjars.yml (1)
39-43: 🛠️ Refactor suggestionConsider making the JAR directory configurable
The current implementation uses a hardcoded path for the JAR files directory. To improve flexibility and reusability, consider making this path configurable through an environment variable or workflow input.
Here's an example of how you could modify the script:
-LIB_DIR="${PWD}/BUNDLES/com.espressif.idf.serial.monitor/lib" +LIB_DIR="${JAR_DIRECTORY:-${PWD}/BUNDLES/com.espressif.idf.serial.monitor/lib}"Then, you can set the
JAR_DIRECTORYenvironment variable in the workflow or allow it to fall back to the default path.
.github/workflows/signjars.yml
Outdated
| - name: Codesign Internal Native Libraries | ||
| env: | ||
| MACOS_CERTIFICATE: ${{ secrets.MACOS_CERTIFICATE }} | ||
| MACOS_CERTIFICATE_PWD: ${{ secrets.MACOS_CERTIFICATE_PWD }} | ||
| JARSIGNER_KEYSTORE_B64: ${{ secrets.JARSIGNER_KEYSTORE_B64 }} | ||
| JARSIGNER_STOREPASS: ${{ secrets.JARSIGNER_STOREPASS }} | ||
| JARSIGNER_ALIAS: ${{ secrets.JARSIGNER_ALIAS }} | ||
| run: | | ||
| # Step 1: Decode and import the certificate into a keychain | ||
| echo $MACOS_CERTIFICATE | base64 --decode > certificate.p12 | ||
| /usr/bin/security create-keychain -p espressif build.keychain | ||
| /usr/bin/security default-keychain -s build.keychain | ||
| /usr/bin/security unlock-keychain -p espressif build.keychain | ||
| /usr/bin/security import certificate.p12 -k build.keychain -P $MACOS_CERTIFICATE_PWD -T /usr/bin/codesign | ||
| /usr/bin/security set-key-partition-list -S apple-tool:,apple:,codesign: -s -k espressif build.keychain |
There was a problem hiding this comment.
Improve keychain management for better security
While the certificate handling process is mostly correct, there are some security improvements that can be made:
- Use a randomly generated password for the keychain instead of a hardcoded one.
- Set a timeout for the keychain to automatically lock after inactivity.
- Use a unique name for the keychain to avoid conflicts.
Here's an improved version:
KEYCHAIN_NAME="build-$(uuidgen).keychain"
KEYCHAIN_PASSWORD="$(openssl rand -base64 32)"
# Create and configure keychain
security create-keychain -p "$KEYCHAIN_PASSWORD" "$KEYCHAIN_NAME"
security default-keychain -s "$KEYCHAIN_NAME"
security unlock-keychain -p "$KEYCHAIN_PASSWORD" "$KEYCHAIN_NAME"
security set-keychain-settings -t 3600 -l "$KEYCHAIN_NAME"
# Import certificate
security import certificate.p12 -k "$KEYCHAIN_NAME" -P "$MACOS_CERTIFICATE_PWD" -T /usr/bin/codesign
security set-key-partition-list -S apple-tool:,apple:,codesign: -s -k "$KEYCHAIN_PASSWORD" "$KEYCHAIN_NAME"
# Don't forget to add a step to delete the keychain after signing is completeThis approach improves security by using a unique, randomly generated password and setting an inactivity timeout.
🧰 Tools
🪛 actionlint
30-30: shellcheck reported issue in this script: SC2086:info:2:6: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2086:info:6:63: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2086:info:10:6: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2162:info:24:64: read without -r will mangle backslashes
(shellcheck)
30-30: shellcheck reported issue in this script: SC2086:info:26:38: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2035:info:31:17: Use ./glob or -- glob so names with dashes won't become options
(shellcheck)
🪛 yamllint
[error] 24-24: trailing spaces
(trailing-spaces)
.github/workflows/signjars.yml
Outdated
| # Step 4: Extract, sign native libraries, repackage, and sign the JARs | ||
| for jar in "${LIB_DIR}"/*.jar; do | ||
| echo "Processing JAR file: ${jar}" | ||
|
|
||
| # Create a temporary directory to extract the JAR contents | ||
| TEMP_DIR=$(mktemp -d) | ||
| unzip -q "$jar" -d "$TEMP_DIR" | ||
|
|
||
| # Find and sign all .jnilib and .dylib files in the extracted JAR directory | ||
| find "$TEMP_DIR" -name "*.jnilib" -o -name "*.dylib" | while read lib; do | ||
| echo "Signing native library: ${lib}" | ||
| /usr/bin/codesign --entitlements $PWD/releng/com.espressif.idf.product/entitlements/espressif-ide.entitlement --options runtime --force -s "ESPRESSIF SYSTEMS (SHANGHAI) CO., LTD. (QWXF6GB4AV)" --timestamp --deep "$lib" | ||
| done | ||
|
|
||
| # Repackage the signed JAR | ||
| pushd "$TEMP_DIR" | ||
| zip -r "$jar" * | ||
| popd | ||
|
|
||
| # Sign the repackaged JAR using jarsigner | ||
| echo "Signing repackaged JAR: ${jar}" | ||
| jarsigner -keystore keystore.jks \ | ||
| -storepass "${JARSIGNER_STOREPASS}" \ | ||
| -signedjar "${jar}" \ | ||
| "${jar}" "${JARSIGNER_ALIAS}" | ||
|
|
||
| # Clean up temporary directory | ||
| rm -rf "$TEMP_DIR" | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize JAR processing and signing
The current implementation works, but it can be improved for better performance, reliability, and security:
- Use parallel processing for signing multiple JARs.
- Add error checking and logging for each step.
- Use
read -rto prevent backslash mangling in filenames. - Quote variables to prevent word splitting and globbing.
Here's an optimized version:
export -f codesign_and_verify
find "$LIB_DIR" -name "*.jar" -print0 | parallel -0 process_jar {}
process_jar() {
local jar_file="$1"
echo "Processing JAR file: $jar_file"
local temp_dir
temp_dir=$(mktemp -d) || { echo "Failed to create temp directory"; return 1; }
if ! unzip -q "$jar_file" -d "$temp_dir"; then
echo "Error extracting $jar_file"
rm -rf "$temp_dir"
return 1
fi
find "$temp_dir" \( -name "*.jnilib" -o -name "*.dylib" \) -print0 |
xargs -0 -I {} bash -c 'codesign_and_verify "$@"' _ {} "$PWD/releng/com.espressif.idf.product/entitlements/espressif-ide.entitlement"
if ! (cd "$temp_dir" && zip -qr "$jar_file" .); then
echo "Error repackaging $jar_file"
rm -rf "$temp_dir"
return 1
fi
if ! jarsigner -keystore keystore.jks -storepass "$JARSIGNER_STOREPASS" -signedjar "$jar_file" "$jar_file" "$JARSIGNER_ALIAS"; then
echo "Error signing JAR $jar_file"
rm -rf "$temp_dir"
return 1
fi
rm -rf "$temp_dir"
echo "Successfully processed $jar_file"
}
codesign_and_verify() {
local lib="$1"
local entitlements="$2"
echo "Signing native library: $lib"
if ! /usr/bin/codesign --entitlements "$entitlements" --options runtime --force -s "ESPRESSIF SYSTEMS (SHANGHAI) CO., LTD. (QWXF6GB4AV)" --timestamp --deep "$lib"; then
echo "Error signing $lib"
return 1
fi
if ! /usr/bin/codesign --verify --deep --strict --verbose=2 "$lib"; then
echo "Verification failed for $lib"
return 1
fi
echo "Successfully signed and verified $lib"
}This version uses GNU Parallel for concurrent processing, adds error checking, uses read -r, and quotes variables for improved security and reliability.
| name: Sign jars and internal native libraries | ||
|
|
||
| on: | ||
| push: | ||
| branches: [ master ] | ||
| pull_request: | ||
| branches: [ master ] | ||
|
|
||
| jobs: | ||
| build: | ||
| runs-on: macos-latest | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Set up JDK 17 | ||
| uses: actions/setup-java@v3 | ||
| with: | ||
| java-version: '17' | ||
| distribution: 'temurin' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider updating the checkout action to v4
The workflow setup looks good overall. However, consider updating the actions/checkout action to v4 for potential improvements and bug fixes:
- uses: actions/checkout@v3
+ uses: actions/checkout@v4This update ensures you're using the latest features and fixes provided by the checkout action.
📝 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.
| name: Sign jars and internal native libraries | |
| on: | |
| push: | |
| branches: [ master ] | |
| pull_request: | |
| branches: [ master ] | |
| jobs: | |
| build: | |
| runs-on: macos-latest | |
| steps: | |
| - name: Checkout code | |
| uses: actions/checkout@v3 | |
| - name: Set up JDK 17 | |
| uses: actions/setup-java@v3 | |
| with: | |
| java-version: '17' | |
| distribution: 'temurin' | |
| name: Sign jars and internal native libraries | |
| on: | |
| push: | |
| branches: [ master ] | |
| pull_request: | |
| branches: [ master ] | |
| jobs: | |
| build: | |
| runs-on: macos-latest | |
| steps: | |
| - name: Checkout code | |
| uses: actions/checkout@v4 | |
| - name: Set up JDK 17 | |
| uses: actions/setup-java@v3 | |
| with: | |
| java-version: '17' | |
| distribution: 'temurin' |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
.github/workflows/signjars.yml (2)
79-89: Address minor issues in artifact upload section
- Quote the
${SIGNED_JARS_DIR}variable in thelscommand to prevent potential issues with spaces in the path:- ls -al ${SIGNED_JARS_DIR} + ls -al "${SIGNED_JARS_DIR}"
- Add a newline at the end of the file to comply with POSIX standards and improve version control diff readability.
These small changes will improve the robustness and style compliance of the workflow.
🧰 Tools
🪛 actionlint
80-80: shellcheck reported issue in this script: SC2086:info:2:8: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
[error] 89-89: no new line character at the end of file
(new-line-at-end-of-file)
24-24: Address remaining static analysis issues
- Remove trailing spaces from line 24:
- - name: Codesign JARs and Internal Native Libraries - env: + - name: Codesign JARs and Internal Native Libraries + env:This change will improve code style consistency and satisfy the yamllint check.
🧰 Tools
🪛 yamllint
[error] 24-24: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/signjars.yml (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/signjars.yml
27-27: shellcheck reported issue in this script: SC2086:info:2:6: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:6:63: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2162:info:30:64: read without -r will mangle backslashes
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:32:44: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2035:info:37:50: Use ./glob or -- glob so names with dashes won't become options
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:42:42: Double quote to prevent globbing and word splitting
(shellcheck)
80-80: shellcheck reported issue in this script: SC2086:info:2:8: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
.github/workflows/signjars.yml
[error] 24-24: trailing spaces
(trailing-spaces)
[error] 89-89: no new line character at the end of file
(new-line-at-end-of-file)
| build: | ||
| runs-on: macos-latest |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider updating the checkout action and improving the job name
- Update the checkout action to the latest version for potential improvements and bug fixes:
- uses: actions/checkout@v3
+ uses: actions/checkout@v4- Consider using a more descriptive job name that reflects its purpose:
- build:
+ sign-jars-and-libraries:These changes will improve the workflow's maintainability and clarity.
Also applies to: 15-15
| # Step 1: Decode and import the certificate into a keychain | ||
| echo $MACOS_CERTIFICATE | base64 --decode > certificate.p12 | ||
| /usr/bin/security create-keychain -p espressif build.keychain | ||
| /usr/bin/security default-keychain -s build.keychain | ||
| /usr/bin/security unlock-keychain -p espressif build.keychain | ||
| /usr/bin/security import certificate.p12 -k build.keychain -P $MACOS_CERTIFICATE_PWD -T /usr/bin/codesign | ||
| /usr/bin/security set-key-partition-list -S apple-tool:,apple:,codesign: -s -k espressif build.keychain |
There was a problem hiding this comment.
Improve keychain management for better security
Consider the following enhancements to the keychain management process:
- Use a randomly generated password for the keychain instead of a hardcoded one.
- Use a unique name for the keychain to avoid conflicts.
- Set a timeout for the keychain to automatically lock after inactivity.
Here's an improved version:
KEYCHAIN_NAME="build-$(uuidgen).keychain"
KEYCHAIN_PASSWORD="$(openssl rand -base64 32)"
# Create and configure keychain
security create-keychain -p "$KEYCHAIN_PASSWORD" "$KEYCHAIN_NAME"
security default-keychain -s "$KEYCHAIN_NAME"
security unlock-keychain -p "$KEYCHAIN_PASSWORD" "$KEYCHAIN_NAME"
security set-keychain-settings -t 3600 -l "$KEYCHAIN_NAME"
# Import certificate
security import certificate.p12 -k "$KEYCHAIN_NAME" -P "$MACOS_CERTIFICATE_PWD" -T /usr/bin/codesign
security set-key-partition-list -S apple-tool:,apple:,codesign: -s -k "$KEYCHAIN_PASSWORD" "$KEYCHAIN_NAME"
# Don't forget to add a step to delete the keychain after signing is completeThis approach improves security by using a unique, randomly generated password and setting an inactivity timeout.
.github/workflows/signjars.yml
Outdated
| # Step 2: Define the directory containing the JARs and native libraries and the temp directory for signed JARs | ||
| LIB_DIR="${PWD}/BUNDLES/com.espressif.idf.serial.monitor/lib" | ||
| SIGNED_JARS_DIR=$(mktemp -d) # Create a temporary directory for signed JARs | ||
|
|
||
| # Step 3: Extract, sign native libraries, repackage, and sign the JARs with Apple codesign | ||
| for jar in "${LIB_DIR}"/*.jar; do | ||
| echo "Processing JAR file: ${jar}" | ||
|
|
||
| # Check if the JAR exists | ||
| if [ -f "$jar" ]; then | ||
| echo "JAR file found: ${jar}" | ||
| else | ||
| echo "JAR file not found: ${jar}" | ||
| continue | ||
| fi | ||
|
|
||
| # Create a temporary directory to extract the JAR contents | ||
| TEMP_DIR=$(mktemp -d) | ||
| unzip -q "$jar" -d "$TEMP_DIR" | ||
|
|
||
| # Find and sign all .jnilib and .dylib files in the extracted JAR directory | ||
| find "$TEMP_DIR" -name "*.jnilib" -o -name "*.dylib" | while read lib; do | ||
| echo "Signing native library: ${lib}" | ||
| /usr/bin/codesign -vvvv --entitlements $PWD/releng/com.espressif.idf.product/entitlements/espressif-ide.entitlement --options runtime --force -s "ESPRESSIF SYSTEMS (SHANGHAI) CO., LTD. (QWXF6GB4AV)" --timestamp --deep "$lib" | ||
| done | ||
|
|
||
| # Repackage the signed JAR | ||
| pushd "$TEMP_DIR" | ||
| zip -r "${SIGNED_JARS_DIR}/$(basename "$jar")" * # Save signed JAR to temporary directory | ||
| popd | ||
|
|
||
| # Sign the entire JAR with Apple codesign, using the same entitlements | ||
| echo "Signing repackaged JAR: ${SIGNED_JARS_DIR}/$(basename "$jar")" | ||
| /usr/bin/codesign -vvvv --entitlements $PWD/releng/com.espressif.idf.product/entitlements/espressif-ide.entitlement --force --deep --options runtime --timestamp -s "ESPRESSIF SYSTEMS (SHANGHAI) CO., LTD. (QWXF6GB4AV)" "${SIGNED_JARS_DIR}/$(basename "$jar")" | ||
|
|
||
| # Verify the signed JAR | ||
| echo "Verifying signed JAR: ${SIGNED_JARS_DIR}/$(basename "$jar")" | ||
| /usr/bin/codesign -dvv "${SIGNED_JARS_DIR}/$(basename "$jar")" | ||
|
|
||
| # Clean up temporary directory | ||
| rm -rf "$TEMP_DIR" | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize JAR processing and signing
Consider the following improvements to enhance performance, reliability, and security:
- Use parallel processing for signing multiple JARs.
- Add error checking and logging for each step.
- Use
read -rto prevent backslash mangling in filenames. - Quote variables to prevent word splitting and globbing.
Here's an optimized version:
export -f codesign_and_verify
find "$LIB_DIR" -name "*.jar" -print0 | parallel -0 process_jar {}
process_jar() {
local jar_file="$1"
echo "Processing JAR file: $jar_file"
local temp_dir
temp_dir=$(mktemp -d) || { echo "Failed to create temp directory"; return 1; }
if ! unzip -q "$jar_file" -d "$temp_dir"; then
echo "Error extracting $jar_file"
rm -rf "$temp_dir"
return 1
fi
find "$temp_dir" \( -name "*.jnilib" -o -name "*.dylib" \) -print0 |
xargs -0 -I {} bash -c 'codesign_and_verify "$@"' _ {} "$PWD/releng/com.espressif.idf.product/entitlements/espressif-ide.entitlement"
if ! (cd "$temp_dir" && zip -qr "$jar_file" .); then
echo "Error repackaging $jar_file"
rm -rf "$temp_dir"
return 1
fi
if ! codesign_and_verify "$jar_file" "$PWD/releng/com.espressif.idf.product/entitlements/espressif-ide.entitlement"; then
echo "Error signing JAR $jar_file"
rm -rf "$temp_dir"
return 1
fi
rm -rf "$temp_dir"
echo "Successfully processed $jar_file"
}
codesign_and_verify() {
local file="$1"
local entitlements="$2"
echo "Signing file: $file"
if ! /usr/bin/codesign --entitlements "$entitlements" --options runtime --force -s "ESPRESSIF SYSTEMS (SHANGHAI) CO., LTD. (QWXF6GB4AV)" --timestamp --deep "$file"; then
echo "Error signing $file"
return 1
fi
if ! /usr/bin/codesign --verify --deep --strict --verbose=2 "$file"; then
echo "Verification failed for $file"
return 1
fi
echo "Successfully signed and verified $file"
}This version uses GNU Parallel for concurrent processing, adds error checking, uses read -r, and quotes variables for improved security and reliability.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
.github/workflows/signjars.yml (1)
26-26: Fix minor formatting issues.Please address the following formatting issues:
- Remove trailing spaces from lines 26 and 75.
- Add a newline character at the end of the file (line 92).
These changes will improve the overall consistency of the file.
Also applies to: 75-75, 92-92
🧰 Tools
🪛 yamllint
[error] 26-26: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/signjars.yml (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/signjars.yml
29-29: shellcheck reported issue in this script: SC2162:info:29:64: read without -r will mangle backslashes
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:31:44: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: shellcheck reported issue in this script: SC2035:info:36:50: Use ./glob or -- glob so names with dashes won't become options
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:41:42: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
🪛 yamllint
.github/workflows/signjars.yml
[error] 26-26: trailing spaces
(trailing-spaces)
[error] 75-75: trailing spaces
(trailing-spaces)
[error] 92-92: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (4)
.github/workflows/signjars.yml (4)
1-7: Workflow name and trigger events look good.The workflow name is descriptive, and the trigger events are appropriate for signing JARs on changes to the master branch.
80-84: Checking for signed JAR files looks good.This step correctly uses the output from the previous step and provides a useful listing of the signed JAR files.
86-92: Artifact upload step is well-configured.The step uses the latest version of the upload-artifact action, correctly references the output from the signing step, and includes a condition to prevent running on job cancellation.
🧰 Tools
🪛 yamllint
[error] 92-92: no new line character at the end of file
(new-line-at-end-of-file)
9-21: 🛠️ Refactor suggestionConsider updating job name and checkout action.
While the current setup works, consider the following improvements:
- Use a more descriptive job name that reflects its purpose:
- build: + sign-jars-and-libraries:
- Update the checkout action to the latest version:
- uses: actions/checkout@v3 + uses: actions/checkout@v4These changes will improve the workflow's clarity and ensure you're using the latest features of the checkout action.
Likely invalid or redundant comment.
| MACOS_CERTIFICATE: ${{ secrets.MACOS_CERTIFICATE }} | ||
| MACOS_CERTIFICATE_PWD: ${{ secrets.MACOS_CERTIFICATE_PWD }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving certificate handling.
While using secrets for the certificate is good, consider these security enhancements:
- Use a randomly generated password for the keychain.
- Create a temporary keychain for the build.
- Set a timeout for the keychain to automatically lock after inactivity.
Here's an example of how to implement these improvements:
KEYCHAIN_NAME="build-$(uuidgen).keychain"
KEYCHAIN_PASSWORD="$(openssl rand -base64 32)"
# Create and configure keychain
security create-keychain -p "$KEYCHAIN_PASSWORD" "$KEYCHAIN_NAME"
security default-keychain -s "$KEYCHAIN_NAME"
security unlock-keychain -p "$KEYCHAIN_PASSWORD" "$KEYCHAIN_NAME"
security set-keychain-settings -t 3600 -l "$KEYCHAIN_NAME"
# Import certificate
security import certificate.p12 -k "$KEYCHAIN_NAME" -P "$MACOS_CERTIFICATE_PWD" -T /usr/bin/codesign
security set-key-partition-list -S apple-tool:,apple:,codesign: -s -k "$KEYCHAIN_PASSWORD" "$KEYCHAIN_NAME"
# Don't forget to add a step to delete the keychain after signing is completeThese changes will improve the security of the certificate handling process.
.github/workflows/signjars.yml
Outdated
| for jar in "${LIB_DIR}"/*.jar; do | ||
| echo "Processing JAR file: ${jar}" | ||
|
|
||
| # Check if the JAR exists | ||
| if [ -f "$jar" ]; then | ||
| echo "JAR file found: ${jar}" | ||
| else | ||
| echo "JAR file not found: ${jar}" | ||
| continue | ||
| fi | ||
|
|
||
| # Create a temporary directory to extract the JAR contents | ||
| TEMP_DIR=$(mktemp -d) | ||
| unzip -q "$jar" -d "$TEMP_DIR" | ||
|
|
||
| # Sign any native libraries found in the JAR | ||
| find "$TEMP_DIR" -name "*.jnilib" -o -name "*.dylib" | while read lib; do | ||
| echo "Signing native library: ${lib}" | ||
| /usr/bin/codesign -vvvv --entitlements $PWD/releng/com.espressif.idf.product/entitlements/espressif-ide.entitlement --options runtime --force -s "ESPRESSIF SYSTEMS (SHANGHAI) CO., LTD. (QWXF6GB4AV)" --timestamp --deep "$lib" | ||
| done | ||
|
|
||
| # Repackage the signed JAR and save it to SIGNED_JARS_DIR | ||
| pushd "$TEMP_DIR" | ||
| zip -r "${SIGNED_JARS_DIR}/$(basename "$jar")" * | ||
| popd | ||
|
|
||
| # Sign the entire JAR | ||
| echo "Signing repackaged JAR: ${SIGNED_JARS_DIR}/$(basename "$jar")" | ||
| /usr/bin/codesign -vvvv --entitlements $PWD/releng/com.espressif.idf.product/entitlements/espressif-ide.entitlement --force --deep --options runtime --timestamp -s "ESPRESSIF SYSTEMS (SHANGHAI) CO., LTD. (QWXF6GB4AV)" "${SIGNED_JARS_DIR}/$(basename "$jar")" | ||
|
|
||
| # Verify the signed JAR | ||
| echo "Verifying signed JAR: ${SIGNED_JARS_DIR}/$(basename "$jar")" | ||
| /usr/bin/codesign -dvv "${SIGNED_JARS_DIR}/$(basename "$jar")" | ||
|
|
||
| # Clean up the temporary extraction directory | ||
| rm -rf "$TEMP_DIR" | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve bash script for better error handling and security.
Consider the following improvements to the JAR signing process:
- Use
set -euo pipefailat the beginning of the script for better error handling. - Use
read -rto prevent backslash interpretation in filenames. - Quote variables to prevent word splitting and globbing.
- Use
findwith-print0andxargs -0for better handling of filenames with spaces.
Here's an example of how to improve the script:
set -euo pipefail
for jar in "${LIB_DIR}"/*.jar; do
if [ ! -f "$jar" ]; then
echo "JAR file not found: ${jar}"
continue
fi
TEMP_DIR=$(mktemp -d)
trap 'rm -rf "$TEMP_DIR"' EXIT
unzip -q "$jar" -d "$TEMP_DIR"
find "$TEMP_DIR" \( -name "*.jnilib" -o -name "*.dylib" \) -print0 |
xargs -0 -I {} /usr/bin/codesign -vvvv --entitlements "$PWD/releng/com.espressif.idf.product/entitlements/espressif-ide.entitlement" --options runtime --force -s "ESPRESSIF SYSTEMS (SHANGHAI) CO., LTD. (QWXF6GB4AV)" --timestamp --deep "{}"
(
cd "$TEMP_DIR" &&
zip -r "${SIGNED_JARS_DIR}/$(basename "$jar")" .
)
/usr/bin/codesign -vvvv --entitlements "$PWD/releng/com.espressif.idf.product/entitlements/espressif-ide.entitlement" --force --deep --options runtime --timestamp -s "ESPRESSIF SYSTEMS (SHANGHAI) CO., LTD. (QWXF6GB4AV)" "${SIGNED_JARS_DIR}/$(basename "$jar")"
/usr/bin/codesign -dvv "${SIGNED_JARS_DIR}/$(basename "$jar")"
doneThese changes will make the script more robust and secure.
🧰 Tools
🪛 yamllint
[error] 75-75: trailing spaces
(trailing-spaces)
.github/workflows/signjars.yml
Outdated
| # Step 1: Sign JARs and set SIGNED_JARS_DIR as output | ||
| - name: Codesign JARs and Internal Native Libraries | ||
| id: sign_jars # Assign an id to reference outputs | ||
| env: | ||
| MACOS_CERTIFICATE: ${{ secrets.MACOS_CERTIFICATE }} | ||
| MACOS_CERTIFICATE_PWD: ${{ secrets.MACOS_CERTIFICATE_PWD }} | ||
| run: | | ||
| # Create the directory for signed JARs | ||
| SIGNED_JARS_DIR="${RUNNER_TEMP}/signed-jars" | ||
| mkdir -p "$SIGNED_JARS_DIR" | ||
| echo "Signed JAR directory: ${SIGNED_JARS_DIR}" | ||
|
|
||
| # Export SIGNED_JARS_DIR as an output | ||
| echo "::set-output name=signed_jars_dir::$SIGNED_JARS_DIR" |
There was a problem hiding this comment.
Update deprecated set-output command.
The set-output command is deprecated. Replace it with the new syntax:
- echo "::set-output name=signed_jars_dir::$SIGNED_JARS_DIR"
+ echo "signed_jars_dir=$SIGNED_JARS_DIR" >> $GITHUB_OUTPUTThis change ensures compatibility with future versions of GitHub Actions.
📝 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.
| # Step 1: Sign JARs and set SIGNED_JARS_DIR as output | |
| - name: Codesign JARs and Internal Native Libraries | |
| id: sign_jars # Assign an id to reference outputs | |
| env: | |
| MACOS_CERTIFICATE: ${{ secrets.MACOS_CERTIFICATE }} | |
| MACOS_CERTIFICATE_PWD: ${{ secrets.MACOS_CERTIFICATE_PWD }} | |
| run: | | |
| # Create the directory for signed JARs | |
| SIGNED_JARS_DIR="${RUNNER_TEMP}/signed-jars" | |
| mkdir -p "$SIGNED_JARS_DIR" | |
| echo "Signed JAR directory: ${SIGNED_JARS_DIR}" | |
| # Export SIGNED_JARS_DIR as an output | |
| echo "::set-output name=signed_jars_dir::$SIGNED_JARS_DIR" | |
| # Step 1: Sign JARs and set SIGNED_JARS_DIR as output | |
| - name: Codesign JARs and Internal Native Libraries | |
| id: sign_jars # Assign an id to reference outputs | |
| env: | |
| MACOS_CERTIFICATE: ${{ secrets.MACOS_CERTIFICATE }} | |
| MACOS_CERTIFICATE_PWD: ${{ secrets.MACOS_CERTIFICATE_PWD }} | |
| run: | | |
| # Create the directory for signed JARs | |
| SIGNED_JARS_DIR="${RUNNER_TEMP}/signed-jars" | |
| mkdir -p "$SIGNED_JARS_DIR" | |
| echo "Signed JAR directory: ${SIGNED_JARS_DIR}" | |
| # Export SIGNED_JARS_DIR as an output | |
| echo "signed_jars_dir=$SIGNED_JARS_DIR" >> $GITHUB_OUTPUT |
🧰 Tools
🪛 actionlint
29-29: shellcheck reported issue in this script: SC2162:info:29:64: read without -r will mangle backslashes
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:31:44: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: shellcheck reported issue in this script: SC2035:info:36:50: Use ./glob or -- glob so names with dashes won't become options
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:41:42: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
🪛 yamllint
[error] 26-26: trailing spaces
(trailing-spaces)
kolipakakondal
force-pushed
the
signjars
branch
from
d07df7d
to
777aef2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
.github/workflows/signjars.yml (1)
1-90: Address static analysis issues.Please address the following issues reported by static analysis tools:
Quote variables in shell commands to prevent word splitting and globbing. For example:
find "$TEMP_DIR" -name "*.jnilib" -o -name "*.dylib" | while read -r lib; doRemove trailing spaces (line 24) and add a newline at the end of the file.
Use
read -rinstead ofreadto prevent backslash mangling.To automatically fix these issues, consider using tools like
shfmtfor shell script formatting and a YAML linter with auto-fix capabilities.🧰 Tools
🪛 actionlint
27-27: shellcheck reported issue in this script: SC2086:info:2:6: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:6:63: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2162:info:31:64: read without -r will mangle backslashes
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:33:44: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2035:info:38:50: Use ./glob or -- glob so names with dashes won't become options
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:43:42: Double quote to prevent globbing and word splitting
(shellcheck)
81-81: shellcheck reported issue in this script: SC2086:info:2:8: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
[error] 24-24: trailing spaces
(trailing-spaces)
[error] 90-90: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/signjars.yml (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/signjars.yml
27-27: shellcheck reported issue in this script: SC2086:info:2:6: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:6:63: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2162:info:31:64: read without -r will mangle backslashes
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:33:44: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2035:info:38:50: Use ./glob or -- glob so names with dashes won't become options
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:43:42: Double quote to prevent globbing and word splitting
(shellcheck)
81-81: shellcheck reported issue in this script: SC2086:info:2:8: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
.github/workflows/signjars.yml
[error] 24-24: trailing spaces
(trailing-spaces)
[error] 90-90: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (3)
.github/workflows/signjars.yml (3)
1-8: LGTM: Workflow name and trigger configuration are appropriate.The workflow name clearly describes its purpose, and the trigger configuration is set up correctly for the master branch.
80-90: LGTM: Artifact upload step is well-implemented.The artifact upload step is correctly configured:
- It checks for the existence of signed JAR files before uploading.
- It uses the latest version of the upload-artifact action (v4).
- The conditional
if: ${{ !cancelled() }}ensures that the upload only occurs if the job hasn't been cancelled.🧰 Tools
🪛 actionlint
81-81: shellcheck reported issue in this script: SC2086:info:2:8: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
[error] 90-90: no new line character at the end of file
(new-line-at-end-of-file)
9-21: 🛠️ Refactor suggestionConsider updating the job name and checkout action.
- The job name "build" could be more descriptive. Consider changing it to something like "sign-jars-and-libraries" to better reflect its purpose.
- Update the checkout action to v4 for the latest features and bug fixes:
- uses: actions/checkout@v3 + uses: actions/checkout@v4Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
.github/workflows/signjars.yml (2)
94-99: LGTM: Artifact upload step is well-implemented.The artifact upload step is correctly implemented and uses the latest version of the action. The conditional execution ensures that artifacts are only uploaded if the job completes successfully.
Minor YAML improvement: Add a newline at the end of the file to satisfy YAML best practices.
name: signed-jar-files path: ${{ runner.temp }}/signed-jars/* +🧰 Tools
🪛 yamllint
[error] 99-99: no new line character at the end of file
(new-line-at-end-of-file)
1-99: Summary: Functional workflow with room for optimizationThis GitHub Actions workflow for signing JAR files and internal native libraries is functional but has room for improvement. Key areas for enhancement include:
- Updating the job name and checkout action version.
- Improving keychain management for better security.
- Optimizing JAR processing and signing for better performance and reliability.
- Minor YAML improvements.
Implementing these suggestions will result in a more robust, secure, and efficient workflow. The verification and artifact upload steps are well-implemented and don't require significant changes.
Consider breaking down this workflow into smaller, reusable composite actions. This would improve maintainability and allow for easier testing of individual components of the signing process.
🧰 Tools
🪛 actionlint
27-27: shellcheck reported issue in this script: SC2086:info:2:6: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:6:63: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2162:info:31:64: read without -r will mangle backslashes
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:33:44: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2035:info:38:50: Use ./glob or -- glob so names with dashes won't become options
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:43:42: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
[error] 24-24: trailing spaces
(trailing-spaces)
[error] 79-79: trailing spaces
(trailing-spaces)
[error] 99-99: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/signjars.yml (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/signjars.yml
27-27: shellcheck reported issue in this script: SC2086:info:2:6: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:6:63: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2162:info:31:64: read without -r will mangle backslashes
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:33:44: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2035:info:38:50: Use ./glob or -- glob so names with dashes won't become options
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:43:42: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
.github/workflows/signjars.yml
[error] 24-24: trailing spaces
(trailing-spaces)
[error] 79-79: trailing spaces
(trailing-spaces)
[error] 99-99: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (2)
.github/workflows/signjars.yml (2)
1-8: LGTM: Workflow name and trigger events are well-defined.The workflow name accurately describes its purpose, and the trigger events are appropriately set for the master branch.
80-92: LGTM: Verification and display steps are well-implemented.The steps to verify the signed JAR files and display their contents are well-implemented. These steps are crucial for ensuring the signing process was successful and provide valuable information for debugging if issues arise.
| - name: Codesign JARs and Internal Native Libraries | ||
| env: | ||
| MACOS_CERTIFICATE: ${{ secrets.MACOS_CERTIFICATE }} | ||
| MACOS_CERTIFICATE_PWD: ${{ secrets.MACOS_CERTIFICATE_PWD }} | ||
| run: | | ||
| # Step 1: Decode and import the certificate into a keychain | ||
| echo $MACOS_CERTIFICATE | base64 --decode > certificate.p12 | ||
| /usr/bin/security create-keychain -p espressif build.keychain | ||
| /usr/bin/security default-keychain -s build.keychain | ||
| /usr/bin/security unlock-keychain -p espressif build.keychain | ||
| /usr/bin/security import certificate.p12 -k build.keychain -P $MACOS_CERTIFICATE_PWD -T /usr/bin/codesign | ||
| /usr/bin/security set-key-partition-list -S apple-tool:,apple:,codesign: -s -k espressif build.keychain |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve keychain management for better security.
Consider the following enhancements to the keychain management process:
- Use a randomly generated password for the keychain instead of a hardcoded one.
- Create a unique name for the keychain to avoid conflicts.
- Set a timeout for the keychain to automatically lock after inactivity.
Here's an improved version of the keychain setup:
KEYCHAIN_NAME="build-$(uuidgen).keychain"
KEYCHAIN_PASSWORD="$(openssl rand -base64 32)"
# Create and configure keychain
security create-keychain -p "$KEYCHAIN_PASSWORD" "$KEYCHAIN_NAME"
security default-keychain -s "$KEYCHAIN_NAME"
security unlock-keychain -p "$KEYCHAIN_PASSWORD" "$KEYCHAIN_NAME"
security set-keychain-settings -t 3600 -l "$KEYCHAIN_NAME"
# Import certificate
security import certificate.p12 -k "$KEYCHAIN_NAME" -P "$MACOS_CERTIFICATE_PWD" -T /usr/bin/codesign
security set-key-partition-list -S apple-tool:,apple:,codesign: -s -k "$KEYCHAIN_PASSWORD" "$KEYCHAIN_NAME"
# Don't forget to add a step to delete the keychain after signing is completeThis approach improves security by using a unique, randomly generated password and setting an inactivity timeout.
🧰 Tools
🪛 actionlint
27-27: shellcheck reported issue in this script: SC2086:info:2:6: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:6:63: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2162:info:31:64: read without -r will mangle backslashes
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:33:44: Double quote to prevent globbing and word splitting
(shellcheck)
27-27: shellcheck reported issue in this script: SC2035:info:38:50: Use ./glob or -- glob so names with dashes won't become options
(shellcheck)
27-27: shellcheck reported issue in this script: SC2086:info:43:42: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
[error] 24-24: trailing spaces
(trailing-spaces)
.github/workflows/signjars.yml
Outdated
| # Step 2: Define the directory containing the JARs and native libraries and the temp directory for signed JARs | ||
| LIB_DIR="${PWD}/BUNDLES/com.espressif.idf.serial.monitor/lib" | ||
| SIGNED_JARS_DIR="${RUNNER_TEMP}/signed-jars" # Use GitHub's RUNNER_TEMP for storing signed JARs | ||
| mkdir -p "$SIGNED_JARS_DIR" | ||
|
|
||
| # Step 3: Extract, sign native libraries, repackage, and sign the JARs with Apple codesign | ||
| for jar in "${LIB_DIR}"/*.jar; do | ||
| echo "Processing JAR file: ${jar}" | ||
|
|
||
| # Check if the JAR exists | ||
| if [ -f "$jar" ]; then | ||
| echo "JAR file found: ${jar}" | ||
| else | ||
| echo "JAR file not found: ${jar}" | ||
| continue | ||
| fi | ||
|
|
||
| # Create a temporary directory to extract the JAR contents | ||
| TEMP_DIR=$(mktemp -d) | ||
| unzip -q "$jar" -d "$TEMP_DIR" | ||
|
|
||
| # Find and sign all .jnilib and .dylib files in the extracted JAR directory | ||
| find "$TEMP_DIR" -name "*.jnilib" -o -name "*.dylib" | while read lib; do | ||
| echo "Signing native library: ${lib}" | ||
| /usr/bin/codesign -vvvv --entitlements $PWD/releng/com.espressif.idf.product/entitlements/espressif-ide.entitlement --options runtime --force -s "ESPRESSIF SYSTEMS (SHANGHAI) CO., LTD. (QWXF6GB4AV)" --timestamp --deep "$lib" | ||
| done | ||
|
|
||
| # Repackage the signed JAR | ||
| pushd "$TEMP_DIR" | ||
| zip -r "${SIGNED_JARS_DIR}/$(basename "$jar")" * # Save signed JAR to the temporary signed directory | ||
| popd | ||
|
|
||
| # Sign the entire JAR with Apple codesign, using the same entitlements | ||
| echo "Signing repackaged JAR: ${SIGNED_JARS_DIR}/$(basename "$jar")" | ||
| /usr/bin/codesign -vvvv --entitlements $PWD/releng/com.espressif.idf.product/entitlements/espressif-ide.entitlement --force --deep --options runtime --timestamp -s "ESPRESSIF SYSTEMS (SHANGHAI) CO., LTD. (QWXF6GB4AV)" "${SIGNED_JARS_DIR}/$(basename "$jar")" | ||
|
|
||
| # Verify the signed JAR | ||
| echo "Verifying signed JAR: ${SIGNED_JARS_DIR}/$(basename "$jar")" | ||
| /usr/bin/codesign -dvv "${SIGNED_JARS_DIR}/$(basename "$jar")" | ||
|
|
||
| # Clean up extracted directory (but leave the signed JAR in SIGNED_JARS_DIR) | ||
| rm -rf "$TEMP_DIR" | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize JAR processing and signing.
The current implementation works, but it can be improved for better performance, reliability, and security:
- Use parallel processing for signing multiple JARs.
- Add error checking and logging for each step.
- Use
read -rto prevent backslash mangling in filenames. - Quote variables to prevent word splitting and globbing.
Here's an optimized version of the JAR processing logic:
export -f codesign_and_verify
find "$LIB_DIR" -name "*.jar" -print0 | parallel -0 process_jar {}
process_jar() {
local jar_file="$1"
echo "Processing JAR file: $jar_file"
local temp_dir
temp_dir=$(mktemp -d) || { echo "Failed to create temp directory"; return 1; }
if ! unzip -q "$jar_file" -d "$temp_dir"; then
echo "Error extracting $jar_file"
rm -rf "$temp_dir"
return 1
fi
find "$temp_dir" \( -name "*.jnilib" -o -name "*.dylib" \) -print0 |
xargs -0 -I {} bash -c 'codesign_and_verify "$@"' _ {} "$PWD/releng/com.espressif.idf.product/entitlements/espressif-ide.entitlement"
if ! (cd "$temp_dir" && zip -qr "$jar_file" .); then
echo "Error repackaging $jar_file"
rm -rf "$temp_dir"
return 1
fi
if ! codesign_and_verify "$jar_file" "$PWD/releng/com.espressif.idf.product/entitlements/espressif-ide.entitlement"; then
echo "Error signing JAR $jar_file"
rm -rf "$temp_dir"
return 1
fi
rm -rf "$temp_dir"
echo "Successfully processed $jar_file"
}
codesign_and_verify() {
local file="$1"
local entitlements="$2"
echo "Signing file: $file"
if ! /usr/bin/codesign --entitlements "$entitlements" --options runtime --force -s "ESPRESSIF SYSTEMS (SHANGHAI) CO., LTD. (QWXF6GB4AV)" --timestamp --deep "$file"; then
echo "Error signing $file"
return 1
fi
if ! /usr/bin/codesign --verify --deep --strict --verbose=2 "$file"; then
echo "Verification failed for $file"
return 1
fi
echo "Successfully signed and verified $file"
}This version uses GNU Parallel for concurrent processing, adds error checking, uses read -r, and quotes variables for improved security and reliability.
| jobs: | ||
| build: | ||
| runs-on: macos-latest | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Set up JDK 17 | ||
| uses: actions/setup-java@v3 | ||
| with: | ||
| java-version: '17' | ||
| distribution: 'temurin' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider updating job name and checkout action.
While the current setup works, consider the following improvements:
- Use a more descriptive job name that reflects its purpose, e.g., "sign-and-verify-jars".
- Update the checkout action to the latest version (v4) for potential improvements and bug fixes.
Here's the suggested change:
jobs:
- build:
+ sign-and-verify-jars:
runs-on: macos-latest
steps:
- name: Checkout code
- uses: actions/checkout@v3
+ uses: actions/checkout@v4These changes will improve the workflow's clarity and ensure you're using the latest features of the checkout action.
📝 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.
| jobs: | |
| build: | |
| runs-on: macos-latest | |
| steps: | |
| - name: Checkout code | |
| uses: actions/checkout@v3 | |
| - name: Set up JDK 17 | |
| uses: actions/setup-java@v3 | |
| with: | |
| java-version: '17' | |
| distribution: 'temurin' | |
| jobs: | |
| sign-and-verify-jars: | |
| runs-on: macos-latest | |
| steps: | |
| - name: Checkout code | |
| uses: actions/checkout@v4 | |
| - name: Set up JDK 17 | |
| uses: actions/setup-java@v3 | |
| with: | |
| java-version: '17' | |
| distribution: 'temurin' |
Description
Use this PR for Apple code signing of internal libraries, as it is mandatory for notarization approval.
This will produce the signed artifacts, download them and replace them in the plugins.