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

sources/ldap: add ability to disable password write on login #8377

Merged
merged 5 commits into from
Mar 25, 2024

Conversation

rissson
Copy link
Member

@rissson rissson commented Jan 31, 2024

Details

Closes #6122


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)
  • The translation files have been updated (make i18n-extract)

If applicable

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

@rissson rissson self-assigned this Jan 31, 2024
@rissson rissson requested review from a team as code owners January 31, 2024 18:29
Copy link

netlify bot commented Jan 31, 2024

Deploy Preview for authentik-storybook ready!

Name Link
🔨 Latest commit 7f6c73c
🔍 Latest deploy log https://app.netlify.com/sites/authentik-storybook/deploys/660168d6d13e2400083d16cb
😎 Deploy Preview https://deploy-preview-8377--authentik-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

authentik translations instructions

Thanks for your pull request!

authentik translations are handled using Transifex. Please edit translations over there and they'll be included automatically.

Copy link

codecov bot commented Jan 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.27%. Comparing base (cef1d2d) to head (2155ea5).
Report is 401 commits behind head on main.

❗ Current head 2155ea5 differs from pull request most recent head 7f6c73c. Consider uploading reports for the commit 7f6c73c to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #8377       +/-   ##
===========================================
+ Coverage   46.62%   92.27%   +45.64%     
===========================================
  Files         626      626               
  Lines       30996    30998        +2     
===========================================
+ Hits        14451    28602    +14151     
+ Misses      16545     2396    -14149     
Flag Coverage Δ
e2e 50.64% <100.00%> (+5.92%) ⬆️
integration 25.99% <20.00%> (+<0.01%) ⬆️
unit 89.52% <40.00%> (?)

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.

Copy link
Contributor

github-actions bot commented Jan 31, 2024

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-ccaaf97850ca75be0f762b704d4bc5e739be42d8
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-ccaaf97850ca75be0f762b704d4bc5e739be42d8-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-ccaaf97850ca75be0f762b704d4bc5e739be42d8

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-ccaaf97850ca75be0f762b704d4bc5e739be42d8-arm64

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

Copy link
Contributor

@septatrix septatrix left a comment

Choose a reason for hiding this comment

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

Perfect. I have no strong opinion about the variable name. They are not exposed to the user so a more technical name using the phrasing of the source (i.e. bind for ldap) would also be fine for me but keeping it consistent across sources which also will use this could also be appealing.

@rissson rissson force-pushed the ldap-password-write-disable branch 2 times, most recently from e703102 to 25c6099 Compare February 5, 2024 13:29
@rissson rissson requested a review from BeryJu February 5, 2024 13:30
Copy link

netlify bot commented Feb 5, 2024

Deploy Preview for authentik ready!

Name Link
🔨 Latest commit e703102
🔍 Latest deploy log https://app.netlify.com/sites/authentik/deploys/65c0e2935641e2000794de10
😎 Deploy Preview https://deploy-preview-8377--authentik.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@septatrix septatrix left a comment

Choose a reason for hiding this comment

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

I know I am in no position to make request but I would really like to have my concerns of having this enabled by default regarding usability, pitfalls and security addressed, please...

@BeryJu
Copy link
Member

BeryJu commented Feb 5, 2024

Yeah I suppose after going through this again we can probably set it to False for new sources as there isn't really a strong case for leaving it enabled by default (the only thing being a future built in scheduled HIBP check, which could just check for this

We should probably also add all of the points from #8377 (comment) to the documentation, so we can update them as they are resolved (especially the first one)

@rissson
Copy link
Member Author

rissson commented Feb 5, 2024

Yeah I suppose after going through this again we can probably set it to False for new sources as there isn't really a strong case for leaving it enabled by default

Changed it back again.

Copy link

netlify bot commented Feb 5, 2024

Deploy Preview for authentik ready!

Name Link
🔨 Latest commit 484ba81
🔍 Latest deploy log https://app.netlify.com/sites/authentik/deploys/65cdc6ebdfd5ee0008fd8a11
😎 Deploy Preview https://deploy-preview-8377--authentik.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@septatrix
Copy link
Contributor

Yeah I suppose after going through this again we can probably set it to False for new sources as there isn't really a strong case for leaving it enabled by default (the only thing being a future built in scheduled HIBP check, which could just check for this

We should probably also add all of the points from #8377 (comment) to the documentation, so we can update them as they are resolved (especially the first one)

I agree in both aspects. For now this should default to false for newly created sources and the drawbacks of enabling should be documented such that one can weight the ups and downs.

