Repository: google/glome (language: python)

Issue:
CLI should handle tags as base64 instead of hex
It's slightly confusing that the CLI prints and verifies tags in hex format, while the tag is exchanged base64-encoded in the login protocol. base64 has arguably better utility support and would allow using the cli as client without re-encoding, so I propose changing the output of `glome tag` to base64.


Discussion / hints:

Fix patch:
diff --git a/cli/README.md b/cli/README.md
index ae10f6f1..ecc826df 100644
--- a/cli/README.md
+++ b/cli/README.md
@@ -7,35 +7,23 @@ This is a CLI utility to facilitate GLOME operations from the command line.
 Generating two key pairs:
 
 ```shell
-$ glome Alice >Alice.pub
-$ glome Bob >Bob.pub
+$ glome genkey | tee Alice | glome pubkey >Alice.pub
+$ glome genkey | tee Bob   | glome pubkey >Bob.pub
 ```
 
 Alice calculates a tag and send it together with message and counter to Bob:
 
 ```shell
-$ tag=$(glome Alice Bob.pub "Hello world!" 0)
-peer-key:   0xbe106dba769f75f215f29b3b5e5e84c792a9d5562a26c9f7e19915c73bb45413
-public-key: 0xe2e97a41a60fd6a3c5de511862671f97e9f8e0d657044cac783e5119eeecae06
-message:   'Hello world!'
-counter:    0
-verify:     0
+$ tag=$(echo "Hello world!" | glome tag --key Alice --peer Bob.pub)
 
 $ echo "${tag?}"
-2b4dc85086e41a5c616301d904ac2dd942f2d71a56985a5be252b5bbca30bdfa
+_QuyLz_nkj5exUJscocS8LDnCMszvSmp9wpQuRshi30=
 ```
 
 Bob can verify that the tag matches:
 
 ```shell
-$ glome Bob Alice.pub "Hello world!" 0 "${tag?}"
-peer-key:   0xe2e97a41a60fd6a3c5de511862671f97e9f8e0d657044cac783e5119eeecae06
-public-key: 0xbe106dba769f75f215f29b3b5e5e84c792a9d5562a26c9f7e19915c73bb45413
-message:   'Hello world!'
-counter:    0
-verify:     1
-mac-tag:    0x2b4dc85086e41a5c616301d904ac2dd942f2d71a56985a5be252b5bbca30bdfa
-unverified: 0x2b4dc85086e41a5c616301d904ac2dd942f2d71a56985a5be252b5bbca30bdfa
+$ echo "Hello world!" | glome verify --key Bob --peer Alice.pub --tag "${tag?}"
 
 $ echo $?
 0
@@ -44,15 +32,15 @@ $ echo $?
 Both parties can agree to shorten the tag to reduce the protocol overhead:
 
 ```shell
