From 73ec1e27b9efb75c37a7902956747f785bb9b441 Mon Sep 17 00:00:00 2001 From: Thomas Miller Date: Tue, 11 Jun 2024 16:59:47 +0100 Subject: [PATCH 01/10] refactor: service implementation of authorised keys. This commit introduces the services layer for authorised keys. The logic implemented in the services layer is taken from what we already have in the keymanager facade. As an implementation detail we still expect authorised keys to live in model config for the forseeable future. --- core/user/user.go | 3 +- domain/keys/doc.go | 17 ++ domain/keys/errors/errors.go | 19 ++ domain/keys/service/package_test.go | 16 ++ domain/keys/service/service.go | 282 ++++++++++++++++++++++ domain/keys/service/service_test.go | 314 +++++++++++++++++++++++++ domain/keys/service/state_mock_test.go | 166 +++++++++++++ internal/ssh/authorisedkey.go | 38 +++ 8 files changed, 854 insertions(+), 1 deletion(-) create mode 100644 domain/keys/doc.go create mode 100644 domain/keys/errors/errors.go create mode 100644 domain/keys/service/package_test.go create mode 100644 domain/keys/service/service.go create mode 100644 domain/keys/service/service_test.go create mode 100644 domain/keys/service/state_mock_test.go create mode 100644 internal/ssh/authorisedkey.go diff --git a/core/user/user.go b/core/user/user.go index 798cf892500..7ff34e8eb39 100644 --- a/core/user/user.go +++ b/core/user/user.go @@ -51,7 +51,8 @@ func NewUUID() (UUID, error) { return UUID(uuid.String()), nil } -// Validate returns an error if the UUID is invalid. +// Validate returns an error if the UUID is invalid. The error returned +// satisfies [errors.NotValid]. func (u UUID) Validate() error { if u == "" { return fmt.Errorf("empty uuid%w", errors.Hide(errors.NotValid)) diff --git a/domain/keys/doc.go b/domain/keys/doc.go new file mode 100644 index 00000000000..adef98ef1ec --- /dev/null +++ b/domain/keys/doc.go @@ -0,0 +1,17 @@ +// Copyright 2024 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +// Package keys provides the domain needed for configuring authorised keys on a +// model for a user. +// +// This package still uses model config as an implementation detail for storing +// user authorised keys. Because of this implementation detail we have to +// discard the association between a user and their authorised keys. +// +// Also because of the way we are persisting this data we also don't have any +// transactionality when modifying authorised keys potentially resulting in data +// that may not be accurate. +// +// Both of the problems described above exist with the original implementation +// using Mongo and model config. +package keys diff --git a/domain/keys/errors/errors.go b/domain/keys/errors/errors.go new file mode 100644 index 00000000000..f286640c5ec --- /dev/null +++ b/domain/keys/errors/errors.go @@ -0,0 +1,19 @@ +package errors + +import ( + "github.com/juju/errors" +) + +const ( + // AuthorisedKeyAlreadyExists indicates that the authorised key already + // exists for the specified user. + AuthorisedKeyAlreadyExists = errors.ConstError("authorised key already exists") + + // InvalidAuthorisedKey indicates a problem with an authorised key where it + // was unable to be understood. + InvalidAuthorisedKey = errors.ConstError("invalid authorised key") + + // ReservedCommentViolation indicates that a key contains a comment that is + // reserved within the Juju system and cannot be used. + ReservedCommentViolation = errors.ConstError("key contains a reserved comment") +) diff --git a/domain/keys/service/package_test.go b/domain/keys/service/package_test.go new file mode 100644 index 00000000000..c99da5b9497 --- /dev/null +++ b/domain/keys/service/package_test.go @@ -0,0 +1,16 @@ +// Copyright 2024 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package service + +import ( + "testing" + + gc "gopkg.in/check.v1" +) + +//go:generate go run go.uber.org/mock/mockgen -typed -package service -destination state_mock_test.go github.com/juju/juju/domain/keys/service State + +func TestPackage(t *testing.T) { + gc.TestingT(t) +} diff --git a/domain/keys/service/service.go b/domain/keys/service/service.go new file mode 100644 index 00000000000..5b7504d339e --- /dev/null +++ b/domain/keys/service/service.go @@ -0,0 +1,282 @@ +// Copyright 2024 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package service + +import ( + "context" + "fmt" + + "github.com/juju/collections/set" + "github.com/juju/errors" + + "github.com/juju/juju/core/user" + keyserrors "github.com/juju/juju/domain/keys/errors" + "github.com/juju/juju/environs/config" + "github.com/juju/juju/internal/ssh" +) + +// keyDataForDelete represents all of the current authorised keys for a user +// indexed by they key data itself, fingerprint and comment. +type keyDataForDelete struct { + allKeys map[string]keyInfo + byFingerprint map[string]string + byComment map[string]string +} + +// keyInfo holds the reverse index keys for raw key data. +type keyInfo struct { + comment string + fingerprint string +} + +// Service provides the means for interacting with a users underlying +// authorised keys for a model. +type Service struct { + st State +} + +// State provides the access layer the [Service] needs for persisting and +// retrieving a user authorised keys on a model. +type State interface { + // AddAuthorisedKeysForUser adds a set of keys as authorised for a user on + // this model. + AddAuthorisedKeysForUser(context.Context, user.UUID, ...string) error + + // GetAuthorisedKeysForUser is responsible for returning all of the + // authorised keys for the current model. + GetAuthorisedKeysForUser(context.Context, user.UUID) ([]string, error) + + // DeleteAuthorisedKeysForUser is responsible for removing the keys form the + // users list of authorised keys. + DeleteAuthorisedKeysForUser(context.Context, user.UUID, ...string) error +} + +var ( + // reservedAuthorisedKeyComments is the set of comments that can not be + // removed or added by a user. + reservedAuthorisedKeyComments = set.NewStrings( + "juju-client-key", + config.JujuSystemKey, + ) +) + +// NewService constructs a new [Service] for interfacting with a users +// authorised keys. +func NewService(state State) *Service { + return &Service{ + st: state, + } +} + +// AddKeysForUser is responsible for adding authorised keys for a user to a +// model. Keys being added for a user will be de-duped. The following errors can be expected: +// - [errors.NotValid] when the user id is not valid +// - [github.com/juju/juju/domain/access/errors.UserNotFound] when the user does +// not exist. +// - [keyserrors.InvalidAuthorisedKey] when an authorised key fails validation. +// - [keyserrors.ReservedCommentViolation] when a key being added contains a +// comment string that is reserved. +// - [keyserrors.AuthorisedKeyAlreadyExists] when an authorised key being added +// for a user already exists. +func (s *Service) AddKeysForUser(ctx context.Context, userID user.UUID, keys ...string) error { + if err := userID.Validate(); err != nil { + return fmt.Errorf("validating user id %q when adding authorised keys: %w", userID, err) + } + + if len(keys) == 0 { + return nil + } + + fingerprints, err := s.getExistingFingerprints(ctx, userID) + if err != nil { + return fmt.Errorf( + "adding %d authorised keys for user %q: %w", + len(keys), userID, err, + ) + } + + // alreadySeen holds a list of fingerprints already seen during the add + // operation. This is to ensure that on the off chance the caller passes the + // the same key twice to add we only add it once. + alreadySeen := map[string]struct{}{} + toAdd := make([]string, 0, len(keys)) + for _, key := range keys { + parsedKey, err := ssh.ParseAuthorisedKey(key) + if err != nil { + return fmt.Errorf("%w %q: %w", keyserrors.InvalidAuthorisedKey, key, err) + } + + fingerprint := parsedKey.Fingerprint() + if fingerprints.Contains(fingerprint) { + return fmt.Errorf( + "authorised key %q already exists for user %q: %w", + key, userID, errors.Hide(keyserrors.AuthorisedKeyAlreadyExists), + ) + } + + if reservedAuthorisedKeyComments.Contains(parsedKey.Comment) { + return fmt.Errorf( + "authorised key %q contains a reserved comment %q that cannot be used: %w", + key, parsedKey.Comment, errors.Hide(keyserrors.ReservedCommentViolation), + ) + } + + if _, seen := alreadySeen[fingerprint]; seen { + continue + } + toAdd = append(toAdd, key) + } + + return s.st.AddAuthorisedKeysForUser(ctx, userID, toAdd...) +} + +// DeleteKeysForUser removes the keys associated with targets from the users +// list of authorised keys. Targets can be an arbitary list of a authorised +// key's fingerprint, comment or full key value to be removed. Where a match is +// found the key will be removed. If no key exists for a target this will result +// in no operation. The following errors can be expected: +// - [errors.NotValid] when the user id is not valid +// - [accesserrors.UserNotFound] when the provided user does not exist. +func (s *Service) DeleteKeysForUser( + ctx context.Context, + userID user.UUID, + targets ...string, +) error { + if err := userID.Validate(); err != nil { + return fmt.Errorf( + "validating user id %q when deleting authorised keys: %w", + userID, err, + ) + } + + keyData, err := s.getExistingKeysForDelete(ctx, userID) + if err != nil { + return fmt.Errorf( + "getting authorised keys for user %q to delete keys: %w", + userID, err, + ) + } + + // Deffensive programming to make sure we don't allocate a bunch of memory + // from a destructive call. + maxToRemoveCount := len(targets) + if len(keyData.allKeys) < maxToRemoveCount { + maxToRemoveCount = len(keyData.allKeys) + } + + var ( + key string + keysToRemove = make([]string, 0, maxToRemoveCount) + exists bool + ) + // Range over the list of targets and see if it actually matches a key we + // have for the user. + for _, target := range targets { + if key, exists = keyData.byComment[target]; exists { + } else if key, exists = keyData.byFingerprint[target]; exists { + } else { + key = target + } + + if _, exists = keyData.allKeys[key]; !exists { + // Break out if the key doesn't exist. + continue + } + + keysToRemove = append(keysToRemove, key) + delete(keyData.byComment, keyData.allKeys[key].comment) + delete(keyData.byFingerprint, keyData.allKeys[key].fingerprint) + delete(keyData.allKeys, key) + } + + if len(keysToRemove) == 0 { + return nil + } + + return s.st.DeleteAuthorisedKeysForUser(ctx, userID, keysToRemove...) +} + +// getExistingFingerprints returns the currently set authorised keys and their +// associated fingerprints for the current user on this model. +func (s *Service) getExistingFingerprints( + ctx context.Context, + userID user.UUID, +) (set.Strings, error) { + keys, err := s.st.GetAuthorisedKeysForUser(ctx, userID) + if err != nil { + return set.Strings{}, fmt.Errorf( + "cannot get authorised keys for user %q: %w", + userID, err, + ) + } + + fingerprints := set.Strings{} + for i, key := range keys { + authKey, err := ssh.ParseAuthorisedKey(key) + if err != nil { + return set.Strings{}, fmt.Errorf( + "parsing user %q authorised key %d: %w", + userID, i, err, + ) + } + fingerprints.Add(authKey.Fingerprint()) + } + + return fingerprints, nil +} + +// getExistingKeysForDelete returns the authorised keys for the user indexed on +// the key data, fingerprint and comment. This function exists to help provide +// the information necessary to assist delete operations. +func (s *Service) getExistingKeysForDelete( + ctx context.Context, + userID user.UUID, +) (keyDataForDelete, error) { + keys, err := s.st.GetAuthorisedKeysForUser(ctx, userID) + if err != nil { + return keyDataForDelete{}, fmt.Errorf( + "cannot get authorised keys for user %q: %w", + userID, err, + ) + } + + rval := keyDataForDelete{ + allKeys: make(map[string]keyInfo, len(keys)), + byFingerprint: make(map[string]string, len(keys)), + byComment: make(map[string]string, len(keys)), + } + for i, key := range keys { + authKey, err := ssh.ParseAuthorisedKey(key) + if err != nil { + return keyDataForDelete{}, fmt.Errorf( + "parsing user %q authorised key %d: %w", + userID, i, err, + ) + } + + fingerprint := authKey.Fingerprint() + rval.byFingerprint[fingerprint] = key + rval.byComment[authKey.Comment] = key + rval.allKeys[key] = keyInfo{ + fingerprint: fingerprint, + comment: authKey.Comment, + } + } + return rval, nil +} + +// ListKeysForUser is responsible for returning the authorised ssh keys for the +// specified user. The following errors can be expected: +// - [errors.NotValid] when the user id is not valid. +// - [usererrors.NotFound] when the given user does not exist. +func (s *Service) ListKeysForUser(ctx context.Context, userID user.UUID) ([]string, error) { + if err := userID.Validate(); err != nil { + return nil, fmt.Errorf( + "validating user id %q when listing authorised keys: %w", + userID, err, + ) + } + + return s.st.GetAuthorisedKeysForUser(ctx, userID) +} diff --git a/domain/keys/service/service_test.go b/domain/keys/service/service_test.go new file mode 100644 index 00000000000..0760ce6c495 --- /dev/null +++ b/domain/keys/service/service_test.go @@ -0,0 +1,314 @@ +// Copyright 2024 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package service + +import ( + "context" + + "github.com/juju/errors" + "github.com/juju/testing" + jc "github.com/juju/testing/checkers" + gomock "go.uber.org/mock/gomock" + gc "gopkg.in/check.v1" + + "github.com/juju/juju/core/user" + usertesting "github.com/juju/juju/core/user/testing" + accesserrors "github.com/juju/juju/domain/access/errors" + keyserrors "github.com/juju/juju/domain/keys/errors" + "github.com/juju/juju/internal/ssh" +) + +type serviceSuite struct { + testing.IsolationSuite + + state *MockState + userID user.UUID +} + +var ( + _ = gc.Suite(&serviceSuite{}) + + existingUserKeys = []string{ + "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAII4GpCvqUUYUJlx6d1kpUO9k/t4VhSYsf0yE0/QTqDzC existing1", + "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIJQJ9wv0uC3yytXM3d2sJJWvZLuISKo7ZHwafHVviwVe existing2", + } + + // testingKeys represents a set of keys that can be used and are valid. + testingKeys = []string{ + // ecdsa testing public key + "ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBG00bYFLb/sxPcmVRMg8NXZK/ldefElAkC9wD41vABdHZiSRvp+2y9BMNVYzE/FnzKObHtSvGRX65YQgRn7k5p0= juju@example.com", + + // ed25519 testing public key + "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIN8h8XBpjS9aBUG5cdoSWubs7wT2Lc/BEZIUQCqoaOZR juju@example.com", + + // rsa testing public key + "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQDvplNOK3UBpULZKvZf/I5JHci/DufpSxj8yR4yKE2grescJxu6754jPT3xztSeLGD31/oJApJZGkMUAMRenvDqIaq+taRfOUo/l19AlGZc+Edv4bTlJzZ1Lzwex1vvL1doaLb/f76IIUHClGUgIXRceQH1ovHiIWj6nGltuLanG8YTWxlzzK33yhitmZt142DmpX1VUVF5c/Hct6Rav5lKmwej1TDed1KmHzXVoTHEsmWhKsOK27ue5yTuq0GX6LrAYDucF+2MqZCsuddXsPAW1tj5GNZSR7RrKW5q1CI0G7k9gSomuCsRMlCJ3BqID/vUSs/0qOWg4he0HUsYKQSrXIhckuZu+jYP8B80MoXT50ftRidoG/zh/PugBdXTk46FloVClQopG5A2fbqrphADcUUbRUxZ2lWQN+OVHKfEsfV2b8L2aSqZUGlryfW1cirB5JCTDvtv7rUy9/ny9iKA+8tAyKSDF0I901RDDqKc9dSkrHCg2bLnJZDoiRoWczE= juju@example.com", + } + + // reservedKeys are keys with reserved comments that can not be added or + // removed via the service. + reservedKeys = []string{ + "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIN8h8XBpjS9aBUG5cdoSWubs7wT2Lc/BEZIUQCqoaOZR juju-client-key", + "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIN8h8XBpjS9aBUG5cdoSWubs7wT2Lc/BEZIUQCqoaOZR juju-system-key", + } +) + +func (s *serviceSuite) setupMocks(c *gc.C) *gomock.Controller { + ctrl := gomock.NewController(c) + s.state = NewMockState(ctrl) + return ctrl +} + +func (s *serviceSuite) SetUpTest(c *gc.C) { + s.userID = usertesting.GenUserUUID(c) +} + +// TestAddKeysForInvalidUser is asserting that if we pass in an invalid user id +// to [Service.AddKeysForUser] we get back a [errors.NotValid] error. +func (s *serviceSuite) TestAddKeysForInvalidUser(c *gc.C) { + defer s.setupMocks(c).Finish() + + err := NewService(s.state). + AddKeysForUser(context.Background(), user.UUID("notvalid"), "key") + c.Check(err, jc.ErrorIs, errors.NotValid) +} + +// TestAddKeysForNonExistentUser is testing that if a user id doesn't exist that +// the services layer correctly passes back the [accesserrors.UserNotFound] +// error. +func (s *serviceSuite) TestAddKeysForNonExistentUser(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID).Return(nil, accesserrors.UserNotFound) + svc := NewService(s.state) + err := svc.AddKeysForUser(context.Background(), s.userID, testingKeys[0]) + c.Check(err, jc.ErrorIs, accesserrors.UserNotFound) +} + +// TestAddInvalidKeys is testing that if we try and add one or more keys that +// are invalid we get back a [keyserrors.InvalidAuthorisedKey] key error and no +// modificiation to state is performed. +func (s *serviceSuite) TestAddInvalidKeys(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). + Return(existingUserKeys, nil).AnyTimes() + svc := NewService(s.state) + + err := svc.AddKeysForUser(context.Background(), s.userID, "notvalid") + c.Check(err, jc.ErrorIs, keyserrors.InvalidAuthorisedKey) + + err = svc.AddKeysForUser(context.Background(), s.userID, "notvalid", testingKeys[0]) + c.Check(err, jc.ErrorIs, keyserrors.InvalidAuthorisedKey) + + err = svc.AddKeysForUser(context.Background(), s.userID, testingKeys[0], "notvalid") + c.Check(err, jc.ErrorIs, keyserrors.InvalidAuthorisedKey) + + err = svc.AddKeysForUser(context.Background(), s.userID, testingKeys[0], "notvalid", testingKeys[1]) + c.Check(err, jc.ErrorIs, keyserrors.InvalidAuthorisedKey) +} + +// TestAddReservedKeys is testing that if we try and add authorised keys with +// reserved comments we get back an error that satisfies +// [keyserrors.ReservedCommentViolation] +func (s *serviceSuite) TestAddReservedKeys(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). + Return(existingUserKeys, nil).AnyTimes() + svc := NewService(s.state) + + err := svc.AddKeysForUser(context.Background(), s.userID, reservedKeys...) + c.Check(err, jc.ErrorIs, keyserrors.ReservedCommentViolation) + + err = svc.AddKeysForUser(context.Background(), s.userID, testingKeys[0], reservedKeys[0]) + c.Check(err, jc.ErrorIs, keyserrors.ReservedCommentViolation) +} + +// TestAddExistingKeysForUser is asserting that when we try and add a key for a +// user that already exists we get back a +// [keyserrors.AuthorisedKeyAlreadyExists] error. +func (s *serviceSuite) TestAddExistingKeysForUser(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). + Return(existingUserKeys, nil).AnyTimes() + svc := NewService(s.state) + + err := svc.AddKeysForUser(context.Background(), s.userID, existingUserKeys[0]) + c.Check(err, jc.ErrorIs, keyserrors.AuthorisedKeyAlreadyExists) + + err = svc.AddKeysForUser(context.Background(), s.userID, testingKeys[1], existingUserKeys[1]) + c.Check(err, jc.ErrorIs, keyserrors.AuthorisedKeyAlreadyExists) +} + +// TestAddKeysForUserDedupe is asserting that adding the same keys many times +// the result is de-duplicated. +func (s *serviceSuite) TestAddKeysForUserDedupe(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). + Return(existingUserKeys, nil).AnyTimes() + svc := NewService(s.state) + + s.state.EXPECT().AddAuthorisedKeysForUser(gomock.Any(), s.userID, testingKeys[0]) + + err := svc.AddKeysForUser(context.Background(), s.userID, testingKeys[0], testingKeys[0]) + c.Check(err, jc.ErrorIsNil) +} + +// TestAddKeysForUser is testing the happy path of [Service.AddKeysForUser] +func (s *serviceSuite) TestAddKeysForUser(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). + Return(existingUserKeys, nil).AnyTimes() + svc := NewService(s.state) + + s.state.EXPECT().AddAuthorisedKeysForUser(gomock.Any(), s.userID, testingKeys) + + err := svc.AddKeysForUser(context.Background(), s.userID, testingKeys...) + c.Check(err, jc.ErrorIsNil) +} + +// TestListKeysForInvalidUserId is testing that if we pass in a junk non valid +// user id to [Service.ListKeysForUser] we get back a [errors.NotValid] error. +func (s *serviceSuite) TestListKeysForInvalidUserId(c *gc.C) { + defer s.setupMocks(c).Finish() + + svc := NewService(s.state) + + _, err := svc.ListKeysForUser(context.Background(), user.UUID("not-valid")) + c.Check(err, jc.ErrorIs, errors.NotValid) +} + +// TestListKeysForNonExistentUser is testing that if we ask for the keys for a +// non existent user we get back an error that satisfies +// [accesserrors.UserNotFound]. +func (s *serviceSuite) TestListKeysForNonExistentUser(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). + Return(nil, accesserrors.UserNotFound).AnyTimes() + svc := NewService(s.state) + + _, err := svc.ListKeysForUser(context.Background(), s.userID) + c.Check(err, jc.ErrorIs, accesserrors.UserNotFound) +} + +// TestListKeysForUser is testing the happy path for [Service.ListKeysForUser]. +func (s *serviceSuite) TestListKeysForUser(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). + Return(existingUserKeys, nil) + svc := NewService(s.state) + + keys, err := svc.ListKeysForUser(context.Background(), s.userID) + c.Check(err, jc.ErrorIsNil) + c.Check(keys, gc.DeepEquals, existingUserKeys) +} + +// TestDeleteKeysForInvalidUser is asserting that if we pass an invalid user id +// to [Service.DeleteKeysForUser] we get back an [errors.NotValid] error. +func (s *serviceSuite) TestDeleteKeysForInvalidUser(c *gc.C) { + defer s.setupMocks(c).Finish() + + err := NewService(s.state). + DeleteKeysForUser(context.Background(), user.UUID("notvalid"), "key") + c.Check(err, jc.ErrorIs, errors.NotValid) +} + +// TestDeleteKeysForUserNotFound is asserting that if the state layer +// propogrates a [accesserrors.UserNotFound] that the service layer returns the +// error back up. +func (s *serviceSuite) TestDeleteKeysForUserNotFound(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). + Return(nil, accesserrors.UserNotFound) + + err := NewService(s.state). + DeleteKeysForUser(context.Background(), s.userID, testingKeys[0]) + c.Check(err, jc.ErrorIs, accesserrors.UserNotFound) +} + +// TestDeleteKeysForUserWithFingerprint is asserting that we can remove keys for +// a user based on the key fingerprint. +func (s *serviceSuite) TestDeleteKeysForUserWithFingerprint(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). + Return(existingUserKeys, nil) + s.state.EXPECT().DeleteAuthorisedKeysForUser(gomock.Any(), s.userID, existingUserKeys[0]). + Return(nil) + + key, err := ssh.ParseAuthorisedKey(existingUserKeys[0]) + c.Assert(err, jc.ErrorIsNil) + + err = NewService(s.state). + DeleteKeysForUser(context.Background(), s.userID, key.Fingerprint()) + c.Check(err, jc.ErrorIsNil) +} + +// TestDeleteKeysForUserWithComment is asserting that we can remove keys for a +// user based on the key comment. +func (s *serviceSuite) TestDeleteKeysForUserWithComment(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). + Return(existingUserKeys, nil) + s.state.EXPECT().DeleteAuthorisedKeysForUser(gomock.Any(), s.userID, existingUserKeys[0]). + Return(nil) + + key, err := ssh.ParseAuthorisedKey(existingUserKeys[0]) + c.Assert(err, jc.ErrorIsNil) + + err = NewService(s.state). + DeleteKeysForUser(context.Background(), s.userID, key.Comment) + c.Check(err, jc.ErrorIsNil) +} + +// TestDeleteKeysForUserData is asserting that we can remove ssh keys for a user +// based on the raw key data. +func (s *serviceSuite) TestDeleteKeysForUserData(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). + Return(existingUserKeys, nil) + s.state.EXPECT().DeleteAuthorisedKeysForUser(gomock.Any(), s.userID, existingUserKeys[0]). + Return(nil) + + err := NewService(s.state). + DeleteKeysForUser(context.Background(), s.userID, existingUserKeys[0]) + c.Check(err, jc.ErrorIsNil) +} + +// TestDeleteKeysForUserCombination is asserting multiple deletes using +// different targets. We want to see that with a set of different target types +// all the correct keys are removed. +func (s *serviceSuite) TestDeleteKeysForUserCombination(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). + Return(existingUserKeys, nil) + s.state.EXPECT().DeleteAuthorisedKeysForUser( + gomock.Any(), + s.userID, + existingUserKeys[0], + existingUserKeys[1], + ).Return(nil) + + key, err := ssh.ParseAuthorisedKey(existingUserKeys[0]) + c.Assert(err, jc.ErrorIsNil) + + err = NewService(s.state). + DeleteKeysForUser( + context.Background(), + s.userID, + key.Comment, + existingUserKeys[1], + ) + c.Check(err, jc.ErrorIsNil) +} diff --git a/domain/keys/service/state_mock_test.go b/domain/keys/service/state_mock_test.go new file mode 100644 index 00000000000..8a82a9b6fd8 --- /dev/null +++ b/domain/keys/service/state_mock_test.go @@ -0,0 +1,166 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/juju/juju/domain/keys/service (interfaces: State) +// +// Generated by this command: +// +// mockgen -typed -package service -destination state_mock_test.go github.com/juju/juju/domain/keys/service State +// + +// Package service is a generated GoMock package. +package service + +import ( + context "context" + reflect "reflect" + + user "github.com/juju/juju/core/user" + gomock "go.uber.org/mock/gomock" +) + +// MockState is a mock of State interface. +type MockState struct { + ctrl *gomock.Controller + recorder *MockStateMockRecorder +} + +// MockStateMockRecorder is the mock recorder for MockState. +type MockStateMockRecorder struct { + mock *MockState +} + +// NewMockState creates a new mock instance. +func NewMockState(ctrl *gomock.Controller) *MockState { + mock := &MockState{ctrl: ctrl} + mock.recorder = &MockStateMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockState) EXPECT() *MockStateMockRecorder { + return m.recorder +} + +// AddAuthorisedKeysForUser mocks base method. +func (m *MockState) AddAuthorisedKeysForUser(arg0 context.Context, arg1 user.UUID, arg2 ...string) error { + m.ctrl.T.Helper() + varargs := []any{arg0, arg1} + for _, a := range arg2 { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "AddAuthorisedKeysForUser", varargs...) + ret0, _ := ret[0].(error) + return ret0 +} + +// AddAuthorisedKeysForUser indicates an expected call of AddAuthorisedKeysForUser. +func (mr *MockStateMockRecorder) AddAuthorisedKeysForUser(arg0, arg1 any, arg2 ...any) *MockStateAddAuthorisedKeysForUserCall { + mr.mock.ctrl.T.Helper() + varargs := append([]any{arg0, arg1}, arg2...) + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddAuthorisedKeysForUser", reflect.TypeOf((*MockState)(nil).AddAuthorisedKeysForUser), varargs...) + return &MockStateAddAuthorisedKeysForUserCall{Call: call} +} + +// MockStateAddAuthorisedKeysForUserCall wrap *gomock.Call +type MockStateAddAuthorisedKeysForUserCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockStateAddAuthorisedKeysForUserCall) Return(arg0 error) *MockStateAddAuthorisedKeysForUserCall { + c.Call = c.Call.Return(arg0) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockStateAddAuthorisedKeysForUserCall) Do(f func(context.Context, user.UUID, ...string) error) *MockStateAddAuthorisedKeysForUserCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockStateAddAuthorisedKeysForUserCall) DoAndReturn(f func(context.Context, user.UUID, ...string) error) *MockStateAddAuthorisedKeysForUserCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + +// DeleteAuthorisedKeysForUser mocks base method. +func (m *MockState) DeleteAuthorisedKeysForUser(arg0 context.Context, arg1 user.UUID, arg2 ...string) error { + m.ctrl.T.Helper() + varargs := []any{arg0, arg1} + for _, a := range arg2 { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "DeleteAuthorisedKeysForUser", varargs...) + ret0, _ := ret[0].(error) + return ret0 +} + +// DeleteAuthorisedKeysForUser indicates an expected call of DeleteAuthorisedKeysForUser. +func (mr *MockStateMockRecorder) DeleteAuthorisedKeysForUser(arg0, arg1 any, arg2 ...any) *MockStateDeleteAuthorisedKeysForUserCall { + mr.mock.ctrl.T.Helper() + varargs := append([]any{arg0, arg1}, arg2...) + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAuthorisedKeysForUser", reflect.TypeOf((*MockState)(nil).DeleteAuthorisedKeysForUser), varargs...) + return &MockStateDeleteAuthorisedKeysForUserCall{Call: call} +} + +// MockStateDeleteAuthorisedKeysForUserCall wrap *gomock.Call +type MockStateDeleteAuthorisedKeysForUserCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockStateDeleteAuthorisedKeysForUserCall) Return(arg0 error) *MockStateDeleteAuthorisedKeysForUserCall { + c.Call = c.Call.Return(arg0) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockStateDeleteAuthorisedKeysForUserCall) Do(f func(context.Context, user.UUID, ...string) error) *MockStateDeleteAuthorisedKeysForUserCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockStateDeleteAuthorisedKeysForUserCall) DoAndReturn(f func(context.Context, user.UUID, ...string) error) *MockStateDeleteAuthorisedKeysForUserCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + +// GetAuthorisedKeysForUser mocks base method. +func (m *MockState) GetAuthorisedKeysForUser(arg0 context.Context, arg1 user.UUID) ([]string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetAuthorisedKeysForUser", arg0, arg1) + ret0, _ := ret[0].([]string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetAuthorisedKeysForUser indicates an expected call of GetAuthorisedKeysForUser. +func (mr *MockStateMockRecorder) GetAuthorisedKeysForUser(arg0, arg1 any) *MockStateGetAuthorisedKeysForUserCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetAuthorisedKeysForUser", reflect.TypeOf((*MockState)(nil).GetAuthorisedKeysForUser), arg0, arg1) + return &MockStateGetAuthorisedKeysForUserCall{Call: call} +} + +// MockStateGetAuthorisedKeysForUserCall wrap *gomock.Call +type MockStateGetAuthorisedKeysForUserCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockStateGetAuthorisedKeysForUserCall) Return(arg0 []string, arg1 error) *MockStateGetAuthorisedKeysForUserCall { + c.Call = c.Call.Return(arg0, arg1) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockStateGetAuthorisedKeysForUserCall) Do(f func(context.Context, user.UUID) ([]string, error)) *MockStateGetAuthorisedKeysForUserCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockStateGetAuthorisedKeysForUserCall) DoAndReturn(f func(context.Context, user.UUID) ([]string, error)) *MockStateGetAuthorisedKeysForUserCall { + c.Call = c.Call.DoAndReturn(f) + return c +} diff --git a/internal/ssh/authorisedkey.go b/internal/ssh/authorisedkey.go new file mode 100644 index 00000000000..ebc4e8ad6d9 --- /dev/null +++ b/internal/ssh/authorisedkey.go @@ -0,0 +1,38 @@ +package ssh + +import ( + "fmt" + + "golang.org/x/crypto/ssh" +) + +// AuthorisedKey represents a single authorised key line that would commonly be +// found in a authorized_keys file. http://man.he.net/man5/authorized_keys +type AuthorisedKey struct { + // Key holds the parse key data for the public key. + Key ssh.PublicKey + + // Comment is the comment string attached to the authorised key. + Comment string +} + +// Fingerprint returns the SHA256 fingerprint of the public key. +func (a *AuthorisedKey) Fingerprint() string { + return ssh.FingerprintSHA256(a.Key) +} + +// ParseAuthorisedKey parses a single line from an authorised keys file +// returning a [AuthorisedKey] representation of the data. +// [ssh.ParseAuthorizedKey] is used to perform the underlying validating and +// parsing. +func ParseAuthorisedKey(key string) (AuthorisedKey, error) { + parsedKey, comment, _, _, err := ssh.ParseAuthorizedKey([]byte(key)) + if err != nil { + return AuthorisedKey{}, fmt.Errorf("parsing authorised key %q: %w", err) + } + + return AuthorisedKey{ + Key: parsedKey, + Comment: comment, + }, nil +} From 0dec159c341b5e13cf7f13ee37b66d572f407aab Mon Sep 17 00:00:00 2001 From: Thomas Miller Date: Tue, 11 Jun 2024 17:15:19 +0100 Subject: [PATCH 02/10] docs: documentation to include fingerprint version. Updates documentation in Juju to include the checksum version we expect for ssh fingerprints. This is to make sure that our docs are aligned with the business logic we have. --- cmd/juju/sshkeys/remove_sshkeys.go | 6 +++--- domain/keys/service/service.go | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cmd/juju/sshkeys/remove_sshkeys.go b/cmd/juju/sshkeys/remove_sshkeys.go index a6dbe0eebf2..52466aa1e7e 100644 --- a/cmd/juju/sshkeys/remove_sshkeys.go +++ b/cmd/juju/sshkeys/remove_sshkeys.go @@ -21,9 +21,9 @@ var usageRemoveSSHKeyDetails = ` Juju maintains a per-model cache of public SSH keys which it copies to each unit. This command will remove a specified key (or space separated list of keys) from the model cache and all current units deployed in that -model. The keys to be removed may be specified by the key's fingerprint, -or by the text label associated with them. Invalid keys in the model cache -can be removed by specifying the key verbatim. +model. The keys to be removed may be specified by the key's fingerprint using a +sah256 sum, or by the text label associated with them. Invalid keys in the model +cache can be removed by specifying the key verbatim. `[1:] diff --git a/domain/keys/service/service.go b/domain/keys/service/service.go index 5b7504d339e..bcb96f78aec 100644 --- a/domain/keys/service/service.go +++ b/domain/keys/service/service.go @@ -133,9 +133,9 @@ func (s *Service) AddKeysForUser(ctx context.Context, userID user.UUID, keys ... // DeleteKeysForUser removes the keys associated with targets from the users // list of authorised keys. Targets can be an arbitary list of a authorised -// key's fingerprint, comment or full key value to be removed. Where a match is -// found the key will be removed. If no key exists for a target this will result -// in no operation. The following errors can be expected: +// key's fingerprint (sha256), comment or full key value to be removed. Where a +// match is found the key will be removed. If no key exists for a target this +// will result in no operation. The following errors can be expected: // - [errors.NotValid] when the user id is not valid // - [accesserrors.UserNotFound] when the provided user does not exist. func (s *Service) DeleteKeysForUser( From 32285b323f955226ed57714c37654d2f778259d7 Mon Sep 17 00:00:00 2001 From: Thomas Miller Date: Wed, 12 Jun 2024 06:59:50 +0100 Subject: [PATCH 03/10] chore: Adding missing copyright headers. --- domain/keys/errors/errors.go | 3 +++ internal/ssh/authorisedkey.go | 3 +++ 2 files changed, 6 insertions(+) diff --git a/domain/keys/errors/errors.go b/domain/keys/errors/errors.go index f286640c5ec..f05fd093185 100644 --- a/domain/keys/errors/errors.go +++ b/domain/keys/errors/errors.go @@ -1,3 +1,6 @@ +// Copyright 2024 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + package errors import ( diff --git a/internal/ssh/authorisedkey.go b/internal/ssh/authorisedkey.go index ebc4e8ad6d9..dd7110218fb 100644 --- a/internal/ssh/authorisedkey.go +++ b/internal/ssh/authorisedkey.go @@ -1,3 +1,6 @@ +// Copyright 2024 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + package ssh import ( From 324b455ad340540b93188cb7151d9c4811e43ba4 Mon Sep 17 00:00:00 2001 From: Thomas Miller Date: Thu, 13 Jun 2024 13:48:05 +0100 Subject: [PATCH 04/10] refactor: changes user public key service. - Renaming keys service to key manager to represent the domain it is operating on. - Changed references to authorise keys just to be public keys to better reflect the work that is being done. - Changed service to better rely on the transactional nature of the database for performing crud operations on a users keys. - Introduced a public key importer. --- domain/{keys => keymanager}/doc.go | 2 +- domain/{keys => keymanager}/errors/errors.go | 8 +- .../service/package_test.go | 2 +- domain/keymanager/service/service.go | 193 ++++++++++++ .../keymanager/service/service_mock_test.go | 258 ++++++++++++++++ .../service/service_test.go | 279 ++++++++++------- domain/keymanager/types.go | 10 + domain/keys/service/service.go | 282 ------------------ domain/keys/service/state_mock_test.go | 166 ----------- internal/ssh/authorisedkey.go | 6 +- 10 files changed, 642 insertions(+), 564 deletions(-) rename domain/{keys => keymanager}/doc.go (97%) rename domain/{keys => keymanager}/errors/errors.go (59%) rename domain/{keys => keymanager}/service/package_test.go (71%) create mode 100644 domain/keymanager/service/service.go create mode 100644 domain/keymanager/service/service_mock_test.go rename domain/{keys => keymanager}/service/service_test.go (51%) create mode 100644 domain/keymanager/types.go delete mode 100644 domain/keys/service/service.go delete mode 100644 domain/keys/service/state_mock_test.go diff --git a/domain/keys/doc.go b/domain/keymanager/doc.go similarity index 97% rename from domain/keys/doc.go rename to domain/keymanager/doc.go index adef98ef1ec..99beedbeae4 100644 --- a/domain/keys/doc.go +++ b/domain/keymanager/doc.go @@ -14,4 +14,4 @@ // // Both of the problems described above exist with the original implementation // using Mongo and model config. -package keys +package keymanager diff --git a/domain/keys/errors/errors.go b/domain/keymanager/errors/errors.go similarity index 59% rename from domain/keys/errors/errors.go rename to domain/keymanager/errors/errors.go index f05fd093185..7a7d5109eb4 100644 --- a/domain/keys/errors/errors.go +++ b/domain/keymanager/errors/errors.go @@ -8,13 +8,13 @@ import ( ) const ( - // AuthorisedKeyAlreadyExists indicates that the authorised key already + // PublicKeyAlreadyExists indicates that the authorised key already // exists for the specified user. - AuthorisedKeyAlreadyExists = errors.ConstError("authorised key already exists") + PublicKeyAlreadyExists = errors.ConstError("public key already exists") - // InvalidAuthorisedKey indicates a problem with an authorised key where it + // InvalidPublicKey indicates a problem with a public key where it // was unable to be understood. - InvalidAuthorisedKey = errors.ConstError("invalid authorised key") + InvalidPublicKey = errors.ConstError("invalid public key") // ReservedCommentViolation indicates that a key contains a comment that is // reserved within the Juju system and cannot be used. diff --git a/domain/keys/service/package_test.go b/domain/keymanager/service/package_test.go similarity index 71% rename from domain/keys/service/package_test.go rename to domain/keymanager/service/package_test.go index c99da5b9497..e4c519edd5f 100644 --- a/domain/keys/service/package_test.go +++ b/domain/keymanager/service/package_test.go @@ -9,7 +9,7 @@ import ( gc "gopkg.in/check.v1" ) -//go:generate go run go.uber.org/mock/mockgen -typed -package service -destination state_mock_test.go github.com/juju/juju/domain/keys/service State +//go:generate go run go.uber.org/mock/mockgen -typed -package service -destination service_mock_test.go github.com/juju/juju/domain/keymanager/service PublicKeyImporter,State func TestPackage(t *testing.T) { gc.TestingT(t) diff --git a/domain/keymanager/service/service.go b/domain/keymanager/service/service.go new file mode 100644 index 00000000000..19607a503d7 --- /dev/null +++ b/domain/keymanager/service/service.go @@ -0,0 +1,193 @@ +// Copyright 2024 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package service + +import ( + "context" + "fmt" + "net/url" + + "github.com/juju/collections/set" + "github.com/juju/errors" + + "github.com/juju/juju/core/user" + "github.com/juju/juju/domain/keymanager" + keyserrors "github.com/juju/juju/domain/keymanager/errors" + "github.com/juju/juju/environs/config" + "github.com/juju/juju/internal/ssh" +) + +// PublicKeyImporter describes a service that is capable of fetching and +// providing public keys for a subject from a set of well known sources that +// don't need to be understood by this service. +type PublicKeyImporter interface { + // FetchPublicKeysForSubject is responsible for gathering all of the + // public keys available for a specified subject. + FetchPublicKeysForSubject(context.Context, *url.URL) ([]string, error) +} + +// Service provides the means for interacting with a users underlying +// public keys for a model. +type Service struct { + // keyImporter is the [PublicKeyImporter] to use for fetching a users + // public key's for subject. + keyImporter PublicKeyImporter + + // st is the provides the state access layer to this service. + st State +} + +// State provides the access layer the [Service] needs for persisting and +// retrieving a users public keys on a model. +type State interface { + // AddPublicKeysForUser adds a set of public keys for a user on + // this model. If one or more of the public keys to add for the user already + // exists a [keyserrors.PublicKeyAlreadyExists] error will be returned. + AddPublicKeysForUser(context.Context, user.UUID, []keymanager.PublicKey) error + + // AddPublicKeyForUserIfNotFound will attempt to add the given set of public + // keys to the user. If the user already contains the public key it will be + // skipped and no [keyserrors.PublicKeyAlreadyExists] error will be returned. + AddPublicKeyForUserIfNotFound(context.Context, user.UUID, []keymanager.PublicKey) error + + // GetPublicKeysForUser is responsible for returning all of the + // public keys for the current user in this model. + GetPublicKeysForUser(context.Context, user.UUID) ([]string, error) + + // DeletePublicKeysForUser is responsible for removing the keys form the + // users list of public keys where the string list represents one of + // the keys fingerprint, public key data or comment. + DeletePublicKeysForUser(context.Context, user.UUID, []string) error +} + +var ( + // reservedPublicKeyComments is the set of comments that can not be + // removed or added by a user. + reservedPublicKeyComments = set.NewStrings( + "juju-client-key", + config.JujuSystemKey, + ) +) + +// NewService constructs a new [Service] for interfacting with a users +// public keys. +func NewService(keyImporter PublicKeyImporter, state State) *Service { + return &Service{ + keyImporter: keyImporter, + st: state, + } +} + +// AddPublicKeysForUser is responsible for adding public keys for a user to a +// model. The following errors can be expected: +// - [errors.NotValid] when the user id is not valid +// - [github.com/juju/juju/domain/access/errors.UserNotFound] when the user does +// not exist. +// - [keyserrors.InvalidPublicKey] when a public key fails validation. +// - [keyserrors.ReservedCommentViolation] when a key being added contains a +// comment string that is reserved. +// - [keyserrors.PublicKeyAlreadyExists] when a public key being added +// for a user already exists. +func (s *Service) AddPublicKeysForUser( + ctx context.Context, + userID user.UUID, + keys ...string, +) error { + if err := userID.Validate(); err != nil { + return fmt.Errorf("validating user id %q when adding public keys: %w", userID, err) + } + + if len(keys) == 0 { + return nil + } + + toAdd := make([]keymanager.PublicKey, 0, len(keys)) + for i, keyToAdd := range keys { + parsedKey, err := ssh.ParsePublicKey(keyToAdd) + if err != nil { + return fmt.Errorf( + "%w %q at index %d: %w", + keyserrors.InvalidPublicKey, keyToAdd, i, err, + ) + } + + if reservedPublicKeyComments.Contains(parsedKey.Comment) { + return fmt.Errorf( + "public key %q at index %d contains a reserved comment %q that cannot be used: %w", + keyToAdd, + i, + parsedKey.Comment, + errors.Hide(keyserrors.ReservedCommentViolation), + ) + } + + toAdd = append(toAdd, keymanager.PublicKey{ + Comment: parsedKey.Comment, + Fingerprint: parsedKey.Fingerprint(), + Key: keyToAdd, + }) + } + + return s.st.AddPublicKeysForUser(ctx, userID, toAdd) +} + +// DeletePublicKeysForUser removes the keys associated with targets from the +// users list of public keys. Targets can be an arbitary list of a +// public key fingerprint (sha256), comment or full key value to be +// removed. Where a match is found the key will be removed. If no key exists for +// a target this will result in no operation. The following errors can be +// expected: +// - [errors.NotValid] when the user id is not valid +// - [accesserrors.UserNotFound] when the provided user does not exist. +func (s *Service) DeleteKeysForUser( + ctx context.Context, + userID user.UUID, + targets ...string, +) error { + if err := userID.Validate(); err != nil { + return fmt.Errorf( + "validating user id %q when deleting public keys: %w", + userID, err, + ) + } + + return s.st.DeletePublicKeysForUser(ctx, userID, targets) +} + +// ImportPublicKeysForUser will import all of the public keys available for a +// given subject and add them to the specified Juju user. If the user already +// has one or more of the public keys being imported they will safely be skipped +// with no errors being returned. +// The following errors can be expected: +// - [errors.NotValid] when the user id is not valid +// - [github.com/juju/juju/domain/access/errors.UserNotFound] when the user does +// not exist. +// - [keyserrors.InvalidPublicKey] when a key being imported fails validation. +// - [keyserrors.ReservedCommentViolation] when a key being added contains a +// comment string that is reserved. +func (s *Service) ImportPublicKeysForUser( + ctx context.Context, + userID user.UUID, + subject *url.URL, +) error { + return nil +} + +// ListPublicKeysForUser is responsible for returning the public ssh keys for +// the specified user. The following errors can be expected: +// - [errors.NotValid] when the user id is not valid. +// - [usererrors.NotFound] when the given user does not exist. +func (s *Service) ListPublicKeysForUser( + ctx context.Context, + userID user.UUID, +) ([]string, error) { + if err := userID.Validate(); err != nil { + return nil, fmt.Errorf( + "validating user id %q when listing public keys: %w", + userID, err, + ) + } + + return s.st.GetPublicKeysForUser(ctx, userID) +} diff --git a/domain/keymanager/service/service_mock_test.go b/domain/keymanager/service/service_mock_test.go new file mode 100644 index 00000000000..23d4dd77964 --- /dev/null +++ b/domain/keymanager/service/service_mock_test.go @@ -0,0 +1,258 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/juju/juju/domain/keymanager/service (interfaces: PublicKeyImporter,State) +// +// Generated by this command: +// +// mockgen -typed -package service -destination service_mock_test.go github.com/juju/juju/domain/keymanager/service PublicKeyImporter,State +// + +// Package service is a generated GoMock package. +package service + +import ( + context "context" + url "net/url" + reflect "reflect" + + user "github.com/juju/juju/core/user" + keymanager "github.com/juju/juju/domain/keymanager" + gomock "go.uber.org/mock/gomock" +) + +// MockPublicKeyImporter is a mock of PublicKeyImporter interface. +type MockPublicKeyImporter struct { + ctrl *gomock.Controller + recorder *MockPublicKeyImporterMockRecorder +} + +// MockPublicKeyImporterMockRecorder is the mock recorder for MockPublicKeyImporter. +type MockPublicKeyImporterMockRecorder struct { + mock *MockPublicKeyImporter +} + +// NewMockPublicKeyImporter creates a new mock instance. +func NewMockPublicKeyImporter(ctrl *gomock.Controller) *MockPublicKeyImporter { + mock := &MockPublicKeyImporter{ctrl: ctrl} + mock.recorder = &MockPublicKeyImporterMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockPublicKeyImporter) EXPECT() *MockPublicKeyImporterMockRecorder { + return m.recorder +} + +// FetchPublicKeysForSubject mocks base method. +func (m *MockPublicKeyImporter) FetchPublicKeysForSubject(arg0 context.Context, arg1 *url.URL) ([]string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "FetchPublicKeysForSubject", arg0, arg1) + ret0, _ := ret[0].([]string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// FetchPublicKeysForSubject indicates an expected call of FetchPublicKeysForSubject. +func (mr *MockPublicKeyImporterMockRecorder) FetchPublicKeysForSubject(arg0, arg1 any) *MockPublicKeyImporterFetchPublicKeysForSubjectCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FetchPublicKeysForSubject", reflect.TypeOf((*MockPublicKeyImporter)(nil).FetchPublicKeysForSubject), arg0, arg1) + return &MockPublicKeyImporterFetchPublicKeysForSubjectCall{Call: call} +} + +// MockPublicKeyImporterFetchPublicKeysForSubjectCall wrap *gomock.Call +type MockPublicKeyImporterFetchPublicKeysForSubjectCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockPublicKeyImporterFetchPublicKeysForSubjectCall) Return(arg0 []string, arg1 error) *MockPublicKeyImporterFetchPublicKeysForSubjectCall { + c.Call = c.Call.Return(arg0, arg1) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockPublicKeyImporterFetchPublicKeysForSubjectCall) Do(f func(context.Context, *url.URL) ([]string, error)) *MockPublicKeyImporterFetchPublicKeysForSubjectCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockPublicKeyImporterFetchPublicKeysForSubjectCall) DoAndReturn(f func(context.Context, *url.URL) ([]string, error)) *MockPublicKeyImporterFetchPublicKeysForSubjectCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + +// MockState is a mock of State interface. +type MockState struct { + ctrl *gomock.Controller + recorder *MockStateMockRecorder +} + +// MockStateMockRecorder is the mock recorder for MockState. +type MockStateMockRecorder struct { + mock *MockState +} + +// NewMockState creates a new mock instance. +func NewMockState(ctrl *gomock.Controller) *MockState { + mock := &MockState{ctrl: ctrl} + mock.recorder = &MockStateMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockState) EXPECT() *MockStateMockRecorder { + return m.recorder +} + +// AddPublicKeyForUserIfNotFound mocks base method. +func (m *MockState) AddPublicKeyForUserIfNotFound(arg0 context.Context, arg1 user.UUID, arg2 []keymanager.PublicKey) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AddPublicKeyForUserIfNotFound", arg0, arg1, arg2) + ret0, _ := ret[0].(error) + return ret0 +} + +// AddPublicKeyForUserIfNotFound indicates an expected call of AddPublicKeyForUserIfNotFound. +func (mr *MockStateMockRecorder) AddPublicKeyForUserIfNotFound(arg0, arg1, arg2 any) *MockStateAddPublicKeyForUserIfNotFoundCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddPublicKeyForUserIfNotFound", reflect.TypeOf((*MockState)(nil).AddPublicKeyForUserIfNotFound), arg0, arg1, arg2) + return &MockStateAddPublicKeyForUserIfNotFoundCall{Call: call} +} + +// MockStateAddPublicKeyForUserIfNotFoundCall wrap *gomock.Call +type MockStateAddPublicKeyForUserIfNotFoundCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockStateAddPublicKeyForUserIfNotFoundCall) Return(arg0 error) *MockStateAddPublicKeyForUserIfNotFoundCall { + c.Call = c.Call.Return(arg0) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockStateAddPublicKeyForUserIfNotFoundCall) Do(f func(context.Context, user.UUID, []keymanager.PublicKey) error) *MockStateAddPublicKeyForUserIfNotFoundCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockStateAddPublicKeyForUserIfNotFoundCall) DoAndReturn(f func(context.Context, user.UUID, []keymanager.PublicKey) error) *MockStateAddPublicKeyForUserIfNotFoundCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + +// AddPublicKeysForUser mocks base method. +func (m *MockState) AddPublicKeysForUser(arg0 context.Context, arg1 user.UUID, arg2 []keymanager.PublicKey) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AddPublicKeysForUser", arg0, arg1, arg2) + ret0, _ := ret[0].(error) + return ret0 +} + +// AddPublicKeysForUser indicates an expected call of AddPublicKeysForUser. +func (mr *MockStateMockRecorder) AddPublicKeysForUser(arg0, arg1, arg2 any) *MockStateAddPublicKeysForUserCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddPublicKeysForUser", reflect.TypeOf((*MockState)(nil).AddPublicKeysForUser), arg0, arg1, arg2) + return &MockStateAddPublicKeysForUserCall{Call: call} +} + +// MockStateAddPublicKeysForUserCall wrap *gomock.Call +type MockStateAddPublicKeysForUserCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockStateAddPublicKeysForUserCall) Return(arg0 error) *MockStateAddPublicKeysForUserCall { + c.Call = c.Call.Return(arg0) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockStateAddPublicKeysForUserCall) Do(f func(context.Context, user.UUID, []keymanager.PublicKey) error) *MockStateAddPublicKeysForUserCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockStateAddPublicKeysForUserCall) DoAndReturn(f func(context.Context, user.UUID, []keymanager.PublicKey) error) *MockStateAddPublicKeysForUserCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + +// DeletePublicKeysForUser mocks base method. +func (m *MockState) DeletePublicKeysForUser(arg0 context.Context, arg1 user.UUID, arg2 []string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeletePublicKeysForUser", arg0, arg1, arg2) + ret0, _ := ret[0].(error) + return ret0 +} + +// DeletePublicKeysForUser indicates an expected call of DeletePublicKeysForUser. +func (mr *MockStateMockRecorder) DeletePublicKeysForUser(arg0, arg1, arg2 any) *MockStateDeletePublicKeysForUserCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeletePublicKeysForUser", reflect.TypeOf((*MockState)(nil).DeletePublicKeysForUser), arg0, arg1, arg2) + return &MockStateDeletePublicKeysForUserCall{Call: call} +} + +// MockStateDeletePublicKeysForUserCall wrap *gomock.Call +type MockStateDeletePublicKeysForUserCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockStateDeletePublicKeysForUserCall) Return(arg0 error) *MockStateDeletePublicKeysForUserCall { + c.Call = c.Call.Return(arg0) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockStateDeletePublicKeysForUserCall) Do(f func(context.Context, user.UUID, []string) error) *MockStateDeletePublicKeysForUserCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockStateDeletePublicKeysForUserCall) DoAndReturn(f func(context.Context, user.UUID, []string) error) *MockStateDeletePublicKeysForUserCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + +// GetPublicKeysForUser mocks base method. +func (m *MockState) GetPublicKeysForUser(arg0 context.Context, arg1 user.UUID) ([]string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetPublicKeysForUser", arg0, arg1) + ret0, _ := ret[0].([]string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetPublicKeysForUser indicates an expected call of GetPublicKeysForUser. +func (mr *MockStateMockRecorder) GetPublicKeysForUser(arg0, arg1 any) *MockStateGetPublicKeysForUserCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetPublicKeysForUser", reflect.TypeOf((*MockState)(nil).GetPublicKeysForUser), arg0, arg1) + return &MockStateGetPublicKeysForUserCall{Call: call} +} + +// MockStateGetPublicKeysForUserCall wrap *gomock.Call +type MockStateGetPublicKeysForUserCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockStateGetPublicKeysForUserCall) Return(arg0 []string, arg1 error) *MockStateGetPublicKeysForUserCall { + c.Call = c.Call.Return(arg0, arg1) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockStateGetPublicKeysForUserCall) Do(f func(context.Context, user.UUID) ([]string, error)) *MockStateGetPublicKeysForUserCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockStateGetPublicKeysForUserCall) DoAndReturn(f func(context.Context, user.UUID) ([]string, error)) *MockStateGetPublicKeysForUserCall { + c.Call = c.Call.DoAndReturn(f) + return c +} diff --git a/domain/keys/service/service_test.go b/domain/keymanager/service/service_test.go similarity index 51% rename from domain/keys/service/service_test.go rename to domain/keymanager/service/service_test.go index 0760ce6c495..9be6d1830fa 100644 --- a/domain/keys/service/service_test.go +++ b/domain/keymanager/service/service_test.go @@ -15,27 +15,29 @@ import ( "github.com/juju/juju/core/user" usertesting "github.com/juju/juju/core/user/testing" accesserrors "github.com/juju/juju/domain/access/errors" - keyserrors "github.com/juju/juju/domain/keys/errors" + "github.com/juju/juju/domain/keymanager" + keyserrors "github.com/juju/juju/domain/keymanager/errors" "github.com/juju/juju/internal/ssh" ) type serviceSuite struct { testing.IsolationSuite - state *MockState - userID user.UUID + keyImporter *MockPublicKeyImporter + state *MockState + userID user.UUID } var ( _ = gc.Suite(&serviceSuite{}) - existingUserKeys = []string{ + existingUserPublicKeys = []string{ "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAII4GpCvqUUYUJlx6d1kpUO9k/t4VhSYsf0yE0/QTqDzC existing1", "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIJQJ9wv0uC3yytXM3d2sJJWvZLuISKo7ZHwafHVviwVe existing2", } - // testingKeys represents a set of keys that can be used and are valid. - testingKeys = []string{ + // testingPublicKeys represents a set of keys that can be used and are valid. + testingPublicKeys = []string{ // ecdsa testing public key "ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBG00bYFLb/sxPcmVRMg8NXZK/ldefElAkC9wD41vABdHZiSRvp+2y9BMNVYzE/FnzKObHtSvGRX65YQgRn7k5p0= juju@example.com", @@ -46,9 +48,9 @@ var ( "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQDvplNOK3UBpULZKvZf/I5JHci/DufpSxj8yR4yKE2grescJxu6754jPT3xztSeLGD31/oJApJZGkMUAMRenvDqIaq+taRfOUo/l19AlGZc+Edv4bTlJzZ1Lzwex1vvL1doaLb/f76IIUHClGUgIXRceQH1ovHiIWj6nGltuLanG8YTWxlzzK33yhitmZt142DmpX1VUVF5c/Hct6Rav5lKmwej1TDed1KmHzXVoTHEsmWhKsOK27ue5yTuq0GX6LrAYDucF+2MqZCsuddXsPAW1tj5GNZSR7RrKW5q1CI0G7k9gSomuCsRMlCJ3BqID/vUSs/0qOWg4he0HUsYKQSrXIhckuZu+jYP8B80MoXT50ftRidoG/zh/PugBdXTk46FloVClQopG5A2fbqrphADcUUbRUxZ2lWQN+OVHKfEsfV2b8L2aSqZUGlryfW1cirB5JCTDvtv7rUy9/ny9iKA+8tAyKSDF0I901RDDqKc9dSkrHCg2bLnJZDoiRoWczE= juju@example.com", } - // reservedKeys are keys with reserved comments that can not be added or - // removed via the service. - reservedKeys = []string{ + // reservedPublicKeys are keys with reserved comments that can not be added + // or removed via the service. + reservedPublicKeys = []string{ "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIN8h8XBpjS9aBUG5cdoSWubs7wT2Lc/BEZIUQCqoaOZR juju-client-key", "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIN8h8XBpjS9aBUG5cdoSWubs7wT2Lc/BEZIUQCqoaOZR juju-system-key", } @@ -56,6 +58,7 @@ var ( func (s *serviceSuite) setupMocks(c *gc.C) *gomock.Controller { ctrl := gomock.NewController(c) + s.keyImporter = NewMockPublicKeyImporter(ctrl) s.state = NewMockState(ctrl) return ctrl } @@ -69,8 +72,8 @@ func (s *serviceSuite) SetUpTest(c *gc.C) { func (s *serviceSuite) TestAddKeysForInvalidUser(c *gc.C) { defer s.setupMocks(c).Finish() - err := NewService(s.state). - AddKeysForUser(context.Background(), user.UUID("notvalid"), "key") + err := NewService(s.keyImporter, s.state). + AddPublicKeysForUser(context.Background(), user.UUID("notvalid"), "key") c.Check(err, jc.ErrorIs, errors.NotValid) } @@ -80,95 +83,155 @@ func (s *serviceSuite) TestAddKeysForInvalidUser(c *gc.C) { func (s *serviceSuite) TestAddKeysForNonExistentUser(c *gc.C) { defer s.setupMocks(c).Finish() - s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID).Return(nil, accesserrors.UserNotFound) - svc := NewService(s.state) - err := svc.AddKeysForUser(context.Background(), s.userID, testingKeys[0]) + keyInfo, err := ssh.ParsePublicKey(testingPublicKeys[0]) + c.Assert(err, jc.ErrorIsNil) + expectedKeys := []keymanager.PublicKey{ + { + Comment: keyInfo.Comment, + Fingerprint: keyInfo.Fingerprint(), + Key: testingPublicKeys[0], + }, + } + + s.state.EXPECT().AddPublicKeysForUser( + gomock.Any(), s.userID, expectedKeys, + ).Return(accesserrors.UserNotFound) + + svc := NewService(s.keyImporter, s.state) + err = svc.AddPublicKeysForUser(context.Background(), s.userID, testingPublicKeys[0]) c.Check(err, jc.ErrorIs, accesserrors.UserNotFound) } -// TestAddInvalidKeys is testing that if we try and add one or more keys that -// are invalid we get back a [keyserrors.InvalidAuthorisedKey] key error and no -// modificiation to state is performed. -func (s *serviceSuite) TestAddInvalidKeys(c *gc.C) { +// TestAddInvalidPublicKeys is testing that if we try and add one or more keys +// that are invalid we get back a [keyserrors.InvalidPublicKey] key error +// and no modificiation to state is performed. +func (s *serviceSuite) TestAddInvalidPublicKeys(c *gc.C) { defer s.setupMocks(c).Finish() - s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). - Return(existingUserKeys, nil).AnyTimes() - svc := NewService(s.state) + svc := NewService(s.keyImporter, s.state) - err := svc.AddKeysForUser(context.Background(), s.userID, "notvalid") - c.Check(err, jc.ErrorIs, keyserrors.InvalidAuthorisedKey) + err := svc.AddPublicKeysForUser(context.Background(), s.userID, "notvalid") + c.Check(err, jc.ErrorIs, keyserrors.InvalidPublicKey) - err = svc.AddKeysForUser(context.Background(), s.userID, "notvalid", testingKeys[0]) - c.Check(err, jc.ErrorIs, keyserrors.InvalidAuthorisedKey) + err = svc.AddPublicKeysForUser(context.Background(), s.userID, "notvalid", testingPublicKeys[0]) + c.Check(err, jc.ErrorIs, keyserrors.InvalidPublicKey) - err = svc.AddKeysForUser(context.Background(), s.userID, testingKeys[0], "notvalid") - c.Check(err, jc.ErrorIs, keyserrors.InvalidAuthorisedKey) + err = svc.AddPublicKeysForUser(context.Background(), s.userID, testingPublicKeys[0], "notvalid") + c.Check(err, jc.ErrorIs, keyserrors.InvalidPublicKey) - err = svc.AddKeysForUser(context.Background(), s.userID, testingKeys[0], "notvalid", testingKeys[1]) - c.Check(err, jc.ErrorIs, keyserrors.InvalidAuthorisedKey) + err = svc.AddPublicKeysForUser( + context.Background(), + s.userID, + testingPublicKeys[0], + "notvalid", + testingPublicKeys[1], + ) + c.Check(err, jc.ErrorIs, keyserrors.InvalidPublicKey) } -// TestAddReservedKeys is testing that if we try and add authorised keys with +// TestAddReservedKeys is testing that if we try and add public keys with // reserved comments we get back an error that satisfies // [keyserrors.ReservedCommentViolation] -func (s *serviceSuite) TestAddReservedKeys(c *gc.C) { +func (s *serviceSuite) TestAddReservedPublicKeys(c *gc.C) { defer s.setupMocks(c).Finish() - s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). - Return(existingUserKeys, nil).AnyTimes() - svc := NewService(s.state) + svc := NewService(s.keyImporter, s.state) - err := svc.AddKeysForUser(context.Background(), s.userID, reservedKeys...) + err := svc.AddPublicKeysForUser(context.Background(), s.userID, reservedPublicKeys...) c.Check(err, jc.ErrorIs, keyserrors.ReservedCommentViolation) - err = svc.AddKeysForUser(context.Background(), s.userID, testingKeys[0], reservedKeys[0]) + err = svc.AddPublicKeysForUser( + context.Background(), + s.userID, + testingPublicKeys[0], + reservedPublicKeys[0], + ) c.Check(err, jc.ErrorIs, keyserrors.ReservedCommentViolation) } // TestAddExistingKeysForUser is asserting that when we try and add a key for a // user that already exists we get back a -// [keyserrors.AuthorisedKeyAlreadyExists] error. +// [keyserrors.PublicKeyAlreadyExists] error. func (s *serviceSuite) TestAddExistingKeysForUser(c *gc.C) { defer s.setupMocks(c).Finish() - s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). - Return(existingUserKeys, nil).AnyTimes() - svc := NewService(s.state) - - err := svc.AddKeysForUser(context.Background(), s.userID, existingUserKeys[0]) - c.Check(err, jc.ErrorIs, keyserrors.AuthorisedKeyAlreadyExists) + svc := NewService(s.keyImporter, s.state) - err = svc.AddKeysForUser(context.Background(), s.userID, testingKeys[1], existingUserKeys[1]) - c.Check(err, jc.ErrorIs, keyserrors.AuthorisedKeyAlreadyExists) -} + keyInfo1, err := ssh.ParsePublicKey(existingUserPublicKeys[1]) + c.Assert(err, jc.ErrorIsNil) + keyInfo2, err := ssh.ParsePublicKey(testingPublicKeys[1]) + c.Assert(err, jc.ErrorIsNil) + expectedKeys := []keymanager.PublicKey{ + { + Comment: keyInfo1.Comment, + Fingerprint: keyInfo1.Fingerprint(), + Key: existingUserPublicKeys[1], + }, + } -// TestAddKeysForUserDedupe is asserting that adding the same keys many times -// the result is de-duplicated. -func (s *serviceSuite) TestAddKeysForUserDedupe(c *gc.C) { - defer s.setupMocks(c).Finish() + s.state.EXPECT().AddPublicKeysForUser( + gomock.Any(), s.userID, expectedKeys, + ).Return(keyserrors.PublicKeyAlreadyExists) - s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). - Return(existingUserKeys, nil).AnyTimes() - svc := NewService(s.state) + err = svc.AddPublicKeysForUser( + context.Background(), + s.userID, + existingUserPublicKeys[1], + ) + c.Check(err, jc.ErrorIs, keyserrors.PublicKeyAlreadyExists) + + expectedKeys = []keymanager.PublicKey{ + { + Comment: keyInfo2.Comment, + Fingerprint: keyInfo2.Fingerprint(), + Key: testingPublicKeys[1], + }, + { + Comment: keyInfo1.Comment, + Fingerprint: keyInfo1.Fingerprint(), + Key: existingUserPublicKeys[1], + }, + } - s.state.EXPECT().AddAuthorisedKeysForUser(gomock.Any(), s.userID, testingKeys[0]) + s.state.EXPECT().AddPublicKeysForUser( + gomock.Any(), + s.userID, + expectedKeys, + ).Return(keyserrors.PublicKeyAlreadyExists) - err := svc.AddKeysForUser(context.Background(), s.userID, testingKeys[0], testingKeys[0]) - c.Check(err, jc.ErrorIsNil) + err = svc.AddPublicKeysForUser( + context.Background(), + s.userID, + testingPublicKeys[1], + existingUserPublicKeys[1], + ) + c.Check(err, jc.ErrorIs, keyserrors.PublicKeyAlreadyExists) } // TestAddKeysForUser is testing the happy path of [Service.AddKeysForUser] func (s *serviceSuite) TestAddKeysForUser(c *gc.C) { defer s.setupMocks(c).Finish() - s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). - Return(existingUserKeys, nil).AnyTimes() - svc := NewService(s.state) + expectedKeys := make([]keymanager.PublicKey, 0, len(testingPublicKeys)) + for _, key := range testingPublicKeys { + keyInfo, err := ssh.ParsePublicKey(key) + c.Assert(err, jc.ErrorIsNil) + expectedKeys = append(expectedKeys, keymanager.PublicKey{ + Comment: keyInfo.Comment, + Fingerprint: keyInfo.Fingerprint(), + Key: key, + }) + } + + s.state.EXPECT().AddPublicKeysForUser( + gomock.Any(), + s.userID, + expectedKeys, + ).Return(nil) - s.state.EXPECT().AddAuthorisedKeysForUser(gomock.Any(), s.userID, testingKeys) + svc := NewService(s.keyImporter, s.state) - err := svc.AddKeysForUser(context.Background(), s.userID, testingKeys...) + err := svc.AddPublicKeysForUser(context.Background(), s.userID, testingPublicKeys...) c.Check(err, jc.ErrorIsNil) } @@ -177,9 +240,9 @@ func (s *serviceSuite) TestAddKeysForUser(c *gc.C) { func (s *serviceSuite) TestListKeysForInvalidUserId(c *gc.C) { defer s.setupMocks(c).Finish() - svc := NewService(s.state) + svc := NewService(s.keyImporter, s.state) - _, err := svc.ListKeysForUser(context.Background(), user.UUID("not-valid")) + _, err := svc.ListPublicKeysForUser(context.Background(), user.UUID("not-valid")) c.Check(err, jc.ErrorIs, errors.NotValid) } @@ -189,25 +252,26 @@ func (s *serviceSuite) TestListKeysForInvalidUserId(c *gc.C) { func (s *serviceSuite) TestListKeysForNonExistentUser(c *gc.C) { defer s.setupMocks(c).Finish() - s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). + s.state.EXPECT().GetPublicKeysForUser(gomock.Any(), s.userID). Return(nil, accesserrors.UserNotFound).AnyTimes() - svc := NewService(s.state) + svc := NewService(s.keyImporter, s.state) - _, err := svc.ListKeysForUser(context.Background(), s.userID) + _, err := svc.ListPublicKeysForUser(context.Background(), s.userID) c.Check(err, jc.ErrorIs, accesserrors.UserNotFound) } -// TestListKeysForUser is testing the happy path for [Service.ListKeysForUser]. +// TestListKeysForUser is testing the happy path for +// [Service.ListPublicKeysForUser]. func (s *serviceSuite) TestListKeysForUser(c *gc.C) { defer s.setupMocks(c).Finish() - s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). - Return(existingUserKeys, nil) - svc := NewService(s.state) + s.state.EXPECT().GetPublicKeysForUser(gomock.Any(), s.userID). + Return(existingUserPublicKeys, nil) + svc := NewService(s.keyImporter, s.state) - keys, err := svc.ListKeysForUser(context.Background(), s.userID) + keys, err := svc.ListPublicKeysForUser(context.Background(), s.userID) c.Check(err, jc.ErrorIsNil) - c.Check(keys, gc.DeepEquals, existingUserKeys) + c.Check(keys, gc.DeepEquals, existingUserPublicKeys) } // TestDeleteKeysForInvalidUser is asserting that if we pass an invalid user id @@ -215,7 +279,7 @@ func (s *serviceSuite) TestListKeysForUser(c *gc.C) { func (s *serviceSuite) TestDeleteKeysForInvalidUser(c *gc.C) { defer s.setupMocks(c).Finish() - err := NewService(s.state). + err := NewService(s.keyImporter, s.state). DeleteKeysForUser(context.Background(), user.UUID("notvalid"), "key") c.Check(err, jc.ErrorIs, errors.NotValid) } @@ -226,11 +290,14 @@ func (s *serviceSuite) TestDeleteKeysForInvalidUser(c *gc.C) { func (s *serviceSuite) TestDeleteKeysForUserNotFound(c *gc.C) { defer s.setupMocks(c).Finish() - s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). - Return(nil, accesserrors.UserNotFound) + s.state.EXPECT().DeletePublicKeysForUser( + gomock.Any(), + s.userID, + []string{testingPublicKeys[0]}, + ).Return(accesserrors.UserNotFound) - err := NewService(s.state). - DeleteKeysForUser(context.Background(), s.userID, testingKeys[0]) + err := NewService(s.keyImporter, s.state). + DeleteKeysForUser(context.Background(), s.userID, testingPublicKeys[0]) c.Check(err, jc.ErrorIs, accesserrors.UserNotFound) } @@ -239,15 +306,14 @@ func (s *serviceSuite) TestDeleteKeysForUserNotFound(c *gc.C) { func (s *serviceSuite) TestDeleteKeysForUserWithFingerprint(c *gc.C) { defer s.setupMocks(c).Finish() - s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). - Return(existingUserKeys, nil) - s.state.EXPECT().DeleteAuthorisedKeysForUser(gomock.Any(), s.userID, existingUserKeys[0]). - Return(nil) - - key, err := ssh.ParseAuthorisedKey(existingUserKeys[0]) + key, err := ssh.ParsePublicKey(existingUserPublicKeys[0]) c.Assert(err, jc.ErrorIsNil) - err = NewService(s.state). + s.state.EXPECT().DeletePublicKeysForUser( + gomock.Any(), s.userID, []string{key.Fingerprint()}, + ).Return(nil) + + err = NewService(s.keyImporter, s.state). DeleteKeysForUser(context.Background(), s.userID, key.Fingerprint()) c.Check(err, jc.ErrorIsNil) } @@ -257,15 +323,16 @@ func (s *serviceSuite) TestDeleteKeysForUserWithFingerprint(c *gc.C) { func (s *serviceSuite) TestDeleteKeysForUserWithComment(c *gc.C) { defer s.setupMocks(c).Finish() - s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). - Return(existingUserKeys, nil) - s.state.EXPECT().DeleteAuthorisedKeysForUser(gomock.Any(), s.userID, existingUserKeys[0]). - Return(nil) - - key, err := ssh.ParseAuthorisedKey(existingUserKeys[0]) + key, err := ssh.ParsePublicKey(existingUserPublicKeys[0]) c.Assert(err, jc.ErrorIsNil) - err = NewService(s.state). + s.state.EXPECT().DeletePublicKeysForUser( + gomock.Any(), + s.userID, + []string{key.Comment}, + ).Return(nil) + + err = NewService(s.keyImporter, s.state). DeleteKeysForUser(context.Background(), s.userID, key.Comment) c.Check(err, jc.ErrorIsNil) } @@ -275,13 +342,14 @@ func (s *serviceSuite) TestDeleteKeysForUserWithComment(c *gc.C) { func (s *serviceSuite) TestDeleteKeysForUserData(c *gc.C) { defer s.setupMocks(c).Finish() - s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). - Return(existingUserKeys, nil) - s.state.EXPECT().DeleteAuthorisedKeysForUser(gomock.Any(), s.userID, existingUserKeys[0]). - Return(nil) + s.state.EXPECT().DeletePublicKeysForUser( + gomock.Any(), + s.userID, + []string{existingUserPublicKeys[0]}, + ).Return(nil) - err := NewService(s.state). - DeleteKeysForUser(context.Background(), s.userID, existingUserKeys[0]) + err := NewService(s.keyImporter, s.state). + DeleteKeysForUser(context.Background(), s.userID, existingUserPublicKeys[0]) c.Check(err, jc.ErrorIsNil) } @@ -291,24 +359,21 @@ func (s *serviceSuite) TestDeleteKeysForUserData(c *gc.C) { func (s *serviceSuite) TestDeleteKeysForUserCombination(c *gc.C) { defer s.setupMocks(c).Finish() - s.state.EXPECT().GetAuthorisedKeysForUser(gomock.Any(), s.userID). - Return(existingUserKeys, nil) - s.state.EXPECT().DeleteAuthorisedKeysForUser( + key, err := ssh.ParsePublicKey(existingUserPublicKeys[0]) + c.Assert(err, jc.ErrorIsNil) + + s.state.EXPECT().DeletePublicKeysForUser( gomock.Any(), s.userID, - existingUserKeys[0], - existingUserKeys[1], + []string{key.Comment, existingUserPublicKeys[1]}, ).Return(nil) - key, err := ssh.ParseAuthorisedKey(existingUserKeys[0]) - c.Assert(err, jc.ErrorIsNil) - - err = NewService(s.state). + err = NewService(s.keyImporter, s.state). DeleteKeysForUser( context.Background(), s.userID, key.Comment, - existingUserKeys[1], + existingUserPublicKeys[1], ) c.Check(err, jc.ErrorIsNil) } diff --git a/domain/keymanager/types.go b/domain/keymanager/types.go new file mode 100644 index 00000000000..b4ebe9d384a --- /dev/null +++ b/domain/keymanager/types.go @@ -0,0 +1,10 @@ +// Copyright 2024 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package keymanager + +type PublicKey struct { + Comment string + Fingerprint string + Key string +} diff --git a/domain/keys/service/service.go b/domain/keys/service/service.go deleted file mode 100644 index bcb96f78aec..00000000000 --- a/domain/keys/service/service.go +++ /dev/null @@ -1,282 +0,0 @@ -// Copyright 2024 Canonical Ltd. -// Licensed under the AGPLv3, see LICENCE file for details. - -package service - -import ( - "context" - "fmt" - - "github.com/juju/collections/set" - "github.com/juju/errors" - - "github.com/juju/juju/core/user" - keyserrors "github.com/juju/juju/domain/keys/errors" - "github.com/juju/juju/environs/config" - "github.com/juju/juju/internal/ssh" -) - -// keyDataForDelete represents all of the current authorised keys for a user -// indexed by they key data itself, fingerprint and comment. -type keyDataForDelete struct { - allKeys map[string]keyInfo - byFingerprint map[string]string - byComment map[string]string -} - -// keyInfo holds the reverse index keys for raw key data. -type keyInfo struct { - comment string - fingerprint string -} - -// Service provides the means for interacting with a users underlying -// authorised keys for a model. -type Service struct { - st State -} - -// State provides the access layer the [Service] needs for persisting and -// retrieving a user authorised keys on a model. -type State interface { - // AddAuthorisedKeysForUser adds a set of keys as authorised for a user on - // this model. - AddAuthorisedKeysForUser(context.Context, user.UUID, ...string) error - - // GetAuthorisedKeysForUser is responsible for returning all of the - // authorised keys for the current model. - GetAuthorisedKeysForUser(context.Context, user.UUID) ([]string, error) - - // DeleteAuthorisedKeysForUser is responsible for removing the keys form the - // users list of authorised keys. - DeleteAuthorisedKeysForUser(context.Context, user.UUID, ...string) error -} - -var ( - // reservedAuthorisedKeyComments is the set of comments that can not be - // removed or added by a user. - reservedAuthorisedKeyComments = set.NewStrings( - "juju-client-key", - config.JujuSystemKey, - ) -) - -// NewService constructs a new [Service] for interfacting with a users -// authorised keys. -func NewService(state State) *Service { - return &Service{ - st: state, - } -} - -// AddKeysForUser is responsible for adding authorised keys for a user to a -// model. Keys being added for a user will be de-duped. The following errors can be expected: -// - [errors.NotValid] when the user id is not valid -// - [github.com/juju/juju/domain/access/errors.UserNotFound] when the user does -// not exist. -// - [keyserrors.InvalidAuthorisedKey] when an authorised key fails validation. -// - [keyserrors.ReservedCommentViolation] when a key being added contains a -// comment string that is reserved. -// - [keyserrors.AuthorisedKeyAlreadyExists] when an authorised key being added -// for a user already exists. -func (s *Service) AddKeysForUser(ctx context.Context, userID user.UUID, keys ...string) error { - if err := userID.Validate(); err != nil { - return fmt.Errorf("validating user id %q when adding authorised keys: %w", userID, err) - } - - if len(keys) == 0 { - return nil - } - - fingerprints, err := s.getExistingFingerprints(ctx, userID) - if err != nil { - return fmt.Errorf( - "adding %d authorised keys for user %q: %w", - len(keys), userID, err, - ) - } - - // alreadySeen holds a list of fingerprints already seen during the add - // operation. This is to ensure that on the off chance the caller passes the - // the same key twice to add we only add it once. - alreadySeen := map[string]struct{}{} - toAdd := make([]string, 0, len(keys)) - for _, key := range keys { - parsedKey, err := ssh.ParseAuthorisedKey(key) - if err != nil { - return fmt.Errorf("%w %q: %w", keyserrors.InvalidAuthorisedKey, key, err) - } - - fingerprint := parsedKey.Fingerprint() - if fingerprints.Contains(fingerprint) { - return fmt.Errorf( - "authorised key %q already exists for user %q: %w", - key, userID, errors.Hide(keyserrors.AuthorisedKeyAlreadyExists), - ) - } - - if reservedAuthorisedKeyComments.Contains(parsedKey.Comment) { - return fmt.Errorf( - "authorised key %q contains a reserved comment %q that cannot be used: %w", - key, parsedKey.Comment, errors.Hide(keyserrors.ReservedCommentViolation), - ) - } - - if _, seen := alreadySeen[fingerprint]; seen { - continue - } - toAdd = append(toAdd, key) - } - - return s.st.AddAuthorisedKeysForUser(ctx, userID, toAdd...) -} - -// DeleteKeysForUser removes the keys associated with targets from the users -// list of authorised keys. Targets can be an arbitary list of a authorised -// key's fingerprint (sha256), comment or full key value to be removed. Where a -// match is found the key will be removed. If no key exists for a target this -// will result in no operation. The following errors can be expected: -// - [errors.NotValid] when the user id is not valid -// - [accesserrors.UserNotFound] when the provided user does not exist. -func (s *Service) DeleteKeysForUser( - ctx context.Context, - userID user.UUID, - targets ...string, -) error { - if err := userID.Validate(); err != nil { - return fmt.Errorf( - "validating user id %q when deleting authorised keys: %w", - userID, err, - ) - } - - keyData, err := s.getExistingKeysForDelete(ctx, userID) - if err != nil { - return fmt.Errorf( - "getting authorised keys for user %q to delete keys: %w", - userID, err, - ) - } - - // Deffensive programming to make sure we don't allocate a bunch of memory - // from a destructive call. - maxToRemoveCount := len(targets) - if len(keyData.allKeys) < maxToRemoveCount { - maxToRemoveCount = len(keyData.allKeys) - } - - var ( - key string - keysToRemove = make([]string, 0, maxToRemoveCount) - exists bool - ) - // Range over the list of targets and see if it actually matches a key we - // have for the user. - for _, target := range targets { - if key, exists = keyData.byComment[target]; exists { - } else if key, exists = keyData.byFingerprint[target]; exists { - } else { - key = target - } - - if _, exists = keyData.allKeys[key]; !exists { - // Break out if the key doesn't exist. - continue - } - - keysToRemove = append(keysToRemove, key) - delete(keyData.byComment, keyData.allKeys[key].comment) - delete(keyData.byFingerprint, keyData.allKeys[key].fingerprint) - delete(keyData.allKeys, key) - } - - if len(keysToRemove) == 0 { - return nil - } - - return s.st.DeleteAuthorisedKeysForUser(ctx, userID, keysToRemove...) -} - -// getExistingFingerprints returns the currently set authorised keys and their -// associated fingerprints for the current user on this model. -func (s *Service) getExistingFingerprints( - ctx context.Context, - userID user.UUID, -) (set.Strings, error) { - keys, err := s.st.GetAuthorisedKeysForUser(ctx, userID) - if err != nil { - return set.Strings{}, fmt.Errorf( - "cannot get authorised keys for user %q: %w", - userID, err, - ) - } - - fingerprints := set.Strings{} - for i, key := range keys { - authKey, err := ssh.ParseAuthorisedKey(key) - if err != nil { - return set.Strings{}, fmt.Errorf( - "parsing user %q authorised key %d: %w", - userID, i, err, - ) - } - fingerprints.Add(authKey.Fingerprint()) - } - - return fingerprints, nil -} - -// getExistingKeysForDelete returns the authorised keys for the user indexed on -// the key data, fingerprint and comment. This function exists to help provide -// the information necessary to assist delete operations. -func (s *Service) getExistingKeysForDelete( - ctx context.Context, - userID user.UUID, -) (keyDataForDelete, error) { - keys, err := s.st.GetAuthorisedKeysForUser(ctx, userID) - if err != nil { - return keyDataForDelete{}, fmt.Errorf( - "cannot get authorised keys for user %q: %w", - userID, err, - ) - } - - rval := keyDataForDelete{ - allKeys: make(map[string]keyInfo, len(keys)), - byFingerprint: make(map[string]string, len(keys)), - byComment: make(map[string]string, len(keys)), - } - for i, key := range keys { - authKey, err := ssh.ParseAuthorisedKey(key) - if err != nil { - return keyDataForDelete{}, fmt.Errorf( - "parsing user %q authorised key %d: %w", - userID, i, err, - ) - } - - fingerprint := authKey.Fingerprint() - rval.byFingerprint[fingerprint] = key - rval.byComment[authKey.Comment] = key - rval.allKeys[key] = keyInfo{ - fingerprint: fingerprint, - comment: authKey.Comment, - } - } - return rval, nil -} - -// ListKeysForUser is responsible for returning the authorised ssh keys for the -// specified user. The following errors can be expected: -// - [errors.NotValid] when the user id is not valid. -// - [usererrors.NotFound] when the given user does not exist. -func (s *Service) ListKeysForUser(ctx context.Context, userID user.UUID) ([]string, error) { - if err := userID.Validate(); err != nil { - return nil, fmt.Errorf( - "validating user id %q when listing authorised keys: %w", - userID, err, - ) - } - - return s.st.GetAuthorisedKeysForUser(ctx, userID) -} diff --git a/domain/keys/service/state_mock_test.go b/domain/keys/service/state_mock_test.go deleted file mode 100644 index 8a82a9b6fd8..00000000000 --- a/domain/keys/service/state_mock_test.go +++ /dev/null @@ -1,166 +0,0 @@ -// Code generated by MockGen. DO NOT EDIT. -// Source: github.com/juju/juju/domain/keys/service (interfaces: State) -// -// Generated by this command: -// -// mockgen -typed -package service -destination state_mock_test.go github.com/juju/juju/domain/keys/service State -// - -// Package service is a generated GoMock package. -package service - -import ( - context "context" - reflect "reflect" - - user "github.com/juju/juju/core/user" - gomock "go.uber.org/mock/gomock" -) - -// MockState is a mock of State interface. -type MockState struct { - ctrl *gomock.Controller - recorder *MockStateMockRecorder -} - -// MockStateMockRecorder is the mock recorder for MockState. -type MockStateMockRecorder struct { - mock *MockState -} - -// NewMockState creates a new mock instance. -func NewMockState(ctrl *gomock.Controller) *MockState { - mock := &MockState{ctrl: ctrl} - mock.recorder = &MockStateMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockState) EXPECT() *MockStateMockRecorder { - return m.recorder -} - -// AddAuthorisedKeysForUser mocks base method. -func (m *MockState) AddAuthorisedKeysForUser(arg0 context.Context, arg1 user.UUID, arg2 ...string) error { - m.ctrl.T.Helper() - varargs := []any{arg0, arg1} - for _, a := range arg2 { - varargs = append(varargs, a) - } - ret := m.ctrl.Call(m, "AddAuthorisedKeysForUser", varargs...) - ret0, _ := ret[0].(error) - return ret0 -} - -// AddAuthorisedKeysForUser indicates an expected call of AddAuthorisedKeysForUser. -func (mr *MockStateMockRecorder) AddAuthorisedKeysForUser(arg0, arg1 any, arg2 ...any) *MockStateAddAuthorisedKeysForUserCall { - mr.mock.ctrl.T.Helper() - varargs := append([]any{arg0, arg1}, arg2...) - call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddAuthorisedKeysForUser", reflect.TypeOf((*MockState)(nil).AddAuthorisedKeysForUser), varargs...) - return &MockStateAddAuthorisedKeysForUserCall{Call: call} -} - -// MockStateAddAuthorisedKeysForUserCall wrap *gomock.Call -type MockStateAddAuthorisedKeysForUserCall struct { - *gomock.Call -} - -// Return rewrite *gomock.Call.Return -func (c *MockStateAddAuthorisedKeysForUserCall) Return(arg0 error) *MockStateAddAuthorisedKeysForUserCall { - c.Call = c.Call.Return(arg0) - return c -} - -// Do rewrite *gomock.Call.Do -func (c *MockStateAddAuthorisedKeysForUserCall) Do(f func(context.Context, user.UUID, ...string) error) *MockStateAddAuthorisedKeysForUserCall { - c.Call = c.Call.Do(f) - return c -} - -// DoAndReturn rewrite *gomock.Call.DoAndReturn -func (c *MockStateAddAuthorisedKeysForUserCall) DoAndReturn(f func(context.Context, user.UUID, ...string) error) *MockStateAddAuthorisedKeysForUserCall { - c.Call = c.Call.DoAndReturn(f) - return c -} - -// DeleteAuthorisedKeysForUser mocks base method. -func (m *MockState) DeleteAuthorisedKeysForUser(arg0 context.Context, arg1 user.UUID, arg2 ...string) error { - m.ctrl.T.Helper() - varargs := []any{arg0, arg1} - for _, a := range arg2 { - varargs = append(varargs, a) - } - ret := m.ctrl.Call(m, "DeleteAuthorisedKeysForUser", varargs...) - ret0, _ := ret[0].(error) - return ret0 -} - -// DeleteAuthorisedKeysForUser indicates an expected call of DeleteAuthorisedKeysForUser. -func (mr *MockStateMockRecorder) DeleteAuthorisedKeysForUser(arg0, arg1 any, arg2 ...any) *MockStateDeleteAuthorisedKeysForUserCall { - mr.mock.ctrl.T.Helper() - varargs := append([]any{arg0, arg1}, arg2...) - call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAuthorisedKeysForUser", reflect.TypeOf((*MockState)(nil).DeleteAuthorisedKeysForUser), varargs...) - return &MockStateDeleteAuthorisedKeysForUserCall{Call: call} -} - -// MockStateDeleteAuthorisedKeysForUserCall wrap *gomock.Call -type MockStateDeleteAuthorisedKeysForUserCall struct { - *gomock.Call -} - -// Return rewrite *gomock.Call.Return -func (c *MockStateDeleteAuthorisedKeysForUserCall) Return(arg0 error) *MockStateDeleteAuthorisedKeysForUserCall { - c.Call = c.Call.Return(arg0) - return c -} - -// Do rewrite *gomock.Call.Do -func (c *MockStateDeleteAuthorisedKeysForUserCall) Do(f func(context.Context, user.UUID, ...string) error) *MockStateDeleteAuthorisedKeysForUserCall { - c.Call = c.Call.Do(f) - return c -} - -// DoAndReturn rewrite *gomock.Call.DoAndReturn -func (c *MockStateDeleteAuthorisedKeysForUserCall) DoAndReturn(f func(context.Context, user.UUID, ...string) error) *MockStateDeleteAuthorisedKeysForUserCall { - c.Call = c.Call.DoAndReturn(f) - return c -} - -// GetAuthorisedKeysForUser mocks base method. -func (m *MockState) GetAuthorisedKeysForUser(arg0 context.Context, arg1 user.UUID) ([]string, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetAuthorisedKeysForUser", arg0, arg1) - ret0, _ := ret[0].([]string) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetAuthorisedKeysForUser indicates an expected call of GetAuthorisedKeysForUser. -func (mr *MockStateMockRecorder) GetAuthorisedKeysForUser(arg0, arg1 any) *MockStateGetAuthorisedKeysForUserCall { - mr.mock.ctrl.T.Helper() - call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetAuthorisedKeysForUser", reflect.TypeOf((*MockState)(nil).GetAuthorisedKeysForUser), arg0, arg1) - return &MockStateGetAuthorisedKeysForUserCall{Call: call} -} - -// MockStateGetAuthorisedKeysForUserCall wrap *gomock.Call -type MockStateGetAuthorisedKeysForUserCall struct { - *gomock.Call -} - -// Return rewrite *gomock.Call.Return -func (c *MockStateGetAuthorisedKeysForUserCall) Return(arg0 []string, arg1 error) *MockStateGetAuthorisedKeysForUserCall { - c.Call = c.Call.Return(arg0, arg1) - return c -} - -// Do rewrite *gomock.Call.Do -func (c *MockStateGetAuthorisedKeysForUserCall) Do(f func(context.Context, user.UUID) ([]string, error)) *MockStateGetAuthorisedKeysForUserCall { - c.Call = c.Call.Do(f) - return c -} - -// DoAndReturn rewrite *gomock.Call.DoAndReturn -func (c *MockStateGetAuthorisedKeysForUserCall) DoAndReturn(f func(context.Context, user.UUID) ([]string, error)) *MockStateGetAuthorisedKeysForUserCall { - c.Call = c.Call.DoAndReturn(f) - return c -} diff --git a/internal/ssh/authorisedkey.go b/internal/ssh/authorisedkey.go index dd7110218fb..c10d9b01bd2 100644 --- a/internal/ssh/authorisedkey.go +++ b/internal/ssh/authorisedkey.go @@ -24,14 +24,14 @@ func (a *AuthorisedKey) Fingerprint() string { return ssh.FingerprintSHA256(a.Key) } -// ParseAuthorisedKey parses a single line from an authorised keys file +// ParsePublicKey parses a single line from an authorised keys file // returning a [AuthorisedKey] representation of the data. // [ssh.ParseAuthorizedKey] is used to perform the underlying validating and // parsing. -func ParseAuthorisedKey(key string) (AuthorisedKey, error) { +func ParsePublicKey(key string) (AuthorisedKey, error) { parsedKey, comment, _, _, err := ssh.ParseAuthorizedKey([]byte(key)) if err != nil { - return AuthorisedKey{}, fmt.Errorf("parsing authorised key %q: %w", err) + return AuthorisedKey{}, fmt.Errorf("parsing public key %q: %w", err) } return AuthorisedKey{ From e55effba7bf555bcd9083a623cb9bedd96f49577 Mon Sep 17 00:00:00 2001 From: Thomas Miller Date: Thu, 13 Jun 2024 15:38:52 +0100 Subject: [PATCH 05/10] refactor: move ssh importer errors to sub package. So that external packages can grok the errors being produced by the ssh importer without having to take on the package dependencies. This was introduced for the keymanager service. --- internal/ssh/importer/{ => errors}/errors.go | 2 +- internal/ssh/importer/github.go | 7 +++++-- internal/ssh/importer/github_test.go | 6 ++++-- internal/ssh/importer/importer.go | 13 ++++++++++--- internal/ssh/importer/importer_test.go | 13 ++++++++----- internal/ssh/importer/launchpad.go | 8 +++++--- internal/ssh/importer/launchpad_test.go | 6 ++++-- 7 files changed, 37 insertions(+), 18 deletions(-) rename internal/ssh/importer/{ => errors}/errors.go (96%) diff --git a/internal/ssh/importer/errors.go b/internal/ssh/importer/errors/errors.go similarity index 96% rename from internal/ssh/importer/errors.go rename to internal/ssh/importer/errors/errors.go index 7278b5d3afb..fe71ff23f56 100644 --- a/internal/ssh/importer/errors.go +++ b/internal/ssh/importer/errors/errors.go @@ -1,7 +1,7 @@ // Copyright 2024 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. -package importer +package errors import ( "github.com/juju/errors" diff --git a/internal/ssh/importer/github.go b/internal/ssh/importer/github.go index f3c76ef2fe8..0e25ec2c9a9 100644 --- a/internal/ssh/importer/github.go +++ b/internal/ssh/importer/github.go @@ -11,6 +11,8 @@ import ( "net/url" "github.com/juju/errors" + + importererrors "github.com/juju/juju/internal/ssh/importer/errors" ) // githubKeyResponse represents the response returned by Github for fetching a @@ -48,7 +50,8 @@ const ( // Github subject in this case a user and returning all of the public ssh keys // the user has for their profile. // The following errors can be expected: -// - [SubjectNotFound] when the subject being asked for does not exist in +// - [importererrors.SubjectNotFound] when the subject being asked for does not +// exist in // the resolvers domain. func (g *GithubResolver) PublicKeysForSubject( ctx context.Context, @@ -81,7 +84,7 @@ func (g *GithubResolver) PublicKeysForSubject( if res.StatusCode == http.StatusNotFound { return nil, fmt.Errorf( "cannot find github user %q%w", - subject, errors.Hide(SubjectNotFound), + subject, errors.Hide(importererrors.SubjectNotFound), ) } else if res.StatusCode != http.StatusOK { return nil, fmt.Errorf( diff --git a/internal/ssh/importer/github_test.go b/internal/ssh/importer/github_test.go index 9abb8a3e34c..27a36387e80 100644 --- a/internal/ssh/importer/github_test.go +++ b/internal/ssh/importer/github_test.go @@ -15,6 +15,8 @@ import ( jc "github.com/juju/testing/checkers" gomock "go.uber.org/mock/gomock" gc "gopkg.in/check.v1" + + importererrors "github.com/juju/juju/internal/ssh/importer/errors" ) type githubSuite struct { @@ -32,7 +34,7 @@ func (s *githubSuite) setupMocks(c *gc.C) *gomock.Controller { } // TestSubjectNotFound is asserting that if the [GithubResolver] gets a 404 -// return it propagates a [SubjectNotFound] error. +// return it propagates a [importererrors.SubjectNotFound] error. func (g *githubSuite) TestSubjectNotFound(c *gc.C) { defer g.setupMocks(c).Finish() @@ -49,7 +51,7 @@ func (g *githubSuite) TestSubjectNotFound(c *gc.C) { gh := GithubResolver{g.client} _, err := gh.PublicKeysForSubject(context.Background(), "tlm") - c.Check(err, jc.ErrorIs, SubjectNotFound) + c.Check(err, jc.ErrorIs, importererrors.SubjectNotFound) } // TestSubjectPublicKeys is asserting the happy path for the [GithubResolver]. diff --git a/internal/ssh/importer/importer.go b/internal/ssh/importer/importer.go index 3cbb6233e1f..9618d81d700 100644 --- a/internal/ssh/importer/importer.go +++ b/internal/ssh/importer/importer.go @@ -9,6 +9,8 @@ import ( "net/url" "github.com/juju/errors" + + importererrors "github.com/juju/juju/internal/ssh/importer/errors" ) // Importer is responsible for providing a pattern for importing a subjects @@ -27,15 +29,20 @@ type Importer struct { // // The following errors can be expected: // - [errors.NotValid] when the opaque data for the URI is empty. -// - [NoResolver] when no resolver is defined for the schema of the URI. -// - [SubjectNotFound] when no subject exists for the [Resolver]. +// - [importererrors.NoResolver] when no resolver is defined for the schema of +// the URI. +// - [importererrors.SubjectNotFound] when no subject exists for the [Resolver]. func (i *Importer) FetchPublicKeysForSubject( ctx context.Context, subject *url.URL, ) ([]string, error) { resolver, has := i.resolvers[subject.Scheme] if !has { - return nil, fmt.Errorf("%w for subject scheme %q", NoResolver, subject.Scheme) + return nil, fmt.Errorf( + "%w for subject scheme %q", + importererrors.NoResolver, + subject.Scheme, + ) } if subject.Opaque == "" { diff --git a/internal/ssh/importer/importer_test.go b/internal/ssh/importer/importer_test.go index 63d43c252f6..1c773f4932f 100644 --- a/internal/ssh/importer/importer_test.go +++ b/internal/ssh/importer/importer_test.go @@ -12,6 +12,8 @@ import ( jc "github.com/juju/testing/checkers" gomock "go.uber.org/mock/gomock" gc "gopkg.in/check.v1" + + importererrors "github.com/juju/juju/internal/ssh/importer/errors" ) type importerSuite struct { @@ -45,7 +47,7 @@ func (i *importerSuite) TestInvalidURI(c *gc.C) { } // TestNoResolver is testing that if we ask for a subjects public key using a -// resolver that dosn't exist we get back a [NoResolver] error. +// resolver that dosn't exist we get back a [importererrors.NoResolver] error. func (i *importerSuite) TestNoResolver(c *gc.C) { defer i.setupMocks(c).Finish() @@ -57,15 +59,16 @@ func (i *importerSuite) TestNoResolver(c *gc.C) { }, } _, err = importer.FetchPublicKeysForSubject(context.Background(), uri) - c.Check(err, jc.ErrorIs, NoResolver) + c.Check(err, jc.ErrorIs, importererrors.NoResolver) } // TestSubjectNotFound is testing that if the resolver tells a subject does not -// exist we return a [SubjectNotFound] error. +// exist we return a [importererrors.SubjectNotFound] error. func (i *importerSuite) TestSubjectNotFound(c *gc.C) { defer i.setupMocks(c).Finish() - i.resolver.EXPECT().PublicKeysForSubject(gomock.Any(), "tlm").Return(nil, SubjectNotFound) + i.resolver.EXPECT().PublicKeysForSubject(gomock.Any(), "tlm"). + Return(nil, importererrors.SubjectNotFound) uri, err := url.Parse("gh:tlm") c.Assert(err, jc.ErrorIsNil) @@ -75,7 +78,7 @@ func (i *importerSuite) TestSubjectNotFound(c *gc.C) { }, } _, err = importer.FetchPublicKeysForSubject(context.Background(), uri) - c.Check(err, jc.ErrorIs, SubjectNotFound) + c.Check(err, jc.ErrorIs, importererrors.SubjectNotFound) } // TestFetchPublicKeysForSubject is testing the happy path for diff --git a/internal/ssh/importer/launchpad.go b/internal/ssh/importer/launchpad.go index 5eac4608963..50a8986c967 100644 --- a/internal/ssh/importer/launchpad.go +++ b/internal/ssh/importer/launchpad.go @@ -11,6 +11,8 @@ import ( "net/url" "github.com/juju/errors" + + importererrors "github.com/juju/juju/internal/ssh/importer/errors" ) // LaunchpadResolver is an implementation of [Resolver] for retrieving the @@ -33,8 +35,8 @@ const ( // Launchpad subject in this case a user and returning all of the public ssh // keys the user has for their profile. // The following errors can be expected: -// - [SubjectNotFound] when the subject being asked for does not exist in -// the resolvers domain. +// - [importererrors.SubjectNotFound] when the subject being asked for does not +// exist in the resolvers domain. func (l *LaunchpadResolver) PublicKeysForSubject( ctx context.Context, subject string, @@ -66,7 +68,7 @@ func (l *LaunchpadResolver) PublicKeysForSubject( if res.StatusCode == http.StatusNotFound { return nil, fmt.Errorf( "cannot find launchpad user %q%w", - subject, errors.Hide(SubjectNotFound), + subject, errors.Hide(importererrors.SubjectNotFound), ) } else if res.StatusCode != http.StatusOK { return nil, fmt.Errorf( diff --git a/internal/ssh/importer/launchpad_test.go b/internal/ssh/importer/launchpad_test.go index d25f25b57cd..2bb67cb7e55 100644 --- a/internal/ssh/importer/launchpad_test.go +++ b/internal/ssh/importer/launchpad_test.go @@ -13,6 +13,8 @@ import ( jc "github.com/juju/testing/checkers" gomock "go.uber.org/mock/gomock" gc "gopkg.in/check.v1" + + importererrors "github.com/juju/juju/internal/ssh/importer/errors" ) type launchpadSuite struct { @@ -30,7 +32,7 @@ func (s *launchpadSuite) setupMocks(c *gc.C) *gomock.Controller { } // TestSubjectNotFound is asserting that if the [LaunchpadResolver] gets a 404 -// return it propagates a [SubjectNotFound] error. +// return it propagates a [importererrors.SubjectNotFound] error. func (l *launchpadSuite) TestSubjectNotFound(c *gc.C) { defer l.setupMocks(c).Finish() @@ -47,7 +49,7 @@ func (l *launchpadSuite) TestSubjectNotFound(c *gc.C) { lp := LaunchpadResolver{l.client} _, err := lp.PublicKeysForSubject(context.Background(), "~tlm") - c.Check(err, jc.ErrorIs, SubjectNotFound) + c.Check(err, jc.ErrorIs, importererrors.SubjectNotFound) } // TestSubjectPublicKeys is asserting the happy path for the [LaunchpadResolver]. From 8befa08aff067129536d36364cc57e49ef84409b Mon Sep 17 00:00:00 2001 From: Thomas Miller Date: Thu, 13 Jun 2024 15:48:10 +0100 Subject: [PATCH 06/10] refactor: ssh key import into keymanager service. This change wires up the ssh importer into the keymanager service. --- domain/keymanager/errors/errors.go | 9 ++ domain/keymanager/service/service.go | 58 ++++++++++++- domain/keymanager/service/service_test.go | 101 ++++++++++++++++++++++ internal/ssh/authorisedkey.go | 14 +-- 4 files changed, 174 insertions(+), 8 deletions(-) diff --git a/domain/keymanager/errors/errors.go b/domain/keymanager/errors/errors.go index 7a7d5109eb4..be18984eaaa 100644 --- a/domain/keymanager/errors/errors.go +++ b/domain/keymanager/errors/errors.go @@ -12,6 +12,11 @@ const ( // exists for the specified user. PublicKeyAlreadyExists = errors.ConstError("public key already exists") + // ImportSubjectNotFound indicates that when importing public keys for a + // subject the source of the public keys has told us that this subject + // does not exist. + ImportSubjectNotFound = errors.ConstError("import subject not found") + // InvalidPublicKey indicates a problem with a public key where it // was unable to be understood. InvalidPublicKey = errors.ConstError("invalid public key") @@ -19,4 +24,8 @@ const ( // ReservedCommentViolation indicates that a key contains a comment that is // reserved within the Juju system and cannot be used. ReservedCommentViolation = errors.ConstError("key contains a reserved comment") + + // UnknownImportSource indicates that an import operation cannot occur + // because the source of the information is unknown. + UnknownImportSource = errors.ConstError("unknown import source") ) diff --git a/domain/keymanager/service/service.go b/domain/keymanager/service/service.go index 19607a503d7..d69a36aa667 100644 --- a/domain/keymanager/service/service.go +++ b/domain/keymanager/service/service.go @@ -16,6 +16,7 @@ import ( keyserrors "github.com/juju/juju/domain/keymanager/errors" "github.com/juju/juju/environs/config" "github.com/juju/juju/internal/ssh" + importererrors "github.com/juju/juju/internal/ssh/importer/errors" ) // PublicKeyImporter describes a service that is capable of fetching and @@ -24,6 +25,11 @@ import ( type PublicKeyImporter interface { // FetchPublicKeysForSubject is responsible for gathering all of the // public keys available for a specified subject. + // The following errors can be expected: + // - [importererrors.NoResolver] when there is import resolver the subject + // schema. + // - [importerrors.SubjectNotFound] when the resolver has reported that no + // subject exists. FetchPublicKeysForSubject(context.Context, *url.URL) ([]string, error) } @@ -166,12 +172,62 @@ func (s *Service) DeleteKeysForUser( // - [keyserrors.InvalidPublicKey] when a key being imported fails validation. // - [keyserrors.ReservedCommentViolation] when a key being added contains a // comment string that is reserved. +// - [keyserrors.UnknownImportSource] when the source for the import operation +// is unknown to the service. +// - [keyserrors.ImportSubjectNotFound] when the source has indicated that the +// subject for the import operation does not exist. func (s *Service) ImportPublicKeysForUser( ctx context.Context, userID user.UUID, subject *url.URL, ) error { - return nil + keys, err := s.keyImporter.FetchPublicKeysForSubject(ctx, subject) + if errors.Is(err, importererrors.NoResolver) { + return fmt.Errorf( + "cannot import public keys for user %q, unknown public key source %q%w", + userID, subject.Scheme, errors.Hide(keyserrors.UnknownImportSource), + ) + } else if errors.Is(err, importererrors.SubjectNotFound) { + return fmt.Errorf( + "cannot import public keys for user %q, import subject %q not found%w", + userID, subject.String(), errors.Hide(keyserrors.ImportSubjectNotFound), + ) + } else if err != nil { + return fmt.Errorf( + "cannot import public keys for user %q using subject %q: %w", + userID, subject.String(), err, + ) + } + + keysToAdd := make([]keymanager.PublicKey, 0, len(keys)) + for i, key := range keys { + parsedKey, err := ssh.ParsePublicKey(key) + if err != nil { + return fmt.Errorf( + "cannot parse key %d for subject %q when importing keys for user %q: %w%w", + i, subject.String(), userID, err, errors.Hide(keyserrors.InvalidPublicKey), + ) + } + + if reservedPublicKeyComments.Contains(parsedKey.Comment) { + return fmt.Errorf( + "cannot import key %d for user %q with subject %q because the comment %q is reserverd%w", + i, + userID, + subject.String(), + parsedKey.Comment, + errors.Hide(keyserrors.ReservedCommentViolation), + ) + } + + keysToAdd = append(keysToAdd, keymanager.PublicKey{ + Comment: parsedKey.Comment, + Key: key, + Fingerprint: parsedKey.Fingerprint(), + }) + } + + return s.st.AddPublicKeyForUserIfNotFound(ctx, userID, keysToAdd) } // ListPublicKeysForUser is responsible for returning the public ssh keys for diff --git a/domain/keymanager/service/service_test.go b/domain/keymanager/service/service_test.go index 9be6d1830fa..8cbc85b45aa 100644 --- a/domain/keymanager/service/service_test.go +++ b/domain/keymanager/service/service_test.go @@ -5,6 +5,7 @@ package service import ( "context" + "net/url" "github.com/juju/errors" "github.com/juju/testing" @@ -18,6 +19,7 @@ import ( "github.com/juju/juju/domain/keymanager" keyserrors "github.com/juju/juju/domain/keymanager/errors" "github.com/juju/juju/internal/ssh" + importererrors "github.com/juju/juju/internal/ssh/importer/errors" ) type serviceSuite struct { @@ -26,6 +28,7 @@ type serviceSuite struct { keyImporter *MockPublicKeyImporter state *MockState userID user.UUID + subjectURI *url.URL } var ( @@ -65,6 +68,10 @@ func (s *serviceSuite) setupMocks(c *gc.C) *gomock.Controller { func (s *serviceSuite) SetUpTest(c *gc.C) { s.userID = usertesting.GenUserUUID(c) + + uri, err := url.Parse("gh:tlm") + c.Check(err, jc.ErrorIsNil) + s.subjectURI = uri } // TestAddKeysForInvalidUser is asserting that if we pass in an invalid user id @@ -377,3 +384,97 @@ func (s *serviceSuite) TestDeleteKeysForUserCombination(c *gc.C) { ) c.Check(err, jc.ErrorIsNil) } + +// TestImportKeyForUnknownSource is asserting that if we try and import keys for +// a subject where the source is unknown. +func (s *serviceSuite) TestImportKeysForUnknownSource(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.keyImporter.EXPECT().FetchPublicKeysForSubject( + gomock.Any(), + s.subjectURI, + ).Return(nil, importererrors.NoResolver) + + err := NewService(s.keyImporter, s.state). + ImportPublicKeysForUser(context.Background(), s.userID, s.subjectURI) + c.Check(err, jc.ErrorIs, keyserrors.UnknownImportSource) +} + +// TestImportKeyForUnknownSubject is asserting that if we ask to import keys for +// a subject that doesn't exist we get back a [keyserrors.ImportSubjectNotFound] +// error. +func (s *serviceSuite) TestImportKeysForUnknownSubject(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.keyImporter.EXPECT().FetchPublicKeysForSubject( + gomock.Any(), + s.subjectURI, + ).Return(nil, importererrors.SubjectNotFound) + + err := NewService(s.keyImporter, s.state). + ImportPublicKeysForUser(context.Background(), s.userID, s.subjectURI) + c.Check(err, jc.ErrorIs, keyserrors.ImportSubjectNotFound) +} + +// TestImportKeysInvalidPublicKeys is asserting that if the key importer returns +// invalid public keys a [keyserrors.InvalidPublicKey] error is returned. +func (s *serviceSuite) TestImportKeysInvalidPublicKeys(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.keyImporter.EXPECT().FetchPublicKeysForSubject( + gomock.Any(), + s.subjectURI, + ).Return([]string{"bad"}, nil) + + err := NewService(s.keyImporter, s.state). + ImportPublicKeysForUser(context.Background(), s.userID, s.subjectURI) + c.Check(err, jc.ErrorIs, keyserrors.InvalidPublicKey) +} + +// TestImportKeysWithReservedComment is asserting that if we import keys where +// one or more of the keys has a reserved comment we return an error that +// satisfies [keyserrors.ReservedCommentViolation]. +func (s *serviceSuite) TestImportKeysWithReservedComment(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.keyImporter.EXPECT().FetchPublicKeysForSubject( + gomock.Any(), + s.subjectURI, + ).Return(reservedPublicKeys, nil) + + err := NewService(s.keyImporter, s.state). + ImportPublicKeysForUser(context.Background(), s.userID, s.subjectURI) + c.Check(err, jc.ErrorIs, keyserrors.ReservedCommentViolation) +} + +// TestImportPublicKeysForUser is asserting the happy path of +// [Service.ImportPublicKeysForUser]. +func (s *serviceSuite) TestImportPublicKeysForUser(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.keyImporter.EXPECT().FetchPublicKeysForSubject( + gomock.Any(), + s.subjectURI, + ).Return(testingPublicKeys, nil) + + expectedKeys := make([]keymanager.PublicKey, 0, len(testingPublicKeys)) + for _, key := range testingPublicKeys { + keyInfo, err := ssh.ParsePublicKey(key) + c.Assert(err, jc.ErrorIsNil) + expectedKeys = append(expectedKeys, keymanager.PublicKey{ + Comment: keyInfo.Comment, + Fingerprint: keyInfo.Fingerprint(), + Key: key, + }) + } + + s.state.EXPECT().AddPublicKeyForUserIfNotFound( + gomock.Any(), + s.userID, + expectedKeys, + ).Return(nil) + + err := NewService(s.keyImporter, s.state). + ImportPublicKeysForUser(context.Background(), s.userID, s.subjectURI) + c.Check(err, jc.ErrorIsNil) +} diff --git a/internal/ssh/authorisedkey.go b/internal/ssh/authorisedkey.go index c10d9b01bd2..dda11720a0e 100644 --- a/internal/ssh/authorisedkey.go +++ b/internal/ssh/authorisedkey.go @@ -9,9 +9,9 @@ import ( "golang.org/x/crypto/ssh" ) -// AuthorisedKey represents a single authorised key line that would commonly be +// PublicKey represents a single authorised key line that would commonly be // found in a authorized_keys file. http://man.he.net/man5/authorized_keys -type AuthorisedKey struct { +type PublicKey struct { // Key holds the parse key data for the public key. Key ssh.PublicKey @@ -20,21 +20,21 @@ type AuthorisedKey struct { } // Fingerprint returns the SHA256 fingerprint of the public key. -func (a *AuthorisedKey) Fingerprint() string { +func (a *PublicKey) Fingerprint() string { return ssh.FingerprintSHA256(a.Key) } // ParsePublicKey parses a single line from an authorised keys file -// returning a [AuthorisedKey] representation of the data. +// returning a [PublicKey] representation of the data. // [ssh.ParseAuthorizedKey] is used to perform the underlying validating and // parsing. -func ParsePublicKey(key string) (AuthorisedKey, error) { +func ParsePublicKey(key string) (PublicKey, error) { parsedKey, comment, _, _, err := ssh.ParseAuthorizedKey([]byte(key)) if err != nil { - return AuthorisedKey{}, fmt.Errorf("parsing public key %q: %w", err) + return PublicKey{}, fmt.Errorf("parsing public key %q: %w", key, err) } - return AuthorisedKey{ + return PublicKey{ Key: parsedKey, Comment: comment, }, nil From c36a8686e337d62583bce5a7fffcf0c3236ea6e7 Mon Sep 17 00:00:00 2001 From: Thomas Miller Date: Thu, 13 Jun 2024 16:54:25 +0100 Subject: [PATCH 07/10] docs: updates keymanger docs. Updating the keymanger domain docs to better reflect the implementation. --- domain/keymanager/doc.go | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/domain/keymanager/doc.go b/domain/keymanager/doc.go index 99beedbeae4..daac1dfa5a9 100644 --- a/domain/keymanager/doc.go +++ b/domain/keymanager/doc.go @@ -1,17 +1,6 @@ // Copyright 2024 Canonical Ltd. // Licensed under the AGPLv3, see LICENCE file for details. -// Package keys provides the domain needed for configuring authorised keys on a +// Package keys provides the domain needed for configuring public keys on a // model for a user. -// -// This package still uses model config as an implementation detail for storing -// user authorised keys. Because of this implementation detail we have to -// discard the association between a user and their authorised keys. -// -// Also because of the way we are persisting this data we also don't have any -// transactionality when modifying authorised keys potentially resulting in data -// that may not be accurate. -// -// Both of the problems described above exist with the original implementation -// using Mongo and model config. package keymanager From 0fbe35c1f91314b1c430c277e5d73d21ee4d1fe1 Mon Sep 17 00:00:00 2001 From: Thomas Miller Date: Thu, 13 Jun 2024 16:56:12 +0100 Subject: [PATCH 08/10] chore: fixing spelling in keymanger service. --- domain/keymanager/doc.go | 4 ++++ domain/keymanager/service/service.go | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/domain/keymanager/doc.go b/domain/keymanager/doc.go index daac1dfa5a9..976509c7407 100644 --- a/domain/keymanager/doc.go +++ b/domain/keymanager/doc.go @@ -3,4 +3,8 @@ // Package keys provides the domain needed for configuring public keys on a // model for a user. +// +// Public keys for a user are per model based and will not follow a user between +// models. Currently under the covers we do not model the public keys and their +// user (owner) as an old legacy implementation details of Juju 3.x. package keymanager diff --git a/domain/keymanager/service/service.go b/domain/keymanager/service/service.go index d69a36aa667..ea4f55099dc 100644 --- a/domain/keymanager/service/service.go +++ b/domain/keymanager/service/service.go @@ -139,7 +139,7 @@ func (s *Service) AddPublicKeysForUser( } // DeletePublicKeysForUser removes the keys associated with targets from the -// users list of public keys. Targets can be an arbitary list of a +// users list of public keys. Targets can be an arbitrary list of a // public key fingerprint (sha256), comment or full key value to be // removed. Where a match is found the key will be removed. If no key exists for // a target this will result in no operation. The following errors can be @@ -211,7 +211,7 @@ func (s *Service) ImportPublicKeysForUser( if reservedPublicKeyComments.Contains(parsedKey.Comment) { return fmt.Errorf( - "cannot import key %d for user %q with subject %q because the comment %q is reserverd%w", + "cannot import key %d for user %q with subject %q because the comment %q is reserved%w", i, userID, subject.String(), From df7428cc50caf7ef1c2468c686ed0fef6dd8c8e7 Mon Sep 17 00:00:00 2001 From: Thomas Miller Date: Thu, 13 Jun 2024 17:14:46 +0100 Subject: [PATCH 09/10] refactor: user id validating for import keys. Importing ssh public keys for a user was not validating the user id. Change brings in the check and returns an appropriate error. --- domain/keymanager/service/service.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/domain/keymanager/service/service.go b/domain/keymanager/service/service.go index ea4f55099dc..6f7ac91142c 100644 --- a/domain/keymanager/service/service.go +++ b/domain/keymanager/service/service.go @@ -181,6 +181,13 @@ func (s *Service) ImportPublicKeysForUser( userID user.UUID, subject *url.URL, ) error { + if err := userID.Validate(); err != nil { + return fmt.Errorf( + "validating user id %q when importing public keys from %q: %w", + userID, subject.String(), err, + ) + } + keys, err := s.keyImporter.FetchPublicKeysForSubject(ctx, subject) if errors.Is(err, importererrors.NoResolver) { return fmt.Errorf( From 55a8f6f79915fd08b5feea579479e703cd1458ca Mon Sep 17 00:00:00 2001 From: Thomas Miller Date: Thu, 20 Jun 2024 15:34:02 +1000 Subject: [PATCH 10/10] style: applying feedback from peer review. Fixes several spelling mistakes in comments. Changes a big block of if/else statements for error checking over to switch schematics for better readability. --- domain/keymanager/service/service.go | 16 +++++++++------- domain/keymanager/types.go | 2 ++ internal/ssh/importer/github.go | 3 +-- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/domain/keymanager/service/service.go b/domain/keymanager/service/service.go index 6f7ac91142c..974d7bd59c5 100644 --- a/domain/keymanager/service/service.go +++ b/domain/keymanager/service/service.go @@ -40,12 +40,12 @@ type Service struct { // public key's for subject. keyImporter PublicKeyImporter - // st is the provides the state access layer to this service. + // st provides the state access layer to this service. st State } // State provides the access layer the [Service] needs for persisting and -// retrieving a users public keys on a model. +// retrieving a user's public keys on a model. type State interface { // AddPublicKeysForUser adds a set of public keys for a user on // this model. If one or more of the public keys to add for the user already @@ -61,7 +61,7 @@ type State interface { // public keys for the current user in this model. GetPublicKeysForUser(context.Context, user.UUID) ([]string, error) - // DeletePublicKeysForUser is responsible for removing the keys form the + // DeletePublicKeysForUser is responsible for removing the keys from the // users list of public keys where the string list represents one of // the keys fingerprint, public key data or comment. DeletePublicKeysForUser(context.Context, user.UUID, []string) error @@ -139,7 +139,7 @@ func (s *Service) AddPublicKeysForUser( } // DeletePublicKeysForUser removes the keys associated with targets from the -// users list of public keys. Targets can be an arbitrary list of a +// user's list of public keys. Targets can be an arbitrary list of a // public key fingerprint (sha256), comment or full key value to be // removed. Where a match is found the key will be removed. If no key exists for // a target this will result in no operation. The following errors can be @@ -189,17 +189,19 @@ func (s *Service) ImportPublicKeysForUser( } keys, err := s.keyImporter.FetchPublicKeysForSubject(ctx, subject) - if errors.Is(err, importererrors.NoResolver) { + + switch { + case errors.Is(err, importererrors.NoResolver): return fmt.Errorf( "cannot import public keys for user %q, unknown public key source %q%w", userID, subject.Scheme, errors.Hide(keyserrors.UnknownImportSource), ) - } else if errors.Is(err, importererrors.SubjectNotFound) { + case errors.Is(err, importererrors.SubjectNotFound): return fmt.Errorf( "cannot import public keys for user %q, import subject %q not found%w", userID, subject.String(), errors.Hide(keyserrors.ImportSubjectNotFound), ) - } else if err != nil { + case err != nil: return fmt.Errorf( "cannot import public keys for user %q using subject %q: %w", userID, subject.String(), err, diff --git a/domain/keymanager/types.go b/domain/keymanager/types.go index b4ebe9d384a..2d0db75bf26 100644 --- a/domain/keymanager/types.go +++ b/domain/keymanager/types.go @@ -3,6 +3,8 @@ package keymanager +// PublicKey represents the domains understand of what a public key is and the +// indiviual parts the domain cares about. type PublicKey struct { Comment string Fingerprint string diff --git a/internal/ssh/importer/github.go b/internal/ssh/importer/github.go index 0e25ec2c9a9..d28f537d80a 100644 --- a/internal/ssh/importer/github.go +++ b/internal/ssh/importer/github.go @@ -51,8 +51,7 @@ const ( // the user has for their profile. // The following errors can be expected: // - [importererrors.SubjectNotFound] when the subject being asked for does not -// exist in -// the resolvers domain. +// exist in the resolvers domain. func (g *GithubResolver) PublicKeysForSubject( ctx context.Context, subject string,