From 161d01d2f3e4a97d82cb741428c131c7e310b5dc Mon Sep 17 00:00:00 2001 From: Kian Parvin <46668016+kian99@users.noreply.github.com> Date: Thu, 14 Sep 2023 08:56:56 +0200 Subject: [PATCH] CSS-4838 Audit Log improvements (#1042) * Audit log fixes * Add omitempty on relevant audit log fields * Fix test * Moved NewConversationID to utils package - Created new utils package --- api/params/params.go | 16 ++++++++-------- cmd/jimmctl/cmd/listauditevents.go | 2 +- cmd/jimmctl/cmd/listauditevents_test.go | 6 ------ cmd/jimmctl/cmd/purge_logs.go | 4 +++- internal/jimm/audit_log.go | 17 ++++------------- internal/rpc/proxy.go | 10 ++++++---- internal/utils/utils.go | 14 ++++++++++++++ internal/utils/utils_test.go | 15 +++++++++++++++ 8 files changed, 51 insertions(+), 33 deletions(-) create mode 100644 internal/utils/utils.go create mode 100644 internal/utils/utils_test.go 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. 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/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: 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..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, } } @@ -166,11 +157,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..6c332f357 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/utils" ) // 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: 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) +}