Clarify public key format and add tests

docs/rfd/002.md

@@ -24,9 +24,9 @@ See also [google/glome#100](https://github.com/google/glome/issues/100).
 
 # Design ideas
 
-Public keys are stored in base64 encoding and tagged with their protocol
-variant. The configuration file format accepts keys in a format similar to
-[OpenSSH's `authorized_keys` format][1].
+Public keys are stored in URL-safe base64 encoding and tagged with their
+protocol variant version. The configuration file format accepts keys in a
+format similar to [OpenSSH's `authorized_keys` format][1].
 
 [1]: https://man.openbsd.org/sshd.8#AUTHORIZED_KEYS_FILE_FORMAT
 
@@ -37,8 +37,8 @@ The format of a _GLOME public key_ adheres to the ABNF below:
 ```abnf
 public-key = key-type SP key-base64
 key-type = "glome-v1"
-key-base64 = 44base64-char
-base64-char = "=" / "+" / "/" / ALPHA / DIGIT
+key-base64 = 44urlsafe-base64-char
+urlsafe-base64-char = "=" / "-" / "_" / ALPHA / DIGIT
 ```
 
 The key type encodes the GLOME variant this key should be used with. As we
@@ -47,7 +47,7 @@ only have one variant right now, we're only defining one `key-type` here.
 An example public key, like it would be printed by `glome pubkey`:
 
 ```
-glome-v1 lXmlq5jynG6um/w4D4N13TRIE+x7jt0TKVNDMSRS2/I=
+glome-v1 lXmlq5jynG6um_w4D4N13TRIE-x7jt0TKVNDMSRS2_I=
 ```
 
 ## Public Key Interpretation

login/config.c

@@ -88,20 +88,14 @@ static void key_value(char *line, char **key, char **val) {
     return;
   }
 
-  char *v = p;
-  for (; !isspace(*p) && *p != '\0'; p++) {
-  }
-  if (*p != '\0') {
-    *p = '\0';
-    for (p++; isspace(*p); p++) {
-    }
-  }
-  if (*p != '\0') {
-    return;
+  // Trim whitespace at the end of the value.
+  int k = strlen(p) - 1;
+  for (; k >= 0 && isspace(p[k]); k--) {
   }
+  p[k+1] = '\0';
 
   *key = line;
-  *val = v;
+  *val = p;
 }
 
 bool glome_login_parse_public_key(const char *encoded_key, uint8_t *public_key,
@@ -265,6 +259,8 @@ static status_t assign_default_option(glome_login_config_t *config,
                                       const char *key, const char *val) {
   if (strcmp(key, "auth-delay") == 0) {
     return assign_positive_int_option(&config->auth_delay_sec, val);
+  } else if (strcmp(key, "input-timeout") == 0) {
+    return assign_positive_int_option(&config->input_timeout_sec, val);
   } else if (strcmp(key, "config-path") == 0) {
     return assign_string_option(&config->config_path, val);
   } else if (strcmp(key, "ephemeral-key") == 0) {
@@ -282,11 +278,6 @@ static status_t assign_default_option(glome_login_config_t *config,
     return assign_positive_int_option(&config->input_timeout_sec, val);
   } else if (strcmp(key, "verbose") == 0) {
     return update_bitfield_option(config, VERBOSE, false, val);
-  } else if (strcmp(key, "public-key") == 0) {
-    if (!glome_login_parse_public_key(val, config->service_key,
-                                      sizeof(config->service_key))) {
-      return status_createf("ERROR: Failed to decode public-key\n");
-    }
   }
 
   return status_createf("ERROR: unrecognized default option: %s", key);
@@ -303,6 +294,12 @@ static status_t assign_service_option(glome_login_config_t *config,
     if (config->url_prefix == NULL) {
       return assign_string_option(&config->url_prefix, val);
     }
+  } else if (strcmp(key, "public-key") == 0) {
+    if (!glome_login_parse_public_key(val, config->service_key,
+                                      sizeof(config->service_key))) {
+      return status_createf("ERROR: failed to decode public-key");
+    }
+    return STATUS_OK;
   }
 
   return status_createf("ERROR: unrecognized service option: %s", key);

login/config_test.c

@@ -0,0 +1,86 @@
+// Copyright 2023 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "config.h"
+
+#include <glib.h>
+#include <stdio.h>
+
+#include "ui.h"
+
+static const char* ENCODED_PUBLIC_KEY = "glome-v1 aqA9yqe1RXoOT6HrmCbF40wVUhYp50FYZR9q8_X5KF4=";
+
+static const uint8_t DECODED_PUBLIC_KEY[32] = {0x6a, 0xa0, 0x3d, 0xca, 0xa7, 0xb5, 0x45, 0x7a, 0x0e, 0x4f, 0xa1, 0xeb, 0x98, 0x26, 0xc5, 0xe3, 0x4c, 0x15, 0x52, 0x16, 0x29, 0xe7, 0x41, 0x58, 0x65, 0x1f, 0x6a, 0xf3, 0xf5, 0xf9, 0x28, 0x5e};
+
+static void test_parse_public_key() {
+  uint8_t decoded[GLOME_MAX_PUBLIC_KEY_LENGTH] = {0};
+  g_assert_true(
+      glome_login_parse_public_key(ENCODED_PUBLIC_KEY, decoded, sizeof(decoded)));
+  g_assert_cmpmem(decoded, sizeof(decoded), DECODED_PUBLIC_KEY, sizeof(DECODED_PUBLIC_KEY));
+
+  g_assert_false(
+      glome_login_parse_public_key(ENCODED_PUBLIC_KEY, decoded, sizeof(decoded) - 1));
+  g_assert_false(glome_login_parse_public_key(
+      "glome-group1-md5 QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUE=", decoded,
+      sizeof(decoded)));
+  g_assert_false(glome_login_parse_public_key("glome-v1 QUFBQUFBQUFB", decoded,
+                                              sizeof(decoded)));
+
+  memset(decoded, 0, sizeof(decoded));
+  const char* extra_chars =
+      "glome-v1 \t aqA9yqe1RXoOT6HrmCbF40wVUhYp50FYZR9q8_X5KF4= "
+      "root@localhost";
+  g_assert_true(
+      glome_login_parse_public_key(extra_chars, decoded, sizeof(decoded)));
+  g_assert_cmpmem(decoded, sizeof(decoded), DECODED_PUBLIC_KEY, sizeof(DECODED_PUBLIC_KEY));
+}
+
+static char* EXAMPLE_CFG = NULL;
+
+static void test_parse_config_file() {
+  g_assert_true(EXAMPLE_CFG != NULL);
+
+  glome_login_config_t config = {0};
+  config.config_path = EXAMPLE_CFG;
+
+  status_t s = glome_login_parse_config_file(&config);
+  if (s) {
+    fprintf(stderr, "glome_login_parse_config_file returned error: %s\n", s);
+  }
+  g_assert_true(s == STATUS_OK);
+
+  g_assert_true(config.auth_delay_sec == 7);
+  g_assert_true(config.input_timeout_sec == 321);
+  g_assert_cmpstr("/bin/true", ==, config.login_path);
+  g_assert_cmpstr("my-host", ==, config.host_id);
+  g_assert_true(config.options & VERBOSE);
+  g_assert_false(config.options & SYSLOG);
+  g_assert_false(config.options & INSECURE);
+
+  g_assert_cmpmem(DECODED_PUBLIC_KEY, sizeof(DECODED_PUBLIC_KEY), config.service_key, GLOME_MAX_PUBLIC_KEY_LENGTH);
+  g_assert_true(config.service_key_id == 42);
+  g_assert_cmpstr("glome://", ==, config.url_prefix);
+}
+
+int main(int argc, char** argv) {
+  g_test_init(&argc, &argv, NULL);
+
+  g_assert_true(argc > 1);
+  EXAMPLE_CFG = argv[1];
+
+  g_test_add_func("/test-parse-public-key", test_parse_public_key);
+  g_test_add_func("/test-parse-config-file", test_parse_config_file);
+
+  return g_test_run();
+}

login/config_test.cfg

@@ -0,0 +1,13 @@
+auth-delay = 7
+input-timeout = 321
+host-id = my-host
+login-path = /bin/true
+disable-syslog = yes
+print-secrets = 0
+verbose = true
+
+[service]
+# Corresponding private key: 6aa03dcaa7b5457a0e4fa1eb9826c5e34c15521629e74158651f6af3f5f9285e
+public-key = glome-v1 aqA9yqe1RXoOT6HrmCbF40wVUhYp50FYZR9q8_X5KF4=
+key-version = 42
+url-prefix = glome://

login/login_test.c

@@ -133,39 +133,12 @@ static void test_vector_2() {
   }
 }
 
-static void test_parse_public_key() {
-  const char* encoded = "glome-v1 QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUE=";
-  uint8_t decoded[GLOME_MAX_PUBLIC_KEY_LENGTH] = {0};
-  uint8_t expected[GLOME_MAX_PUBLIC_KEY_LENGTH] =
-      "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";
-  g_assert_true(
-      glome_login_parse_public_key(encoded, decoded, sizeof(decoded)));
-  g_assert_cmpmem(decoded, sizeof(decoded), expected, sizeof(expected));
-
-  g_assert_false(
-      glome_login_parse_public_key(encoded, decoded, sizeof(decoded) - 1));
-  g_assert_false(glome_login_parse_public_key(
-      "glome-group1-md5 QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUE=", decoded,
-      sizeof(decoded)));
-  g_assert_false(glome_login_parse_public_key("glome-v1 QUFBQUFBQUFB", decoded,
-                                              sizeof(decoded)));
-
-  memset(decoded, 0, sizeof(decoded));
-  const char* extra_chars =
-      "glome-v1 \t QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUE= "
-      "root@localhost";
-  g_assert_true(
-      glome_login_parse_public_key(extra_chars, decoded, sizeof(decoded)));
-  g_assert_cmpmem(decoded, sizeof(decoded), expected, sizeof(expected));
-}
-
 int main(int argc, char** argv) {
   g_test_init(&argc, &argv, NULL);
 
   g_test_add_func("/test-shell-action", test_shell_action);
   g_test_add_func("/test-vector-1", test_vector_1);
   g_test_add_func("/test-vector-2", test_vector_2);
-  g_test_add_func("/test-parse-public-key", test_parse_public_key);
 
   return g_test_run();
 }

login/meson.build

@@ -58,6 +58,13 @@ if get_option('tests')
         link_with : [glome_lib, login_lib],
         include_directories : glome_incdir)
     test('crypto test', crypto_test)
+
+    config_test = executable(
+        'config_test', 'config_test.c',
+        dependencies : [openssl_dep, glib_dep],
+        link_with : [glome_lib, login_lib],
+        include_directories : glome_incdir)
+    test('config test', config_test, args: files('config_test.cfg'))
 endif
 
 if get_option('pam-glome')