Skip to content

[WIP] Harden training script: credentials validation, error handling, security fixes #432

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
EZoni wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[WIP] Harden training script: credentials validation, error handling, security fixes #432

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ff6d8f
Harden training script: credentials validation, error handling, secur…
EZoni May 1, 2026
c81cb53
Add FIXME comment to training batch script
EZoni May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 25 additions & 8 deletions ml/training_pm.sbatch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#!/bin/bash -l
set -euo pipefail

@EZoni EZoni May 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

-e exits on error, -u catches unbound variables, and -o pipefail catches failures mid-pipeline.


#SBATCH -t 00:15:00
#SBATCH -N 1
Expand All @@ -19,13 +20,29 @@ model=${1} # e.g., "NN", "GP", etc.
# Ids, but should compare image Digests.
# We did not find a way to compare digests between the docker registry
# and the local podman-hpc images (they change, conversion?)
SECONDS=0 # built-in bash timer: reset
source $HOME/registry.profile # credential variables: REGISTRY_USER and REGISTRY_PASSWORD
SECONDS=0 # built-in bash timer: reset
REGISTRY_PROFILE="$HOME/registry.profile"
DB_ENV_FILE="$HOME/db-podman.profile"
REGISTRY_NAME="registry.nersc.gov"
IMAGE_NAME="m558/superfacility/synapse-ml"
IMAGE_VERSION="latest"

podman-hpc login --username "${REGISTRY_USER}" --password "${REGISTRY_PASSWORD}" ${REGISTRY_NAME}
if [ ! -r "${REGISTRY_PROFILE}" ]; then
echo "Missing registry credentials file: ${REGISTRY_PROFILE}" >&2
echo "Create it with REGISTRY_USER and REGISTRY_PASSWORD for ${REGISTRY_NAME}." >&2
exit 1
fi
source "${REGISTRY_PROFILE}" # credential variables: REGISTRY_USER and REGISTRY_PASSWORD
: "${REGISTRY_USER:?REGISTRY_USER must be set in ${REGISTRY_PROFILE}}"
: "${REGISTRY_PASSWORD:?REGISTRY_PASSWORD must be set in ${REGISTRY_PROFILE}}"

if [ ! -r "${DB_ENV_FILE}" ]; then
echo "Missing container environment file: ${DB_ENV_FILE}" >&2
echo "Create it with SF_DB_READONLY_PASSWORD and AM_SC_API_KEY." >&2
exit 1
fi
Comment on lines +34 to +47

@EZoni EZoni May 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Failing fast with a clear error message beats a cryptic "unbound variable" or silent auth failure later.


printf '%s\n' "${REGISTRY_PASSWORD}" | podman-hpc login --username "${REGISTRY_USER}" --password-stdin "${REGISTRY_NAME}"

@EZoni EZoni May 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Passwords passed as arguments are visible in ps output and shell history; reading from stdin avoids this. Using printf '%s\n' is also slightly better than echo since echo behavior with special characters can be shell-dependent.

