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 4824/expire audit logs #1007

Merged
merged 17 commits into from
Jul 25, 2023
7 changes: 7 additions & 0 deletions charms/jimm-k8s/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@
# See LICENSE file for licensing details.

options:
audit-log-retention-period:
type: string
description: |
How long to hold audit logs for in days, i.e., 10 = 10 days.
If the value 0 is used, the logs will never be purged.
Logs are purged at 9AM UTC. Defaults to 0 days.
ale8k marked this conversation as resolved.
Show resolved Hide resolved
default: "0"
candid-agent-private-key:
type: string
description: |
Expand Down
1 change: 1 addition & 0 deletions charms/jimm-k8s/src/charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ def _update_workload(self, event):
config_values = {
"CANDID_PUBLIC_KEY": self.config.get("candid-public-key", ""),
"CANDID_URL": self.config.get("candid-url", ""),
"JIMM_AUDIT_LOG_RETENTION_PERIOD_IN_DAYS": self.config.get("audit-log-retention-period", ""),
"JIMM_ADMINS": self.config.get("controller-admins", ""),
"JIMM_DNS_NAME": dns_name,
"JIMM_LOG_LEVEL": self.config.get("log-level", ""),
Expand Down
4 changes: 4 additions & 0 deletions charms/jimm-k8s/tests/unit/test_charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
"public-key": "izcYsQy3TePp6bLjqOo3IRPFvkQd2IKtyODGqC6SdFk=",
"private-key": "ly/dzsI9Nt/4JxUILQeAX79qZ4mygDiuYGqc2ZEiDEc=",
"vault-access-address": "10.0.1.123",
"audit-log-retention-period": "10",
}


Expand Down Expand Up @@ -79,6 +80,7 @@ def test_on_pebble_ready(self):
"JIMM_LOG_LEVEL": "info",
"JIMM_UUID": "1234567890",
"JIMM_WATCH_CONTROLLERS": "1",
"JIMM_AUDIT_LOG_RETENTION_PERIOD_IN_DAYS": "10",
"PRIVATE_KEY": "ly/dzsI9Nt/4JxUILQeAX79qZ4mygDiuYGqc2ZEiDEc=",
"PUBLIC_KEY": "izcYsQy3TePp6bLjqOo3IRPFvkQd2IKtyODGqC6SdFk=",
},
Expand Down Expand Up @@ -117,6 +119,7 @@ def test_on_config_changed(self):
"JIMM_LOG_LEVEL": "info",
"JIMM_UUID": "1234567890",
"JIMM_WATCH_CONTROLLERS": "1",
"JIMM_AUDIT_LOG_RETENTION_PERIOD_IN_DAYS": "10",
"PRIVATE_KEY": "ly/dzsI9Nt/4JxUILQeAX79qZ4mygDiuYGqc2ZEiDEc=",
"PUBLIC_KEY": "izcYsQy3TePp6bLjqOo3IRPFvkQd2IKtyODGqC6SdFk=",
},
Expand Down Expand Up @@ -159,6 +162,7 @@ def test_bakery_configuration(self):
"BAKERY_AGENT_FILE": "/root/config/agent.json",
"CANDID_URL": "test-candid-url",
"JIMM_DASHBOARD_LOCATION": "https://jaas.ai/models",
"JIMM_AUDIT_LOG_RETENTION_PERIOD_IN_DAYS": "0",
"JIMM_DNS_NAME": "juju-jimm-k8s-0.juju-jimm-k8s-endpoints.None.svc.cluster.local",
"JIMM_ENABLE_JWKS_ROTATOR": "1",
"JIMM_LISTEN_ADDR": ":8080",
Expand Down
7 changes: 7 additions & 0 deletions charms/jimm/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@
# See LICENSE file for licensing details.

