Implement PAM authentication module for GLOME #73

vvidic · 2020-12-20T19:10:30Z

Reuses much of the existing glome-login functions.

vvidic · 2020-12-20T19:28:25Z

Fixes #39

burgerdev

Thanks a lot for your PR, it's much appreciated!

login/pam.c

burgerdev · 2020-12-21T22:17:03Z

login/pam.c

+}
+
+int pam_sm_setcred(pam_handle_t *pamh, int flags, int argc, const char **argv) {
+  return PAM_SUCCESS;


Intuitively, I would have expected a PAM_CRED_UNAVAIL or PAM_CRED_ERR, and after reading the docs I'm confused. Could you please document the reason for returning success?

Added a comment for PAM_SUCCESS in the new version of the patch.

But if we return PAM_SUCCESS here, applications will see pam_setcred(3) return with PAM_SUCCESS as well, won't they? According to the manual, this return value should be interpreted as Data was successful stored, and since we're not storing anything may be disingenuous.

PAM_CRED_UNAVAIL: This module cannot retrieve the user's credentials. sounds correct to me.

login/pam.c

burgerdev · 2020-12-22T09:15:07Z

login/pam.c

+  config->reboot_user = DEFAULT_REBOOT_USER;
+  config->login_path = DEFAULT_LOGIN_PATH;
+  config->lockdown_path = NULL;
+  config->url_prefix = NULL;
+  config->auth_delay_sec = DEFAULT_AUTH_DELAY;
+  config->input_timeout_sec = DEFAULT_INPUT_TIMEOUT;
+  config->options = SYSLOG;


Some of these options are imho not relevant for the PAM module:

auth_delay_sec should probably be implemented with pam_faildelay

Not entirely sure about input_timeout_sec, but I did not find other PAM modules with a configurable timeout.

Lockdown path is a runtime environment detail that should be handled outside of the GLOME module (c.f. pam_time).

The PAM module arguably should not cause reboots, so we don't need to know about a reboot_user.

The same goes for login_path.

Actually, I think it might be better not to use the configuration facilities of /sbin/glome-login at all and rather implement only the strictly necessary options (service key, debug, machineid?) as PAM module arguments.

Removed the parameters that are not relevant for PAM module.

As for input_timeout_sec it seems that PAM prompt waits 60 seconds for input so this should work by default too.

burgerdev · 2020-12-22T09:29:27Z

login/pam.c

+  int r = parse_pam_args(pamh, argc, argv, &config);
+  if (r < 0) {
+    pam_syslog(pamh, LOG_ERR, "failed to parse pam module arguments (%d)", r);
+    return rc;
+  }
+
+  r = parse_config_file(&config);
+  if (r < 0) {
+    pam_syslog(pamh, LOG_ERR, "failed to read config file: %s (%d)",
+               config.config_path, r);
+    return rc;
+  }


If I have a pam module config that specifies a custom service key and there happens to be a global config file with another service key, the global one would be chosen, wouldn't it? This may be unexpected.

Yes, this behaviour is mentioned already in #33. Patch is now updated to prefer the parameter over the config file.

vvidic · 2021-01-07T15:15:58Z

On Thu, Jan 07, 2021 at 06:50:51AM -0800, Markus Rudy wrote: `PAM_CRED_UNAVAIL: This module cannot retrieve the user's credentials.` sounds correct to me.

I will test with PAM_CRED_UNAVAIL, but I suspect it will cause PAM stack to fail as mentioned here under RETURN VALUES: https://man7.org/linux/man-pages/man3/pam_sm_setcred.3.html Also Google Authenticator uses PAM_SUCCESS for this function: https://github.com/google/google-authenticator-libpam/blob/master/src/pam_google_authenticator.c#L2192

…

-- Valentin

burgerdev · 2021-01-07T15:35:07Z

On Thu, Jan 07, 2021 at 06:50:51AM -0800, Markus Rudy wrote: PAM_CRED_UNAVAIL: This module cannot retrieve the user's credentials. sounds correct to me.
I will test with PAM_CRED_UNAVAIL, but I suspect it will cause PAM stack to fail as mentioned here under RETURN VALUES: https://man7.org/linux/man-pages/man3/pam_sm_setcred.3.html Also Google Authenticator uses PAM_SUCCESS for this function: https://github.com/google/google-authenticator-libpam/blob/master/src/pam_google_authenticator.c#L2192
…
-- Valentin

I see: if we were to stack GLOME and some password-based login, we would not be able to change the password if the GLOME stack returned UNAVAIL. I don't quite understand the design decision (by PAM), but in that case, and since we have a precedent in Google Authenticator, let's keep it as is. Thanks for following up.

Time to resolve that merge conflict :)

pkern

Thank you so much for your contribution! This LGTM as-is except one nit (install dir).

pkern · 2021-01-08T12:07:27Z

login/meson.build

    link_with : login_lib,
    include_directories : glome_incdir,
    install : true,
    install_dir : get_option('sbindir'))

