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

Add PAM authentication support #66

Closed
wants to merge 1 commit into from
Closed

Add PAM authentication support #66

wants to merge 1 commit into from

Conversation

konradgraefe
Copy link
Contributor

@konradgraefe konradgraefe commented Mar 31, 2022 •
edited

Hello @cminyard ,

this PR addresses cminyard/gensio#26 . I think I was on the wrong track the whole time, since the certauth only collects the username and password but leaves the actual password verification to the server application. Therefore I figured this should be the place to PAM authentication into the application which is ser2net in our case. Also it is a lot less code to maintain.

Currently I am not done yet. But I want to kindly ask for your opinion before going further in this direction as it's still our goal to merge this into mainline ser2net.

Test setup

Server

❯ cat ser2net.yaml 
connection: &test-server
  accepter: certauth(enable-password),ssl(key=ser2net.key,cert=ser2net.crt),tcp,2013
  connector: serialdev,/dev/serial/by-id/usb-FTDI_TTL232R-3V3_FTASU5TK-if00-port0,115200N81
  options:
    banner: ser2net Echo Server\r\n
    mdns: false
    authdir: .
    pamauth: true
❯ cat pam.d/other 
auth     required /usr/lib/x86_64-linux-gnu/pam_wrapper/pam_matrix.so passdb=/data/projects/ser2net-pam/ser2net-test/pam.d/passdb
account  required /usr/lib/x86_64-linux-gnu/pam_wrapper/pam_matrix.so passdb=/data/projects/ser2net-pam/ser2net-test/pam.d/passdb
password required /usr/lib/x86_64-linux-gnu/pam_wrapper/pam_matrix.so passdb=/data/projects/ser2net-pam/ser2net-test/pam.d/passdb
session  required /usr/lib/x86_64-linux-gnu/pam_wrapper/pam_matrix.so passdb=/data/projects/ser2net-pam/ser2net-test/pam.d/passdb
❯ cat pam.d/passdb 
bob:secret:ser2net
❯ LD_PRELOAD=libpam_wrapper.so PAM_WRAPPER=1 PAM_WRAPPER_SERVICE_DIR=$PWD/pam.d ser2net -d -c ./ser2net.yaml
Unable to start mdns: Operation not supported

Client

❯ gtlssh -p 2013 --cadir . --nomux bob@localhost   
Password: secret

ser2net Echo Server

embedded device login:

TODO

  • Fix TODOs
  • Add conditional to build without PAM support
  • Documentation

@cminyard
Copy link
Owner

cminyard commented Mar 31, 2022 via email

@konradgraefe
Copy link
Contributor Author

