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

policies/reputation: save to database directly #10059

Merged
merged 5 commits into from
Jun 14, 2024

Conversation

rissson
Copy link
Member

@rissson rissson commented Jun 10, 2024

Details

REPLACE ME


Checklist

  • Local tests pass (ak test authentik/)
  • The code has been formatted (make lint-fix)

If an API change has been made

  • The API schema has been updated (make gen-build)

If changes to the frontend have been made

  • The code has been formatted (make web)

If applicable

  • The documentation has been updated
  • The documentation has been formatted (make website)

@rissson rissson self-assigned this Jun 10, 2024
@rissson rissson requested a review from a team as a code owner June 10, 2024 12:04
Copy link

netlify bot commented Jun 10, 2024

Deploy Preview for authentik-storybook canceled.

Name Link
🔨 Latest commit b5dd60b
🔍 Latest deploy log https://app.netlify.com/sites/authentik-storybook/deploys/666983a0ad252500080c072d

Copy link

netlify bot commented Jun 10, 2024

Deploy Preview for authentik-docs canceled.

Name Link
🔨 Latest commit b5dd60b
🔍 Latest deploy log https://app.netlify.com/sites/authentik-docs/deploys/666983a0766eef0008fe83b9

@rissson rissson force-pushed the reputation-delete-once-imported branch from c17571f to 5294625 Compare June 10, 2024 12:06
Copy link

codecov bot commented Jun 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.64%. Comparing base (7bb90b1) to head (b5dd60b).
Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10059      +/-   ##
==========================================
- Coverage   92.64%   92.64%   -0.01%     
==========================================
  Files         713      711       -2     
  Lines       34884    34854      -30     
==========================================
- Hits        32317    32289      -28     
+ Misses       2567     2565       -2     
Flag Coverage Δ
e2e 49.59% <100.00%> (-0.05%) ⬇️
integration 25.45% <80.00%> (-0.03%) ⬇️
unit 90.11% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rissson rissson force-pushed the reputation-delete-once-imported branch from b7132d1 to 6f1758a Compare June 11, 2024 08:44
@rissson rissson changed the title policies/reputation: delete reputation data from cache once save in database policies/reputation: save to database directly Jun 11, 2024
rissson added 3 commits June 11, 2024 10:55
Signed-off-by: Marc 'risson' Schmitt <[email protected]>
Signed-off-by: Marc 'risson' Schmitt <[email protected]>
@rissson rissson force-pushed the reputation-delete-once-imported branch from 542891b to e756aac Compare June 11, 2024 08:55
Copy link
Member

@BeryJu BeryJu left a comment

Choose a reason for hiding this comment

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

Some things

authentik/lib/default.yml Show resolved Hide resolved
"ip_geo_data": GEOIP_CONTEXT_PROCESSOR.city_dict(remote_ip) or {},
"ip_asn_data": ASN_CONTEXT_PROCESSOR.asn_dict(remote_ip) or {},
}
)
Copy link
Member

Choose a reason for hiding this comment

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

We'll also need to update the expiry timestamp here based on the setting above...and maybe wrap this call in a transaction/catch errors as any saving here would prevent logins from happening due to this being in a sync signal

Copy link
Member Author

Choose a reason for hiding this comment

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

Weird that the expiry wasn't updated previously.

Copy link
Member Author

Choose a reason for hiding this comment

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

As for the catching exception, the previous code didn't and I think it's a good thing, as if we can't save the reputation score anywhere, you shouldn't be able to login as that data can be used for security purposes.

def save_reputation(self: SystemTask):
"""Save currently cached reputation to database"""
objects_to_update = []
for _, score in cache.get_many(cache.keys(CACHE_KEY_PREFIX + "*")).items():
Copy link
Member

Choose a reason for hiding this comment

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

We might still want to keep this task around/update the other task to attempt to run a vacuum on this table occasionally since there could be quite a bit of writing/deleting to/from it.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's already the expiring model cleanup which will remove the expired data.

For vacuuming, I think we should let postgres' autovacuum do its thing for now, and if we have report that things are slow, we can implement that.

Copy link
Member

Choose a reason for hiding this comment

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

true, although I dont know what the default settings for autovacuum are both for compose and k8s installs

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be pretty sensible, I think they're the same as a default postgres installation

