Certz-2: Changes to create certificates and uploading the certificates - #6094
priyaranjannanda wants to merge 1 commit into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the reliability and portability of the certificate generation script used for test data. By enforcing absolute paths, removing dependencies on system-level OpenSSL configurations, and explicitly defining certificate extensions, the script now produces consistent and valid test certificates across different environments. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the mk_cas.sh script to improve argument parsing, enforce safer shell options (set -eu), and cleanly generate CA certificates using explicit OpenSSL configurations rather than relying on system defaults. The review feedback recommends quoting all instances of ${OUTDIR} to prevent word splitting issues with paths containing spaces, and suggests defining an EXT_FILE variable to simplify the certificate generation logic and avoid spawning unnecessary subshells.
| openssl req -new -nodes \ | ||
| -key ${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-key.pem \ | ||
| -out ${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-cert.pem \ | ||
| -out ${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-req.pem \ | ||
| -subj "/CN=CA ${OFFSET}/C=AQ/ST=NZ/L=NZ/O=OpenConfigFeatureProfiles" | ||
|
|
||
| openssl x509 -req -days ${LIFETIME} \ | ||
| -in ${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-req.pem \ | ||
| -signkey ${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-key.pem \ | ||
| -out ${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-cert.pem \ | ||
| -sha256 \ | ||
| -extfile <(printf "basicConstraints=critical,CA:TRUE\nkeyUsage=critical,keyCertSign,cRLSign") | ||
|
|
||
| rm -f ${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-req.pem |
There was a problem hiding this comment.
The variable ${OUTDIR} is not quoted in several places within this block. If the output directory path contains spaces, the script will fail due to word splitting. Quoting all instances of ${OUTDIR} ensures robustness.
| openssl req -new -nodes \ | |
| -key ${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-key.pem \ | |
| -out ${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-cert.pem \ | |
| -out ${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-req.pem \ | |
| -subj "/CN=CA ${OFFSET}/C=AQ/ST=NZ/L=NZ/O=OpenConfigFeatureProfiles" | |
| openssl x509 -req -days ${LIFETIME} \ | |
| -in ${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-req.pem \ | |
| -signkey ${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-key.pem \ | |
| -out ${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-cert.pem \ | |
| -sha256 \ | |
| -extfile <(printf "basicConstraints=critical,CA:TRUE\nkeyUsage=critical,keyCertSign,cRLSign") | |
| rm -f ${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-req.pem | |
| openssl req -new -nodes \ | |
| -key "${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-key.pem" \ | |
| -out "${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-req.pem" \ | |
| -subj "/CN=CA ${OFFSET}/C=AQ/ST=NZ/L=NZ/O=OpenConfigFeatureProfiles" | |
| openssl x509 -req -days ${LIFETIME} \ | |
| -in "${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-req.pem" \ | |
| -signkey "${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-key.pem" \ | |
| -out "${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-cert.pem" \ | |
| -sha256 \ | |
| -extfile <(printf "basicConstraints=critical,CA:TRUE\nkeyUsage=critical,keyCertSign,cRLSign") | |
| rm -f "${OUTDIR}/ca-${d}/ca-${OFFSET}-${t}-req.pem" |
| if [ "${cs}" = "client" ]; then | ||
| openssl req -new -key ${OUTDIR}/ca-${d}/${cs}-${t}-${g}-key.pem \ | ||
| -out ${OUTDIR}/ca-${d}/${cs}-${t}-${g}-req.pem \ | ||
| -config "${CLIENT_CNF}" | ||
| else | ||
| openssl req -new -key ${OUTDIR}/ca-${d}/${cs}-${t}-${g}-key.pem \ | ||
| -out ${OUTDIR}/ca-${d}/${cs}-${t}-${g}-req.pem \ | ||
| -config "${SERVER_CNF}" | ||
| fi |
There was a problem hiding this comment.
To improve readability and avoid spawning a subshell inside the openssl x509 arguments, we can define an EXT_FILE variable within this if/else block. Additionally, quoting the path variables prevents potential issues with spaces in the output directory path.
| if [ "${cs}" = "client" ]; then | |
| openssl req -new -key ${OUTDIR}/ca-${d}/${cs}-${t}-${g}-key.pem \ | |
| -out ${OUTDIR}/ca-${d}/${cs}-${t}-${g}-req.pem \ | |
| -config "${CLIENT_CNF}" | |
| else | |
| openssl req -new -key ${OUTDIR}/ca-${d}/${cs}-${t}-${g}-key.pem \ | |
| -out ${OUTDIR}/ca-${d}/${cs}-${t}-${g}-req.pem \ | |
| -config "${SERVER_CNF}" | |
| fi | |
| if [ "${cs}" = "client" ]; then | |
| EXT_FILE="${CLIENT_EXT}" | |
| openssl req -new -key "${OUTDIR}/ca-${d}/${cs}-${t}-${g}-key.pem" \ | |
| -out "${OUTDIR}/ca-${d}/${cs}-${t}-${g}-req.pem" \ | |
| -config "${CLIENT_CNF}" | |
| else | |
| EXT_FILE="${SERVER_EXT}" | |
| openssl req -new -key "${OUTDIR}/ca-${d}/${cs}-${t}-${g}-key.pem" \ | |
| -out "${OUTDIR}/ca-${d}/${cs}-${t}-${g}-req.pem" \ | |
| -config "${SERVER_CNF}" | |
| fi |
| -days ${LIFETIME} \ | ||
| -sha256 \ | ||
| -extfile ${cs}_cert_ext.cnf | ||
| -extfile "$( [ "${cs}" = "client" ] && printf '%s' "${CLIENT_EXT}" || printf '%s' "${SERVER_EXT}" )" |
There was a problem hiding this comment.
Using the EXT_FILE variable defined in the previous block simplifies this command and avoids spawning a subshell.
| -extfile "$( [ "${cs}" = "client" ] && printf '%s' "${CLIENT_EXT}" || printf '%s' "${SERVER_EXT}" )" | |
| -extfile "${EXT_FILE}" |
This branch has not been deployed
2 participants
mk_cas.sh
Resolves its own directory with SCRIPT_DIR, so certificate config files work regardless of the current working directory.
Supports:
./mk_cas.sh
./mk_cas.sh /tmp/output
./mk_cas.sh "01,02,10" /tmp/output
Resolves OUTDIR to an absolute path.
Uses absolute paths for client/server OpenSSL config and extension files.
Restores proper CA extensions:
basicConstraints=critical,CA:TRUE
keyUsage=critical,keyCertSign,cRLSign
Generates valid server/client certificates and trust bundles in the requested output directory.
Uses the selected CA list instead of always generating every default set.