PAM authentication works without root permissions (I'm on Ubuntu 20.04).

@konradgraefe
Copy link
Contributor Author

konradgraefe commented Apr 8, 2022 •
edited

You don't change the certificate authentication. It's not very hard, and you can steal the GENSIO_EVENT_AUTH_BEGIN handling code from gtlsshd.c where it sets that authdir to the .gtlssh/allowed_certs directory. I think users would appreciate certificate authentication, it's more secure and they don't have to type passwords. Plus it will still do standard ser2net certificate authentication if you don't change it.

In our scenario the user authenticates against a RADIUS server via PAM to gain access to the TTY. In that case the user does not exist locally and has no home directory on the device (nor does he need to).

@cminyard
Copy link
Owner

cminyard commented Apr 8, 2022 via email

@konradgraefe konradgraefe changed the title WIP: Add PAM support Add PAM authentication support Apr 20, 2022
@konradgraefe
Copy link
Contributor Author

@cminyard I think I addressed all open points. I also changed pamauth to be a string and use that as PAM service name so that one could configure different ser2net servers to authenticate against different PAM profiles.

@Zugschlus
Copy link

Debian maintainer of the program here. Is there documentation about PAM support, and a suggested file for /etc/pam.d included? Or will ser2net continue to function without?

@kgraefe
Copy link

kgraefe commented Apr 20, 2022 •
edited

(wrong account, sorry for the noise)

@konradgraefe
Copy link
Contributor Author

Is there documentation about PAM support,

I documented how to enable PAM support and set the PAM service name.

and a suggested file for /etc/pam.d included?

No, because that depends on your use case. Also it's not needed as PAM would use /etc/pam.conf if /etc/pam.d/service_name is not present.

Our use case is to authenticate against a RADIUS server, so we would use:

#%PAM-1.0

auth     required pam_radius_auth.so debug
account  required pam_permit.so

Or will ser2net continue to function without?

Yes. PAM authentication is disabled by default. You need to set pamauth in ser2net.yaml to activate it. Also you can disable PAM support on compile time by passing --without-pam to ./configure.

@cminyard
Copy link
Owner

cminyard commented Apr 20, 2022 •
edited

A few comments:

+       for (i = 0; i < num_msg; i++) {
+           free(resp[i].resp);
+       }

You need:

    if (resp[i].resp)
         free(...)

as resp may not be set.

Is it possible to use both pam authentication and authdir authentication at the same time? That way you could use passwords from PAM and certificates from authdir. The documentation you've written (well, modified) says that it works that way, but the code won't.

+    if (password == NULL) {
+       return PAM_BUF_ERR;
+    }

That should probably return PAM_CONV_ERR or perhaps PAM_AUTHINFO_UNAVAIL (or PAM_CRED_UNAVAIL? It's kind of unclear what to return here.).

Other than those, this looks good.

@konradgraefe
Copy link
Contributor Author

A few comments:

* ```
    for (i = 0; i < num_msg; i++) {
  ```

* ```
        free(resp[i].resp);
  ```

* ```
    }
  ```

You need: if (resp[i].resp) free(...) as resp may not be set.

See free(3p): If ptr is a null pointer, no action shall occur.

calloc() initializes all fields to zero resp. NULL and free() happily accepts NULL. So it would be less code to read and maintain. :-) However, if you prefer an additional guard I won't lose sleep over it.

Is it possible to use both pam authentication and authdir authentication at the same time? That way you could use passwords from PAM and certificates from authdir. The documentation you've written (well, modified) says that it works that way, but the code won't.

* if (password == NULL) {

* ```
    return PAM_BUF_ERR;
  ```

* }

That should probably return PAM_CONV_ERR or perhaps PAM_AUTHINFO_UNAVAIL (or PAM_CRED_UNAVAIL? It's kind of unclear what to return here.).

I will look into this, thanks.

Other than those, this looks good.

@konradgraefe
Copy link
Contributor Author

According to pam_conv(3) we should return either PAM_BUF_ERR, PAM_CONV_ERR or PAM_SUCCESS. So I guessPAM_CONV_ERR is it for password == NULL.

@konradgraefe
Copy link
Contributor Author

Is it possible to use both pam authentication and authdir authentication at the same time? That way you could use passwords from PAM and certificates from authdir. The documentation you've written (well, modified) says that it works that way, but the code won't.

I just verified that certificate authentication through certauth still works with pamauth enabled. What makes you think it doesn't? It takes place before the password authentication and is not touched by this PR.

@cminyard
Copy link
Owner

cminyard commented Apr 22, 2022 via email

@konradgraefe
Add PAM authentication support
2171a18 
Signed-off-by: Konrad Gräfe <k.graefe@gateware.de>
@konradgraefe
Copy link
Contributor Author

I think I addressed all comments. Please come back to me if I've overseen something or if you find more issues

@cminyard
Copy link
Owner

Ok, this is merged. Thank you.

@cminyard cminyard closed this Apr 22, 2022
@konradgraefe
Copy link
Contributor Author

Great, thank you.

@konradgraefe konradgraefe mentioned this pull request Apr 25, 2022
Closed
@cminyard
Copy link
Owner

cminyard commented Oct 11, 2022 via email

@konradgraefe
Copy link
Contributor Author

POSIX agrees: https://pubs.opengroup.org/onlinepubs/9699919799/functions/free.html ;-)

and according to POSIX the C standard also agrees:

The functionality described on this reference page is aligned with the ISO C standard.
Any conflict between the requirements described here and the ISO C standard is
unintentional. This volume of POSIX.1-2017 defers to the ISO C standard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@konradgraefe @cminyard @Zugschlus @kgraefe