Add sanity test for /etc/lsb-release file in CrOS images

scripts/image_signing/ensure_sane_lsb-release.sh

+#!/bin/bash
+
+# Copyright (c) 2011 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.
+
+# Abort on error.
+set -e
+
+LSB_FILE=/etc/lsb-release
+
+# Load common constants and variables.
+. "$(dirname "$0")/common.sh"
+
+usage() {
+    echo "Usage $PROG image [config]"

[petkov, 2011/02/02 15:57:12]

why 4-space indent? i think our style is 2 space.

[jimhebert, 2011/02/02 23:51:36]

Ack. I started out by copying the style from ensure_no_password.sh, guess I
shoulda read the style guide instead.

I'll submit an indentation fix by itself as the last patchkit to this CL review,
but in the meantime to keep the diffs-between-patchkits readable I'll do a round
of fixes for all of the other issues pointed out, maintaining the current
indentation level. (Hope that makes sense: patchkit 2 fix all the issues, you
get to read the diffs, then patchkit 3 fix indentation.)

+}
+
+# Usage: lsbval path-to-lsb-file key
+# Returns the value for the given lsb-release file variable.
+lsbval() {
+    local lsbfile="$1"
+    local key="$2"
+    grep ^$key= "$lsbfile" | sed s/^$key=//
+}
+

[petkov, 2011/02/02 15:57:12]

remove extra blank line

[jimhebert, 2011/02/02 23:51:36]

Done

+
+# Usage: lsbequals path-to-lsb-file key expected-value
+# Returns 0 if they match, 1 otherwise.
+# Also outputs a warning message if they don't match.
+lsbequals() {
+    local lsbfile="$1"
+    local key="$2"
+    local expectval="$3"
+    local realval=$(lsbval "$lsbfile" $key)
+    if [ "$realval" != "$expectval" ]; then
+        echo "$key mismatch. Expected '$expectval', image contains '$realval'"
+        return 1
+    fi
+    return 0
+}
+
+# Usage: lsb_syntaxcheck path-to-lsb-file
+# Enforces a number of basic sanity checks on the overall format and contents
+# of the lsb-release file:
+# - Every line is "key=value".
+# - No space after key, no space before value.
+# - key is all A-Z or _, but not starting with _.
+# - value is made up of printable characters, or is empty.
+# - Each line is a reasonable size (256 bytes).
+# - The whole file is a reasonable size (4kb).
+lsb_syntaxcheck() {
+    local lsbfile="$1"
+    syntaxbad=0
+    # Checks for key being A-Z_, 1 or more characters, not starting with _.
+    # Also checks for = with no spaces on either side.
+    # Checks that the value contains printables (and not starting with space).
+    # Alternatively, the 2nd grep permits the value to be empty (0 chars) too.
+    badlines=$(grep -E -v '^[A-Z][A-Z_]*=[[:graph:]][[:print:]]*' "$lsbfile" |\

[petkov, 2011/02/02 15:57:12]

no need for trailing \

you could do this in a single grep, I think:

^[A-Z][A-Z_]*=([[:graph:]][[:print:]]*)?$

[jimhebert, 2011/02/02 23:51:36]

Done

+               grep -E -v '^[A-Z][A-Z_]*=$')
+    if [ -n "$badlines" ]; then
+        syntaxbad=1
+        echo "$lsbfile: Some lines seem non-well-formed:"
+        echo "$badlines"
+    fi
+
+    # Checks for a lines exceeding a reasonable overall length.
+    # regex(7) says {number} cannot exceed RE_DUP_MAX (255!) so we use this
+    # workaround to get the equivalent of {257}.
+    badlines=$(grep -E '^.{255}.{2}' "$lsbfile")

[petkov, 2011/02/02 15:57:12]

stick with 255 for simplicity? :)

[jimhebert, 2011/02/02 23:51:36]

Done

+    if [ -n "$badlines" ]; then
+        syntaxbad=1
+        echo "$lsbfile: Some lsb-release lines seem unreasonably long:"
+        echo "$badlines"
+    fi
+    # Overall file size check:
+    size=$(ls -sk "$lsbfile" | cut -d ' ' -f 1)
+    if [ $size -gt 4 ]; then
+        syntaxbad=1
+        echo "$lsbfile: This file exceeds 4kb"
+    fi
+    return $syntaxbad
+}
+
+

[petkov, 2011/02/02 15:57:12]

remove extra blank line

[jimhebert, 2011/02/02 23:51:36]

Done

