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.

Sign up for GitHub

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

Jump to bottom

PBIS AD login integration #78351

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

PBIS AD login integration #78351

wants to merge 2 commits into from

Conversation

tekeri
Copy link
Contributor

@tekeri tekeri commented Jan 23, 2020

Motivation for this change

Bring Active Directory authentication to NixOS.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

aanderse
aanderse requested changes Jan 25, 2020
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

I've taken a first pass at review of your module. A number of my comments are just minor points, convention, etc... but there are a few material points I look forward to getting your feedback on. I will test this on one of my servers at some point.

Out of curiosity what are the benefits you saw in this solution compared to others available?

Thanks for contributing this module!

nixos/modules/services/security/pbis.nix Outdated Show resolved Hide resolved
nixos/modules/services/security/pbis.nix Outdated Show resolved Hide resolved
nixos/modules/services/security/pbis.nix Outdated Show resolved Hide resolved
nixos/modules/services/security/pbis.nix Outdated Show resolved Hide resolved
nixos/modules/services/security/pbis.nix Outdated Show resolved Hide resolved
nixos/modules/services/security/pbis.nix Outdated Show resolved Hide resolved
nixos/modules/services/security/pbis.nix Outdated Show resolved Hide resolved
nixos/modules/services/security/pbis.nix Outdated Show resolved Hide resolved
nixos/modules/services/security/pbis.nix Show resolved Hide resolved
nixos/modules/services/security/pbis.nix
@@ -0,0 +1,148 @@
{ config, lib, pkgs, ... }:
Copy link
Member

Choose a reason for hiding this comment

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

I would encourage you to add yourself as a maintainer to this module, if you are interested in doing so.

@tekeri
Copy link
Contributor Author

tekeri commented Jan 27, 2020

Out of curiosity what are the benefits you saw in this solution compared to others available?

Compatibility. PBIS derives uid/gid from users' SID.
Pros: no need to manage IDs.
Cons: it feels like vendor lock-in when I consider uid/gid consistency across different systems or host-container.

@aanderse
Copy link
Member

aanderse commented Jan 27, 2020 •
edited

Pros: no need to manage IDs.

Oh... you could add map passwd uidNumber objectSid:YOUR_OBJECT_SID to users.ldap.daemon.extraConfig and have the same result. Is that the only reason?

@tekeri
Copy link
Contributor Author

tekeri commented Jan 28, 2020

It is not realistic to manage uids by hand every time people join or leave...
And pbis is needed to make NixOS as Ubuntu replacement.

@aanderse
Copy link
Member

@tekeri I think you misunderstand... what that configuration does is compute a unique uid for each and every user based on the users underlying objectSid from AD. That way you don't have to assign or manage a uid for anyone as all uid values are a calculated function, unique across all members, and consistent across all machines.

See https://www.markturner.net/2019/09/27/ad-ldap-authentication-on-linux-hosts/ for a good explanation.

@tekeri
Copy link
Contributor Author

tekeri commented Jan 29, 2020 •
edited

@aanderse Umm.. I'm not sure that configuration utilize same hash function defined in pbis? https://github.com/BeyondTrust/pbis-open/blob/master/lwadtool/libadtool/ids.c#L183
At least, the last component of my SID does not match with my uid.

@aanderse
Copy link
Member

@tekeri I was just intending to show you that having a stable uid (and gid) is possible with other solutions. As far as the SID... https://serverfault.com/questions/851864/get-sid-by-its-objectsid-using-ldapsearch

@tekeri
Copy link
Contributor Author

tekeri commented Jan 29, 2020

@aanderse Thanks for the info. But I want to integrate NixOS into the existing network which is out of my control and using pbis there. uid inconsistency cause inconvenience with rsync, ls from container host, etc.
I've worked hard to avoid pbis and I'm still willing to migrate to any solution if it won't break.

@aanderse
Copy link
Member

@tekeri yeah, makes sense to me. I was just trying to provide some information in case it provided any value. I think you've done a very good job on this module so far, which I imagine wasn't easy given the constraints of the software.

I'm happy to give this a test after someone else (possibly @infinisil?) provides some more review on this.

👍

@tekeri
Copy link
Contributor Author

tekeri commented Jan 29, 2020

@aanderse I'm happy to hear that, too.
Package is messy but I hope it should work like a charm :)

@tekeri
Copy link
Contributor Author

tekeri commented Apr 1, 2020

ping, just for remind 😃

@stale
Copy link

stale bot commented Jun 4, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 4, 2021
@wegank wegank marked this pull request as draft March 20, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanderse aanderse aanderse requested changes

@infinisil infinisil Awaiting requested review from infinisil

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
2.status: merge conflict 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tekeri @aanderse @ryantm