# As the local image, we use the digest id.
# podman-hpc lists to entries with the same digest id,
# one local and one migrated (read-write and read-only).
Expand All @@ -35,21 +52,21 @@ LOCAL_SHA="sha256:"$(podman-hpc images --digests --format "{{.Id}}" ${REGISTRY_N
OCI_INDEX_TYPE="application/vnd.oci.image.index.v1+json"
OCI_MANIFEST_TYPE="application/vnd.oci.image.manifest.v1+json"
# first we unwarp the provenance information to pick the right image manifest
MANIFEST_INDEX=$(curl -s \
MANIFEST_INDEX=$(curl -fsS \

@EZoni EZoni May 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

-f makes curl return a non-zero exit code on HTTP errors (4xx/5xx), and -S shows error messages even in silent mode. Without -f, a 401 or 404 would silently return an error document and let the script continue.

-H "Accept: ${OCI_INDEX_TYPE}" \
-u "${REGISTRY_USER}:${REGISTRY_PASSWORD}" \
"https://${REGISTRY_NAME}/v2/${IMAGE_NAME}/manifests/${IMAGE_VERSION}" | \
Comment on lines +59 to 62

@EZoni EZoni May 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was pre-existing and is unrelated to the changes in this PR.

jq -r '.manifests[] | select(.annotations["vnd.docker.reference.type"] != "attestation-manifest") | .digest' || echo "NOOCI")
jq -er '.manifests[] | select(.annotations["vnd.docker.reference.type"] != "attestation-manifest") | .digest' || echo "NOOCI")
# fallback for old Docker builders without OCI index
Comment on lines +59 to 64

@EZoni EZoni May 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The original .manifests[] would cause jq to error if the manifests key was absent from the JSON response. For example, when the registry returns a plain Docker V2 manifest instead of an OCI index. The // [] fallback means jq gracefully produces no output rather than erroring, which allows the || echo "NOOCI" fallback to trigger correctly and the script to continue to the Docker V2 code path.

Suggested change
MANIFEST_INDEX=$(curl -fsS \
-H "Accept: ${OCI_INDEX_TYPE}" \
-u "${REGISTRY_USER}:${REGISTRY_PASSWORD}" \
"https://${REGISTRY_NAME}/v2/${IMAGE_NAME}/manifests/${IMAGE_VERSION}" | \
jq -r '.manifests[] | select(.annotations["vnd.docker.reference.type"] != "attestation-manifest") | .digest' || echo "NOOCI")
jq -er '.manifests[] | select(.annotations["vnd.docker.reference.type"] != "attestation-manifest") | .digest' || echo "NOOCI")
# fallback for old Docker builders without OCI index
MANIFEST_INDEX=$(curl -fsS \
-H "Accept: ${OCI_INDEX_TYPE}" \
-u "${REGISTRY_USER}:${REGISTRY_PASSWORD}" \
"https://${REGISTRY_NAME}/v2/${IMAGE_NAME}/manifests/${IMAGE_VERSION}" | \
jq -er '(.manifests // [])[] | select(.annotations["vnd.docker.reference.type"] != "attestation-manifest") | .digest' || echo "NOOCI")
# fallback for old Docker builders without OCI index

if [ "${MANIFEST_INDEX}" == "NOOCI" ]; then
MANIFEST_INDEX=${IMAGE_VERSION}
fi
# then we download the manifest of the image and parse out the config digest SHA
REMOTE_SHA=$(curl -s \
REMOTE_SHA=$(curl -fsS \

@EZoni EZoni May 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

-f makes curl return a non-zero exit code on HTTP errors (4xx/5xx), and -S shows error messages even in silent mode. Without -f, a 401 or 404 would silently return an error document and let the script continue.

-H "Accept: ${OCI_MANIFEST_TYPE}" \
-u "${REGISTRY_USER}:${REGISTRY_PASSWORD}" \
"https://${REGISTRY_NAME}/v2/${IMAGE_NAME}/manifests/${MANIFEST_INDEX}" | \
jq -r '.config.digest')
jq -er '.config.digest')

@EZoni EZoni May 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

jq -er instead of -r adds -e, which makes jq exit non-zero if the output is false or null. Combined with set -e and curl -f, this closes a gap where a malformed or unexpected JSON response could let a null digest pass through and cause a misleading "IDs are different" message or worse.


if [ "$LOCAL_SHA" != "$REMOTE_SHA" ]; then
echo "Ids are different."
Expand All @@ -67,6 +84,6 @@ echo "Container check/pull took ${SECONDS} seconds."
srun podman-hpc run --gpu \
-v /etc/localtime:/etc/localtime \
-v /global/cfs/cdirs/m558/superfacility/model_training/config.yaml:/app/ml/config.yaml \
--env-file $HOME/db-podman.profile \
--env-file "${DB_ENV_FILE}" \

@EZoni EZoni May 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Quoting ${DB_ENV_FILE} in --env-file is a minor but correct fix for paths with spaces.

--rm -it ${REGISTRY_NAME}/${IMAGE_NAME}:${IMAGE_VERSION} \
python -u /app/ml/train_model.py --config_file /app/ml/config.yaml --model ${model}
Comment on lines 92 to 93
Loading