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

PAM authentication implemented #48

Closed
wants to merge 1 commit into from
Closed

Conversation

softScheck
Copy link
Contributor

Hi there,

we added the option to use the Linux Pluggable Authentication Modules (PAM) for MFA.

The PAM file to use can be picked with the PAM.ServiceName configuration and as PAM backend github.com/msteinert/pam is used, this is the same package used by github.com/go-gitea/gitea for example.

Hope you like and merge it. Let me know if you need something changed.

@NHAS
Copy link
Owner

NHAS commented Jul 24, 2023

Hi there and thanks so much for implementing this!

I must admit I hadn't even remotely considered implementing pam integration for wag, so I find this patch super fun.

Currently the closest thing wag has to this is the oidc provider which can dynamically configure the groups a user is part of.
For pam it might be worth doing something similar and getting the users linux groups and using them to determine which wag ACLs apply.

I.e if user John is in the linux group bonk, then the rules for group bonk in the wag config file are applied.

At minimum, I think users should have to be part of a system group in order to have wag be enabled for them. Just so we don't end up in a situation where someone accidentally enables a user to access wag when it was unintended.
(super unlikely in the current implementation as you'd have to both make a user on the system then also a registration token to match, but still worth thinking about)

Keen to hear your ideas on this.

On a side note this could also be used to display users in the Web ui to associate their groups.

@NHAS
Copy link
Owner

NHAS commented Jul 25, 2023

Sweet! So I've done a few changes and have merged them in to the unstable branch.

Just a bunch of reorganising the look and feeling of the .html files just to better fit the existing UI.

I've made not supplying the pam servicename not a validation file but it falls back to using /etc/pam.d/login which I think is reasonable.

I've also improved logging a bit so its clearer what pam config file wag is using and to log errors a bit better.

Otherwise looks awesome! Please check my work on the unstable branch and I'll give that a merge after you're happy with what I've done.

@coldBug
Copy link

coldBug commented Jul 25, 2023

Thanks, using /etc/pam.d/login as fallback sound good. The rest looks good as well.

There are only some minor things: I'm not sure what 'pamRules' is used for, as you use pamRulesFile later in pam.go

In the README.md you added Side note the name of PAM-Auth file is /etc/pam.d/wagvpn` - that seems to be a reference to the version you first implemented without the ability to select a service file.

@NHAS
Copy link
Owner

NHAS commented Jul 25, 2023

Yep that was when I was testing out just using the service name wagvpn statically set, rather than letting the user set it. Which isnt a great idea and I got rid of it, so I'll just update those little hangovers.

@softScheck
Copy link
Contributor Author

nice 👍 but in the init function you set t.serviceName = settings["ServiceName"] and in the AuthorizeFunc you use a new local variable serviceName := config.Values().Authenticators.PAM.ServiceName. And if you use settings["ServiceName" don't you have to set that variable in config.go first?

@NHAS
Copy link
Owner

NHAS commented Jul 26, 2023

Yes, one of those moments where I'd made a change but not saved it in my IDE unfortunately.

The most recent work is up there now

@softScheck
Copy link
Contributor Author

Looks good and worked for me :)

@NHAS
Copy link
Owner

NHAS commented Jul 27, 2023

Sweet! I'll do a release then.

As I've already pulled this code in to the unstable branch to test it I'll be closing this pull request but it is counted as merged.

And as a final note, if you or your organization are using wag to support your day to day operations please considering supporting the project!

Thanks again for the feature :)

@NHAS NHAS closed this Jul 27, 2023
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.

3 participants