Merge pull request #8788 from jboeuf/fix_jwt_verifier

Fix jwt verifier

8 files changed, 121 insertions, 9 deletions

diff --git a/src/core/lib/security/credentials/jwt/jwt_verifier.c b/src/core/lib/security/credentials/jwt/jwt_verifier.c
index 42bd89dd0a..03097a57c0 100644
--- a/src/core/lib/security/credentials/jwt/jwt_verifier.c
+++ b/src/core/lib/security/credentials/jwt/jwt_verifier.c
@@ -39,6 +39,7 @@
 #include "src/core/lib/http/httpcli.h"
 #include "src/core/lib/iomgr/polling_entity.h"
 #include "src/core/lib/security/util/b64.h"
+#include "src/core/lib/support/string.h"
 #include "src/core/lib/tsi/ssl_types.h"
 
 #include <grpc/support/alloc.h>
@@ -305,6 +306,17 @@ grpc_jwt_verifier_status grpc_jwt_claims_check(const grpc_jwt_claims *claims,
     return GRPC_JWT_VERIFIER_TIME_CONSTRAINT_FAILURE;
   }
 
+  /* This should be probably up to the upper layer to decide but let's harcode
+     the 99% use case here for email issuers, where the JWT must be self
+     issued. */
+  if (grpc_jwt_issuer_email_domain(claims->iss) != NULL &&
+      claims->sub != NULL && strcmp(claims->iss, claims->sub) != 0) {
+    gpr_log(GPR_ERROR,
+            "Email issuer (%s) cannot assert another subject (%s) than itself.",
+            claims->iss, claims->sub);
+    return GRPC_JWT_VERIFIER_BAD_SUBJECT;
+  }
+
   if (audience == NULL) {
     audience_ok = claims->aud == NULL;
   } else {
@@ -705,10 +717,26 @@ static void verifier_put_mapping(grpc_jwt_verifier *v, const char *email_domain,
   GPR_ASSERT(v->num_mappings <= v->allocated_mappings);
 }
 
+/* Very non-sophisticated way to detect an email address. Should be good
+   enough for now... */
+const char *grpc_jwt_issuer_email_domain(const char *issuer) {
+  const char *at_sign = strchr(issuer, '@');
+  if (at_sign == NULL) return NULL;
+  const char *email_domain = at_sign + 1;
+  if (*email_domain == '\0') return NULL;
+  const char *dot = strrchr(email_domain, '.');
+  if (dot == NULL || dot == email_domain) return email_domain;
+  GPR_ASSERT(dot > email_domain);
+  /* There may be a subdomain, we just want the domain. */
+  dot = gpr_memrchr(email_domain, '.', (size_t)(dot - email_domain));
+  if (dot == NULL) return email_domain;
+  return dot + 1;
+}
+
 /* Takes ownership of ctx. */
 static void retrieve_key_and_verify(grpc_exec_ctx *exec_ctx,
                                     verifier_cb_ctx *ctx) {
-  const char *at_sign;
+  const char *email_domain;
   grpc_closure *http_cb;
   char *path_prefix = NULL;
   const char *iss;
@@ -733,13 +761,9 @@ static void retrieve_key_and_verify(grpc_exec_ctx *exec_ctx,
      Nobody seems to implement the account/email/webfinger part 2. of the spec
      so we will rely instead on email/url mappings if we detect such an issuer.
      Part 4, on the other hand is implemented by both google and salesforce. */
-
-  /* Very non-sophisticated way to detect an email address. Should be good
-     enough for now... */
-  at_sign = strchr(iss, '@');
-  if (at_sign != NULL) {
+  email_domain = grpc_jwt_issuer_email_domain(iss);
+  if (email_domain != NULL) {
     email_key_mapping *mapping;
-    const char *email_domain = at_sign + 1;
     GPR_ASSERT(ctx->verifier != NULL);
     mapping = verifier_get_mapping(ctx->verifier, email_domain);
     if (mapping == NULL) {
diff --git a/src/core/lib/security/credentials/jwt/jwt_verifier.h b/src/core/lib/security/credentials/jwt/jwt_verifier.h
index f09f9d5d47..54ff9b05e5 100644
--- a/src/core/lib/security/credentials/jwt/jwt_verifier.h
+++ b/src/core/lib/security/credentials/jwt/jwt_verifier.h
@@ -43,8 +43,7 @@
 /* --- Constants. --- */
 
 #define GRPC_OPENID_CONFIG_URL_SUFFIX "/.well-known/openid-configuration"
-#define GRPC_GOOGLE_SERVICE_ACCOUNTS_EMAIL_DOMAIN \
-  "developer.gserviceaccount.com"
+#define GRPC_GOOGLE_SERVICE_ACCOUNTS_EMAIL_DOMAIN "gserviceaccount.com"
 #define GRPC_GOOGLE_SERVICE_ACCOUNTS_KEY_URL_PREFIX \
   "www.googleapis.com/robot/v1/metadata/x509"
 
@@ -57,6 +56,7 @@ typedef enum {
   GRPC_JWT_VERIFIER_BAD_AUDIENCE,
   GRPC_JWT_VERIFIER_KEY_RETRIEVAL_ERROR,
   GRPC_JWT_VERIFIER_TIME_CONSTRAINT_FAILURE,
+  GRPC_JWT_VERIFIER_BAD_SUBJECT,
   GRPC_JWT_VERIFIER_GENERIC_ERROR
 } grpc_jwt_verifier_status;
 
@@ -132,5 +132,6 @@ void grpc_jwt_verifier_verify(grpc_exec_ctx *exec_ctx,
 grpc_jwt_claims *grpc_jwt_claims_from_json(grpc_json *json, grpc_slice buffer);
 grpc_jwt_verifier_status grpc_jwt_claims_check(const grpc_jwt_claims *claims,
                                                const char *audience);
+const char *grpc_jwt_issuer_email_domain(const char *issuer);
 
 #endif /* GRPC_CORE_LIB_SECURITY_CREDENTIALS_JWT_JWT_VERIFIER_H */
diff --git a/src/core/lib/support/string.c b/src/core/lib/support/string.c
index f10a30f0fd..f263f82baf 100644
--- a/src/core/lib/support/string.c
+++ b/src/core/lib/support/string.c
@@ -275,3 +275,14 @@ int gpr_stricmp(const char *a, const char *b) {
   } while (ca == cb && ca && cb);
   return ca - cb;
 }
+
+void *gpr_memrchr(const void *s, int c, size_t n) {
+  if (s == NULL) return NULL;
+  char *b = (char *)s;
+  for (size_t i = 0; i < n; i++) {
+    if (b[n - i - 1] == c) {
+      return &b[n - i - 1];
+    }
+  }
+  return NULL;
+}
diff --git a/src/core/lib/support/string.h b/src/core/lib/support/string.h
index e933e2eb46..6d1f7cc632 100644
--- a/src/core/lib/support/string.h
+++ b/src/core/lib/support/string.h
@@ -118,6 +118,8 @@ char *gpr_strvec_flatten(gpr_strvec *strs, size_t *total_length);
     lower(a)==lower(b), >0 if lower(a)>lower(b) */
 int gpr_stricmp(const char *a, const char *b);
 
+void *gpr_memrchr(const void *s, int c, size_t n);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/test/core/security/create_jwt.c b/test/core/security/create_jwt.c
index 741ace9bdd..ac795f29d2 100644
--- a/test/core/security/create_jwt.c
+++ b/test/core/security/create_jwt.c
@@ -72,6 +72,7 @@ int main(int argc, char **argv) {
   char *scope = NULL;
   char *json_key_file_path = NULL;
   char *service_url = NULL;
+  grpc_init();
   gpr_cmdline *cl = gpr_cmdline_create("create_jwt");
   gpr_cmdline_add_string(cl, "json_key", "File path of the json key.",
                          &json_key_file_path);
@@ -102,5 +103,6 @@ int main(int argc, char **argv) {
   create_jwt(json_key_file_path, service_url, scope);
 
   gpr_cmdline_destroy(cl);
+  grpc_shutdown();
   return 0;
 }
diff --git a/test/core/security/jwt_verifier_test.c b/test/core/security/jwt_verifier_test.c
index f8afba8d6d..9a21814adc 100644
--- a/test/core/security/jwt_verifier_test.c
+++ b/test/core/security/jwt_verifier_test.c
@@ -166,6 +166,13 @@ static const char claims_without_time_constraint[] =
     "  \"jti\": \"jwtuniqueid\","
     "  \"foo\": \"bar\"}";
 
+static const char claims_with_bad_subject[] =
+    "{ \"aud\": \"https://foo.com\","
+    "  \"iss\": \"evil@blah.foo.com\","
+    "  \"sub\": \"juju@blah.foo.com\","
+    "  \"jti\": \"jwtuniqueid\","
+    "  \"foo\": \"bar\"}";
+
 static const char invalid_claims[] =
     "{ \"aud\": \"https://foo.com\","
     "  \"iss\": 46," /* Issuer cannot be a number. */
@@ -179,6 +186,38 @@ typedef struct {
   const char *expected_subject;
 } verifier_test_config;
 
+static void test_jwt_issuer_email_domain(void) {
+  const char *d = grpc_jwt_issuer_email_domain("https://foo.com");
+  GPR_ASSERT(d == NULL);
+  d = grpc_jwt_issuer_email_domain("foo.com");
+  GPR_ASSERT(d == NULL);
+  d = grpc_jwt_issuer_email_domain("");
+  GPR_ASSERT(d == NULL);
+  d = grpc_jwt_issuer_email_domain("@");
+  GPR_ASSERT(d == NULL);
+  d = grpc_jwt_issuer_email_domain("bar@foo");
+  GPR_ASSERT(strcmp(d, "foo") == 0);
+  d = grpc_jwt_issuer_email_domain("bar@foo.com");
+  GPR_ASSERT(strcmp(d, "foo.com") == 0);
+  d = grpc_jwt_issuer_email_domain("bar@blah.foo.com");
+  GPR_ASSERT(strcmp(d, "foo.com") == 0);
+  d = grpc_jwt_issuer_email_domain("bar.blah@blah.foo.com");
+  GPR_ASSERT(strcmp(d, "foo.com") == 0);
+  d = grpc_jwt_issuer_email_domain("bar.blah@baz.blah.foo.com");
+  GPR_ASSERT(strcmp(d, "foo.com") == 0);
+
+  /* This is not a very good parser but make sure we do not crash on these weird
+     inputs. */
+  d = grpc_jwt_issuer_email_domain("@foo");
+  GPR_ASSERT(strcmp(d, "foo") == 0);
+  d = grpc_jwt_issuer_email_domain("bar@.");
+  GPR_ASSERT(d != NULL);
+  d = grpc_jwt_issuer_email_domain("bar@..");
+  GPR_ASSERT(d != NULL);
+  d = grpc_jwt_issuer_email_domain("bar@...");
+  GPR_ASSERT(d != NULL);
+}
+
 static void test_claims_success(void) {
   grpc_jwt_claims *claims;
   grpc_slice s = grpc_slice_from_copied_string(claims_without_time_constraint);
@@ -242,6 +281,19 @@ static void test_bad_audience_claims_failure(void) {
   grpc_jwt_claims_destroy(claims);
 }
 
+static void test_bad_subject_claims_failure(void) {
+  grpc_jwt_claims *claims;
+  grpc_slice s = grpc_slice_from_copied_string(claims_with_bad_subject);
+  grpc_json *json = grpc_json_parse_string_with_len(
+      (char *)GRPC_SLICE_START_PTR(s), GRPC_SLICE_LENGTH(s));
+  GPR_ASSERT(json != NULL);
+  claims = grpc_jwt_claims_from_json(json, s);
+  GPR_ASSERT(claims != NULL);
+  GPR_ASSERT(grpc_jwt_claims_check(claims, "https://foo.com") ==
+             GRPC_JWT_VERIFIER_BAD_SUBJECT);
+  grpc_jwt_claims_destroy(claims);
+}
+
 static char *json_key_str(const char *last_part) {
   size_t result_len = strlen(json_key_str_part1) + strlen(json_key_str_part2) +
                       strlen(last_part);
@@ -563,10 +615,12 @@ static void test_jwt_verifier_bad_format(void) {
 int main(int argc, char **argv) {
   grpc_test_init(argc, argv);
   grpc_init();
+  test_jwt_issuer_email_domain();
   test_claims_success();
   test_expired_claims_failure();
   test_invalid_claims_failure();
   test_bad_audience_claims_failure();
+  test_bad_subject_claims_failure();
   test_jwt_verifier_google_email_issuer_success();
   test_jwt_verifier_custom_email_issuer_success();
   test_jwt_verifier_url_issuer_success();
diff --git a/test/core/security/verify_jwt.c b/test/core/security/verify_jwt.c
index 043d29e6bb..ccc85c9f32 100644
--- a/test/core/security/verify_jwt.c
+++ b/test/core/security/verify_jwt.c
@@ -93,6 +93,7 @@ int main(int argc, char **argv) {
   char *aud = NULL;
   grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT;
 
+  grpc_init();
   cl = gpr_cmdline_create("JWT verifier tool");
   gpr_cmdline_add_string(cl, "jwt", "JSON web token to verify", &jwt);
   gpr_cmdline_add_string(cl, "aud", "Audience for the JWT", &aud);
@@ -131,5 +132,6 @@ int main(int argc, char **argv) {
 
   grpc_jwt_verifier_destroy(verifier);
   gpr_cmdline_destroy(cl);
+  grpc_shutdown();
   return !sync.success;
 }
diff --git a/test/core/support/string_test.c b/test/core/support/string_test.c
index 78b77fad8e..af232db350 100644
--- a/test/core/support/string_test.c
+++ b/test/core/support/string_test.c
@@ -243,6 +243,8 @@ static void test_int64toa() {
 static void test_leftpad() {
   char *padded;
 
+  LOG_TEST_NAME("test_leftpad");
+
   padded = gpr_leftpad("foo", ' ', 5);
   GPR_ASSERT(0 == strcmp("  foo", padded));
   gpr_free(padded);
@@ -273,12 +275,25 @@ static void test_leftpad() {
 }
 
 static void test_stricmp(void) {
+  LOG_TEST_NAME("test_stricmp");
+
   GPR_ASSERT(0 == gpr_stricmp("hello", "hello"));
   GPR_ASSERT(0 == gpr_stricmp("HELLO", "hello"));
   GPR_ASSERT(gpr_stricmp("a", "b") < 0);
   GPR_ASSERT(gpr_stricmp("b", "a") > 0);
 }
 
+static void test_memrchr(void) {
+  LOG_TEST_NAME("test_memrchr");
+
+  GPR_ASSERT(NULL == gpr_memrchr(NULL, 'a', 0));
+  GPR_ASSERT(NULL == gpr_memrchr("", 'a', 0));
+  GPR_ASSERT(NULL == gpr_memrchr("hello", 'b', 5));
+  GPR_ASSERT(0 == strcmp((const char *)gpr_memrchr("hello", 'h', 5), "hello"));
+  GPR_ASSERT(0 == strcmp((const char *)gpr_memrchr("hello", 'o', 5), "o"));
+  GPR_ASSERT(0 == strcmp((const char *)gpr_memrchr("hello", 'l', 5), "lo"));
+}
+
 int main(int argc, char **argv) {
   grpc_test_init(argc, argv);
   test_strdup();
@@ -291,5 +306,6 @@ int main(int argc, char **argv) {
   test_int64toa();
   test_leftpad();
   test_stricmp();
+  test_memrchr();
   return 0;
 }