From b33f472060de0b0e6897c1ded5abbc1ecf9f2fdf Mon Sep 17 00:00:00 2001
From: Samuele Pedroni <pedronis@lucediurna.net>
Date: Thu, 2 Dec 2021 18:04:53 +0100
Subject: [PATCH] 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                           |  2 +
 asserts/export_test.go                        |  7 +++
 asserts/extkeypairmgr.go                      |  6 ++-
 asserts/extkeypairmgr_test.go                 | 14 +++++-
 asserts/fsentryutils.go                       |  5 ++
 asserts/fskeypairmgr.go                       | 14 ++++++
 asserts/fskeypairmgr_test.go                  | 30 ++++++++++++
 asserts/gpgkeypairmgr.go                      | 49 +++++++++++++++----
 asserts/gpgkeypairmgr_test.go                 | 17 +++++++
 asserts/memkeypairmgr.go                      | 12 +++++
 asserts/memkeypairmgr_test.go                 | 19 +++++++
 cmd/snap/cmd_delete_key.go                    |  2 +-
 cmd/snap/keymgr.go                            |  2 +-
 overlord/devicestate/devicemgr.go             | 16 +++++-
 .../devicestate/devicestate_serial_test.go    |  3 ++
 tests/main/generic-unregister/task.yaml       |  6 ++-
 16 files changed, 186 insertions(+), 18 deletions(-)

diff --git a/asserts/database.go b/asserts/database.go
index cebf084678d..59488ceed64 100644
--- a/asserts/database.go
+++ b/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.
diff --git a/asserts/export_test.go b/asserts/export_test.go
index e83e59c5b61..eae2bf8246d 100644
--- a/asserts/export_test.go
+++ b/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
diff --git a/asserts/extkeypairmgr.go b/asserts/extkeypairmgr.go
index 011da0cbd76..a84fde34443 100644
--- a/asserts/extkeypairmgr.go
+++ b/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"}
 }
 
diff --git a/asserts/extkeypairmgr_test.go b/asserts/extkeypairmgr_test.go
index c8e734b50f7..f534dbf21a8 100644
--- a/asserts/extkeypairmgr_test.go
+++ b/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{})
 
diff --git a/asserts/fsentryutils.go b/asserts/fsentryutils.go
index ca057d8c760..858405a459e 100644
--- a/asserts/fsentryutils.go
+++ b/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)
+}
diff --git a/asserts/fskeypairmgr.go b/asserts/fskeypairmgr.go
index 5a58ae17164..9fc81427c1b 100644
--- a/asserts/fskeypairmgr.go
+++ b/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
+}
diff --git a/asserts/fskeypairmgr_test.go b/asserts/fskeypairmgr_test.go
index 422ccddeda2..0391b99443c 100644
--- a/asserts/fskeypairmgr_test.go
+++ b/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")
+}
diff --git a/asserts/gpgkeypairmgr.go b/asserts/gpgkeypairmgr.go
index c6d0cafd8fd..1f5044b696e 100644
--- a/asserts/gpgkeypairmgr.go
+++ b/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
diff --git a/asserts/gpgkeypairmgr_test.go b/asserts/gpgkeypairmgr_test.go
index 6885214bc33..709982c537b 100644
--- a/asserts/gpgkeypairmgr_test.go
+++ b/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.*`)
+}
diff --git a/asserts/memkeypairmgr.go b/asserts/memkeypairmgr.go
index 68293a25d6e..75555f81f51 100644
--- a/asserts/memkeypairmgr.go
+++ b/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
+}
diff --git a/asserts/memkeypairmgr_test.go b/asserts/memkeypairmgr_test.go
index a99018ff43f..c812787cd35 100644
--- a/asserts/memkeypairmgr_test.go
+++ b/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")
+}
diff --git a/cmd/snap/cmd_delete_key.go b/cmd/snap/cmd_delete_key.go
index 517f6b936c6..f4ed9b644de 100644
--- a/cmd/snap/cmd_delete_key.go
+++ b/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"))
 	}
diff --git a/cmd/snap/keymgr.go b/cmd/snap/keymgr.go
index 52dcb3fe082..20dac991737 100644
--- a/cmd/snap/keymgr.go
+++ b/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) {
diff --git a/overlord/devicestate/devicemgr.go b/overlord/devicestate/devicemgr.go
index e3b32d8166a..3bef611879b 100644
--- a/overlord/devicestate/devicemgr.go
+++ b/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.
diff --git a/overlord/devicestate/devicestate_serial_test.go b/overlord/devicestate/devicestate_serial_test.go
index 5cae183515f..125289789aa 100644
--- a/overlord/devicestate/devicestate_serial_test.go
+++ b/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")
diff --git a/tests/main/generic-unregister/task.yaml b/tests/main/generic-unregister/task.yaml
index 43109974d8f..4b44bf394c1 100644
--- a/tests/main/generic-unregister/task.yaml
+++ b/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
-