-$ glome Bob Alice.pub "Hello world!" 0 "${tag:0:12}"
-peer-key:   0xe2e97a41a60fd6a3c5de511862671f97e9f8e0d657044cac783e5119eeecae06
-public-key: 0xbe106dba769f75f215f29b3b5e5e84c792a9d5562a26c9f7e19915c73bb45413
-message:   'Hello world!'
-counter:    0
-verify:     1
-mac-tag:    0x2b4dc85086e41a5c616301d904ac2dd942f2d71a56985a5be252b5bbca30bdfa
-unverified: 0x2b4dc85086e41a5c616301d90
+$ echo "Hello world!" | glome verify --key Bob --peer Alice.pub --tag "${tag:0:12}"
 
 $ echo $?
 0
 ```
+
+CLI also supports ganerating tags for the GLOME Login requests:
+
+```shell
+$ glome login --key Bob https://glome.example.com/v1/AYUg8AmJMKdUdIt93LQ-91oNvzoNJjga9OukqY6qm05q0PU=/my-server.local/shell/root/
+MT_Zc-hucXRjTXTBEo53ehoeUsFn1oFyVadViXf-I4k=
+```
diff --git a/cli/commands.c b/cli/commands.c
index dcd293e7..fa35ab16 100644
--- a/cli/commands.c
+++ b/cli/commands.c
@@ -37,18 +37,20 @@
 // Arguments
 static const char *key_file = NULL;
 static const char *peer_file = NULL;
-static const char *tag_hex = NULL;
+static const char *tag_b64 = NULL;
 static unsigned long counter = 0;
 
 static bool parse_args(int argc, char **argv) {
+  int c;
   struct option long_options[] = {{"key", required_argument, 0, 'k'},
                                   {"peer", required_argument, 0, 'p'},
                                   {"counter", required_argument, 0, 'c'},
                                   {"tag", required_argument, 0, 't'},
                                   {0, 0, 0, 0}};
 
-  int c;
-  while ((c = getopt_long(argc, argv, "c:k:p:t:", long_options, NULL)) != -1) {
+  // First argument is the command name so skip it.
+  while ((c = getopt_long(argc - 1, argv + 1, "c:k:p:t:", long_options,
+                          NULL)) != -1) {
     switch (c) {
       case 'c': {
         char *endptr;
@@ -66,7 +68,7 @@ static bool parse_args(int argc, char **argv) {
         peer_file = optarg;
         break;
       case 't':
-        tag_hex = optarg;
+        tag_b64 = optarg;
         break;
       case '?':
         return false;
@@ -78,26 +80,6 @@ static bool parse_args(int argc, char **argv) {
   return true;
 }
 
-static int decode_hex(uint8_t *dst, size_t dst_len, const char *in) {
-  size_t len = strlen(in);
-  if (len > 2 && in[0] == '0' && in[1] == 'x') {
-    len -= 2;
-    in += 2;
-  }
-  if (len > dst_len * 2 || len % 2 != 0) {
-    return -1;
-  }
-  size_t i;
-  for (i = 0; i < len / 2; i++) {
-    if (sscanf(in + (i * 2), "%02hhX", dst + i) != 1) {
-      fprintf(stderr, "ERROR while parsing byte %zu ('%c%c') as hex\n", i,
-              in[2 * i], in[2 * i + 1]);
-      return -3;
-    }
-  }
-  return i;
-}
-
 static bool read_file(const char *fname, uint8_t *buf, const size_t num_bytes) {
   FILE *f = fopen(fname, "r");
   if (!f) {
@@ -118,17 +100,6 @@ static bool read_file(const char *fname, uint8_t *buf, const size_t num_bytes) {
   return true;
 }
 
-static void print_hex(FILE *stream, const char *prefix, uint8_t *buf,
-                      size_t len) {
-  if (prefix != NULL) {
-    fputs(prefix, stream);
-  }
-  for (size_t i = 0; i < len; i++) {
-    fprintf(stream, "%02x", buf[i]);
-  }
-  fputs("\n", stream);
-}
-
 int genkey(int argc, char **argv) {
   UNUSED(argc);
   UNUSED(argv);
@@ -190,7 +161,7 @@ int tag_impl(uint8_t tag[GLOME_MAX_TAG_LENGTH], bool verify,
   }
   if (glome_tag(verify, counter, private_key, peer_key, (uint8_t *)message,
                 msg_len, tag)) {
-    fputs("MAC tag generation failed", stderr);
+    fputs("MAC tag generation failed\n", stderr);
     return EXIT_FAILURE;
   }
   return EXIT_SUCCESS;
@@ -209,33 +180,61 @@ int tag(int argc, char **argv) {
   if (res) {
     return res;
   }
-  print_hex(stdout, "", tag, sizeof tag);
+  char tag_encoded[ENCODED_BUFSIZE(sizeof tag)] = {0};
+  if (base64url_encode(tag, sizeof tag, (uint8_t *)tag_encoded,
+                       sizeof tag_encoded) == 0) {
+    fprintf(stderr, "GLOME tag encode failed\n");
+    return EXIT_FAILURE;
+  }
+  puts(tag_encoded);
   return EXIT_SUCCESS;
 }
 
 int verify(int argc, char **argv) {
   uint8_t tag[GLOME_MAX_TAG_LENGTH] = {0};
-  uint8_t expected_tag[GLOME_MAX_TAG_LENGTH] = {0};
-  size_t expected_tag_len = 0;
+  uint8_t *expected_tag = NULL;
+  int ret = EXIT_FAILURE;
   if (!parse_args(argc, argv)) {
-    return EXIT_FAILURE;
+    goto out;
   }
-  if (!key_file || !peer_file || !tag_hex) {
+  if (!key_file || !peer_file || !tag_b64) {
     fprintf(stderr, "not enough arguments for subcommand %s\n", argv[1]);
-    return EXIT_FAILURE;
+    goto out;
   }
   int res = tag_impl(tag, /*verify=*/true, key_file, peer_file);
   if (res) {
-    return res;
+    goto out;
+  }
+
+  // decode the tag
+  size_t tag_b64_len = strlen(tag_b64);
+  size_t tag_b64_decoded_len = DECODED_BUFSIZE(tag_b64_len);
+  expected_tag = malloc(tag_b64_decoded_len);
+  if (expected_tag == NULL) {
+    fprintf(stderr, "GLOME tag malloc %ld bytes failed\n", tag_b64_decoded_len);
+    goto out;
+  }
+  size_t expected_tag_len =
+      base64url_decode((uint8_t *)tag_b64, tag_b64_len, (uint8_t *)expected_tag,
+                       tag_b64_decoded_len);
+  if (expected_tag_len == 0) {
+    fprintf(stderr, "GLOME tag decode failed\n");
+    goto out;
+  }
+  if (expected_tag_len > sizeof tag) {
+    expected_tag_len = sizeof tag;
   }
 
   // compare the tag
-  expected_tag_len = decode_hex(expected_tag, sizeof expected_tag, tag_hex);
   if (CRYPTO_memcmp(expected_tag, tag, expected_tag_len) != 0) {
-    fputs("MAC tag verification failed", stderr);
-    return EXIT_FAILURE;
+    fputs("MAC tag verification failed\n", stderr);
+    goto out;
   }
-  return EXIT_SUCCESS;
+  ret = EXIT_SUCCESS;
+
+out:
+  free(expected_tag);
+  return ret;
 }
 
 static bool parse_login_path(char *path, char **handshake, char **host,
@@ -383,6 +382,11 @@ int login(int argc, char **argv) {
 
   size_t handshake_b64_len = strlen(handshake_b64);
   handshake = malloc(DECODED_BUFSIZE(handshake_b64_len));
+  if (handshake == NULL) {
+    fprintf(stderr, "failed to malloc %ld bytes for base64 decode\n",
+            DECODED_BUFSIZE(handshake_b64_len));
+    goto out;
+  }
   int handshake_len = base64url_decode((uint8_t *)handshake_b64,
                                        handshake_b64_len, (uint8_t *)handshake,
                                        DECODED_BUFSIZE(handshake_b64_len));


Test patch:
diff --git a/cli/test.sh b/cli/test.sh
index 6da4ef01..ae81af11 100755
--- a/cli/test.sh
+++ b/cli/test.sh
@@ -36,7 +36,7 @@ printf '\135\253\010\176\142\112\212\113\171\341\177\213\203\200\016\346\157'\
 '\073\261\051\046\030\266\375\034\057\213\047\377\210\340\353' >"$t/vector-1/b"
 printf "The quick brown fox" >"$t/vector-1/msg"
 printf "0" >"$t/vector-1/n"
-printf "9c44389f462d35d0672faf73a5e118f8b9f5c340bbe8d340e2b947c205ea4fa3" >"$t/vector-1/tag"
+printf "nEQ4n0YtNdBnL69zpeEY-Ln1w0C76NNA4rlHwgXqT6M=" >"$t/vector-1/tag"
 
 mkdir -p "$t/vector-2"
 printf '\261\005\360\015\261\005\360\015\261\005\360\015\261\005\360\015\261'\
@@ -45,7 +45,7 @@ printf '\376\341\336\255\376\341\336\255\376\341\336\255\376\341\336\255\376'\
 '\341\336\255\376\341\336\255\376\341\336\255\376\341\336\255' >"$t/vector-2/b"
 printf "The quick brown fox" >"$t/vector-2/msg"
 printf "100" >"$t/vector-2/n"
-printf "06476f1f314b06c7f96e5dc62b2308268cbdb6140aefeeb55940731863032277" >"$t/vector-2/tag"
+printf "BkdvHzFLBsf5bl3GKyMIJoy9thQK7-61WUBzGGMDInc=" >"$t/vector-2/tag"
 
 errors=0
 for n in 1 2; do