all: add support for tracking last-accessed timestamps - #163
all: add support for tracking last-accessed timestamps#163creachadair wants to merge 12 commits into
Conversation
bb30dd7 to
7da3ff3
Compare
There was a problem hiding this comment.
Best reviewed with whitespace off.
7da3ff3 to
0cdea43
Compare
0cdea43 to
1fb7d21
Compare
816d184 to
7cd0238
Compare
This is preparation for adding a Reader type in the next commit. Update usage throughout.
This allows a caller to read back through the contents of an audit log. The Reader exposes a basic iterator interface.
This field can be used to report the last time the server recorded a use of each secret, where "use" is defined as any authorized query that is not the "info" operation.
This is in preparation for adding a new option for the access index.
Wire in an initial access index, and ensure it gets updated whenever we successfully authorize an operation besides "info". Note that we will update the index even if the operation reports an error, because we are using the audit log as the source of truth, and we did in fact allow the operation even if it did not wind up doing anything.
Surface the last accessed information in list and info responses.
7cd0238 to
c267227
Compare
danderson
left a comment
danderson
left a comment
There was a problem hiding this comment.
Can I just gush about how well organized this PR is? Having all the concerns separated into different commits made it a joy to review, and very easy to follow the flow of each change.
| db.mu.Lock() | ||
| defer db.mu.Unlock() | ||
|
|
||
| // This case is special in that we only log an access if the condition |
There was a problem hiding this comment.
Depending on what this audit log access time is used for, these semantics could be confusing: a secret that hasn't been rotated for a long time but continues to be in use would show up as not accessed, outside of client restarts that force an unconditional fetch.
Not necessarily an issue depending on the intended use of the access time field, just flagging a potential for confusion if we're using the access time as a proxy for "this secret is in use by something".
There was a problem hiding this comment.
I agree, we should update the timestamp even if we do not report a value. The point of the timestamp is to reflect "interest" in the secret, and a conditional get (even one that does not report a value) should respect that.
The complicating factor is that the timestamp we record will no longer be reflected in an audit record, so later on if we restart the service it will "regress" to the previous value.
There was a problem hiding this comment.
Per our separate conversation, what do you think about e55f760?
a663495 to
e55f760
Compare
2 participants
The commits in this branch are independent, and can be reviewed separately.
Add a new field to the SecretInfo type, recording a last-accessed timestamp.
This reflects the last time at which the server authorized any access to a
secret other than "info" (acl.ActionInfo).
Rather than adding new data to the encrypted payload, we use the audit log to
initialize an index at startup, then maintain it in memory while the server
runs. We use the audit log timestamps as the source-of-truth, so the results
will be consistent even if the server stops/crashes and restarts.
Updates tailscale/corp#40218