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

feat: Audit failed authz attempts #3155

Merged
merged 12 commits into from
Jun 9, 2024
Merged

feat: Audit failed authz attempts #3155

merged 12 commits into from
Jun 9, 2024

Conversation

markphelps
Copy link
Collaborator

@markphelps markphelps commented Jun 7, 2024

Fixes: #3147

Adds failed authz attempts to the audit log/event stream:

2024-06-08T08:13:41-04:00	ERROR	unauthorized	{"server": "grpc", "reason": "permission denied"}
2024-06-08T08:13:41-04:00	AUDIT	{"version": "0.2", "type": "flag", "action": "created", "status": "denied", "metadata": {"actor":{"authentication":"oidc","ip":"127.0.0.1","email":"test@flipt.io","name":"Test Test"}}, "payload": null, "timestamp": "2024-06-08T08:13:41-04:00"}

The new field is called status, and the only options currently are success and denied, happy to rethink the naming of this field or values though.

I bumped the event.version field to 0.2 to reflect this new field being added.

Also moved the cache middleware after authz middleware (we should figure out how to make this more foolproof in the future I think). I think this was a potential bug as cache could return a value that the user shouldn't have access to (ex GetFlag is cached).

Screen Shot 2024-06-08 at 08 13 45

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
…audit-authz

* 'audit-authz' of https://github.com/flipt-io/flipt:
  refactor(build): remove authentication from IT matrix (#3152)
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
…audit-authz

* 'audit-authz' of https://github.com/flipt-io/flipt:
  chore: add hybrid cloud banner to readme (#3156)
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
@markphelps markphelps marked this pull request as ready for review June 8, 2024 23:05
@markphelps markphelps requested a review from a team as a code owner June 8, 2024 23:05
Copy link
Contributor

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

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

Good catch on the authz issue.

I think the best way to avoid this kind of problem is to stop caching in grpc middleware.
While it feels like a nice spot, I think it actually has both ergonomical issues (type switching on request types, ultimately slowing everything down) and it's always easy to cause issues by ordering middleware incorrectly. They all implement the same interface, so there is no way to define compile time order constraints in this way.

I think the best course of action is to either move cache logic into the service / storage layer that is applies to. Or potentially, implement a decorator of these types. Specifically the service or storage interface type being cached. As opposed to generic grpc. This latter way can have the same issues, if you end up with lots of decorating types, then you can create a new ordering problem. However, it is easier to unit test. Whereas, the former is explicit about where caching takes place (directly around storage layer), but harder to unit test (baked into storage / service implementation) and complicates the logic of the service / storage.

Tradeoffs as always. But I think the middleground of moving it into an adaptor around the service / storage type, is at-least a step in the right direction while still easy to make optional and unit test.

Copy link
Contributor

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

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

I like the choice of status. Makes sense to me. Nice.

@markphelps markphelps added the automerge Used by Kodiak bot to automerge PRs label Jun 9, 2024
@kodiakhq kodiakhq bot merged commit 113c2cb into main Jun 9, 2024
34 checks passed
@kodiakhq kodiakhq bot deleted the audit-authz branch June 9, 2024 12:12
@markphelps markphelps added the needs docs Requires documentation updates label Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Used by Kodiak bot to automerge PRs needs docs Requires documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FLI-1068] Add failed authz attempts to audit logs
2 participants