Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSS-4838 Audit Log improvements #1042

Merged
merged 4 commits into from
Sep 14, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions api/params/params.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion cmd/jimmctl/cmd/listauditevents.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")

Expand Down
4 changes: 3 additions & 1 deletion cmd/jimmctl/cmd/purge_logs.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ const purgeLogsDoc = `

// NewPurgeLogsCommand returns a command to purge logs.
func NewPurgeLogsCommand() cmd.Command {
cmd := &purgeLogsCommand{}
cmd := &purgeLogsCommand{
store: jujuclient.NewFileClientStore(),
kian99 marked this conversation as resolved.
Show resolved Hide resolved
}
return modelcmd.WrapBase(cmd)
}

Expand Down
10 changes: 5 additions & 5 deletions internal/jimm/audit_log.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -112,7 +112,7 @@ type recorder struct {
func NewRecorder(logger DbAuditLogger) recorder {
return recorder{
start: time.Now(),
conversationId: newConversationID(),
conversationId: NewConversationID(),
logger: logger,
}
}
Expand Down Expand Up @@ -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))
kian99 marked this conversation as resolved.
Show resolved Hide resolved
deleted, err := a.db.DeleteAuditLogsBefore(ctx, retentionDate)
if err != nil {
zapctx.Error(ctx, "failed to cleanup audit logs", zap.Error(err))
Expand Down
10 changes: 6 additions & 4 deletions internal/rpc/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, not sure how I feel about bring internal/jimm as a dependency in this package, it's only for the jimm.NewConversationID() function.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move the NewConversationID into a utils package

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use juju's new convo id func? If it's exported*

Copy link
Contributor Author

@kian99 kian99 Sep 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not exported https://github.com/juju/juju/blob/3.2/core/auditlog/auditlog.go#L176 (it's a very simple function that doesn't need to change when Juju changes, so I figured it's okay in this case to just copy-paste it)
I'll move it into a utils package.

)

// TokenGenerator authenticates a user and generates a JWT token.
Expand Down Expand Up @@ -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,
Expand Down
Loading