+cc = meson.get_compiler('c')
+libpam = cc.find_library('pam')
+pam_glome = shared_library('pam_glome',


I was a little tempted to say that this should be conditional. On the other hand I'm of the belief that in the public build glome-login should eventually die and only ship the PAM module. :)

I suppose there is no harm in building both until the decision is made on glome-login.

pkern · 2021-01-08T12:12:30Z

login/pam.c

+    return failure(EXITCODE_INVALID_INPUT_SIZE, error_tag, "authcode-length");
+  }
+
+  // We can use insecure strncmp() because it's a *one-time* code.


Something like this always makes me a little uncomfortable. Is that because we don't have a constant-time check available? You already need to point out that it's safe, so if there's no drawback to just doing the right thing, it'd be better to do that? (Or maybe I'm off here and it just instills a wrong sense of security.)

Yes, this is a strange comment and I don't know why is strncmp insecure here since the length is checked few lines before. The comment comes from the original login.c so we should probably check with the author there.

The original author didn't mean memory safety. :)

It's about strncmp() being susceptible to timing attacks which doesn't matter in this particular context given that we are comparing one time code. I am fine with using constant-time comparison here, especially if it makes more people sleep better at nights.

Added a XOR loop now, please check if this is ok now.

Since we already have the OpenSSL dependency I'd prefer using CRYPTO_memcmp.

Used CRYPTO_memcmp, thanks.

pkern · 2021-01-08T12:15:13Z

login/README.md

+
+* `config_path=PATH` - location of the configuration file to parse (defaults to
+  `/etc/glome/config`)
+* `service_key=KEY` - use hex-encoded `KEY` as the service key (defaults to key


Don't we also need a key ID in this case?

Added a new option service_key_version for setting the key ID.

pkern · 2021-01-08T12:16:56Z

login/meson.build

+    dependencies : [libpam],
+    link_with : [glome_lib, login_lib],
+    include_directories : glome_incdir,
+    install : true)


Please add a TODO to install this into the correct directory or add the support for that. I know it's messy, at least last time I looked at how to solve this there was no pre-built support in meson.

Added support to install as $libdir/security/pam_glome.so

pkern · 2021-01-08T12:23:53Z

And this PR needs to be merged/rebased first to resolve the conflict.

vvidic · 2021-01-08T21:03:24Z

And this PR needs to be merged/rebased first to resolve the conflict.

PR rebased on top of master.

Reuses much of the existing glome-login functions.

googlebot added the cla: yes label Dec 20, 2020

google-cla bot added the cla: yes label Dec 20, 2020

vvidic force-pushed the pam branch from 5409dd2 to 1301160 Compare December 20, 2020 19:22

burgerdev reviewed Dec 22, 2020

View reviewed changes

vvidic force-pushed the pam branch 2 times, most recently from a578764 to de41c2c Compare December 22, 2020 20:59

burgerdev approved these changes Jan 7, 2021

View reviewed changes

pkern reviewed Jan 8, 2021

View reviewed changes

vvidic force-pushed the pam branch 2 times, most recently from 94753a9 to a92c9e2 Compare January 8, 2021 21:00

vvidic force-pushed the pam branch 3 times, most recently from ce66037 to 51a0e3e Compare January 8, 2021 22:58

Implement PAM authentication module for GLOME

4f10d88

Reuses much of the existing glome-login functions.

vvidic force-pushed the pam branch from 51a0e3e to 4f10d88 Compare January 11, 2021 19:43

copybara-service bot merged commit 8263f3e into google:master Jan 12, 2021

burgerdev mentioned this pull request Jan 13, 2021

glome-login refers to a mysterious "embedded" service key #33

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement PAM authentication module for GLOME #73

Implement PAM authentication module for GLOME #73

vvidic commented Dec 20, 2020

vvidic commented Dec 20, 2020

burgerdev left a comment

burgerdev Dec 21, 2020

vvidic Dec 22, 2020

burgerdev Jan 7, 2021

burgerdev Jan 7, 2021

burgerdev Dec 22, 2020

vvidic Dec 22, 2020

burgerdev Dec 22, 2020

vvidic Dec 22, 2020

vvidic commented Jan 7, 2021 via email

burgerdev commented Jan 7, 2021

pkern left a comment

pkern Jan 8, 2021

vvidic Jan 8, 2021

pkern Jan 8, 2021

vvidic Jan 8, 2021

l9i Jan 8, 2021

vvidic Jan 8, 2021

burgerdev Jan 11, 2021

vvidic Jan 11, 2021

pkern Jan 8, 2021

vvidic Jan 8, 2021

pkern Jan 8, 2021

vvidic Jan 8, 2021

pkern commented Jan 8, 2021

vvidic commented Jan 8, 2021

Implement PAM authentication module for GLOME #73

Implement PAM authentication module for GLOME #73

Conversation

vvidic commented Dec 20, 2020

vvidic commented Dec 20, 2020

burgerdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvidic commented Jan 7, 2021 via email

burgerdev commented Jan 7, 2021

pkern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkern commented Jan 8, 2021

vvidic commented Jan 8, 2021