identifier=identifier,
defaults={
"score": amount,
"ip_geo_data": GEOIP_CONTEXT_PROCESSOR.city_dict(remote_ip) or {},
Copy link
Member

Choose a reason for hiding this comment

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

I think both this and ip_asn_data we should maybe dynamically fetch instead of storing in it in the database as the lookup can take quite a bit of time? And then we can cache results of GEOIP_CONTEXT_PROCESSOR based on the IP within the process? (or in redis..?) (maybe this is premature optimization)

Copy link
Member Author

Choose a reason for hiding this comment

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

Everything should be in memory so it should be quite fast, maybe even faster than storing things in Redis.

Signed-off-by: Marc 'risson' Schmitt <[email protected]>
Comment on lines +99 to +103
indexes = [
models.Index(fields=["identifier"]),
models.Index(fields=["ip"]),
models.Index(fields=["ip", "identifier"]),
]
Copy link
Member Author

Choose a reason for hiding this comment

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

Not fully sure about those either, but it should help a bit

"ip_geo_data": GEOIP_CONTEXT_PROCESSOR.city_dict(remote_ip) or {},
"ip_asn_data": ASN_CONTEXT_PROCESSOR.asn_dict(remote_ip) or {},
}
)
Copy link
Member Author

Choose a reason for hiding this comment

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

As for the catching exception, the previous code didn't and I think it's a good thing, as if we can't save the reputation score anywhere, you shouldn't be able to login as that data can be used for security purposes.

Signed-off-by: Marc 'risson' Schmitt <[email protected]>
Copy link
Contributor

authentik PR Installation instructions

Instructions for docker-compose

Add the following block to your .env file:

AUTHENTIK_IMAGE=ghcr.io/goauthentik/dev-server
AUTHENTIK_TAG=gh-ghcr.io/goauthentik/dev-server:gh-b5dd60b62b6d8a890d9b470561c7dfa5c6983362
AUTHENTIK_OUTPOSTS__CONTAINER_IMAGE_BASE=ghcr.io/goauthentik/dev-%(type)s:gh-%(build_hash)s

For arm64, use these values:

AUTHENTIK_IMAGE=ghcr.io/goauthentik/dev-server
AUTHENTIK_TAG=gh-ghcr.io/goauthentik/dev-server:gh-b5dd60b62b6d8a890d9b470561c7dfa5c6983362-arm64
AUTHENTIK_OUTPOSTS__CONTAINER_IMAGE_BASE=ghcr.io/goauthentik/dev-%(type)s:gh-%(build_hash)s

Afterwards, run the upgrade commands from the latest release notes.

Instructions for Kubernetes

Add the following block to your values.yml file:

authentik:
    outposts:
        container_image_base: ghcr.io/goauthentik/dev-%(type)s:gh-%(build_hash)s
image:
    repository: ghcr.io/goauthentik/dev-server
    tag: gh-ghcr.io/goauthentik/dev-server:gh-b5dd60b62b6d8a890d9b470561c7dfa5c6983362

For arm64, use these values:

authentik:
    outposts:
        container_image_base: ghcr.io/goauthentik/dev-%(type)s:gh-%(build_hash)s
image:
    repository: ghcr.io/goauthentik/dev-server
    tag: gh-ghcr.io/goauthentik/dev-server:gh-b5dd60b62b6d8a890d9b470561c7dfa5c6983362-arm64

Afterwards, run the upgrade commands from the latest release notes.

