keygeneration: clean up for UEFI key generation code

Follow up the code review comments on CL:995174, which was merged as
7dff0105d66fa597741604cf1652a72c7a8463ac
("keygeneration: add support for UEFI key generation")

BUG=b:62189155
TEST=With CL:*613656, set up a local signer and tested key generation
     and signing.
     Also, manually ran the scripts like the following.

     $ export PATH=$(readlink -f ../../../cros-signing/signer/signingtools-bin):$PATH
     $ cd scripts/keygeneration && ./create_new_keys.sh --uefi --output ./key
     $ chmod -R u+w key/uefi
     $ ./uefi/increment_kek_key.sh key/uefi
     $ ./uefi/increment_kek_key.sh key/uefi
     $ ./uefi/increment_db_child_key.sh key/uefi
     $ ./uefi/increment_db_child_key.sh key/uefi
     $ ./uefi/increment_db_child_key.sh key/uefi
     $ ./uefi/increment_db_key.sh key/uefi
     $ ./uefi/increment_db_child_key.sh key/uefi
     $ ./uefi/increment_db_key.sh key/uefi
     $ ./uefi/increment_db_child_key.sh key/uefi
     $ ./uefi/increment_db_child_key.sh key/uefi
     $ openssl x509 -noout -subject -in key/uefi/db/db.children/db_child.pem

BRANCH=none

Change-Id: I6c0cd47914a0a77970cd074fe087bba33c16cffc
Reviewed-on: https://chromium-review.googlesource.com/1024918
Commit-Ready: Edward Jee <edjee@google.com>
Tested-by: Edward Jee <edjee@google.com>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

6 files changed, 80 insertions, 46 deletions

diff --git a/scripts/keygeneration/uefi/create_new_uefi_keys.sh b/scripts/keygeneration/uefi/create_new_uefi_keys.sh
index 6f86382d..5a57b2f3 100755
--- a/scripts/keygeneration/uefi/create_new_uefi_keys.sh
+++ b/scripts/keygeneration/uefi/create_new_uefi_keys.sh
@@ -1,5 +1,4 @@
 #!/bin/bash
-
 # Copyright 2018 The Chromium OS Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
