Skip to content

PBIS AD login integration #78351

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
Closed
wants to merge 2 commits into from
Closed

PBIS AD login integration #78351

wants to merge 2 commits into from
+175 −4

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.

Sorry, something went wrong.

@tekeri tekeri requested a review from infinisil as a code owner January 23, 2020 10:34
@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jan 23, 2020
@tekeri tekeri force-pushed the pbis-login-integration branch from 318e437 to 952262f Compare January 24, 2020 09:36
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
@@ -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 tekeri force-pushed the pbis-login-integration branch from 952262f to e5bafdb Compare January 27, 2020 06:22
@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
Loading

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
Loading

@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 😃

@ryantm ryantm added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 3, 2020
@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
@Aleksanaa Aleksanaa closed this Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanderse aanderse

@infinisil infinisil

Assignees
No one assigned
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tekeri @aanderse @ryantm @Aleksanaa