Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions build_image.sh
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ elif [ "$IMAGE_TYPE" = "aboot" ]; then
sed -i -e "s/%%IMAGE_VERSION%%/$IMAGE_VERSION/g" files/Aboot/boot0
pushd files/Aboot && zip -g $OLDPWD/$OUTPUT_ABOOT_IMAGE boot0; popd
pushd files/Aboot && zip -g $OLDPWD/$ABOOT_BOOT_IMAGE boot0; popd
pushd files/image_config/secureboot && zip -g $OLDPWD/$OUTPUT_ABOOT_IMAGE whitelist; popd
Copy link
Collaborator

@qiluo-msft qiluo-msft May 13, 2020

Choose a reason for hiding this comment

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

whitelist [](start = 78, length = 9)

Name it as whitelist_paths.txt ? #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can we use whitelist_paths? Do we need to remove the .txt when added into the image package?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to whitelist_paths.conf

echo "$IMAGE_VERSION" >> .imagehash
zip -g $OUTPUT_ABOOT_IMAGE .imagehash
zip -g $ABOOT_BOOT_IMAGE .imagehash
Expand Down
3 changes: 3 additions & 0 deletions files/Aboot/boot0.j2
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,9 @@ write_boot_configs() {
fi
fi

# setting secure_boot_enable=true when secure boot enabled
[ -f /bin/securebootctl ] && securebootctl secureboot -display | grep -i "Secure Boot enable" -q && echo "secure_boot_enable=true" >> /tmp/append
Copy link
Collaborator

@qiluo-msft qiluo-msft May 13, 2020

Choose a reason for hiding this comment

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

securebootctl secureboot -display | grep -i "Secure Boot enable" -q [](start = 33, length = 67)

Is it possible to test secure boot enabled without grep? #Pending

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will ask Arista about it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The arista secure boot using the similar way.


mkdir -p "$image_path"
cat /tmp/append > $cmdline_image
[ -s ${target_path}/machine.conf ] || write_machine_config
Expand Down
32 changes: 32 additions & 0 deletions files/image_config/secureboot/whitelist
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# It is the patterns of the relative paths in /host/image-{hash}/rw folder.
# The patterns will not be used if the Sonic Secure Boot feature is not enabled.
# The files that are not in the whitelist will be removed when the Sonic System cold reboot.

home/.*
var/core/.*
var/log/.*
etc/group
etc/gshadow
etc/hostname
etc/hosts
etc/machine-id
etc/network/interfaces
etc/nsswitch.conf
etc/pam.d/common-auth-sonic
etc/pam.d/sshd
etc/pam.d/login
etc/passwd
etc/rsyslog.conf
etc/shadow
etc/sonic/acl.json
etc/sonic/config_db.json
etc/sonic/minigraph.xml
etc/sonic/snmp.yml
etc/sonic/updategraph.conf
etc/ssh/ssh_host_rsa_key.pub
etc/ssh/ssh_host_rsa_key
etc/subgid
etc/subuid
etc/tacplus_nss.conf
etc/tacplus_user
lib/systemd/system/[email protected]
Copy link
Collaborator

@qiluo-msft qiluo-msft May 13, 2020

Choose a reason for hiding this comment

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

Need to double check #Closed

39 changes: 39 additions & 0 deletions files/initramfs-tools/union-mount.j2
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,49 @@ set_tmpfs_log_partition_size()
[ $maxsize -le $varlogsize ] && varlogsize=$maxsize
}

whitelist_rw_folder()
Copy link
Collaborator

@qiluo-msft qiluo-msft May 13, 2020

Choose a reason for hiding this comment

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

Sh code is not easy to understand, add some comments? #Closed

{
image_dir=$1
Copy link
Contributor

Choose a reason for hiding this comment

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

If it doesn't bother you too much. can you switch to 4 space indentation? Most of our files are 4 space indentations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

whitelist_file=${rootmnt}/host/$image_dir/whitelist

# Return if the whitelist file does not exist
if ! test -f "${whitelist_file}"; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this check be moved down or add as a protection of accessing the file?

If secure boot is enabled and this file is missing, should you at least whitelist rw_dir? I understand that is not enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The file should not be missing, it is extracted from the image every time in boot0.
If secure boot enabled, the file should always exist, I will improve it.

return
fi

# Return if the secure_boot_enable option is not set
if cat /proc/cmdline | grep -v -q "secure_boot_enable=true"; then
Copy link
Collaborator

@qiluo-msft qiluo-msft May 13, 2020

Choose a reason for hiding this comment

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

secure_boot_enable=true [](start = 37, length = 23)

The convention is
secure_boot_enable=[0|1]
or
secure_boot_enable=[n|y]
#Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

return
fi

whitelist_log=${rootmnt}/host/$image_dir/whitelist.log
rw_dir=${rootmnt}/host/$image_dir/rw
whitelist=$(cat ${rootmnt}/host/$image_dir/whitelist | grep -v "^\s*#" | awk '{$1=$1};1')
Copy link
Collaborator

@qiluo-msft qiluo-msft May 13, 2020

Choose a reason for hiding this comment

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

| grep -v "^\s*#" | awk '{$1=$1};1' [](start = 54, length = 36)

Is it only for parsing comments? If yes, we may remove the complexity. And add a whiltelist_README.md side-by-side to explain usage. #Closed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Offline discussed, move comment out into separate file. It is not a runtime conf file.


In reply to: 424122684 [](ancestors = 424122684)

set -o noglob
find ${rw_dir} -type f |
while IFS= read -r file; do
found="false"
for line in $whitelist; do
pattern="^${rw_dir}/${line}\$"
if echo "$file" | grep -q "$pattern"; then
found="true"
break
fi
done
if [ $found = "false" ]; then
echo $file >> ${whitelist_log}
rm -f $file
fi
done
Copy link
Collaborator

@qiluo-msft qiluo-msft May 13, 2020

Choose a reason for hiding this comment

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

I am worried about the performance if rw contains many files and deep directories. Since whitelist is short, can we mv folder to a temp one, and copied back one by one.

Not sure the validation of this solution, is there any metadata / hidden files in rw for overlay?

If it working, no need to add /* for directories in whitelist. #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The find command can get hidden files, what is the metadata?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the performance is not good enough, but it is not relative to the depth of the directories, it is the grep.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The performance issue should be fixed. Tested on the device, reduce from 100 seconds to 1 seconds for 300 files. And checked the changed files on product, it only has about 700 changed files in max.

set +o noglob
}

## Mount the overlay file system: rw layer over squashfs
image_dir=$(cat /proc/cmdline | sed -e 's/.*loop=\(\S*\)\/.*/\1/')
mkdir -p ${rootmnt}/host/$image_dir/rw
mkdir -p ${rootmnt}/host/$image_dir/work
## Whitelist rw folder
whitelist_rw_folder "$image_dir"
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest name change to reflect the fact that files are removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to remove_not_whitelist_files

Copy link
Collaborator

@qiluo-msft qiluo-msft Jun 2, 2020

Choose a reason for hiding this comment

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

$image_dir [](start = 21, length = 10)

If there is any executable files inside rw, let's chmod a-x to change it to non executables.

However, user's home directory should keep as is. #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

mount -n -o lowerdir=${rootmnt},upperdir=${rootmnt}/host/$image_dir/rw,workdir=${rootmnt}/host/$image_dir/work -t overlay root-overlay ${rootmnt}
## Check if the root block device is still there
[ -b ${ROOT} ] || mdev -s
Expand Down