options:
audit-log-retention-period:
ale8k marked this conversation as resolved.
Show resolved Hide resolved
type: string
description: |
How long to hold audit logs for in days, i.e., 10 = 10 days.
If the value 0 is used, the logs will never be purged.
Logs are purged at 9AM UTC. Defaults to 0 days.
ale8k marked this conversation as resolved.
Show resolved Hide resolved
default: "0"
candid-agent-private-key:
type: string
description: |
Expand Down
1 change: 1 addition & 0 deletions charms/jimm/src/charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ def _on_config_changed(self, _):
"dashboard_location": self.config.get("juju-dashboard-location"),
"public_key": self.config.get("public-key"),
"private_key": self.config.get("private-key"),
"audit_retention_period": self.config.get("audit-log-retention-period", ""),
}

with open(self._env_filename(), "wt") as f:
Expand Down
3 changes: 2 additions & 1 deletion charms/jimm/templates/jimm.env
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,5 @@ PRIVATE_KEY={{private_key}}
{% endif %}
{% if public_key %}
PUBLIC_KEY={{public_key}}
{% endif %}
{% endif %}
JIMM_AUDIT_LOG_RETENTION_PERIOD_IN_DAYS={{audit_retention_period}}
Copy link
Contributor

Choose a reason for hiding this comment

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

new line, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing this now

25 changes: 20 additions & 5 deletions charms/jimm/tests/test_charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,14 @@ def test_config_changed(self):
"uuid": "caaa4ba4-e2b5-40dd-9bf3-2bd26d6e17aa",
"public-key": "izcYsQy3TePp6bLjqOo3IRPFvkQd2IKtyODGqC6SdFk=",
"private-key": "ly/dzsI9Nt/4JxUILQeAX79qZ4mygDiuYGqc2ZEiDEc=",
"audit-log-retention-period": "10",
}
)
self.assertTrue(os.path.exists(config_file))
with open(config_file) as f:
lines = f.readlines()
os.unlink(config_file)
self.assertEqual(len(lines), 16)
self.assertEqual(len(lines), 18)
self.assertEqual(lines[0].strip(), "BAKERY_AGENT_FILE=")
self.assertEqual(lines[1].strip(), "CANDID_URL=https://candid.example.com")
self.assertEqual(lines[2].strip(), "JIMM_ADMINS=user1 user2 group1")
Expand All @@ -142,6 +143,10 @@ def test_config_changed(self):
lines[15].strip(),
"PUBLIC_KEY=izcYsQy3TePp6bLjqOo3IRPFvkQd2IKtyODGqC6SdFk=",
)
self.assertEqual(
lines[17].strip(),
"JIMM_AUDIT_LOG_RETENTION_PERIOD_IN_DAYS=10",
)

def test_config_changed_redirect_to_dashboard(self):
config_file = os.path.join(self.harness.charm.charm_dir, "juju-jimm.env")
Expand All @@ -155,13 +160,14 @@ def test_config_changed_redirect_to_dashboard(self):
"juju-dashboard-location": "https://test.jaas.ai/models",
"public-key": "izcYsQy3TePp6bLjqOo3IRPFvkQd2IKtyODGqC6SdFk=",
"private-key": "ly/dzsI9Nt/4JxUILQeAX79qZ4mygDiuYGqc2ZEiDEc=",
"audit-log-retention-period": "10",
}
)
self.assertTrue(os.path.exists(config_file))
with open(config_file) as f:
lines = f.readlines()
os.unlink(config_file)
self.assertEqual(len(lines), 16)
self.assertEqual(len(lines), 18)
self.assertEqual(lines[0].strip(), "BAKERY_AGENT_FILE=")
self.assertEqual(lines[1].strip(), "CANDID_URL=https://candid.example.com")
self.assertEqual(lines[2].strip(), "JIMM_ADMINS=user1 user2 group1")
Expand All @@ -180,6 +186,10 @@ def test_config_changed_redirect_to_dashboard(self):
lines[15].strip(),
"PUBLIC_KEY=izcYsQy3TePp6bLjqOo3IRPFvkQd2IKtyODGqC6SdFk=",
)
self.assertEqual(
lines[17].strip(),
"JIMM_AUDIT_LOG_RETENTION_PERIOD_IN_DAYS=10",
)

