Merge pull request #338 from google/remove-protector-from-policy

cmd/fscrypt: don't load protector in remove-protector-from-policy

Author: ebiggers

actions/policy.go

@@ -461,14 +461,15 @@ func (policy *Policy) AddProtector(protector *Protector) error {
 }
 
 // RemoveProtector updates the data that is wrapping the Policy Key so that the
-// provided Protector is no longer protecting the specified Policy. If an error
-// is returned, no data has been changed. Note that no protector links are
+// protector with the given descriptor is no longer protecting the specified
+// Policy.  If an error is returned, no data has been changed.  Note that the
+// protector itself won't be removed, nor will a link to the protector be
 // removed (in the case where the protector and policy are on different
-// filesystems). The policy and protector can be locked or unlocked.
-func (policy *Policy) RemoveProtector(protector *Protector) error {
-	idx, ok := policy.findWrappedKeyIndex(protector.Descriptor())
+// filesystems).  The policy can be locked or unlocked.
+func (policy *Policy) RemoveProtector(protectorDescriptor string) error {
+	idx, ok := policy.findWrappedKeyIndex(protectorDescriptor)
 	if !ok {
-		return &ErrNotProtected{policy.Descriptor(), protector.Descriptor()}
+		return &ErrNotProtected{policy.Descriptor(), protectorDescriptor}
 	}
 
 	if len(policy.data.WrappedPolicyKeys) == 1 {

actions/policy_test.go

@@ -114,7 +114,7 @@ func TestPolicyGoodRemoveProtector(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	err = pol.RemoveProtector(pro1)
+	err = pol.RemoveProtector(pro1.Descriptor())
 	if err != nil {
 		t.Error(err)
 	}
@@ -135,11 +135,11 @@ func TestPolicyBadRemoveProtector(t *testing.T) {
 	}
 	defer cleanupProtector(pro2)
 
-	if pol.RemoveProtector(pro2) == nil {
+	if pol.RemoveProtector(pro2.Descriptor()) == nil {
 		t.Error("we should not be able to remove a protector we did not add")
 	}
 
-	if pol.RemoveProtector(pro1) == nil {
+	if pol.RemoveProtector(pro1.Descriptor()) == nil {
 		t.Error("we should not be able to remove all the protectors from a policy")
 	}
 }

cli-tests/common.sh

@@ -72,6 +72,40 @@ _get_enabled_fs_count()
 	echo "$count"
 }
 
+# Gets the descriptor of the given protector.
+_get_protector_descriptor()
+{
+	local mnt=$1
+	local source=$2
+
+	case $source in
+	custom)
+		local name=$3
+		local description="custom protector \\\"$name\\\""
+		;;
+	login)
+		local user=$3
+		local description="login protector for $user"
+		;;
+	*)
+		_fail "Unknown protector source $source"
+	esac
+
+	local descriptor
+	descriptor=$(fscrypt status "$mnt" |
+		     awk -F '   *' '{ if ($3 == "'"$description"'") print $1 }')
+	if [ -z "$descriptor" ]; then
+		_fail "Can't find $description on $mnt"
+	fi
+	echo "$descriptor"
+}
+
+# Gets the descriptor of the login protector for $TEST_USER.
+_get_login_descriptor()
+{
+	_get_protector_descriptor "$MNT_ROOT" login "$TEST_USER"
+}
+
 # Prints the number of filesystems that have fscrypt metadata.
 _get_setup_fs_count()
 {

cli-tests/t_change_passphrase.sh

@@ -14,7 +14,7 @@ echo pass1 | fscrypt encrypt --quiet --name=prot --skip-unlock "$dir"
 _print_header "Try to unlock with wrong passphrase"
 _expect_failure "echo pass2 | fscrypt unlock --quiet '$dir'"
 _expect_failure "mkdir '$dir/subdir'"
-protector=$(fscrypt status "$dir" | awk '/custom protector/{print $1}')
+protector=$(_get_protector_descriptor "$dir" custom prot)
 
 _print_header "Change passphrase"
 echo $'pass1\npass2' | \

cli-tests/t_encrypt_login.sh

@@ -27,18 +27,13 @@ show_status()
 	fi
 }
 
-get_login_protector()
-{
-	fscrypt status "$dir" | awk '/login protector/{print $1}'
-}
-
 begin "Encrypt with login protector"
 chown "$TEST_USER" "$dir"
 _user_do "echo TEST_USER_PASS | fscrypt encrypt --quiet --source=pam_passphrase '$dir'"
 show_status true
 recovery_passphrase=$(grep -E '^ +[a-z]{20}$' "$dir/fscrypt_recovery_readme.txt" | sed 's/^ +//')
-recovery_protector=$(fscrypt status "$dir" | awk '/Recovery passphrase/{print $1}')
-login_protector=$(get_login_protector)
+recovery_protector=$(_get_protector_descriptor "$MNT" custom 'Recovery passphrase for dir')
+login_protector=$(_get_login_descriptor)
 _print_header "=> Lock, then unlock with login passphrase"
 _user_do "fscrypt lock '$dir'"
 # FIXME: should we be able to use $MNT:$login_protector here?
@@ -63,7 +58,7 @@ begin "Encrypt with login protector as root"
 echo TEST_USER_PASS | fscrypt encrypt --quiet --source=pam_passphrase --user="$TEST_USER" "$dir"
 show_status true
 # The newly-created login protector should be owned by the user, not root.
-login_protector=$(get_login_protector)
+login_protector=$(_get_login_descriptor)
 owner=$(stat -c "%U:%G" "$MNT_ROOT/.fscrypt/protectors/$login_protector")
 echo -e "\nProtector is owned by $owner"
 

cli-tests/t_metadata.out

@@ -0,0 +1,17 @@
+ext4 filesystem "MNT" has 3 protectors and 1 policy
+
+PROTECTOR         LINKED  DESCRIPTION
+desc1  No      custom protector "foo"
+desc2  No      custom protector "bar"
+desc3  No      custom protector "baz"
+
+POLICY                            UNLOCKED  PROTECTORS
+desc4  No        desc1, desc2, desc3
+ext4 filesystem "MNT" has 2 protectors and 1 policy
+
+PROTECTOR         LINKED  DESCRIPTION
+desc1  No      custom protector "foo"
+desc2  No      custom protector "bar"
+
+POLICY                            UNLOCKED  PROTECTORS
+desc4  No        desc1

cli-tests/t_metadata.sh

@@ -0,0 +1,36 @@
+#!/bin/bash
+
+# Test 'fscrypt metadata'.
+
+cd "$(dirname "$0")"
+. common.sh
+
+# Create three protectors, and a policy protected by them.
+echo foo | fscrypt metadata create protector "$MNT" \
+	--quiet --name=foo --source=custom_passphrase
+echo bar | fscrypt metadata create protector "$MNT" \
+	--quiet --name=bar --source=custom_passphrase
+echo baz | fscrypt metadata create protector "$MNT" \
+	--quiet --name=baz --source=custom_passphrase
+prot_foo=$MNT:$(_get_protector_descriptor "$MNT" custom foo)
+prot_bar=$MNT:$(_get_protector_descriptor "$MNT" custom bar)
+desc_baz=$(_get_protector_descriptor "$MNT" custom baz)
+prot_baz=$MNT:$desc_baz
+echo foo | fscrypt metadata create policy "$MNT" --quiet \
+	--protector="$prot_foo"
+policy=$MNT:$(fscrypt status "$MNT" | grep -A10 "^POLICY" | \
+	      tail -1 | awk '{print $1}')
+echo -e "bar\nfoo" | fscrypt metadata add-protector-to-policy --quiet \
+	--policy="$policy" --protector="$prot_bar"
+echo -e "baz\nfoo" | fscrypt metadata add-protector-to-policy --quiet \
+	--policy="$policy" --protector="$prot_baz" --unlock-with="$prot_foo"
+fscrypt status "$MNT"
+
+# Remove two of the protectors from the policy.
+# Make sure that this works even if the protector was already deleted.
+fscrypt metadata remove-protector-from-policy --quiet --force \
+	--policy="$policy" --protector="$prot_bar"
+rm "$MNT/.fscrypt/protectors/$desc_baz"
+fscrypt metadata remove-protector-from-policy --quiet --force \
+	--policy="$policy" --protector="$prot_baz"
+fscrypt status "$MNT"

cmd/fscrypt/commands.go

@@ -1083,29 +1083,30 @@ func removeProtectorAction(c *cli.Context) error {
 		return err
 	}
 
-	// We do not need to unlock anything for this operation
-	protector, err := getProtectorFromFlag(protectorFlag.Value, nil)
+	// We only need the protector descriptor, not the protector itself.
+	ctx, protectorDescriptor, err := parseMetadataFlag(protectorFlag.Value, nil)
 	if err != nil {
 		return newExitError(c, err)
 	}
-	policy, err := getPolicyFromFlag(policyFlag.Value, protector.Context.TargetUser)
+	// We don't need to unlock the policy for this operation.
+	policy, err := getPolicyFromFlag(policyFlag.Value, ctx.TargetUser)
 	if err != nil {
 		return newExitError(c, err)
 	}
 
 	prompt := fmt.Sprintf("Stop protecting policy %s with protector %s?",
-		policy.Descriptor(), protector.Descriptor())
+		policy.Descriptor(), protectorDescriptor)
 	warning := "All files using this policy will NO LONGER be accessible with this protector!!"
 	if err := askConfirmation(prompt, false, warning); err != nil {
 		return newExitError(c, err)
 	}
 
-	if err := policy.RemoveProtector(protector); err != nil {
+	if err := policy.RemoveProtector(protectorDescriptor); err != nil {
 		return newExitError(c, err)
 	}
 
 	fmt.Fprintf(c.App.Writer, "Protector %s no longer protecting policy %s.\n",
-		protector.Descriptor(), policy.Descriptor())
+		protectorDescriptor, policy.Descriptor())
 	return nil
 }