From 0c70986e4611cf25367e1eeb4de06b85e687e5a2 Mon Sep 17 00:00:00 2001 From: Kian Parvin Date: Thu, 7 Sep 2023 11:18:14 +0200 Subject: [PATCH 1/4] Audit log fixes --- cmd/jimmctl/cmd/listauditevents.go | 2 +- cmd/jimmctl/cmd/purge_logs.go | 4 +++- internal/jimm/audit_log.go | 10 +++++----- internal/rpc/proxy.go | 10 ++++++---- 4 files changed, 15 insertions(+), 11 deletions(-) diff --git a/cmd/jimmctl/cmd/listauditevents.go b/cmd/jimmctl/cmd/listauditevents.go index e25446d3d..3b3723fe8 100644 --- a/cmd/jimmctl/cmd/listauditevents.go +++ b/cmd/jimmctl/cmd/listauditevents.go @@ -71,7 +71,7 @@ func (c *listAuditEventsCommand) SetFlags(f *gnuflag.FlagSet) { f.StringVar(&c.args.UserTag, "user-tag", "", "display events performed by authenticated user") f.StringVar(&c.args.Method, "method", "", "display events for a specific method call") f.StringVar(&c.args.Model, "model", "", "display events for a specific model (model name is controller/model)") - f.IntVar(&c.args.Limit, "offset", 0, "offset the set of returned audit events") + f.IntVar(&c.args.Offset, "offset", 0, "offset the set of returned audit events") f.IntVar(&c.args.Limit, "limit", 0, "limit the maximum number of returned audit events") f.BoolVar(&c.args.SortTime, "reverse", false, "reverse the order of logs, showing the most recent first") diff --git a/cmd/jimmctl/cmd/purge_logs.go b/cmd/jimmctl/cmd/purge_logs.go index cbb126bb4..0a168b249 100644 --- a/cmd/jimmctl/cmd/purge_logs.go +++ b/cmd/jimmctl/cmd/purge_logs.go @@ -25,7 +25,9 @@ const purgeLogsDoc = ` // NewPurgeLogsCommand returns a command to purge logs. func NewPurgeLogsCommand() cmd.Command { - cmd := &purgeLogsCommand{} + cmd := &purgeLogsCommand{ + store: jujuclient.NewFileClientStore(), + } return modelcmd.WrapBase(cmd) } diff --git a/internal/jimm/audit_log.go b/internal/jimm/audit_log.go index bb049f37c..504016bd0 100644 --- a/internal/jimm/audit_log.go +++ b/internal/jimm/audit_log.go @@ -26,9 +26,9 @@ type DbAuditLogger struct { getUser func() names.UserTag } -// newConversationID generates a unique ID that is used for the +// NewConversationID generates a unique ID that is used for the // lifetime of a websocket connection. -func newConversationID() string { +func NewConversationID() string { buf := make([]byte, 8) rand.Read(buf) // Can't fail return hex.EncodeToString(buf) @@ -38,7 +38,7 @@ func newConversationID() string { func NewDbAuditLogger(j *JIMM, getUserFunc func() names.UserTag) DbAuditLogger { logger := DbAuditLogger{ jimm: j, - conversationId: newConversationID(), + conversationId: NewConversationID(), getUser: getUserFunc, } return logger @@ -112,7 +112,7 @@ type recorder struct { func NewRecorder(logger DbAuditLogger) recorder { return recorder{ start: time.Now(), - conversationId: newConversationID(), + conversationId: NewConversationID(), logger: logger, } } @@ -166,11 +166,11 @@ func (a *auditLogCleanupService) Start(ctx context.Context) { // from the service's context. It calculates the poll duration at 9am each day // UTC. func (a *auditLogCleanupService) poll(ctx context.Context) { - retentionDate := time.Now().AddDate(0, 0, -(a.auditLogRetentionPeriodInDays)) for { select { case <-time.After(calculateNextPollDuration(time.Now().UTC())): + retentionDate := time.Now().AddDate(0, 0, -(a.auditLogRetentionPeriodInDays)) deleted, err := a.db.DeleteAuditLogsBefore(ctx, retentionDate) if err != nil { zapctx.Error(ctx, "failed to cleanup audit logs", zap.Error(err)) diff --git a/internal/rpc/proxy.go b/internal/rpc/proxy.go index 295257440..2f1b8cae6 100644 --- a/internal/rpc/proxy.go +++ b/internal/rpc/proxy.go @@ -17,6 +17,7 @@ import ( "github.com/canonical/jimm/internal/auth" "github.com/canonical/jimm/internal/dbmodel" "github.com/canonical/jimm/internal/errors" + "github.com/canonical/jimm/internal/jimm" ) // TokenGenerator authenticates a user and generates a JWT token. @@ -459,10 +460,11 @@ func ProxySockets(ctx context.Context, helpers ProxyHelpers) error { // after the first message has been received so that any errors can be properly sent back to the client. clProxy := clientProxy{ modelProxy: modelProxy{ - src: &client, - msgs: &msgInFlight, - tokenGen: helpers.TokenGen, - auditLog: helpers.AuditLog, + src: &client, + msgs: &msgInFlight, + tokenGen: helpers.TokenGen, + auditLog: helpers.AuditLog, + conversationId: jimm.NewConversationID(), }, errChan: errChan, createControllerConn: helpers.ConnectController, From 758e91a2fc33d28fc64bde89a6e2d35aca05a665 Mon Sep 17 00:00:00 2001 From: Kian Parvin Date: Thu, 7 Sep 2023 18:38:09 +0200 Subject: [PATCH 2/4] Add omitempty on relevant audit log fields --- api/params/params.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/api/params/params.go b/api/params/params.go index 9cebfd7e2..4b8aa2721 100644 --- a/api/params/params.go +++ b/api/params/params.go @@ -100,32 +100,32 @@ type AuditEvent struct { MessageId uint64 `json:"message-id" yaml:"message-id"` // FacadeName contains the request facade name. - FacadeName string `json:"facade-name" yaml:"facade-name"` + FacadeName string `json:"facade-name,omitempty" yaml:"facade-name,omitempty"` // FacadeMethod contains the specific method to be executed on the facade. - FacadeMethod string `json:"facade-method" yaml:"facade-method"` + FacadeMethod string `json:"facade-method,omitempty" yaml:"facade-method,omitempty"` // FacadeVersion contains the requested version for the facade method. - FacadeVersion int `json:"facade-version" yaml:"facade-version"` + FacadeVersion int `json:"facade-version,omitempty" yaml:"facade-version,omitempty"` // ObjectId contains the object id to act on, only used by certain facades. - ObjectId string `json:"object-id" yaml:"object-id"` + ObjectId string `json:"object-id,omitempty" yaml:"object-id,omitempty"` // UserTag contains the user tag of authenticated user that performed // the action. - UserTag string `json:"user-tag" yaml:"user-tag"` + UserTag string `json:"user-tag,omitempty" yaml:"user-tag,omitempty"` // Model contains the name of the model the event was performed against. - Model string `json:"model" yaml:"model"` + Model string `json:"model,omitempty" yaml:"model,omitempty"` // IsResponse indicates whether the message is a request/response. IsResponse bool `json:"is-response" yaml:"is-response"` // Params contains client request parameters. - Params map[string]any `json:"params" yaml:"params"` + Params map[string]any `json:"params,omitempty" yaml:"params,omitempty"` // Errors contains error info received from the controller. - Errors map[string]any `json:"error" yaml:"errors"` + Errors map[string]any `json:"error,omitempty" yaml:"errors,omitempty"` } // An AuditEvents contains events from the audit log. From af6bf593fa8e5d624cec2e7d92d300c2b9d4c886 Mon Sep 17 00:00:00 2001 From: Kian Parvin Date: Fri, 8 Sep 2023 14:33:59 +0200 Subject: [PATCH 3/4] Fix test --- cmd/jimmctl/cmd/listauditevents_test.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/cmd/jimmctl/cmd/listauditevents_test.go b/cmd/jimmctl/cmd/listauditevents_test.go index 4eccc8bd2..a16411dfa 100644 --- a/cmd/jimmctl/cmd/listauditevents_test.go +++ b/cmd/jimmctl/cmd/listauditevents_test.go @@ -37,24 +37,18 @@ func (s *listAuditEventsSuite) TestListAuditEventsSuperuser(c *gc.C) { facade-name: Admin facade-method: Login facade-version: \d - object-id: "" user-tag: user- - model: "" is-response: false params: params: redacted - errors: .* - time: .* conversation-id: .* message-id: 1 facade-name: Admin facade-method: Login facade-version: \d - object-id: "" user-tag: user- - model: "" is-response: true - params: {} errors: results: - error: From 6f09789e76fc4c829c48c54b2a9897b56754e726 Mon Sep 17 00:00:00 2001 From: Kian Parvin Date: Tue, 12 Sep 2023 13:49:35 +0200 Subject: [PATCH 4/4] Moved NewConversationID to utils package - Created new utils package --- internal/jimm/audit_log.go | 15 +++------------ internal/rpc/proxy.go | 4 ++-- internal/utils/utils.go | 14 ++++++++++++++ internal/utils/utils_test.go | 15 +++++++++++++++ 4 files changed, 34 insertions(+), 14 deletions(-) create mode 100644 internal/utils/utils.go create mode 100644 internal/utils/utils_test.go diff --git a/internal/jimm/audit_log.go b/internal/jimm/audit_log.go index 504016bd0..4694d816c 100644 --- a/internal/jimm/audit_log.go +++ b/internal/jimm/audit_log.go @@ -4,8 +4,6 @@ package jimm import ( "context" - "crypto/rand" - "encoding/hex" "encoding/json" "time" @@ -18,6 +16,7 @@ import ( "github.com/canonical/jimm/internal/db" "github.com/canonical/jimm/internal/dbmodel" "github.com/canonical/jimm/internal/servermon" + "github.com/canonical/jimm/internal/utils" ) type DbAuditLogger struct { @@ -26,19 +25,11 @@ type DbAuditLogger struct { getUser func() names.UserTag } -// NewConversationID generates a unique ID that is used for the -// lifetime of a websocket connection. -func NewConversationID() string { - buf := make([]byte, 8) - rand.Read(buf) // Can't fail - return hex.EncodeToString(buf) -} - // NewDbAuditLogger returns a new audit logger that logs to the database. func NewDbAuditLogger(j *JIMM, getUserFunc func() names.UserTag) DbAuditLogger { logger := DbAuditLogger{ jimm: j, - conversationId: NewConversationID(), + conversationId: utils.NewConversationID(), getUser: getUserFunc, } return logger @@ -112,7 +103,7 @@ type recorder struct { func NewRecorder(logger DbAuditLogger) recorder { return recorder{ start: time.Now(), - conversationId: NewConversationID(), + conversationId: utils.NewConversationID(), logger: logger, } } diff --git a/internal/rpc/proxy.go b/internal/rpc/proxy.go index 2f1b8cae6..6c332f357 100644 --- a/internal/rpc/proxy.go +++ b/internal/rpc/proxy.go @@ -17,7 +17,7 @@ import ( "github.com/canonical/jimm/internal/auth" "github.com/canonical/jimm/internal/dbmodel" "github.com/canonical/jimm/internal/errors" - "github.com/canonical/jimm/internal/jimm" + "github.com/canonical/jimm/internal/utils" ) // TokenGenerator authenticates a user and generates a JWT token. @@ -464,7 +464,7 @@ func ProxySockets(ctx context.Context, helpers ProxyHelpers) error { msgs: &msgInFlight, tokenGen: helpers.TokenGen, auditLog: helpers.AuditLog, - conversationId: jimm.NewConversationID(), + conversationId: utils.NewConversationID(), }, errChan: errChan, createControllerConn: helpers.ConnectController, diff --git a/internal/utils/utils.go b/internal/utils/utils.go new file mode 100644 index 000000000..4652cc3a8 --- /dev/null +++ b/internal/utils/utils.go @@ -0,0 +1,14 @@ +package utils + +import ( + "crypto/rand" + "encoding/hex" +) + +// NewConversationID generates a unique ID that is used for the +// lifetime of a websocket connection. +func NewConversationID() string { + buf := make([]byte, 8) + rand.Read(buf) // Can't fail + return hex.EncodeToString(buf) +} diff --git a/internal/utils/utils_test.go b/internal/utils/utils_test.go new file mode 100644 index 000000000..8b842aab9 --- /dev/null +++ b/internal/utils/utils_test.go @@ -0,0 +1,15 @@ +package utils_test + +import ( + "testing" + + qt "github.com/frankban/quicktest" + + "github.com/canonical/jimm/internal/utils" +) + +func TestNewConversationID(t *testing.T) { + c := qt.New(t) + res := utils.NewConversationID() + c.Assert(res, qt.HasLen, 16) +}