def test_config_changed_ready(self):
config_file = os.path.join(self.harness.charm.charm_dir, "juju-jimm.env")
Expand All @@ -192,13 +202,14 @@ def test_config_changed_ready(self):
"uuid": "caaa4ba4-e2b5-40dd-9bf3-2bd26d6e17aa",
"public-key": "izcYsQy3TePp6bLjqOo3IRPFvkQd2IKtyODGqC6SdFk=",
"private-key": "ly/dzsI9Nt/4JxUILQeAX79qZ4mygDiuYGqc2ZEiDEc=",
"audit-log-retention-period": "10",
}
)
self.assertTrue(os.path.exists(config_file))
with open(config_file) as f:
lines = f.readlines()
os.unlink(config_file)
self.assertEqual(len(lines), 14)
self.assertEqual(len(lines), 16)
self.assertEqual(lines[0].strip(), "BAKERY_AGENT_FILE=")
self.assertEqual(lines[1].strip(), "CANDID_URL=https://candid.example.com")
self.assertEqual(lines[2].strip(), "JIMM_ADMINS=user1 user2 group1")
Expand All @@ -216,6 +227,10 @@ def test_config_changed_ready(self):
lines[13].strip(),
"PUBLIC_KEY=izcYsQy3TePp6bLjqOo3IRPFvkQd2IKtyODGqC6SdFk=",
)
self.assertEqual(
lines[15].strip(),
"JIMM_AUDIT_LOG_RETENTION_PERIOD_IN_DAYS=10",
)

def test_config_changed_with_agent(self):
config_file = os.path.join(self.harness.charm.charm_dir, "juju-jimm.env")
Expand All @@ -242,7 +257,7 @@ def test_config_changed_with_agent(self):

with open(config_file) as f:
lines = f.readlines()
self.assertEqual(len(lines), 14)
self.assertEqual(len(lines), 16)
self.assertEqual(
lines[0].strip(),
"BAKERY_AGENT_FILE=" + self.harness.charm._agent_filename,
Expand All @@ -268,7 +283,7 @@ def test_config_changed_with_agent(self):
)
with open(config_file) as f:
lines = f.readlines()
self.assertEqual(len(lines), 14)
self.assertEqual(len(lines), 16)
self.assertEqual(lines[0].strip(), "BAKERY_AGENT_FILE=")
self.assertEqual(lines[1].strip(), "CANDID_URL=https://candid.example.com")
self.assertEqual(lines[2].strip(), "JIMM_ADMINS=user1 user2 group1")
Expand Down
5 changes: 3 additions & 2 deletions cmd/jimmsrv/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,9 @@ func start(ctx context.Context, s *service.Service) error {
Token: os.Getenv("OPENFGA_TOKEN"),
Port: os.Getenv("OPENFGA_PORT"),
},
PrivateKey: os.Getenv("BAKERY_PRIVATE_KEY"),
PublicKey: os.Getenv("BAKERY_PUBLIC_KEY"),
PrivateKey: os.Getenv("BAKERY_PRIVATE_KEY"),
PublicKey: os.Getenv("BAKERY_PUBLIC_KEY"),
AuditLogRetentionPeriodInDays: os.Getenv("JIMM_AUDIT_LOG_RETENTION_PERIOD_IN_DAYS"),
})
if err != nil {
return err
Expand Down
1 change: 1 addition & 0 deletions docker-compose.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ services:
JIMM_TEST_PGXDSN: ""
JIMM_JWT_EXPIRY: 30s
JIMM_ENABLE_JWKS_ROTATOR: "1"
JIMM_AUDIT_LOG_RETENTION_PERIOD_IN_DAYS: "1"
TEST_LOGGING_CONFIG: ""
PUBLIC_KEY: "izcYsQy3TePp6bLjqOo3IRPFvkQd2IKtyODGqC6SdFk="
PRIVATE_KEY: "ly/dzsI9Nt/4JxUILQeAX79qZ4mygDiuYGqc2ZEiDEc="
Expand Down
15 changes: 15 additions & 0 deletions internal/db/audit.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/canonical/jimm/internal/dbmodel"
"github.com/canonical/jimm/internal/errors"
"github.com/canonical/jimm/internal/servermon"
)