@BeryJu BeryJu merged commit 30e39c7 into main Jun 14, 2024
69 checks passed
@BeryJu BeryJu deleted the reputation-delete-once-imported branch June 14, 2024 13:34
kensternberg-authentik added a commit that referenced this pull request Jun 14, 2024
* main:
  website/docs: release notes for 2024.6 (#9812)
  policies/reputation: save to database directly (#10059)
  providers/enterprise: import user/group data when manually linking objects (#10089)
  core, web: update translations (#10108)
  web: Add enterprise / FIPS notification to the AdminOverviewPage (#10090)
  core: bump github.com/getsentry/sentry-go from 0.28.0 to 0.28.1 (#10095)
  web: bump API Client version (#10107)
  admin: system api: do not show FIPS status if no valid license (#10091)
  root: add configuration option to enable fips (#10088)
  web: bump the sentry group across 1 directory with 2 updates (#10101)
  web: bump ts-pattern from 5.1.2 to 5.2.0 in /web (#10098)
  web: bump the storybook group across 1 directory with 7 updates (#10102)
  core: bump github.com/gorilla/websocket from 1.5.2 to 1.5.3 (#10103)
  core: bump pydantic from 2.7.3 to 2.7.4 (#10093)
  core: bump bandit from 1.7.8 to 1.7.9 (#10094)
kensternberg-authentik added a commit that referenced this pull request Jun 17, 2024
* main:
  website/docs: release notes for 2024.6 (#9812)
  policies/reputation: save to database directly (#10059)
  providers/enterprise: import user/group data when manually linking objects (#10089)
  core, web: update translations (#10108)
  web: Add enterprise / FIPS notification to the AdminOverviewPage (#10090)
  core: bump github.com/getsentry/sentry-go from 0.28.0 to 0.28.1 (#10095)
  web: bump API Client version (#10107)
  admin: system api: do not show FIPS status if no valid license (#10091)
  root: add configuration option to enable fips (#10088)
  web: bump the sentry group across 1 directory with 2 updates (#10101)
  web: bump ts-pattern from 5.1.2 to 5.2.0 in /web (#10098)
  web: bump the storybook group across 1 directory with 7 updates (#10102)
  core: bump github.com/gorilla/websocket from 1.5.2 to 1.5.3 (#10103)
  core: bump pydantic from 2.7.3 to 2.7.4 (#10093)
  core: bump bandit from 1.7.8 to 1.7.9 (#10094)
kensternberg-authentik added a commit that referenced this pull request Jun 17, 2024
* dev: (335 commits)
  website/docs: release notes for 2024.6 (#9812)
  policies/reputation: save to database directly (#10059)
  providers/enterprise: import user/group data when manually linking objects (#10089)
  core, web: update translations (#10108)
  web: Add enterprise / FIPS notification to the AdminOverviewPage (#10090)
  core: bump github.com/getsentry/sentry-go from 0.28.0 to 0.28.1 (#10095)
  web: bump API Client version (#10107)
  admin: system api: do not show FIPS status if no valid license (#10091)
  root: add configuration option to enable fips (#10088)
  web: bump the sentry group across 1 directory with 2 updates (#10101)
  web: bump ts-pattern from 5.1.2 to 5.2.0 in /web (#10098)
  web: bump the storybook group across 1 directory with 7 updates (#10102)
  core: bump github.com/gorilla/websocket from 1.5.2 to 1.5.3 (#10103)
  core: bump pydantic from 2.7.3 to 2.7.4 (#10093)
  core: bump bandit from 1.7.8 to 1.7.9 (#10094)
  website/developer-docs: add a baby Style Guide (#9900)
  website/integrations: gitlab: update certificate key pair location and specify sha (#9925)
  root: handle asgi exception (#10085)
  website: bump prettier from 3.3.1 to 3.3.2 in /website (#10082)
  web: bump prettier from 3.3.1 to 3.3.2 in /web (#10081)
  ...
kensternberg-authentik added a commit that referenced this pull request Jun 17, 2024
* main:
  website/docs: release notes for 2024.6 (#9812)
  policies/reputation: save to database directly (#10059)
  providers/enterprise: import user/group data when manually linking objects (#10089)
  core, web: update translations (#10108)
  web: Add enterprise / FIPS notification to the AdminOverviewPage (#10090)
  core: bump github.com/getsentry/sentry-go from 0.28.0 to 0.28.1 (#10095)
  web: bump API Client version (#10107)
  admin: system api: do not show FIPS status if no valid license (#10091)
  root: add configuration option to enable fips (#10088)
  web: bump the sentry group across 1 directory with 2 updates (#10101)
  web: bump ts-pattern from 5.1.2 to 5.2.0 in /web (#10098)
  web: bump the storybook group across 1 directory with 7 updates (#10102)
  core: bump github.com/gorilla/websocket from 1.5.2 to 1.5.3 (#10103)
  core: bump pydantic from 2.7.3 to 2.7.4 (#10093)
  core: bump bandit from 1.7.8 to 1.7.9 (#10094)
kensternberg-authentik added a commit that referenced this pull request Jun 17, 2024
* main: (196 commits)
  website/docs: release notes for 2024.6 (#9812)
  policies/reputation: save to database directly (#10059)
  providers/enterprise: import user/group data when manually linking objects (#10089)
  core, web: update translations (#10108)
  web: Add enterprise / FIPS notification to the AdminOverviewPage (#10090)
  core: bump github.com/getsentry/sentry-go from 0.28.0 to 0.28.1 (#10095)
  web: bump API Client version (#10107)
  admin: system api: do not show FIPS status if no valid license (#10091)
  root: add configuration option to enable fips (#10088)
  web: bump the sentry group across 1 directory with 2 updates (#10101)
  web: bump ts-pattern from 5.1.2 to 5.2.0 in /web (#10098)
  web: bump the storybook group across 1 directory with 7 updates (#10102)
  core: bump github.com/gorilla/websocket from 1.5.2 to 1.5.3 (#10103)
  core: bump pydantic from 2.7.3 to 2.7.4 (#10093)
  core: bump bandit from 1.7.8 to 1.7.9 (#10094)
  website/developer-docs: add a baby Style Guide (#9900)
  website/integrations: gitlab: update certificate key pair location and specify sha (#9925)
  root: handle asgi exception (#10085)
  website: bump prettier from 3.3.1 to 3.3.2 in /website (#10082)
  web: bump prettier from 3.3.1 to 3.3.2 in /web (#10081)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants