devicestate: Unregister deletes the device key pair as well

* daemon,tests: support forgetting device serial via API

this is done by posting {"action":"forget"} to /v2/model/serial

a flag no-registration-until-reboot is also supported

* many: more consistent naming Delete => DeleteByName on keypair mgrs

we actually want to introduce a Delete by key id on some of them now

* asserts: implement KeypairManager.Delete

* devicestate: Unregister deletes the device key pair as well

* tests: test device key deletion in generic-unregister

* asserts: avoid skipping the GPGKeypairManager.Delete test

pair --yes to --batch in the test

asserts/database.go

@@ -102,6 +102,8 @@ type KeypairManager interface {
 	Put(privKey PrivateKey) error
 	// Get returns the private/public key pair with the given key id.
 	Get(keyID string) (PrivateKey, error)
+	// Delete deletes the private/public key pair with the given key id.
+	Delete(keyID string) error
 }
 
 // DatabaseConfig for an assertion database.

asserts/export_test.go

@@ -271,6 +271,13 @@ func MockRunGPG(mock func(prev GPGRunner, input []byte, args ...string) ([]byte,
 	}
 }
 
+func GPGBatchYes() (restore func()) {
+	gpgBatchYes = true
+	return func() {
+		gpgBatchYes = false
+	}
+}
+
 // Headers helpers to test
 var (
 	ParseHeaders = parseHeaders

asserts/extkeypairmgr.go

@@ -204,7 +204,11 @@ func (em *ExternalKeypairManager) Put(privKey PrivateKey) error {
 	return &ExternalUnsupportedOpError{"cannot import private key into external keypair manager"}
 }
 
-func (em *ExternalKeypairManager) Delete(keyName string) error {
+func (em *ExternalKeypairManager) Delete(keyID string) error {
+	return &ExternalUnsupportedOpError{"no support to delete external keypair manager keys"}
+}
+
+func (em *ExternalKeypairManager) DeleteByName(keyName string) error {
 	return &ExternalUnsupportedOpError{"no support to delete external keypair manager keys"}
 }
 

asserts/extkeypairmgr_test.go

@@ -300,11 +300,21 @@ func (s *extKeypairMgrSuite) TestListError(c *C) {
 	c.Check(err, ErrorMatches, `cannot get all external keypair manager key names:.*exit status 1.*`)
 }
 
-func (s *extKeypairMgrSuite) TestDeleteUnsupported(c *C) {
+func (s *extKeypairMgrSuite) TestDeleteByNameUnsupported(c *C) {
 	kmgr, err := asserts.NewExternalKeypairManager("keymgr")
 	c.Assert(err, IsNil)
 
-	err = kmgr.Delete("key")
+	err = kmgr.DeleteByName("key")
+	c.Check(err, ErrorMatches, `no support to delete external keypair manager keys`)
+	c.Check(err, FitsTypeOf, &asserts.ExternalUnsupportedOpError{})
+
+}
+
+func (s *extKeypairMgrSuite) TestDelete(c *C) {
+	kmgr, err := asserts.NewExternalKeypairManager("keymgr")
+	c.Assert(err, IsNil)
+
+	err = kmgr.Delete("key-id")
 	c.Check(err, ErrorMatches, `no support to delete external keypair manager keys`)
 	c.Check(err, FitsTypeOf, &asserts.ExternalUnsupportedOpError{})
 

asserts/fsentryutils.go

@@ -68,3 +68,8 @@ func readEntry(top string, subpath ...string) ([]byte, error) {
 	fpath := filepath.Join(top, filepath.Join(subpath...))
 	return ioutil.ReadFile(fpath)
 }
+
+func removeEntry(top string, subpath ...string) error {
+	fpath := filepath.Join(top, filepath.Join(subpath...))
+	return os.Remove(fpath)
+}

asserts/fskeypairmgr.go

@@ -90,3 +90,17 @@ func (fskm *filesystemKeypairManager) Get(keyID string) (PrivateKey, error) {
 	}
 	return privKey, nil
 }
+
+func (fskm *filesystemKeypairManager) Delete(keyID string) error {
+	fskm.mu.RLock()
+	defer fskm.mu.RUnlock()
+
+	err := removeEntry(fskm.top, keyID)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return errKeypairNotFound
+		}
+		return err
+	}
+	return nil
+}

asserts/fskeypairmgr_test.go

@@ -63,3 +63,33 @@ func (fsbss *fsKeypairMgrSuite) TestOpenWorldWritableFail(c *C) {
 	c.Assert(err, ErrorMatches, "assert storage root unexpectedly world-writable: .*")
 	c.Check(bs, IsNil)
 }
+
+func (fsbss *fsKeypairMgrSuite) TestDelete(c *C) {
+	// ensure umask is clean when creating the DB dir
+	oldUmask := syscall.Umask(0)
+	defer syscall.Umask(oldUmask)
+
+	topDir := filepath.Join(c.MkDir(), "asserts-db")
+	err := os.MkdirAll(topDir, 0775)
+	c.Assert(err, IsNil)
+
+	keypairMgr, err := asserts.OpenFSKeypairManager(topDir)
+	c.Check(err, IsNil)
+
+	pk1 := testPrivKey1
+	keyID := pk1.PublicKey().ID()
+	err = keypairMgr.Put(pk1)
+	c.Assert(err, IsNil)
+
+	_, err = keypairMgr.Get(keyID)
+	c.Assert(err, IsNil)
+
+	err = keypairMgr.Delete(keyID)
+	c.Assert(err, IsNil)
+
+	err = keypairMgr.Delete(keyID)
+	c.Check(err, ErrorMatches, "cannot find key pair")
+
+	_, err = keypairMgr.Get(keyID)
+	c.Check(err, ErrorMatches, "cannot find key pair")
+}

asserts/gpgkeypairmgr.go

@@ -31,6 +31,7 @@ import (
 	"golang.org/x/crypto/openpgp/packet"
 
 	"github.com/snapcore/snapd/osutil"
+	"github.com/snapcore/snapd/strutil"
 )
 
 func ensureGPGHomeDirectory() (string, error) {
@@ -73,6 +74,8 @@ func findGPGCommand() (string, error) {
 	return path, err
 }
 
+var gpgBatchYes = false
+
 func runGPGImpl(input []byte, args ...string) ([]byte, error) {
 	homedir, err := ensureGPGHomeDirectory()
 	if err != nil {
@@ -92,6 +95,9 @@ func runGPGImpl(input []byte, args ...string) ([]byte, error) {
 	}
 
 	general := []string{"--homedir", homedir, "-q", "--no-auto-check-trustdb"}
+	if gpgBatchYes && strutil.ListContains(args, "--batch") {
+		general = append(general, "--yes")
+	}
 	allArgs := append(general, args...)
 
 	path, err := findGPGCommand()
@@ -236,12 +242,20 @@ func (gkm *GPGKeypairManager) Put(privKey PrivateKey) error {
 	return fmt.Errorf("cannot import private key into GPG keyring")
 }
 
-func (gkm *GPGKeypairManager) Get(keyID string) (PrivateKey, error) {
+type gpgKeypairInfo struct {
+	privKey     PrivateKey
+	fingerprint string
+}
+
+func (gkm *GPGKeypairManager) findByID(keyID string) (*gpgKeypairInfo, error) {
 	stop := errors.New("stop marker")
-	var hit PrivateKey
+	var hit *gpgKeypairInfo
 	match := func(privk PrivateKey, fpr string, uid string) error {
 		if privk.PublicKey().ID() == keyID {
-			hit = privk
+			hit = &gpgKeypairInfo{
+				privKey:     privk,
+				fingerprint: fpr,
+			}
 			return stop
 		}
 		return nil
@@ -256,6 +270,26 @@ func (gkm *GPGKeypairManager) Get(keyID string) (PrivateKey, error) {
 	return nil, fmt.Errorf("cannot find key %q in GPG keyring", keyID)
 }
 
+func (gkm *GPGKeypairManager) Get(keyID string) (PrivateKey, error) {
+	keyInfo, err := gkm.findByID(keyID)
+	if err != nil {
+		return nil, err
+	}
+	return keyInfo.privKey, nil
+}
+
+func (gkm *GPGKeypairManager) Delete(keyID string) error {
+	keyInfo, err := gkm.findByID(keyID)
+	if err != nil {
+		return err
+	}
+	_, err = gkm.gpg(nil, "--batch", "--delete-secret-and-public-key", "0x"+keyInfo.fingerprint)
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
 func (gkm *GPGKeypairManager) sign(fingerprint string, content []byte) (*packet.Signature, error) {
 	out, err := gkm.gpg(content, "--personal-digest-preferences", "SHA512", "--default-key", "0x"+fingerprint, "--detach-sign")
 	if err != nil {
@@ -276,11 +310,6 @@ func (gkm *GPGKeypairManager) sign(fingerprint string, content []byte) (*packet.
 	return sig, nil
 }
 
-type gpgKeypairInfo struct {
-	privKey     PrivateKey
-	fingerprint string
-}
-
 func (gkm *GPGKeypairManager) findByName(name string) (*gpgKeypairInfo, error) {
 	stop := errors.New("stop marker")
 	var hit *gpgKeypairInfo
@@ -353,8 +382,8 @@ func (gkm *GPGKeypairManager) Export(name string) ([]byte, error) {
 	return EncodePublicKey(keyInfo.privKey.PublicKey())
 }
 
-// Delete removes the named key pair from GnuPG's storage.
-func (gkm *GPGKeypairManager) Delete(name string) error {
+// DeleteByName removes the named key pair from GnuPG's storage.
+func (gkm *GPGKeypairManager) DeleteByName(name string) error {
 	keyInfo, err := gkm.findByName(name)
 	if err != nil {
 		return err

asserts/gpgkeypairmgr_test.go

@@ -338,3 +338,20 @@ func (gkms *gpgKeypairMgrSuite) TestList(c *C) {
 	c.Check(keys[0].ID, Equals, assertstest.DevKeyID)
 	c.Check(keys[0].Name, Not(Equals), "")
 }
+
+func (gkms *gpgKeypairMgrSuite) TestDelete(c *C) {
+	defer asserts.GPGBatchYes()()
+
+	keyID := assertstest.DevKeyID
+	_, err := gkms.keypairMgr.Get(keyID)
+	c.Assert(err, IsNil)
+
+	err = gkms.keypairMgr.Delete(keyID)
+	c.Assert(err, IsNil)
+
+	err = gkms.keypairMgr.Delete(keyID)
+	c.Check(err, ErrorMatches, `cannot find key.*`)
+
+	_, err = gkms.keypairMgr.Get(keyID)
+	c.Check(err, ErrorMatches, `cannot find key.*`)
+}

asserts/memkeypairmgr.go

@@ -57,3 +57,15 @@ func (mkm *memoryKeypairManager) Get(keyID string) (PrivateKey, error) {
 	}
 	return privKey, nil
 }
+
+func (mkm *memoryKeypairManager) Delete(keyID string) error {
+	mkm.mu.RLock()
+	defer mkm.mu.RUnlock()
+
+	_, ok := mkm.pairs[keyID]
+	if !ok {
+		return errKeypairNotFound
+	}
+	delete(mkm.pairs, keyID)
+	return nil
+}

asserts/memkeypairmgr_test.go

@@ -71,3 +71,22 @@ func (mkms *memKeypairMgtSuite) TestGetNotFound(c *C) {
 	c.Check(got, IsNil)
 	c.Check(err, ErrorMatches, "cannot find key pair")
 }
+
+func (mkms *memKeypairMgtSuite) TestDelete(c *C) {
+	pk1 := testPrivKey1
+	keyID := pk1.PublicKey().ID()
+	err := mkms.keypairMgr.Put(pk1)
+	c.Assert(err, IsNil)
+
+	_, err = mkms.keypairMgr.Get(keyID)
+	c.Assert(err, IsNil)
+
+	err = mkms.keypairMgr.Delete(keyID)
+	c.Assert(err, IsNil)
+
+	err = mkms.keypairMgr.Delete(keyID)
+	c.Check(err, ErrorMatches, "cannot find key pair")
+
+	_, err = mkms.keypairMgr.Get(keyID)
+	c.Check(err, ErrorMatches, "cannot find key pair")
+}

cmd/snap/cmd_delete_key.go

@@ -62,7 +62,7 @@ func (x *cmdDeleteKey) Execute(args []string) error {
 	if err != nil {
 		return err
 	}
-	err = keypairMgr.Delete(string(x.Positional.KeyName))
+	err = keypairMgr.DeleteByName(string(x.Positional.KeyName))
 	if _, ok := err.(*asserts.ExternalUnsupportedOpError); ok {
 		return fmt.Errorf(i18n.G("cannot delete external keypair manager key via snap command, use the appropriate external procedure"))
 	}

cmd/snap/keymgr.go

@@ -36,7 +36,7 @@ type KeypairManager interface {
 	GetByName(keyNname string) (asserts.PrivateKey, error)
 	Export(keyName string) ([]byte, error)
 	List() ([]asserts.ExternalKeyInfo, error)
-	Delete(keyName string) error
+	DeleteByName(keyName string) error
 }
 
 func getKeypairManager() (KeypairManager, error) {

overlord/devicestate/devicemgr.go

@@ -1401,16 +1401,28 @@ func (m *DeviceManager) Unregister(opts *UnregisterOptions) error {
 			return err
 		}
 	}
+	oldKeyID := device.KeyID
 	device.Serial = ""
 	device.KeyID = ""
 	device.SessionMacaroon = ""
 	if err := m.setDevice(device); err != nil {
 		return err
 	}
-	// TODO: delete device keypair
+	// commit forgetting serial and key
+	m.state.Unlock()
+	m.state.Lock()
+	// delete the device key
+	err = m.withKeypairMgr(func(keypairMgr asserts.KeypairManager) error {
+		err := keypairMgr.Delete(oldKeyID)
+		if err != nil {
+			return fmt.Errorf("cannot delete device key pair: %v", err)
+		}
+		return nil
+	})
+
 	m.lastBecomeOperationalAttempt = time.Time{}
 	m.becomeOperationalBackoff = 0
-	return nil
+	return err
 }
 
 // device returns current device state.

overlord/devicestate/devicestate_serial_test.go

@@ -2108,6 +2108,9 @@ func (s *deviceMgrSerialSuite) testFullDeviceUnregisterReregisterClassicGeneric(
 	c.Check(device.KeyID, Equals, "")
 	// and session
 	c.Check(device.SessionMacaroon, Equals, "")
+	// key was deleted
+	_, err = devicestate.KeypairManager(s.mgr).Get(keyID1)
+	c.Check(err, ErrorMatches, "cannot find key pair")
 
 	noRegistrationUntilReboot := opts != nil && opts.NoRegistrationUntilReboot
 	noregister := filepath.Join(dirs.SnapRunDir, "noregister")

tests/main/generic-unregister/task.yaml

@@ -31,15 +31,19 @@ execute: |
        exit 0
     fi
 
+    keyfile=(/var/lib/snapd/device/private-keys-v1/*)
+    test -f "${keyfile[0]}"
+
     curl --data '{"action":"forget","no-registration-until-reboot":true}' --unix-socket /run/snapd.socket http://localhost/v2/model/serial
 
     test -f /run/snapd/noregister
 
     snap model --serial 2>&1|MATCH "error: device not registered yet"
 
+    not test -e "${keyfile[0]}"
+
     systemctl restart snapd.service
 
     snap model --serial 2>&1|MATCH "error: device not registered yet"
     snap find pc
     NOMATCH '"session-macaroon":"[^"]' < /var/lib/snapd/state.json
-