// AddAuditLogEntry adds a new entry to the audit log.
Expand Down Expand Up @@ -103,3 +104,17 @@ func (d *Database) ForEachAuditLogEntry(ctx context.Context, filter AuditLogFilt
}
return nil
}

// CleanupAuditLogs cleans up audit logs after the auditLogRetentionPeriodInDays,
// HARD deleting them from the database.
func (d *Database) CleanupAuditLogs(ctx context.Context, auditLogRetentionPeriodInDays int) (int64, error) {
retentionDate := time.Now().AddDate(0, 0, -(auditLogRetentionPeriodInDays))
Copy link
Contributor

Choose a reason for hiding this comment

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

we might run into problems testing because time is calculated here.. would be better to pass in the cut-off date.. better for testability

Copy link
Contributor

Choose a reason for hiding this comment

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

This function is present in both this and #1008 so please discuss and align them

duration := time.Since(time.Now())
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't work.. instead

start := time.Now()

tx := d.DB.
WithContext(ctx).
Unscoped().
Where("time < ?", retentionDate).
Delete(&dbmodel.AuditLogEntry{})
servermon.QueryTimeAuditLogCleanUpHistogram.Observe(duration.Seconds())
Copy link
Contributor

Choose a reason for hiding this comment

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

then

servermon.QueryTimeAuditLogCleanUpHistogram.Observe(time.Since(start))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed aha

return tx.RowsAffected, tx.Error
}
47 changes: 47 additions & 0 deletions internal/db/auditlog_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,3 +149,50 @@ func (s *dbSuite) TestForEachAuditLogEntry(c *qt.C) {
c.Check(calls, qt.Equals, 1)
c.Check(err, qt.DeepEquals, testError)
}

func (s *dbSuite) TestCleanupAuditLogs(c *qt.C) {
ctx := context.Background()
now := time.Now()

err := s.Database.AddAuditLogEntry(ctx, &dbmodel.AuditLogEntry{
ale8k marked this conversation as resolved.
Show resolved Hide resolved
Time: now.AddDate(0, 0, -1),
})
c.Check(errors.ErrorCode(err), qt.Equals, errors.CodeUpgradeInProgress)

err = s.Database.Migrate(context.Background(), true)
c.Assert(err, qt.IsNil)

// A log from 1 day ago
c.Assert(s.Database.AddAuditLogEntry(ctx, &dbmodel.AuditLogEntry{
Time: now.AddDate(0, 0, -1),
}), qt.IsNil)

// A log from 2 days ago
c.Assert(s.Database.AddAuditLogEntry(ctx, &dbmodel.AuditLogEntry{
Time: now.AddDate(0, 0, -2),
}), qt.IsNil)

// A log from 3 days ago
c.Assert(s.Database.AddAuditLogEntry(ctx, &dbmodel.AuditLogEntry{
Time: now.AddDate(0, 0, -3),
}), qt.IsNil)

// Ensure 3 exist
logs := make([]dbmodel.AuditLogEntry, 0)
err = s.Database.DB.Find(&logs).Error
c.Assert(err, qt.IsNil)
c.Assert(logs, qt.HasLen, 3)

// Delete all 2 or more days older, leaving 1 log left
deleted, err := s.Database.CleanupAuditLogs(ctx, 2)
c.Assert(err, qt.IsNil)

// Check that 2 were infact deleted
c.Assert(deleted, qt.Equals, int64(2))

// Check only 1 remains
logs = make([]dbmodel.AuditLogEntry, 0)
err = s.Database.DB.Find(&logs).Error
c.Assert(err, qt.IsNil)
c.Assert(logs, qt.HasLen, 1)
}
73 changes: 73 additions & 0 deletions internal/jimm/audit_log.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/juju/zaputil/zapctx"
"go.uber.org/zap"

