NHAS / wag

Simple Wireguard 2FA
BSD 3-Clause "New" or "Revised" License
499 stars 27 forks source link

PAM authentication implemented #48

Closed softScheck closed 1 year ago

softScheck commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

nice :+1: 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 commented 1 year ago

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 commented 1 year ago

Looks good and worked for me :)

NHAS commented 1 year ago

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 :)