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

Make remove-protector-from-policy work even if the protector cannot be
loaded (for example, due to having been deleted already).

Fixes https://github.com/google/fscrypt/issues/258
Fixes https://github.com/google/fscrypt/issues/272

diff --git a/actions/policy.go b/actions/policy.go
index 6c481178bd09375a383154e60ced3a53f087e3aa..7204380b350033861bae5c2b79a88726640dd982 100644 (file)
--- a/actions/policy.go
+++ b/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 {

diff --git a/actions/policy_test.go b/actions/policy_test.go
index 11c9c3e414c0e3c42c0c937fd30f0c0b20c66661..07943b88ad2eae91661e1d87a77b731e685cc10d 100644 (file)
--- a/actions/policy_test.go
+++ b/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")
        }
 }

diff --git a/cli-tests/t_metadata.out b/cli-tests/t_metadata.out
new file mode 100644 (file)
index 0000000..fba816a
--- /dev/null
+++ b/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

diff --git a/cli-tests/t_metadata.sh b/cli-tests/t_metadata.sh
new file mode 100755 (executable)
index 0000000..e688eda
--- /dev/null
+++ b/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"

diff --git a/cmd/fscrypt/commands.go b/cmd/fscrypt/commands.go
index 0b382a6f642ee0e87a1d67677405f24d46233897..023c0fa2d4eab9d40dcbd638d769d0245abcf578 100644 (file)
--- a/cmd/fscrypt/commands.go
+++ b/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
 }