OIDC redirect socket (#735)

* more robust error handling of OIDC redirect * make sure OIDC redirect sockets are in blocking mode * use win32 recv/send * copy oidc.audience * update [email protected]
openziti · Oct 1, 2024 · c4411fc · c4411fc
1 parent 7a3d145
commit c4411fc
Show file tree

Hide file tree

Showing 6 changed files with 110 additions and 32 deletions.
diff --git a/deps/CMakeLists.txt b/deps/CMakeLists.txt
@@ -9,7 +9,7 @@ if (NOT TARGET tlsuv)
     else ()
         FetchContent_Declare(tlsuv
                 GIT_REPOSITORY https://github.com/openziti/tlsuv.git
-                GIT_TAG v0.32.2
+                GIT_TAG v0.32.3
         )
         FetchContent_MakeAvailable(tlsuv)
     endif (tlsuv_DIR)

diff --git a/library/auth_queries.c b/library/auth_queries.c
@@ -134,8 +134,8 @@ void ziti_mfa_enroll_post_internal_cb(void *empty, const ziti_error *err, void *
     if (err == NULL) {
         ziti_ctrl_get_mfa(ztx_get_controller(ztx), ziti_mfa_enroll_get_internal_cb, ctx);
     } else {
-        ZTX_LOG(ERROR, "error during create MFA call: %d - %s - %s", err->http_code, err->code, err->message);
-        mfa_enroll_cb_ctx->cb(mfa_enroll_cb_ctx->ztx, err->err, NULL, mfa_enroll_cb_ctx->cb_ctx);
+        ZTX_LOG(ERROR, "error during create MFA call: %d - %s - %s", (int)err->http_code, err->code, err->message);
+        mfa_enroll_cb_ctx->cb(mfa_enroll_cb_ctx->ztx, (int)err->err, NULL, mfa_enroll_cb_ctx->cb_ctx);
         FREE(ctx);
     }
 }
@@ -146,8 +146,8 @@ void ziti_mfa_enroll_get_internal_cb(ziti_mfa_enrollment *mfa_enrollment, const
 
     if (err != NULL) {
         if (err->http_code != 404) {
-            ZTX_LOG(ERROR, "error during enroll MFA call: %d - %s - %s", err->http_code, err->code, err->message);
-            mfa_enroll_cb_ctx->cb(mfa_enroll_cb_ctx->ztx, err->err, NULL, mfa_enroll_cb_ctx->cb_ctx);
+            ZTX_LOG(ERROR, "error during enroll MFA call: %d - %s - %s", (int)err->http_code, err->code, err->message);
+            mfa_enroll_cb_ctx->cb(mfa_enroll_cb_ctx->ztx, (int)err->err, NULL, mfa_enroll_cb_ctx->cb_ctx);
             FREE(ctx);
             return;
         }
@@ -182,8 +182,8 @@ void ziti_mfa_remove_internal_cb(void *empty, const ziti_error *err, void *ctx)
     ziti_context ztx = mfa_cb_ctx->ztx;
 
     if (err != NULL) {
-        ZTX_LOG(ERROR, "error during remove MFA call: %d - %s - %s", err->http_code, err->code, err->message);
-        mfa_cb_ctx->cb(mfa_cb_ctx->ztx, err->err, mfa_cb_ctx->cb_ctx);
+        ZTX_LOG(ERROR, "error during remove MFA call: %d - %s - %s", (int)err->http_code, err->code, err->message);
+        mfa_cb_ctx->cb(mfa_cb_ctx->ztx, (int)err->err, mfa_cb_ctx->cb_ctx);
     } else {
         mfa_cb_ctx->cb(mfa_cb_ctx->ztx, ZITI_OK, mfa_cb_ctx->cb_ctx);
     }
@@ -212,8 +212,8 @@ void ziti_mfa_verify_internal_cb(void *empty, const ziti_error *err, void *ctx)
     ziti_context ztx = mfa_cb_ctx->ztx;
 
     if (err != NULL) {
-        ZTX_LOG(ERROR, "error during verify MFA call: %d - %s - %s", err->http_code, err->code, err->message);
-        mfa_cb_ctx->cb(mfa_cb_ctx->ztx, err->err, mfa_cb_ctx->cb_ctx);
+        ZTX_LOG(ERROR, "error during verify MFA call: %d - %s - %s", (int)err->http_code, err->code, err->message);
+        mfa_cb_ctx->cb(mfa_cb_ctx->ztx, (int)err->err, mfa_cb_ctx->cb_ctx);
     } else {
         ziti_force_api_session_refresh(ztx);
         mfa_cb_ctx->cb(mfa_cb_ctx->ztx, ZITI_OK, mfa_cb_ctx->cb_ctx);
@@ -242,10 +242,13 @@ void ziti_mfa_get_recovery_codes_internal_cb(ziti_mfa_recovery_codes *rc, const
     ziti_mfa_recovery_codes_cb_ctx *mfa_recovery_codes_cb_ctx = ctx;
     ziti_context ztx = mfa_recovery_codes_cb_ctx->ztx;
     if (err != NULL) {
-        ZTX_LOG(ERROR, "error during get recovery codes MFA call: %d - %s - %s", err->http_code, err->code, err->message);
-        mfa_recovery_codes_cb_ctx->cb(mfa_recovery_codes_cb_ctx->ztx, err->err, NULL, mfa_recovery_codes_cb_ctx->cb_ctx);
+        ZTX_LOG(ERROR, "error during get recovery codes MFA call: %d - %s - %s",
+                (int)err->http_code, err->code, err->message);
+        mfa_recovery_codes_cb_ctx->cb(mfa_recovery_codes_cb_ctx->ztx, (int)err->err, NULL,
+                                      mfa_recovery_codes_cb_ctx->cb_ctx);
     } else {
-        mfa_recovery_codes_cb_ctx->cb(mfa_recovery_codes_cb_ctx->ztx, ZITI_OK, rc->recovery_codes, mfa_recovery_codes_cb_ctx->cb_ctx);
+        mfa_recovery_codes_cb_ctx->cb(mfa_recovery_codes_cb_ctx->ztx, ZITI_OK, rc->recovery_codes,
+                                      mfa_recovery_codes_cb_ctx->cb_ctx);
         free_ziti_mfa_recovery_codes(rc);
     }
 
@@ -273,8 +276,9 @@ void ziti_mfa_post_recovery_codes_internal_cb(void *empty, const ziti_error *err
     ziti_context ztx = mfa_recovery_codes_cb_ctx->ztx;
 
     if (err != NULL) {
-        ZTX_LOG(ERROR, "error during create recovery codes MFA call: %d - %s - %s", err->http_code, err->code, err->message);
-        mfa_recovery_codes_cb_ctx->cb(mfa_recovery_codes_cb_ctx->ztx, err->err, NULL, mfa_recovery_codes_cb_ctx->cb_ctx);
+        ZTX_LOG(ERROR, "error during create recovery codes MFA call: %d - %s - %s",
+                (int)err->http_code, err->code, err->message);
+        mfa_recovery_codes_cb_ctx->cb(mfa_recovery_codes_cb_ctx->ztx, (int)err->err, NULL, mfa_recovery_codes_cb_ctx->cb_ctx);
     } else {
         ziti_mfa_get_recovery_codes(mfa_recovery_codes_cb_ctx->ztx, mfa_recovery_codes_cb_ctx->code, mfa_recovery_codes_cb_ctx->cb,
                                     mfa_recovery_codes_cb_ctx->cb_ctx);

diff --git a/library/authenticators.c b/library/authenticators.c
@@ -33,7 +33,8 @@ static void extend_cb(ziti_extend_cert_authenticator_resp* resp, const ziti_erro
     ziti_context ztx = wrapped_ctx->ztx;
 
     if(err){
-        ZTX_LOG(ERROR, "error response returned when attempting to extend authenticator: %d %s: %s, calling cb", err->http_code, err->code, err->message);
+        ZTX_LOG(ERROR, "error response returned when attempting to extend authenticator: %d %s: %s, calling cb",
+                (int)err->http_code, err->code, err->message);
         if(err->http_code == 404) {
             wrapped_ctx->extend_cb(ztx, NULL, ZITI_NOT_FOUND, wrapped_ctx->ctx);
         } else if(strncmp(err->code, CAN_NOT_UPDATE_AUTHENTICATOR, strlen(CAN_NOT_UPDATE_AUTHENTICATOR)) == 0){
@@ -62,7 +63,8 @@ static void verify_cb(void* empty, const ziti_error* err, void* ctx){
     ziti_context ztx = wrapped_ctx->ztx;
 
     if(err) {
-        ZTX_LOG(ERROR, "error response returned when attempting to verify extended authenticator: %d %s: %s", err->http_code, err->code, err->message);
+        ZTX_LOG(ERROR, "error response returned when attempting to verify extended authenticator: %d %s: %s",
+                (int)err->http_code, err->code, err->message);
         if(err->http_code == 404) {
             wrapped_ctx->verify_cb(ztx, ZITI_NOT_FOUND, wrapped_ctx->ctx);
         } else if(strncmp(err->code, CAN_NOT_UPDATE_AUTHENTICATOR, strlen(CAN_NOT_UPDATE_AUTHENTICATOR)) == 0){

diff --git a/library/oidc.c b/library/oidc.c
@@ -1,4 +1,4 @@
-// Copyright (c) 2023. NetFoundry Inc.
+// Copyright (c) 2023-2024. NetFoundry Inc.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -25,6 +25,7 @@
 #include "ziti/errors.h"
 #include "ziti/ziti_buffer.h"
 #include "ziti/ziti_model.h"
+#include "buffer.h"
 
 #define code_len 40
 #define code_verifier_len sodium_base64_ENCODED_LEN(code_len, sodium_base64_VARIANT_URLSAFE_NO_PADDING)
@@ -368,13 +369,60 @@ struct ext_link_req {
     uv_os_sock_t sock;
     auth_req *req;
     char *code;
+    int err;
 };
 
+static int set_blocking(uv_os_sock_t sock) {
+#ifdef _WIN32
+    unsigned long mode = 0;
+    if (ioctlsocket(sock, FIONBIO, &mode) != 0) {
+        int err = WSAGetLastError();
+        ZITI_LOG(ERROR, "failed to set socket to blocking: %d", err);
+        return err;
+    }
+#else
+    int flags = fcntl(sock, F_GETFL, 0);
+    flags = flags & ~O_NONBLOCK;
+    if (fcntl(sock, F_SETFL, flags) != 0) {
+        int err = errno;
+        ZITI_LOG(ERROR, "failed to set socket to blocking: %d/%s", err, strerror(err));
+        return err;
+    }
+#endif
+    return 0;
+}
+
+#if _WIN32
+#define close_socket(s) closesocket(s)
+#else
+#define close_socket(s) close(s)
+#endif
+
 static void ext_accept(uv_work_t *wr) {
     struct ext_link_req *elr = (struct ext_link_req *) wr;
     uv_os_sock_t clt = accept(elr->sock, NULL, NULL);
+    set_blocking(clt);
+
     char buf[1024];
-    ssize_t c = read(clt, buf, sizeof(buf) - 1);
+    ssize_t c;
+#if _WIN32
+    c = recv(clt, buf, sizeof(buf) -1, 0);
+#else
+    c = read(clt, buf, sizeof(buf) - 1);
+#endif
+    if (c < 0) {
+        int err;
+#if _WIN32
+        err = WSAGetLastError();
+#else
+        err = errno;
+#endif
+        ZITI_LOG(ERROR, "read failed: %d/%s", err, strerror(err));
+        elr->err = err;
+        close_socket(clt);
+        return;
+    }
+
     buf[c] = 0;
 
     char *cs = strstr(buf, "code=");
@@ -385,9 +433,14 @@ static void ext_accept(uv_work_t *wr) {
                       "Connection: close\r\n"
                       "\r\n"
                       "<body>Unexpected auth request:<pre>";
+#if _WIN32
+        send(clt, resp, sizeof(resp), 0);
+        send(clt, buf, c, 0);
+#else
         write(clt, resp, sizeof(resp));
         write(clt, buf, c);
-        close(clt);
+#endif
+        close_socket(clt);
         return;
     }
     cs += strlen("code=");
@@ -402,23 +455,38 @@ static void ext_accept(uv_work_t *wr) {
     memcpy(code, cs, ce - cs);
     elr->code = code;
 
-    char resp[] = "HTTP/1.1 200 OK\r\n"
-                  "Content-Type: text/html\r\n"
-                  "\r\n"
-                  "<script type=\"text/javascript\">window.close()</script>"
-                  "<body onload=\"window.close()\">You may close this window</body>";
-    write(clt, resp, sizeof(resp));
-    close(clt);
+    string_buf_t resp_buf;
+    string_buf_init(&resp_buf);
+
+    char resp_body[] = "<script type=\"text/javascript\">window.close()</script>"
+                       "<body onload=\"window.close()\">You may close this window</body>";
+#define RESP_FMT "HTTP/1.1 200 OK\r\n"\
+                  "Content-Type: text/html\r\n"\
+                  "Content-Length: %zd\r\n"\
+                  "\r\n%s"
+    string_buf_fmt(&resp_buf, RESP_FMT, strlen(resp_body), resp_body);
+    size_t resp_len;
+    char *resp = string_buf_to_string(&resp_buf, &resp_len);
+#if _WIN32
+    send(clt, resp, resp_len, 0);
+#else
+    write(clt, resp, resp_len);
+#endif
+
+    free(resp);
+    string_buf_free(&resp_buf);
+
+    close_socket(clt);
 }
 
 static void ext_done(uv_work_t *wr, int status) {
     struct ext_link_req *elr = (struct ext_link_req *) wr;
-    close(elr->sock);
+    close_socket(elr->sock);
 
     if (elr->code) {
         request_token(elr->req, elr->code);
     } else {
-        failed_auth_req(elr->req, "code not received");
+        failed_auth_req(elr->req, elr->err ? strerror(elr->err) : "code not received");
     }
 
     free(elr->code);
@@ -442,6 +510,7 @@ static void start_ext_auth(auth_req *req, const char *ep, int qc, tlsuv_http_pai
         close(sock);
         return;
     }
+    set_blocking(sock);
 
     struct ext_link_req *elr = calloc(1, sizeof(*elr));
     elr->sock = sock;

diff --git a/library/posture.c b/library/posture.c
@@ -433,7 +433,8 @@ static void handle_pr_resp_timer_events(ziti_context ztx, ziti_pr_response *pr_r
         for(service_timer = pr_resp->services; *service_timer != NULL; service_timer++){
             NEWP(val, bool);
             *val = true;
-            ZTX_LOG(DEBUG, "handle_pr_resp_timer_events: forcing service name[%s] id[%s] with timeout[%d] timeoutRemaining[%d]", (*service_timer)->name, (*service_timer)->id, *(*service_timer)->timeout, *(*service_timer)->timeoutRemaining);
+            ZTX_LOG(DEBUG, "handle_pr_resp_timer_events: forcing service name[%s] id[%s] with timeout[%d] timeoutRemaining[%d]",
+                    (*service_timer)->name, (*service_timer)->id, (int)*(*service_timer)->timeout, (int)*(*service_timer)->timeoutRemaining);
             ziti_force_service_update(ztx, (*service_timer)->id);
         }
 
@@ -452,7 +453,7 @@ static void ziti_pr_post_bulk_cb(ziti_pr_response *pr_resp, const ziti_error *er
     // if ztx is disabled this request is cancelled and posture_checks is cleared
     if (ztx->posture_checks) {
         if (err != NULL) {
-            ZTX_LOG(ERROR, "error during bulk posture response submission (%d) %s", err->http_code, err->message);
+            ZTX_LOG(ERROR, "error during bulk posture response submission (%d) %s", (int)err->http_code, err->message);
             ztx->posture_checks->must_send = true; //error, must try again
         } else {
             ztx->posture_checks->must_send = false; //did not error, can skip submissions
@@ -489,7 +490,7 @@ static void ziti_pr_post_cb(ziti_pr_response *pr_resp, const ziti_error *err, vo
     ZTX_LOG(DEBUG, "ziti_pr_post_cb: starting");
 
     if (err != NULL) {
-        ZTX_LOG(ERROR, "error during individual posture response submission (%d) %s - object: %s", err->http_code,
+        ZTX_LOG(ERROR, "error during individual posture response submission (%d) %s - object: %s", (int)err->http_code,
                  err->message, pr_ctx->info->obj);
         ziti_pr_set_info_errored(pr_ctx->ztx, pr_ctx->info->id);
     } else {
@@ -850,7 +851,8 @@ static void process_check_work(uv_work_t *w) {
 void ziti_endpoint_state_pr_cb(ziti_pr_response *pr_resp, const ziti_error *err, void *ctx) {
     ziti_context ztx = ctx;
     if (err) {
-        ZTX_LOG(ERROR, "error during endpoint state posture response submission: %d - %s", err->http_code, err->message);
+        ZTX_LOG(ERROR, "error during endpoint state posture response submission: %d - %s",
+                (int)err->http_code, err->message);
     } else {
         ZTX_LOG(INFO, "endpoint state sent");
         handle_pr_resp_timer_events(ztx, pr_resp);

diff --git a/library/ziti.c b/library/ziti.c
@@ -1609,6 +1609,7 @@ int ziti_context_init(ziti_context *ztx, const ziti_config *config) {
         ctx->config.id.oidc = calloc(1, sizeof(*ctx->config.id.oidc));
         ctx->config.id.oidc->client_id = strdup(config->id.oidc->client_id);
         ctx->config.id.oidc->provider_url = strdup(config->id.oidc->provider_url);
+        ctx->config.id.oidc->audience = strdup(config->id.oidc->audience);
         const char *scope;
         MODEL_LIST_FOREACH(scope, config->id.oidc->scopes) {
             model_list_append(&ctx->config.id.oidc->scopes, strdup(scope));