+main() {
+    # We want to catch all the discrepancies, not just the first one.
+    # So, any time we find one, we set testfail=1 and continue.
+    # When finished we will use testfail to determine our exit value.
+    local testfail=0
+
+    if [ $# -ne 1 ] && [ $# -ne 2 ]; then
+        usage
+        exit 1
+    fi
+
+    local image="$1"
+
+    # Default config location: same name/directory as this script,
+    # with a .config file extension, ie ensure_sane_lsb-release.config.
+    local configfile="$(dirname "$0")/${0/%.sh/.config}"
+    # Or, maybe a config was provided on the command line.
+    if [ $# -eq 2 ]; then
+        configfile="$2"
+    fi
+    # Either way, load test-expectations data from config.
+    . "$configfile"

[petkov, 2011/02/02 15:57:12]

you may want to print an info message showing which config file you're using.

[jimhebert, 2011/02/02 23:51:36]

Done

+
+    local rootfs=$(make_temp_dir)
+    mount_image_partition_ro "$image" 3 "$rootfs"

[petkov, 2011/02/02 15:57:12]

on error, nothing gets unmounted? and the temp dir remains?

[jimhebert, 2011/02/02 23:51:36]

From my reading of common_minimal.sh, this is all handled automatically? Did I
misunderstand? (trap "cleanup_temps_and_mounts" EXIT)

+    local lsb="$rootfs/$LSB_FILE"
+
+    # Basic syntax check first.
+    lsb_syntaxcheck "$lsb" || testfail=1
+
+    # Pick the right set of test-expectation data to use. The cuts
+    # turn e.g. x86-foo-pvtkeys into x86-foo.
+    local board=$(lsbval $lsb CHROMEOS_RELEASE_BOARD | \

[petkov, 2011/02/02 15:57:12]

no need for trailing \ after | I think. also, I think style is each pipe on a
separate line, i.e.,

cmd 1 |
  cmd 2 |
  cmd 3

[jimhebert, 2011/02/02 23:51:36]

Done.

+                  cut -d = -f 2 | cut -d - -f 1,2)

[petkov, 2011/02/02 15:57:12]

this may be a bit flaky but i don't have much better suggestions. i don't think
there's a rule that the basic board name is id-id. somebody may come up with
x86-mario-v2, for example.

[gauravsh, 2011/02/03 01:12:38]

Yes, I agree with this concern. The proper solution (IMO) would be to have a
canonical board/platform name in lsb-release. Right now we overload that to
indicate how the image is signed.

+    # a copy of the board string with '-' squished to variable-name-safe '_'.
+    local boardvar=${board//-/_}

[petkov, 2011/02/02 15:57:12]

i see why you're doing this but it's a bit flaky. it's ok as is, however, is
there any way we can convert expected values to a proper map so that there's no
need for the key to be a real identifier? maybe through an array indexed by
looking up the board index in the expected boards array?

[jimhebert, 2011/02/02 23:51:36]

I agree with the sentiments and had the same thoughts when writing it. For me it
came down to "well, if I want it that sophisticated, I ought to be writing this
in python where I have dictionary types and such." ;-)

With the limited data structures available, looking up the board offset in 1
array, then looking up an appid at the same offset in another array would be
possible, though I fear it could become maintainer-unfriendly as the number of
boards grows and things could get mis-correlated if some item gets added to the
wrong place in the array.

Hopefully, having moved the board-check to being above the eval, I've addressed
most of the concern about flake - this should only ever eval to a small
whitelist of things now.

+    eval "expected_appid=\"\$expected_appid_$boardvar\""
+

[gauravsh, 2011/02/02 01:33:42]

extra newline

[jimhebert, 2011/02/02 23:51:36]

Done

+
+    lsbequals $lsb CHROMEOS_AUSERVER "$expected_auserver" || testfail=1
+    lsbequals $lsb CHROMEOS_RELEASE_NAME "$expected_release_name" || testfail=1
+    lsbequals $lsb CHROMEOS_RELEASE_APPID "$expected_appid" || testfail=1
+
+    # CHROMEOS_RELEASE_BOARD and CHROMEOS_RELEASE_TRACK must appear in their
+    # respective whitelists, expected_boards and expected_release_tracks.
+    # First, boards:
+    local board_recognized=0
+    for b in ${expected_boards[@]}; do

[gauravsh, 2011/02/02 01:33:42]

shouldn't the board check happen before the eval on line 122, (in which case the
app id check should also be skipped)?

[jimhebert, 2011/02/02 23:51:36]

Done.

+        if [ $b == $board ]; then
+            board_recognized=1
+        fi
+    done
+    if [ $board_recognized -eq 0 ]; then
+        echo "Board '$board' unrecognized"
+        testfail=1
+    fi
+
+    # The same check as above, for tracks
+    local track_recognized=0
+    local track=$(lsbval $lsb CHROMEOS_RELEASE_TRACK)
+    for t in ${expected_release_tracks[@]}; do
+        if [ $t == $track ]; then
+            track_recognized=1

[gauravsh, 2011/02/02 01:33:42]

these 2 checks for CHROMEOS_RELEASE_BOARD and CHROMEOS_RELEASE_TRACK basically
use the same code, can you re-factor them into a single function? (something
like - check_keyval_in_list <lsbfile> <key> <listname>)

[jimhebert, 2011/02/02 23:51:36]

Done.

+        fi
+    done
+    if [ $track_recognized -eq 0 ]; then
+        echo "Track '$track' unrecognized"
+        testfail=1
+    fi
+
+    exit $testfail
+}
+main $@

[petkov, 2011/02/02 15:57:12]

add blank line before main

[petkov, 2011/02/02 15:57:12]

"$@"

[jimhebert, 2011/02/02 23:51:36]

Done

[jimhebert, 2011/02/02 23:51:36]

Done