@@ -45,13 +44,11 @@ main() {
   local dir="$1"
 
   check_uefi_key_dir_name "${dir}"
-  pushd "${dir}" > /dev/null
+  pushd "${dir}" >/dev/null || die "Wrong output directory name"
 
   if [[ ! -e "${UEFI_VERSION_FILE}" ]]; then
     echo "No version file found. Creating default ${UEFI_VERSION_FILE}."
-    (
-      printf '%s_key_version=1\n' {pk,kek,db,db_child}
-    ) > "${UEFI_VERSION_FILE}"
+    printf '%s_key_version=1\n' {pk,kek,db,db_child} > "${UEFI_VERSION_FILE}"
   fi
 
   local pk_key_version kek_key_version db_key_version db_child_key_version
@@ -67,7 +64,7 @@ main() {
   make_db_keypair "${db_key_version}"
   make_db_child_keypair "${db_key_version}" "${db_child_key_version}"
 
-  popd > /dev/null
+  popd >/dev/null
 }
 
 main "$@"
diff --git a/scripts/keygeneration/uefi/increment_db_child_key.sh b/scripts/keygeneration/uefi/increment_db_child_key.sh
index b7cc53b1..fe206f82 100755
--- a/scripts/keygeneration/uefi/increment_db_child_key.sh
+++ b/scripts/keygeneration/uefi/increment_db_child_key.sh
@@ -30,7 +30,7 @@ main() {
                                                 "db_child_key_version")
 
   cd "${KEY_DIR}"
-  backup_existing_db_child_keypair "${CURR_DB_CHILD_KEY_VER}"
+  backup_db_child_keypair "${CURR_DB_CHILD_KEY_VER}"
 
   cat <<EOF
 Generating new UEFI DB child key version.
diff --git a/scripts/keygeneration/uefi/increment_db_key.sh b/scripts/keygeneration/uefi/increment_db_key.sh
index a3f3e5fb..b21454c4 100755
--- a/scripts/keygeneration/uefi/increment_db_key.sh
+++ b/scripts/keygeneration/uefi/increment_db_key.sh
@@ -30,7 +30,7 @@ main() {
   new_db_child_key_ver=1
 
   cd "${KEY_DIR}"
-  backup_existing_db_keypair_and_children "${CURR_DB_KEY_VER}"
+  backup_db_keypair_and_children "${CURR_DB_KEY_VER}"
 
   cat <<EOF
 Generating new UEFI DB key version.
diff --git a/scripts/keygeneration/uefi/increment_kek_key.sh b/scripts/keygeneration/uefi/increment_kek_key.sh
index e99fd70c..0c813bae 100755
--- a/scripts/keygeneration/uefi/increment_kek_key.sh
+++ b/scripts/keygeneration/uefi/increment_kek_key.sh
@@ -29,7 +29,7 @@ main() {
   new_kek_key_ver=$(increment_uefi_version "${KEY_DIR}" "kek_key_version")
 
   cd "${KEY_DIR}"
-  backup_existing_kek_keypair "${CURR_KEK_KEY_VER}"
+  backup_kek_keypair "${CURR_KEK_KEY_VER}"
 
   cat <<EOF
 Generating new UEFI Key Exchange Key (KEK) version.
diff --git a/scripts/keygeneration/uefi/increment_pk_key.sh b/scripts/keygeneration/uefi/increment_pk_key.sh
index 206b2ba0..75442e75 100755
--- a/scripts/keygeneration/uefi/increment_pk_key.sh
+++ b/scripts/keygeneration/uefi/increment_pk_key.sh
@@ -29,7 +29,7 @@ main() {
   new_pk_key_ver=$(increment_uefi_version "${KEY_DIR}" "pk_key_version")
 
   cd "${KEY_DIR}"
-  backup_existing_pk_keypair "${CURR_PK_KEY_VER}"
+  backup_pk_keypair "${CURR_PK_KEY_VER}"
 
   cat <<EOF
 Generating new UEFI Platform Key (PK) version.
diff --git a/scripts/keygeneration/uefi/uefi_common.sh b/scripts/keygeneration/uefi/uefi_common.sh
index e35a20f2..87585450 100644
--- a/scripts/keygeneration/uefi/uefi_common.sh
+++ b/scripts/keygeneration/uefi/uefi_common.sh
@@ -7,6 +7,9 @@
 
 . "$(dirname "$0")/../common.sh"
 
+# Checks whether the given key directory name is "uefi".
+# Dies if it isn't.
+# ARGS: KEY_DIR
 check_uefi_key_dir_name() {
   local key_dir="$1"
   local key_dir_fullpath="$(readlink -f "${key_dir}")"
@@ -19,6 +22,7 @@ check_uefi_key_dir_name() {
 # File to read current versions from.
 UEFI_VERSION_FILE="uefi_key.versions"
 
+# Prints the version value for the given VERSION_TYPE, from UEFI_VERSION_FILE.
 # ARGS: <VERSION_TYPE> [UEFI_VERSION_FILE]
 get_uefi_version() {
   local key="$1"
@@ -29,6 +33,7 @@ get_uefi_version() {
 # Loads the current versions, prints them to stdout, and sets the global version
 # variables: CURR_PK_KEY_VER CURR_KEK_KEY_VER CURR_DB_KEY_VER
 # CURR_DB_CHILD_KEY_VER
+# ARGS: KEY_DIR
 load_current_uefi_key_versions() {
   local key_dir="$1"
   local UEFI_VERSION_FILE="${key_dir}/${UEFI_VERSION_FILE}"
@@ -48,9 +53,12 @@ Current UEFI DB child key version: ${CURR_DB_CHILD_KEY_VER}
 EOF
 }
 
+# The common part for the subject of a UEFI key.
 _CHROMIUM_OS_SUBJECT=\
 '/C=US/ST=California/L=Mountain View/O=Google LLC./OU=Chromium OS'
 
+# Prints a UEFI key subject.
+# ARGS: TITLE VERSION
 _get_subj() {
   local title="$1"
   local version="$2"
@@ -58,63 +66,86 @@ _get_subj() {
   echo "${_CHROMIUM_OS_SUBJECT}/CN=${title} v${version}"
 }
 
-# Generate a pair of a private key and a self-signed cert at the current
+# Generates a pair of a private key and a self-signed cert at the current
 # directory. Generated files are
 #   $1/$1.rsa: The private key
 #   $1/$1.pem: The self-signed cert in PEM format
+# ARGS: KEY_NAME SUBJECT
 _make_self_signed_pair() {
   local key_name="$1"
   local subj="$2"
 
   mkdir -p "${key_name}"
-  pushd "${key_name}" > /dev/null
+  pushd "${key_name}" >/dev/null || return 1
   openssl req -new -x509 -nodes -newkey rsa:2048 -sha256 \
       -keyout "${key_name}.rsa" -out "${key_name}.pem" \
       -subj "${subj}" -days 73000
-  popd > /dev/null
+  popd >/dev/null
 }
 
-# Generate a pair of a private key and a cert signed by the given CA.
+# Generates a pair of a private key and a cert signed by the given CA.
 # "$1" (the first argument) is the CA file name without extension.
 # The results are signed by "$1/$1.{rsa,pem}", and are generated in
 # "$1/$1.children" directory under the current directory. Generated files are
 #   $1/$1.children/$2.rsa: The private key
 #   $1/$1.children/$2.csr: The Certificate Signing Request
 #   $1/$1.children/$2.pem: The certificate signed by "$1.{rsa,pem}"
+# ARGS: CA_NAME CHILD_KEY_NAME SUBJECT
 _make_child_pair() {
   local ca_name="$1"  # Base filename without extension.
   local child_key_name="$2"
   local subj="$3"
 
   mkdir -p "${ca_name}/${ca_name}.children"
-  pushd "${ca_name}/${ca_name}.children" > /dev/null
+  pushd "${ca_name}/${ca_name}.children" >/dev/null || return 1
   openssl req -new -nodes -newkey rsa:2048 -sha256 \
       -keyout "${child_key_name}.rsa" -out "${child_key_name}.csr" \
       -subj "${subj}" -days 73000
   openssl x509 -req -sha256 -CA "../${ca_name}.pem" -CAkey "../${ca_name}.rsa" \
       -CAcreateserial -in "${child_key_name}.csr" \
       -out "${child_key_name}.pem" -days 73000
-  popd > /dev/null
+  popd >/dev/null
 }
 
+# Makes a PK (Platform Key) keypair.
+# Generated files are
+#   pk/pk.rsa: The private key
+#   pk/pk.pem: The self-signed cert in PEM format
+# ARGS: VERSION
 make_pk_keypair() {
   local version="$1"
   _make_self_signed_pair pk \
       "$(_get_subj "UEFI Platform Key" "${version}")"
 }
 
+# Makes a KEK (Key Exchange Key) keypair.
+# Generated files are
+#   kek/kek.rsa: The private key
+#   kek/kek.pem: The self-signed cert in PEM format
+# ARGS: VERSION
 make_kek_keypair() {
   local version="$1"
   _make_self_signed_pair kek \
       "$(_get_subj "UEFI Key Exchange Key" "${version}")"
 }
 
+# Makes a DB keypair.
+# Generated files are
+#   db/db.rsa: The private key
+#   db/db.pem: The self-signed cert in PEM format
+# ARGS: VERSION
 make_db_keypair() {
   local version="$1"
   _make_self_signed_pair db \
       "$(_get_subj "UEFI DB Key" "${version}")"
 }
 
+# Makes a DB child keypair (a keypair signed by the db key).
+# Generated files are
+#   db/db.children/db_child.rsa: The private key
+#   db/db.children/db_child.csr: The Certificate Signing Request
+#   db/db.children/db_child.pem: The certificate signed by "db/db.{rsa,pem}"
+# ARGS: DB_KEY_VERSION CHILD_KEY_VERSION
 make_db_child_keypair() {
   local db_key_version="$1"
   local child_key_version="$2"
@@ -123,66 +154,76 @@ make_db_child_keypair() {
           "${db_key_version}.${child_key_version}")"
 }
 
-_backup_existing_self_signed_pair() {
+# Makes a backup of a self-signed keypair.
+# ARGS: KEY_NAME VERSION
+_backup_self_signed_pair() {
   local key_name="$1"
   local version="$2"
-  pushd "${key_name}" > /dev/null
+  pushd "${key_name}" >/dev/null || return 1
   mv --no-clobber "${key_name}".{rsa,"v${version}.rsa"}
   mv --no-clobber "${key_name}".{pem,"v${version}.pem"}
-  popd > /dev/null
+  popd >/dev/null
 }
 
-_backup_existing_self_signed_pair_and_children() {
+# Makes a backup of a self-signed keypair and its child keys.
+# ARGS: KEY_NAME VERSION
+_backup_self_signed_pair_and_children() {
   local key_name="$1"
   local version="$2"
-  _backup_existing_self_signed_pair "${key_name}" "${version}"
-  pushd "${key_name}" > /dev/null
+  _backup_self_signed_pair "${key_name}" "${version}"
+  pushd "${key_name}" >/dev/null || return 1
   mv --no-clobber "${key_name}".{children,"v${version}.children"}
-  popd > /dev/null
+  popd >/dev/null
 }
 
-_backup_existing_child_pair() {
+# Makes a backup of a child keypair signed by a CA.
+# ARGS: CA_NAME CHILD_KEY_NAME CHILD_KEY_VERSION
+_backup_child_pair() {
   local ca_name="$1"
   local child_key_name="$2"
   local child_key_version="$3"
-  pushd "${ca_name}/${ca_name}.children" > /dev/null
+  pushd "${ca_name}/${ca_name}.children" >/dev/null || return 1
   mv --no-clobber "${child_key_name}".{rsa,"v${child_key_version}.rsa"}
   mv --no-clobber "${child_key_name}".{csr,"v${child_key_version}.csr"}
   mv --no-clobber "${child_key_name}".{pem,"v${child_key_version}.pem"}
-  popd
+  popd >/dev/null
 }
 
-# Make backup of existing pk keypair.
+# Makes a backup of the PK (Platform Key) keypair.
 # Backup format: pk.v<pk key version>.{rsa,pem}
-backup_existing_pk_keypair() {
+# ARGS: PK_KEY_VERSION
+backup_pk_keypair() {
   local pk_key_version="$1"
-  _backup_existing_self_signed_pair pk "${pk_key_version}"
+  _backup_self_signed_pair pk "${pk_key_version}"
 }
 
-# Make backup of existing kek keypair.
+# Makes a backup of the KEK (Key Exchange Key) keypair.
 # Backup format: kek.v<kek key version>.{rsa,pem}
-backup_existing_kek_keypair() {
+# ARGS: KEK_KEY_VERSION
+backup_kek_keypair() {
   local kek_key_version="$1"
-  _backup_existing_self_signed_pair kek "${kek_key_version}"
+  _backup_self_signed_pair kek "${kek_key_version}"
 }
 
-# Make backup of existing db keypair and children.
+# Makes a backup of the DB keypair and its children.
 # Backup format:
-# for db keypair: db.v<db key version>.{rsa,pem}
-# for child keypair: db.v<db key version>.childern/child*.{rsa,csr,pem}
-backup_existing_db_keypair_and_children() {
+#   for db keypair: db.v<db key version>.{rsa,pem}
+#   for child keypair: db.v<db key version>.childern/child*.{rsa,csr,pem}
+# ARGS: DB_KEY_VERSION
+backup_db_keypair_and_children() {
   local db_key_version="$1"
-  _backup_existing_self_signed_pair_and_children db "${db_key_version}"
+  _backup_self_signed_pair_and_children db "${db_key_version}"
 }
 
-# Make backup of existing db child keypair.
+# Makes a backup of the DB child keypair.
 # Backup format: db.children/child.v<db child key version>.{rsa,csr,pem}
-backup_existing_db_child_keypair() {
+# ARGS: DB_CHILD_KEY_VERSION
+backup_db_child_keypair() {
   local db_child_key_version="$1"
-  _backup_existing_child_pair db db_child "${db_child_key_version}"
+  _backup_child_pair db db_child "${db_child_key_version}"
 }
 
-# Write new key version file with the updated key versions.
+# Writes new key version file with the updated key versions.
 # Args: PK_KEY_VERSION KEK_KEY_VERSION DB_KEY_VERSION DB_CHILD_KEY_VERSION
 write_updated_uefi_version_file() {
   local pk_key_version="$1"
@@ -208,9 +249,5 @@ increment_uefi_version() {
   local old_version=$(get_uefi_version "$2")
   local new_version=$(( old_version + 1 ))
 
-  if [[ ${new_version} -gt 0xffff ]]; then
-    echo "Version overflow!" >&2
-    return 1
-  fi
-  echo ${new_version}
+  echo "${new_version}"
 }