From 763cbd42ed17e477d42192e526c9b750f87ad4aa Mon Sep 17 00:00:00 2001 From: Kian Parvin Date: Mon, 13 Jan 2025 11:58:29 +0200 Subject: [PATCH] chore: store public key fingerprint Store the public key fingerprint in the DB rather than calculating it at runtime. --- internal/db/sshkeys.go | 43 +++++-------------- internal/db/sshkeys_test.go | 23 +++++----- .../022_add_fingerprint_ssh_keys.up.sql | 3 ++ internal/dbmodel/sshkeys.go | 2 + 4 files changed, 29 insertions(+), 42 deletions(-) create mode 100644 internal/dbmodel/sql/postgres/022_add_fingerprint_ssh_keys.up.sql diff --git a/internal/db/sshkeys.go b/internal/db/sshkeys.go index 92a1ede5a..ebe41e93c 100644 --- a/internal/db/sshkeys.go +++ b/internal/db/sshkeys.go @@ -4,9 +4,6 @@ package db import ( "context" - "fmt" - - gossh "golang.org/x/crypto/ssh" "github.com/canonical/jimm/v3/internal/dbmodel" "github.com/canonical/jimm/v3/internal/errors" @@ -42,27 +39,19 @@ func (d *Database) RemoveSSHKeyByFingerprint(ctx context.Context, identityName s defer durationObserver() defer servermon.ErrorCounter(servermon.DBQueryErrorCount, &err, string(op)) - var keys []dbmodel.SSHKey - if err := d.DB.Where("identity_name = ?", identityName).Find(&keys).Error; err != nil { + query := d.DB.Where("identity_name = ?", identityName). + Where("md5_fingerprint = ?", fingerprint). + Delete(&dbmodel.SSHKey{}) + + if err := query.Error; err != nil { return errors.E(op, dbError(err)) } - // It is expected that we only have 1 key that matches this fingerprint - // because of the unique constraint between users and public keys. - for _, key := range keys { - fp, err := calculateSSHFingerprint(key.PublicKey) - if err != nil { - return errors.E(op, err) - } - if fp == fingerprint { - if err := d.DB.WithContext(ctx).Delete(key).Error; err != nil { - return errors.E(op, dbError(err)) - } - return nil - } + if query.RowsAffected == 0 { + return errors.E(op, errors.CodeNotFound, "key not found") } - return errors.E(op, errors.CodeNotFound, "key not found") + return nil } // RemoveSSHKeyByComment removes a user's ssh key identified by its comment. @@ -89,9 +78,9 @@ func (d *Database) RemoveSSHKeyByComment(ctx context.Context, identityName strin return nil } -// ListSSHKeys all a user's SSH keys. -func (d *Database) ListSSHKeys(ctx context.Context, identityName string) (keys []dbmodel.SSHKey, err error) { - const op = errors.Op("db.ListSSHKeys") +// ListSSHKeysForUser lists all user's SSH keys. +func (d *Database) ListSSHKeysForUser(ctx context.Context, identityName string) (keys []dbmodel.SSHKey, err error) { + const op = errors.Op("db.ListSSHKeysForUser") if err := d.ready(); err != nil { return nil, errors.E(op, err) @@ -107,13 +96,3 @@ func (d *Database) ListSSHKeys(ctx context.Context, identityName string) (keys [ return keys, nil } - -// calculateSSHFingerprint parses an SSH public key and returns its MD5 fingerprint -func calculateSSHFingerprint(publicKey []byte) (string, error) { - parsedKey, err := gossh.ParsePublicKey(publicKey) - if err != nil { - return "", fmt.Errorf("invalid SSH key: %v", err) - } - - return gossh.FingerprintLegacyMD5(parsedKey), nil -} diff --git a/internal/db/sshkeys_test.go b/internal/db/sshkeys_test.go index dea01b8e0..303e236e9 100644 --- a/internal/db/sshkeys_test.go +++ b/internal/db/sshkeys_test.go @@ -22,9 +22,10 @@ func (s *dbSuite) TestCreateSSHKey(c *qt.C) { c.Assert(s.Database.DB.Create(u).Error, qt.IsNil) key := dbmodel.SSHKey{ - Identity: *u, - PublicKey: []byte("foo"), - KeyComment: "bar", + Identity: *u, + PublicKey: []byte("foo"), + KeyComment: "bar", + MD5Fingerprint: "fake-fingerprint", } err = s.Database.AddSSHKey(context.Background(), &key) c.Assert(err, qt.IsNil) @@ -40,7 +41,7 @@ func (s *dbSuite) TestCreateSSHKey(c *qt.C) { c.Assert(err, qt.ErrorMatches, `.*duplicate key value violates unique constraint.*`) } -func (s *dbSuite) TestListSSHKeys(c *qt.C) { +func (s *dbSuite) TestListSSHKeysForUser(c *qt.C) { err := s.Database.Migrate(context.Background()) c.Assert(err, qt.Equals, nil) @@ -52,24 +53,26 @@ func (s *dbSuite) TestListSSHKeys(c *qt.C) { c.Assert(err, qt.IsNil) c.Assert(s.Database.DB.Create(u2).Error, qt.IsNil) - key := dbmodel.SSHKey{Identity: *u, PublicKey: []byte("foo"), KeyComment: "bar"} + key := dbmodel.SSHKey{Identity: *u, PublicKey: []byte("foo"), KeyComment: "bar", MD5Fingerprint: "fake-fingerprint"} c.Assert(s.Database.DB.Create(&key).Error, qt.IsNil) - key2 := dbmodel.SSHKey{Identity: *u, PublicKey: []byte("foo2"), KeyComment: "bar2"} + key2 := dbmodel.SSHKey{Identity: *u, PublicKey: []byte("foo2"), KeyComment: "bar2", MD5Fingerprint: "fake-fingerprint"} c.Assert(s.Database.DB.Create(&key2).Error, qt.IsNil) // Key3 is owned by Alice and should not be returned. - key3 := dbmodel.SSHKey{Identity: *u2, PublicKey: []byte("foo3"), KeyComment: "bar3"} + key3 := dbmodel.SSHKey{Identity: *u2, PublicKey: []byte("foo3"), KeyComment: "bar3", MD5Fingerprint: "fake-fingerprint"} c.Assert(s.Database.DB.Create(&key3).Error, qt.IsNil) - gotKeys, err := s.Database.ListSSHKeys(context.Background(), "bob@canonical.com") + gotKeys, err := s.Database.ListSSHKeysForUser(context.Background(), "bob@canonical.com") c.Assert(err, qt.IsNil) c.Assert(gotKeys, qt.HasLen, 2) c.Assert(gotKeys[0].IdentityName, qt.Equals, "bob@canonical.com") c.Assert(gotKeys[0].KeyComment, qt.Equals, "bar") + c.Assert(gotKeys[0].MD5Fingerprint, qt.Equals, "fake-fingerprint") c.Assert(string(gotKeys[0].PublicKey), qt.Equals, "foo") c.Assert(gotKeys[1].IdentityName, qt.Equals, "bob@canonical.com") c.Assert(gotKeys[1].KeyComment, qt.Equals, "bar2") + c.Assert(gotKeys[1].MD5Fingerprint, qt.Equals, "fake-fingerprint") c.Assert(string(gotKeys[1].PublicKey), qt.Equals, "foo2") } @@ -91,11 +94,11 @@ func (s *dbSuite) TestRemoveSSHKeyByFingerprint(c *qt.C) { publicKey, err := gossh.NewPublicKey(&rsaKey.PublicKey) c.Assert(err, qt.IsNil) - key := dbmodel.SSHKey{Identity: *u, PublicKey: publicKey.Marshal(), KeyComment: "bar"} + key := dbmodel.SSHKey{Identity: *u, PublicKey: publicKey.Marshal(), KeyComment: "bar", MD5Fingerprint: gossh.FingerprintLegacyMD5(publicKey)} c.Assert(s.Database.DB.Create(&key).Error, qt.IsNil) // key2 with the same public-key but different owner should not be deleted. - key2 := dbmodel.SSHKey{Identity: *u2, PublicKey: publicKey.Marshal(), KeyComment: "bar"} + key2 := dbmodel.SSHKey{Identity: *u2, PublicKey: publicKey.Marshal(), KeyComment: "bar", MD5Fingerprint: gossh.FingerprintLegacyMD5(publicKey)} c.Assert(s.Database.DB.Create(&key2).Error, qt.IsNil) var keyCount int64 diff --git a/internal/dbmodel/sql/postgres/022_add_fingerprint_ssh_keys.up.sql b/internal/dbmodel/sql/postgres/022_add_fingerprint_ssh_keys.up.sql new file mode 100644 index 000000000..3157337ac --- /dev/null +++ b/internal/dbmodel/sql/postgres/022_add_fingerprint_ssh_keys.up.sql @@ -0,0 +1,3 @@ +-- store public keys fingerprints + +ALTER TABLE ssh_keys ADD COLUMN md5_fingerprint VARCHAR(50) NOT NULL; diff --git a/internal/dbmodel/sshkeys.go b/internal/dbmodel/sshkeys.go index 8fceac153..d6a2814cd 100644 --- a/internal/dbmodel/sshkeys.go +++ b/internal/dbmodel/sshkeys.go @@ -18,6 +18,8 @@ type SSHKey struct { // PublicKey holds the user's public SSH key. PublicKey []byte `gorm:"uniqueIndex:unique_identity_ssh_key"` + // MD5Fingerprint is the MD5 fingerprint of the public key. + MD5Fingerprint string // KeyComment holds a user provided comment. KeyComment string }