Regarding the HIBP check: How would that even work with stored password (hashed and salted)? This seems like it would only work upon logging in (when we have access to the raw password) or if the leaked database used exactly the same hash function (including number of rounds etc) and salt?

Regarding my first point (password update) this should already work for sources which have a pwdLastSet property. Though sadly this is not really documented anywhere. Also this would again conflict if users might have a different password set in Authentik themselves.

At some point I was thinking if it would make more sense to drop this from the providers and instead make this into a flow step. I.e. upon successful login one would issue a user update and set the received password. This would likely make the updating more clear, reduce code duplication and offer more customizability (e.g. skip the update of admins where the redundant password store and delay until a password gets invalidated might be considered a security risk).

Signed-off-by: Marc 'risson' Schmitt <marc.schmitt@risson.space>
@rissson rissson requested a review from a team as a code owner February 5, 2024 16:23
@rissson
Copy link
Member Author

rissson commented Feb 5, 2024

Documentation updated to document this feature and associated security risks.

Copy link

netlify bot commented Feb 29, 2024

Deploy Preview for authentik-docs ready!

Name Link
🔨 Latest commit 7f6c73c
🔍 Latest deploy log https://app.netlify.com/sites/authentik-docs/deploys/660168d79ba0a900089ce6fc
😎 Deploy Preview https://deploy-preview-8377--authentik-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@septatrix
Copy link
Contributor

Any outlook on getting this merged soon?

Signed-off-by: Jens Langhammer <jens@goauthentik.io>
@BeryJu BeryJu enabled auto-merge (squash) March 25, 2024 12:09
@BeryJu BeryJu merged commit 06af8e3 into main Mar 25, 2024
63 of 65 checks passed
@BeryJu BeryJu deleted the ldap-password-write-disable branch March 25, 2024 12:22
kensternberg-authentik added a commit that referenced this pull request Mar 26, 2024
* main:
  web: bump API Client version (#9021)
  sources/ldap: add ability to disable password write on login (#8377)
  web: bump API Client version (#9020)
  lifecycle: migrate: ensure template schema exists before migrating (#8952)
  website/integrations: Update nextcloud Admin Group Expression (#7314)
  web/flow: general ux improvements (#8558)
  website: bump @types/react from 18.2.67 to 18.2.69 in /website (#9016)
  core: bump requests-oauthlib from 1.4.0 to 2.0.0 (#9018)
  web: bump the sentry group in /web with 2 updates (#9017)
  web/admin: small fixes (#9002)
  website: bump webpack-dev-middleware from 5.3.3 to 5.3.4 in /website (#9001)
  core: bump ruff from 0.3.3 to 0.3.4 (#8998)
  website/docs: Upgrade nginx reverse porxy config (#8947)
  website/docs: improve flow inspector docs (#8993)
  website/deverlop-docs website/integrations: add links to integrations template (#8995)
kensternberg-authentik added a commit that referenced this pull request Mar 26, 2024
* main: (27 commits)
  web: bump API Client version (#9035)
  website/docs: maintenance, re-add system settings (#9026)
  core: bump duo-client from 5.2.0 to 5.3.0 (#9029)
  website: bump express from 4.18.2 to 4.19.2 in /website (#9027)
  web: bump express from 4.18.3 to 4.19.2 in /web (#9028)
  web: bump the eslint group in /web with 2 updates (#9030)
  core: bump goauthentik.io/api/v3 from 3.2024022.3 to 3.2024022.5 (#9031)
  website: bump @types/react from 18.2.69 to 18.2.70 in /website (#9032)
  web: bump the eslint group in /tests/wdio with 2 updates (#9033)
  web: bump katex from 0.16.9 to 0.16.10 in /web (#9025)
  translate: Updates for file locale/en/LC_MESSAGES/django.po in fr (#9023)
  website/docs: include OS-specific docker-compose install instructions + minor fixes (#8975)
  web: bump API Client version (#9021)
  sources/ldap: add ability to disable password write on login (#8377)
  web: bump API Client version (#9020)
  lifecycle: migrate: ensure template schema exists before migrating (#8952)
  website/integrations: Update nextcloud Admin Group Expression (#7314)
  web/flow: general ux improvements (#8558)
  website: bump @types/react from 18.2.67 to 18.2.69 in /website (#9016)
  core: bump requests-oauthlib from 1.4.0 to 2.0.0 (#9018)
  ...
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.

Suppress authentik password cache for LDAP sources
3 participants