"github.com/canonical/jimm/internal/db"
"github.com/canonical/jimm/internal/dbmodel"
"github.com/canonical/jimm/internal/servermon"
)
Expand Down Expand Up @@ -127,3 +128,75 @@ func (o recorder) HandleReply(r rpc.Request, header *rpc.Header, body interface{
servermon.WebsocketRequestDuration.WithLabelValues(r.Type, r.Action).Observe(float64(d) / float64(time.Second))
return o.logger.LogResponse(r, header, body)
}

// AuditLogCleanupService is a service capable of cleaning up audit logs
// on a defined retention period. The retention period is in DAYS.
type auditLogCleanupService struct {
auditLogRetentionPeriodInDays int
db db.Database
}

// pollTimeOfDay holds the time hour, minutes and seconds to poll at.
type pollTimeOfDay struct {
Hours int
Minutes int
Seconds int
}

var pollDuration = pollTimeOfDay{
Hours: 9,
}

// NewAuditLogCleanupService returns a service capable of cleaning up audit logs
// on a defined retention period. The retention period is in DAYS.
func NewAuditLogCleanupService(db db.Database, auditLogRetentionPeriodInDays int) *auditLogCleanupService {
return &auditLogCleanupService{
auditLogRetentionPeriodInDays: auditLogRetentionPeriodInDays,
db: db,
}
}

// Start starts a routine which checks daily for any logs
// needed to be cleaned up.
func (a *auditLogCleanupService) Start(ctx context.Context) {
go a.poll(ctx)
}

// calculateNextPollDuration returns the next duration to poll on.
// We recalculate each time and not rely on running every 24 hours
// for absolute consistency within ns apart.
func (a *auditLogCleanupService) calculateNextPollDuration() time.Duration {
ale8k marked this conversation as resolved.
Show resolved Hide resolved
now := time.Now().UTC()
nineAM := time.Date(now.Year(), now.Month(), now.Day(), pollDuration.Hours, 0, 0, 0, time.UTC)
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 this be time.Date(now.Year(), now.Month(), now.Day(), pollDuration.Hours, 0, 0, 0, time.UTC).Add(0,0,1) ... so nine am tomorrow.. not today..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's handled below, but we need to check if its a negative duration first then turn it absolute

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 have added teststo show it work

nineAMDuration := nineAM.Sub(now)
d := time.Hour
// If 9am is behind the current time, i.e., 1pm
if nineAMDuration < 0 {
// Add 24 hours, flip it to an absolute duration, i.e., -10h == 10h
// and subtract it from 24 hours to calculate 9am tomorrow
d = time.Hour*24 - nineAMDuration.Abs()
} else {
d = nineAMDuration.Abs()
}
return d
}

// poll is designed to be run in a routine where it can be cancelled safely
// from the service's context. It calculates the poll duration at 9am each day
// UTC.
func (a *auditLogCleanupService) poll(ctx context.Context) {
for {
select {
case <-time.After(a.calculateNextPollDuration()):
deleted, err := a.db.CleanupAuditLogs(ctx, a.auditLogRetentionPeriodInDays)
if err != nil {
zapctx.Error(ctx, "failed to cleanup audit logs", zap.Error(err))
continue
}
zapctx.Debug(ctx, "audit log cleanup run successfully", zap.Int64("count", deleted))
case <-ctx.Done():
zapctx.Debug(ctx, "exiting audit log cleanup polling")
return
ale8k marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Loading