Repository: google/glome (language: python)

Issue:
PAM module arguments naming is inconsistent with the config
It would be great if PAM module arguments naming was consistent with the config (and flags in the future once glome-login suports long flags).

Examples of inconsistency:
- `service_key` (PAM) vs `key` (config)
- `service_key_version` (PAM) vs `key-version` (config)
- `url_prefix` (PAM) vs `url-prefix` (config)

Personally, I like the config naming better. It's shorter and uses dashes instead of underscores.


Discussion / hints:
I think underscores are also more consistent with other PAM modules, so I'm fine with this. We do have a way to specify a config file as well, right?

Fix patch:
diff --git a/login/README.md b/login/README.md
index 6543b3ac..e6314f7c 100644
--- a/login/README.md
+++ b/login/README.md
@@ -77,16 +77,16 @@ PAM module supports the following options:
 
 * `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
+* `key=KEY` - use hex-encoded `KEY` as the service key (defaults to key
   from configuration file)
-* `service_key_version=N` - use `N` for the service key version (defaults to key
+* `key_version=N` - use `N` for the service key version (defaults to key
   version from configuration file)
 * `url_prefix=URL` - use given URL prefix (defaults to prefix from configuration
   file)
 * `debug` - enable verbose logging
-* `insecure_debug` - enable logging of secrets (INSECURE!)
-* `insecure_host_id=NAME` - use `NAME` as the host-id
-* `insecure_secret_key=KEY` - use hex-encoded `KEY` instead of the ephemeral
+* `print_secrets` - enable logging of secrets (INSECURE!)
+* `host_id=NAME` - use `NAME` as the host-id
+* `ephemeral_key=KEY` - use hex-encoded `KEY` instead of the ephemeral
   secret key (INSECURE!)
 
 ## Troubleshooting
diff --git a/login/pam.c b/login/pam.c
index 88e51809..6acf01f3 100644
--- a/login/pam.c
+++ b/login/pam.c
@@ -33,11 +33,21 @@
 
 #define UNUSED(var) (void)(var)
 
-static const char *arg_value(const char *arg, const char *key) {
-  int key_len = strlen(key);
-  if (strncmp(arg, key, key_len) == 0 && arg[key_len] == '=') {
+static const char *arg_value(const char *arg, const char *key,
+                             const char *default_value) {
+  int i, key_len = strlen(key);
+  for (i = 0; i < key_len; i++) {
+    // Compare key with arg char by char while also allowing _ in place of -
+    if (!(key[i] == arg[i] || (key[i] == '-' && arg[i] == '_'))) {
+      return NULL;
+    }
+  }
+  if (arg[key_len] == '=') {
     return arg + key_len + 1;
   }
+  if (arg[key_len] == '\0') {
+    return default_value;
+  }
   return NULL;
 }
 
@@ -49,27 +59,27 @@ static int parse_pam_args(pam_handle_t *pamh, int argc, const char **argv,
   const char *val;
 
   for (int i = 0; i < argc; ++i) {
-    if ((val = arg_value(argv[i], "config_path"))) {
+    if ((val = arg_value(argv[i], "config-path", NULL))) {
       status = glome_login_assign_config_option(config, "default",
                                                 "config-path", val);
-    } else if ((val = arg_value(argv[i], "service_key"))) {
+    } else if ((val = arg_value(argv[i], "key", NULL))) {
       status = glome_login_assign_config_option(config, "service", "key", val);
-    } else if ((val = arg_value(argv[i], "service_key_version"))) {
+    } else if ((val = arg_value(argv[i], "key-version", NULL))) {
       status = glome_login_assign_config_option(config, "service",
                                                 "key-version", val);
-    } else if ((val = arg_value(argv[i], "url_prefix"))) {
+    } else if ((val = arg_value(argv[i], "url-prefix", NULL))) {
       status = glome_login_assign_config_option(config, "service", "url-prefix",
                                                 val);
-    } else if (!strcmp(argv[i], "debug")) {
-      status = glome_login_assign_config_option(config, "default", "verbose",
-                                                "true");
-    } else if (!strcmp(argv[i], "insecure_debug")) {
+    } else if ((val = arg_value(argv[i], "debug", "true"))) {
+      status =
+          glome_login_assign_config_option(config, "default", "verbose", val);
+    } else if ((val = arg_value(argv[i], "print-secrets", "true"))) {
       status = glome_login_assign_config_option(config, "default",
-                                                "print-secrets", "true");
-    } else if ((val = arg_value(argv[i], "insecure_host_id"))) {
+                                                "print-secrets", val);
+    } else if ((val = arg_value(argv[i], "host-id", NULL))) {
       status =
           glome_login_assign_config_option(config, "default", "host-id", val);
-    } else if ((val = arg_value(argv[i], "insecure_secret_key"))) {
+    } else if ((val = arg_value(argv[i], "ephemeral-key", NULL))) {
       status = glome_login_assign_config_option(config, "default",
                                                 "ephemeral-key", val);
     } else {


Test patch:
diff --git a/login/pam_test.c b/login/pam_test.c
index 2b90a9c0..6f7c935d 100644
--- a/login/pam_test.c
+++ b/login/pam_test.c
@@ -81,12 +81,12 @@ int test_service() {
 
   fprintf(f,
           "auth required %s url_prefix=https://test.service "
-          "service_key="
+          "key="
           "de9edb7d7b7dc1b4d35b61c2ece435373f8343c85b78674dadfc7e146f882b4f "
-          "service_key_version=1 "
-          "insecure_secret_key="
+          "key_version=1 "
+          "ephemeral_key="
           "77076d0a7318a57d3c16c17251b26645df4c2f87ebc0992ab177fba51db92c2a "
-          "insecure_host_id=my-server.local",
+          "host_id=my-server.local",
           pam_glome);
   fclose(f);
 
@@ -165,9 +165,9 @@ int test_config() {
 
   fprintf(f,
           "auth required %s config_path=%s "
-          "insecure_secret_key="
+          "ephemeral-key="
           "77076d0a7318a57d3c16c17251b26645df4c2f87ebc0992ab177fba51db92c2a "
-          "insecure_host_id=my-server.local",
+          "host-id=my-server.local",
           pam_glome, config_file);
   fclose(f);
   free(config_file);