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

Fix/ntlm auth cache #6340

Merged
merged 14 commits into from
Aug 26, 2021
Merged

Fix/ntlm auth cache #6340

merged 14 commits into from
Aug 26, 2021

Conversation

nqb
Copy link
Contributor

@nqb nqb commented Apr 30, 2021

Description

Refresh NTLM auth cache integration on Debian 11 and EL systems.

Impacts

NTLM Auth cache

Issue

fixes #6341
fixes #6342

Delete branch after merge

YES

Checklist

(REQUIRED) - [yes, no or n/a]

  • Document the feature
  • Add unit tests
  • Add acceptance tests (TestLink)

@nqb nqb self-assigned this Aug 24, 2021
@nqb nqb added this to the PacketFence-11.0 milestone Aug 24, 2021
@nqb nqb force-pushed the fix/ntlm-auth-cachegs branch 2 times, most recently from 33ae658 to ef8ffef Compare August 25, 2021 09:56
@nqb
Copy link
Contributor Author

nqb commented Aug 25, 2021

Instead of trying to fix Python errors for addons/AD/secretsdump.py, I decided to use secretsdump.py script provided by python3-impacket package on Debian 11 and EL8. They both provided impacket at version 0.9.22

I updated RPM package on inverse.ca, package provided is now: python3-impacket-0.9.22-4.el8.noarch.rpm

@julsemaan
Copy link
Collaborator

Instead of trying to fix Python errors for addons/AD/secretsdump.py, I decided to use secretsdump.py script provided by python3-impacket package on Debian 11 and EL8. They both provided impacket at version 0.9.22

I updated RPM package on inverse.ca, package provided is now: python3-impacket-0.9.22-4.el8.noarch.rpm

I'm happy to go with this but from memory some adjustments were made to the initial implementation of secretsdump.py

These were (still going from memory) related to the background job. We should make sure that the background job still works or deprecate it (it doesn't scale anyway)

Most importantly, as discussed during our group meeting, we should make sure that cache on connection works

Should we decide to deprecate the background job, we'll need an upgrade note (not script, just note) and strip it off the admin for v11. We should also open an issue to strip off the backend code post v11 since it wouldn't be wise to touch this at this point of the QA phase

@nqb
Copy link
Contributor Author

nqb commented Aug 25, 2021

100% agree with your proposal @julsemaan. I built a package, I will test cache on connection to be sure it works as expected.

migrate secretsdump.py to py3 using 2to3

use new library name (Cryptodome)

string.letters doesn't exist in py3
Previously, it doesn't work in a standalone environment.
Otherwise, strip Bind DN and use it directly
@nqb
Copy link
Contributor Author

nqb commented Aug 26, 2021

Tests:

With a DN as bind DN defined on AD source or with a DN as sAMAccountName defined on AD source

  • ✔️ EL8 standalone, NTLM cache on connection
  • ❎ EL8 standalone, NTLM cache background job (fetch all): -usersfile is certainly the custom option we added to secretsdump.py and this is why it failed
  • ✔️ EL8 standalone, NTLM cache background job + NTLM cache background job individual fetch

@nqb
Copy link
Contributor Author

nqb commented Aug 26, 2021

I added the upgrade note @julsemaan. Feel free to modify it. Because we will remove option from GUI, I told users to edit file by hand.

@satkunas, could you hide or remove "NTLM cache background job" (only this option) from NTLM Auth cache section ?

@julsemaan
Copy link
Collaborator

I added the upgrade note @julsemaan. Feel free to modify it. Because we will remove option from GUI, I told users to edit file by hand.

Perhaps I'm blind, I'm not seeing this in this PR or in devel and its not in the PR description
My 2 cents on the notes: We should simply state the deprecation, we can leave the parameter in domain.conf but just make sure that its presence will be ignored by our backend. Adding an upgrade script at this stage is kind of risky since we're currently testing the upgrade process so its not the time to introduce a regression

@satkunas
Copy link
Contributor

@nqb done

@nqb
Copy link
Contributor Author

nqb commented Aug 26, 2021

Perhaps I'm blind, I'm not seeing this in this PR or in devel and its not in the PR description

With a git push, it's always better

@nqb
Copy link
Contributor Author

nqb commented Aug 26, 2021

My 2 cents on the notes: We should simply state the deprecation, we can leave the parameter in domain.conf but just make sure that its presence will be ignored by our backend.

Agree on that, my approach was different.

@fdurand fdurand merged commit 377bb69 into devel Aug 26, 2021
@fdurand fdurand deleted the fix/ntlm-auth-cachegs branch March 11, 2024 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants