diff --git a/src/Client/OAuthFlowRunner.cpp b/src/Client/OAuthFlowRunner.cpp index ec9d673c2004..58fc8907d7b3 100644 --- a/src/Client/OAuthFlowRunner.cpp +++ b/src/Client/OAuthFlowRunner.cpp @@ -3,6 +3,7 @@ #if USE_JWT_CPP && USE_SSL +#include #include #include @@ -31,9 +32,11 @@ #include #include +#include #include #include #include +#include #include #include #include @@ -41,6 +44,7 @@ #if defined(__APPLE__) || defined(__linux__) # include # include +# include #endif namespace DB @@ -56,15 +60,6 @@ void writeCachedRefreshToken(const std::string & client_id, const std::string & namespace { -constexpr int HTTP_TIMEOUT_SECONDS = 30; - -/// Hard cap on the size of an OAuth2 token / device-authorization response -/// body. Real responses are a few hundred bytes; we accept up to 1 MiB so a -/// hostile or compromised endpoint cannot stream gigabytes into a std::string -/// (memory-exhaustion DoS of clickhouse-client). Anything larger is treated -/// as a protocol error. -constexpr size_t OAUTH_MAX_RESPONSE_BYTES = 1 * 1024 * 1024; - /// Bounds for RFC 8628 device-flow timing values. The client treats the device /// authorization endpoint as untrusted: a hostile or misconfigured server must /// not be able to push the client into a tight poll loop (interval <= 0), an @@ -114,29 +109,6 @@ int extractDeviceFlowInt(const Poco::JSON::Object::Ptr & resp, const std::string } } -/// Read up to max_bytes from `in` into `out`. Throws AUTHENTICATION_FAILED if -/// the stream contains more than max_bytes. Used to bound response sizes from -/// untrusted OAuth endpoints. -void copyStreamWithLimit(std::istream & in, std::string & out, size_t max_bytes) -{ - constexpr size_t buf_size = 8192; - char buffer[buf_size]; - out.clear(); - while (in) - { - in.read(buffer, static_cast(buf_size)); - const auto got = static_cast(in.gcount()); - if (got == 0) - break; - if (out.size() + got > max_bytes) - throw Exception( - ErrorCodes::AUTHENTICATION_FAILED, - "OAuth2 endpoint response exceeds size limit of {} bytes", - max_bytes); - out.append(buffer, got); - } -} - std::string htmlEscape(const std::string & s) { std::string out; @@ -180,8 +152,24 @@ PKCEPair generatePKCE() void openBrowser(const std::string & url) { - std::cerr << "Opening browser for authentication.\n" - << "If the browser does not open, visit:\n " << url << "\n"; + std::cerr << "Opening browser for authentication.\n"; + + /// The authorization URL carries the CSRF `state` and PKCE `code_challenge`. + /// It is written to stderr as a manual fallback in two cases: whenever a + /// browser could not be launched (the user has no other way to continue), + /// and, on the success path, only when stderr is an interactive terminal — + /// the user's own screen, where they may still need the URL if the browser + /// opened somewhere they cannot see. When stderr is redirected or piped (a + /// CI job, a captured log) the URL is kept off the success path so those + /// secrets do not land in shared output. On every launch the URL also + /// reaches the browser via argv; that exposure is documented in + /// `runOAuthAuthCodeFlow`. + auto print_manual_fallback = [&] + { + std::cerr << "If a browser did not open, visit this URL to continue:\n " << url << "\n"; + }; + + bool launched = false; #if defined(__APPLE__) || defined(__linux__) const char * cmd = @@ -197,44 +185,44 @@ void openBrowser(const std::string & url) /// xdg-open is not installed on a headless host). A zero return followed /// by a nonzero waitpid exit status means the helper ran but failed to /// launch a browser (xdg-open exits 3 when no handler is registered). - /// Without the diagnostic below, the caller would silently block in the - /// 120s callback wait, which is the L2 hazard. + /// Either way the caller would otherwise silently block in the 120s + /// callback wait, which is the L2 hazard. if (posix_spawnp(&pid, cmd, nullptr, nullptr, const_cast(argv), nullptr) != 0) { - std::cerr << "Unable to launch '" << cmd << "'; please copy the URL above into a browser manually.\n"; - return; + std::cerr << "Unable to launch '" << cmd << "'.\n"; } - int status = 0; - if (waitpid(pid, &status, 0) < 0 || !WIFEXITED(status) || WEXITSTATUS(status) != 0) - std::cerr << "Unable to launch a browser via '" << cmd - << "'; please copy the URL above into a browser manually.\n"; + else + { + int status = 0; + pid_t waited; + /// Retry across EINTR so a stray signal (e.g. SIGWINCH without + /// SA_RESTART) is not misread as a launch failure — which would both + /// print the secret URL and leave the child unreaped as a zombie. + do + { + waited = waitpid(pid, &status, 0); + } while (waited < 0 && errno == EINTR); + + if (waited < 0 || !WIFEXITED(status) || WEXITSTATUS(status) != 0) + std::cerr << "Unable to launch a browser via '" << cmd << "'.\n"; + else + launched = true; + } + + const bool stderr_is_tty = isatty(STDERR_FILENO) != 0; #else - std::cerr << "Automatic browser launch is not supported on this platform; " - "please copy the URL above into a browser manually.\n"; + std::cerr << "Automatic browser launch is not supported on this platform.\n"; + const bool stderr_is_tty = false; #endif -} -struct AuthCodeState -{ - std::mutex mtx; - std::condition_variable cv; - std::string code; - std::string error; - std::string received_state; - bool done = false; - - /// Pre-loaded before the server starts so that the loopback server can - /// serve the auth URL via a 302 redirect on /start. The browser helper is - /// then launched with only the loopback URL on its argv, keeping the CSRF - /// state and PKCE challenge out of /proc//cmdline of the helper. - /// Read-only after server.start(); never mutated by the handler. - std::string auth_url; -}; + if (!launched || stderr_is_tty) + print_manual_fallback(); +} class AuthCodeHandler : public Poco::Net::HTTPRequestHandler { public: - explicit AuthCodeHandler(AuthCodeState & state_) : state(state_) { } + explicit AuthCodeHandler(OAuthCallbackState & state_) : state(state_) { } void handleRequest(Poco::Net::HTTPServerRequest & request, Poco::Net::HTTPServerResponse & response) override { @@ -244,11 +232,9 @@ class AuthCodeHandler : public Poco::Net::HTTPRequestHandler /// at the registered redirect URI, and the OAuth2 redirect is always a /// GET. Any other method or path is either a stray request from the /// browser (e.g. /favicon.ico) or a local attacker probing the - /// ephemeral port; in both cases we must respond with an error and - /// must not unblock the main thread, otherwise the legitimate IdP - /// redirect can be pre-empted (causing either a DoS of the flow or, - /// if the attacker has obtained the CSRF state via /proc//cmdline - /// of the spawned browser helper, a code-injection race). + /// ephemeral port. The loopback server exposes no endpoint other than + /// /callback: in particular it never serves the authorization URL + /// (which carries the CSRF state and PKCE challenge) back to a caller. if (request.getMethod() != Poco::Net::HTTPRequest::HTTP_GET) { response.setStatus(Poco::Net::HTTPResponse::HTTP_METHOD_NOT_ALLOWED); @@ -259,29 +245,6 @@ class AuthCodeHandler : public Poco::Net::HTTPRequestHandler const std::string & path = uri.getPath(); - /// The browser helper is launched against /start instead of the full - /// auth URL so the CSRF state and PKCE challenge do not appear in any - /// process argv. We hand the URL to the browser via a same-origin 302 - /// served by this loopback server. /start does not mutate auth state, - /// so it is safe to serve it more than once. - if (path == "/start") - { - std::string target; - { - std::lock_guard lock(state.mtx); - target = state.auth_url; - } - if (target.empty()) - { - response.setStatus(Poco::Net::HTTPResponse::HTTP_NOT_FOUND); - response.setContentType("text/plain"); - response.send() << "Not Found"; - return; - } - response.redirect(target, Poco::Net::HTTPResponse::HTTP_FOUND); - return; - } - if (path != "/callback") { response.setStatus(Poco::Net::HTTPResponse::HTTP_NOT_FOUND); @@ -305,6 +268,29 @@ class AuthCodeHandler : public Poco::Net::HTTPRequestHandler received_state = v; } + /// The loopback socket is reachable by any local process, not just this + /// UID, so a /callback can arrive from an attacker as well as from the + /// legitimate IdP redirect. This handler is the sole CSRF-state gate: + /// it unblocks the waiting flow only for a callback whose `state` + /// matches the one we generated. A spec-compliant IdP always echoes + /// `state` (RFC 6749 §4.1.2 / §4.1.2.1, on both success and error), so a + /// mismatch means the delivery is stray or forged, and dropping it here + /// keeps a blind local attacker from aborting a genuine login by racing + /// in a bogus callback. The trade-off: an IdP that violates the spec by + /// omitting `state` on an error redirect no longer fast-fails with the + /// error — the callback is ignored and the flow waits out its timeout. + if (received_state != state.expected_state) + { + { + std::lock_guard lock(state.mtx); + state.saw_state_mismatch = true; + } + response.setStatus(Poco::Net::HTTPResponse::HTTP_BAD_REQUEST); + response.setContentType("text/plain"); + response.send() << "Invalid or missing state"; + return; + } + response.setStatus(Poco::Net::HTTPResponse::HTTP_OK); response.setContentType("text/html"); auto & out = response.send(); @@ -315,27 +301,26 @@ class AuthCodeHandler : public Poco::Net::HTTPRequestHandler out.flush(); std::lock_guard lock(state.mtx); - /// Only the first valid /callback delivery wins; subsequent requests - /// (e.g. an attacker racing the IdP after the legitimate redirect has - /// already been recorded) are ignored so they cannot overwrite a - /// previously-validated code/state pair. + /// Only the first state-matching /callback delivery wins; subsequent + /// requests (e.g. an attacker racing the IdP after the legitimate + /// redirect has already been recorded) are ignored so they cannot + /// overwrite a previously-validated code/error result. if (state.done) return; state.code = code; state.error = error; - state.received_state = received_state; state.done = true; state.cv.notify_one(); } private: - AuthCodeState & state; + OAuthCallbackState & state; }; class AuthCodeHandlerFactory : public Poco::Net::HTTPRequestHandlerFactory { public: - explicit AuthCodeHandlerFactory(AuthCodeState & state_) : state(state_) { } + explicit AuthCodeHandlerFactory(OAuthCallbackState & state_) : state(state_) { } Poco::Net::HTTPRequestHandler * createRequestHandler(const Poco::Net::HTTPServerRequest &) override { @@ -343,11 +328,36 @@ class AuthCodeHandlerFactory : public Poco::Net::HTTPRequestHandlerFactory } private: - AuthCodeState & state; + OAuthCallbackState & state; }; } +Poco::Net::HTTPRequestHandlerFactory * createOAuthCallbackHandlerFactory(OAuthCallbackState & state) +{ + return new AuthCodeHandlerFactory(state); +} + +void copyStreamWithLimit(std::istream & in, std::string & out, std::size_t max_bytes) +{ + constexpr size_t buf_size = 8192; + char buffer[buf_size]; + out.clear(); + while (in) + { + in.read(buffer, static_cast(buf_size)); + const auto got = static_cast(in.gcount()); + if (got == 0) + break; + if (out.size() + got > max_bytes) + throw Exception( + ErrorCodes::AUTHENTICATION_FAILED, + "OAuth2 endpoint response exceeds size limit of {} bytes", + max_bytes); + out.append(buffer, got); + } +} + std::string urlEncodeOAuth(const std::string & value) { std::string result; @@ -365,26 +375,23 @@ Poco::JSON::Object::Ptr postOAuthForm(const std::string & url, const std::string Poco::Net::HTTPResponse response; std::string response_body; + std::unique_ptr session; if (uri.getScheme() == "https") { Poco::Net::Context::Ptr ctx = Poco::Net::SSLManager::instance().defaultClientContext(); - Poco::Net::HTTPSClientSession session(uri.getHost(), uri.getPort(), ctx); - session.setTimeout(Poco::Timespan(HTTP_TIMEOUT_SECONDS, 0)); - auto & req_stream = session.sendRequest(request); - req_stream << body; - auto & resp_stream = session.receiveResponse(response); - copyStreamWithLimit(resp_stream, response_body, OAUTH_MAX_RESPONSE_BYTES); + session = std::make_unique(uri.getHost(), uri.getPort(), ctx); } else { - Poco::Net::HTTPClientSession session(uri.getHost(), uri.getPort()); - session.setTimeout(Poco::Timespan(HTTP_TIMEOUT_SECONDS, 0)); - auto & req_stream = session.sendRequest(request); - req_stream << body; - auto & resp_stream = session.receiveResponse(response); - copyStreamWithLimit(resp_stream, response_body, OAUTH_MAX_RESPONSE_BYTES); + session = std::make_unique(uri.getHost(), uri.getPort()); } + session->setTimeout(Poco::Timespan(OAUTH_HTTP_TIMEOUT_SECONDS, 0)); + auto & req_stream = session->sendRequest(request); + req_stream << body; + auto & resp_stream = session->receiveResponse(response); + copyStreamWithLimit(resp_stream, response_body, OAUTH_MAX_RESPONSE_BYTES); + Poco::Dynamic::Var parsed; try { @@ -401,7 +408,20 @@ Poco::JSON::Object::Ptr postOAuthForm(const std::string & url, const std::string response_body.substr(0, 512)); } - auto obj = parsed.extract(); + Poco::JSON::Object::Ptr obj; + /// `extract` throws (not returns null) when the parsed value is not an + /// object — e.g. a bare array, scalar, or JSON `null` — so it must be + /// guarded too, otherwise a valid-JSON-but-non-object body escapes as a raw + /// Poco exception instead of the diagnostic below. + try + { + obj = parsed.extract(); + } + catch (const Poco::Exception &) + { + obj = nullptr; + } + if (!obj) throw Exception( ErrorCodes::AUTHENTICATION_FAILED, @@ -443,8 +463,7 @@ std::string runOAuthAuthCodeFlow(const OAuthCredentials & creds) /// is listening, the auth code is silently dropped, and the main thread /// hits the 120s wait_for() timeout even though the user successfully /// completed the login. Using the IP literal keeps the redirect target - /// aligned with the bound socket regardless of resolver behaviour, and - /// matches the host already used for the /start browser entry URL below. + /// aligned with the bound socket regardless of resolver behaviour. const std::string redirect_uri = "http://127.0.0.1:" + std::to_string(port) + "/callback"; std::string auth_url @@ -459,50 +478,64 @@ std::string runOAuthAuthCodeFlow(const OAuthCredentials & creds) if (provider_policy->useAccessTypeOfflineForAuthCode()) auth_url += "&access_type=offline"; - AuthCodeState state; - /// Publish the auth URL to the loopback server before it starts so /start - /// can immediately redirect to it. This is set under the mutex for - /// happens-before with the handler thread; in practice it is only ever - /// read after server.start() but we keep the synchronization explicit. - { - std::lock_guard lock(state.mtx); - state.auth_url = auth_url; - } + OAuthCallbackState state; + /// Publish the expected CSRF state before the server thread starts, so the + /// callback handler only unblocks this flow for a redirect echoing exactly + /// this value (read-only for the handler thereafter). + state.expected_state = csrf_state; auto params = Poco::AutoPtr(new Poco::Net::HTTPServerParams()); params->setMaxQueued(1); params->setMaxThreads(1); - Poco::Net::HTTPServer server(new AuthCodeHandlerFactory(state), server_socket, params); + Poco::Net::HTTPServer server(createOAuthCallbackHandlerFactory(state), server_socket, params); server.start(); - /// Launch the browser against the loopback /start endpoint instead of the - /// real auth URL. The full auth URL (including CSRF state and PKCE - /// challenge) therefore never appears in any process's argv, closing the - /// /proc//cmdline disclosure path that local same-UID attackers - /// previously had against the spawned xdg-open / open helper. - const std::string browser_entry_url = "http://127.0.0.1:" + std::to_string(port) + "/start"; - openBrowser(browser_entry_url); + /// Launch the browser with the authorization URL. Handing the URL to a + /// local browser is the standard native-app pattern (as in `gcloud` / + /// `aws sso`), but it is not free of local exposure: the URL carries the + /// CSRF `state` and PKCE `code_challenge`, and it reaches the browser via + /// the helper's argv, which on a default Linux /proc is world-readable + /// (`/proc//cmdline`, no `hidepid`) — so a different-UID local process + /// can read it while the helper/browser runs. This is the same disclosure + /// class the removed `/start` redirect had (there via the loopback socket); + /// PKCE and `state` do not fully close it, because both values travel in + /// this one URL. It is an inherent limitation of auto-launching a browser + /// on a shared host; the state-matching gate in the callback handler is the + /// partial mitigation (a blind local attacker who has not read the URL + /// cannot forge a matching callback). `openBrowser` keeps the URL off + /// redirected/piped stderr on the successful-launch path (see its comment), + /// so it is not additionally disclosed into captured logs. + openBrowser(auth_url); bool timed_out = false; + bool saw_state_mismatch = false; std::string received_code; std::string received_error; - std::string received_state; { std::unique_lock lock(state.mtx); timed_out = !state.cv.wait_for(lock, std::chrono::seconds(120), [&] { return state.done; }); received_code = state.code; received_error = state.error; - received_state = state.received_state; + saw_state_mismatch = state.saw_state_mismatch; } server.stop(); + /// The CSRF check (received_state == csrf_state) is enforced by the callback + /// handler, which only unblocks this wait for a state-matching delivery, so + /// any callback that reaches this point already passed it. If instead we + /// timed out after seeing a state-mismatching callback, surface that as the + /// likely cause (forged/stray callback, or a proxy that rewrote `state`) + /// rather than a bare timeout. + if (timed_out && saw_state_mismatch) + throw Exception( + ErrorCodes::AUTHENTICATION_FAILED, + "OAuth2 login timed out; a browser callback arrived with a non-matching state " + "(CSRF check failed) and no valid callback followed"); if (timed_out) throw Exception(ErrorCodes::AUTHENTICATION_FAILED, "OAuth2 login timed out waiting for browser callback"); if (!received_error.empty()) throw Exception(ErrorCodes::AUTHENTICATION_FAILED, "OAuth2 authorization error: {}", received_error); if (received_code.empty()) throw Exception(ErrorCodes::AUTHENTICATION_FAILED, "OAuth2 callback did not contain an authorization code"); - if (received_state != csrf_state) - throw Exception(ErrorCodes::AUTHENTICATION_FAILED, "OAuth2 CSRF check failed: unexpected state in callback"); std::string body = "grant_type=authorization_code" @@ -534,16 +567,27 @@ std::string runOAuthAuthCodeFlow(const OAuthCredentials & creds) return resp->getValue("id_token"); } +std::string buildDeviceAuthorizationRequestBody(const OAuthCredentials & creds, const std::string & scope) +{ + std::string body + = "client_id=" + urlEncodeOAuth(creds.client_id) + + "&scope=" + urlEncodeOAuth(scope); + /// Per RFC 8628 §3.1 a confidential client must authenticate on the + /// device authorization request the same way as on the token endpoint. + /// See runOAuthAuthCodeFlow() above: omit the parameter for public + /// clients, do not send an empty value. + if (!creds.client_secret.empty()) + body += "&client_secret=" + urlEncodeOAuth(creds.client_secret); + return body; +} + std::string runOAuthDeviceFlow(OAuthCredentials creds) { auto provider_policy = IOAuthProviderPolicy::create(creds); if (creds.device_auth_uri.empty()) creds.device_auth_uri = provider_policy->resolveDeviceAuthorizationEndpoint(creds); - const std::string device_scope = provider_policy->getDeviceScope(); - const std::string device_body - = "client_id=" + urlEncodeOAuth(creds.client_id) - + "&scope=" + urlEncodeOAuth(device_scope); + const std::string device_body = buildDeviceAuthorizationRequestBody(creds, provider_policy->getDeviceScope()); auto device_resp = postOAuthForm(creds.device_auth_uri, device_body); diff --git a/src/Client/OAuthFlowRunner.h b/src/Client/OAuthFlowRunner.h index de5ba06dc31e..647e35a544f7 100644 --- a/src/Client/OAuthFlowRunner.h +++ b/src/Client/OAuthFlowRunner.h @@ -7,13 +7,42 @@ #include +#include +#include #include namespace DB { +/// Hard cap on the size of an HTTP response body buffered into memory from an +/// OAuth2 / OIDC endpoint. Real responses are a few hundred bytes to a few KiB; +/// the 1 MiB cap stops a hostile or misconfigured endpoint from streaming an +/// unbounded body into a `std::string` (memory-exhaustion DoS of +/// `clickhouse-client`). Anything larger is treated as a protocol error. +/// Applies to the browser (authorization-code) and device login flows +/// implemented in `OAuthFlowRunner.cpp` and `OAuthProviderPolicy.cpp` via +/// `copyStreamWithLimit`. +constexpr std::size_t OAUTH_MAX_RESPONSE_BYTES = 1 * 1024 * 1024; + +/// Timeout (seconds) for every HTTP request to an OAuth2 / OIDC endpoint, so a +/// hung or slow endpoint cannot stall `clickhouse-client` indefinitely. Shared +/// by the browser, device, and discovery flows. +constexpr int OAUTH_HTTP_TIMEOUT_SECONDS = 30; + +/// Read up to max_bytes from `in` into `out`, throwing `AUTHENTICATION_FAILED` +/// if the stream carries more than max_bytes. Used to bound response sizes from +/// untrusted OAuth/OIDC endpoints. +void copyStreamWithLimit(std::istream & in, std::string & out, std::size_t max_bytes); + std::string urlEncodeOAuth(const std::string & value); Poco::JSON::Object::Ptr postOAuthForm(const std::string & url, const std::string & body); + +/// Build the form body of the RFC 8628 device authorization request. Exposed +/// (rather than kept local to `runOAuthDeviceFlow`) only so the regression +/// tests can verify that a confidential client's `client_secret` is included +/// (RFC 8628 §3.1) while public clients omit the parameter entirely. +std::string buildDeviceAuthorizationRequestBody(const OAuthCredentials & creds, const std::string & scope); + std::string runOAuthAuthCodeFlow(const OAuthCredentials & creds); std::string runOAuthDeviceFlow(OAuthCredentials creds); diff --git a/src/Client/OAuthLoopbackServer.h b/src/Client/OAuthLoopbackServer.h new file mode 100644 index 000000000000..1a672cf99011 --- /dev/null +++ b/src/Client/OAuthLoopbackServer.h @@ -0,0 +1,60 @@ +#pragma once + +#include + +#if USE_JWT_CPP && USE_SSL + +#include +#include +#include + +namespace Poco::Net +{ +class HTTPRequestHandlerFactory; +} + +namespace DB +{ + +/// Shared state between the loopback OAuth authorization-code callback server +/// and the flow that waits on it. Populated by the first `/callback` delivery +/// whose `state` matches `expected_state`. +/// +/// This type and the factory below are production internals of +/// `runOAuthAuthCodeFlow`; they live in their own header (rather than in the +/// widely-included `OAuthFlowRunner.h`) only so the regression tests can drive +/// the real callback handler over loopback HTTP without widening the surface +/// every other OAuth translation unit sees. +struct OAuthCallbackState +{ + std::mutex mtx; + std::condition_variable cv; + + /// The CSRF state the flow expects the IdP to echo back. Set before the + /// server starts; read-only afterwards. The handler unblocks the flow only + /// for a `/callback` carrying exactly this value, so a local process that + /// does not know the state cannot pre-empt the flow with a forged callback. + std::string expected_state; + + std::string code; + std::string error; + bool done = false; + + /// Set (but never unblocks the flow) when a `/callback` arrived whose + /// `state` did not match `expected_state`. Lets the waiting flow, if it + /// times out, report a likely CSRF/state-mismatch instead of a bare + /// timeout — recovering the diagnostic that the removed main-thread check + /// used to give — without letting the mismatched callback abort the flow. + bool saw_state_mismatch = false; +}; + +/// Build the production request-handler factory for the loopback callback +/// server. The returned pointer is owned by the caller (Poco::Net::HTTPServer +/// takes ownership when constructed with it). The server it builds recognizes +/// only `/callback`; every other path returns 404 and never serves the +/// authorization URL or CSRF state back to a caller. +Poco::Net::HTTPRequestHandlerFactory * createOAuthCallbackHandlerFactory(OAuthCallbackState & state); + +} + +#endif // USE_JWT_CPP && USE_SSL diff --git a/src/Client/OAuthProviderPolicy.cpp b/src/Client/OAuthProviderPolicy.cpp index 1bf589e95912..7e6e24443df5 100644 --- a/src/Client/OAuthProviderPolicy.cpp +++ b/src/Client/OAuthProviderPolicy.cpp @@ -3,6 +3,7 @@ #if USE_JWT_CPP && USE_SSL +#include #include #include @@ -12,7 +13,6 @@ #include #include #include -#include #include #include @@ -27,8 +27,6 @@ extern const int AUTHENTICATION_FAILED; namespace { -constexpr int HTTP_TIMEOUT_SECONDS = 30; - std::string fetchDeviceEndpointFromIssuer(const std::string & issuer) { const std::string discovery_url = issuer + "/.well-known/openid-configuration"; @@ -38,24 +36,25 @@ std::string fetchDeviceEndpointFromIssuer(const std::string & issuer) Poco::Net::HTTPResponse response; std::string body; + std::unique_ptr session; if (disc_uri.getScheme() == "https") { Poco::Net::Context::Ptr ctx = Poco::Net::SSLManager::instance().defaultClientContext(); - Poco::Net::HTTPSClientSession session(disc_uri.getHost(), disc_uri.getPort(), ctx); - session.setTimeout(Poco::Timespan(HTTP_TIMEOUT_SECONDS, 0)); - session.sendRequest(request); - auto & stream = session.receiveResponse(response); - Poco::StreamCopier::copyToString(stream, body); + session = std::make_unique(disc_uri.getHost(), disc_uri.getPort(), ctx); } else { - Poco::Net::HTTPClientSession session(disc_uri.getHost(), disc_uri.getPort()); - session.setTimeout(Poco::Timespan(HTTP_TIMEOUT_SECONDS, 0)); - session.sendRequest(request); - auto & stream = session.receiveResponse(response); - Poco::StreamCopier::copyToString(stream, body); + session = std::make_unique(disc_uri.getHost(), disc_uri.getPort()); } + session->setTimeout(Poco::Timespan(OAUTH_HTTP_TIMEOUT_SECONDS, 0)); + session->sendRequest(request); + auto & stream = session->receiveResponse(response); + + /// Check the HTTP status (available from the response headers) before + /// reading the body, so a non-2xx response yields the actionable + /// status-based diagnostic rather than the generic size-limit error when + /// the error body happens to exceed OAUTH_MAX_RESPONSE_BYTES. if (response.getStatus() != Poco::Net::HTTPResponse::HTTP_OK) throw Exception( ErrorCodes::AUTHENTICATION_FAILED, @@ -64,9 +63,24 @@ std::string fetchDeviceEndpointFromIssuer(const std::string & issuer) static_cast(response.getStatus()), response.getReason()); - Poco::JSON::Parser parser; - auto result = parser.parse(body); - const auto & obj = result.extract(); + copyStreamWithLimit(stream, body, OAUTH_MAX_RESPONSE_BYTES); + + Poco::JSON::Object::Ptr obj; + try + { + Poco::JSON::Parser parser; + obj = parser.parse(body).extract(); + } + catch (const Poco::Exception &) + { + obj = nullptr; + } + + if (!obj) + throw Exception( + ErrorCodes::AUTHENTICATION_FAILED, + "OIDC discovery document at '{}' is not a valid JSON object", + discovery_url); if (!obj->has("device_authorization_endpoint")) throw Exception( @@ -74,7 +88,20 @@ std::string fetchDeviceEndpointFromIssuer(const std::string & issuer) "OIDC discovery document at '{}' does not contain device_authorization_endpoint", discovery_url); - return obj->getValue("device_authorization_endpoint"); + /// The key can be present but not a string (e.g. null or a number), in + /// which case getValue throws a raw Poco exception; convert it to the same + /// AUTHENTICATION_FAILED diagnostic used for every other malformed case. + try + { + return obj->getValue("device_authorization_endpoint"); + } + catch (const Poco::Exception &) + { + throw Exception( + ErrorCodes::AUTHENTICATION_FAILED, + "OIDC discovery document at '{}' has a non-string device_authorization_endpoint", + discovery_url); + } } std::string inferIssuerFromTokenUri(const std::string & token_uri) diff --git a/src/Client/tests/gtest_oauth_login.cpp b/src/Client/tests/gtest_oauth_login.cpp index 2efcbc17aff8..a4aaabc1f076 100644 --- a/src/Client/tests/gtest_oauth_login.cpp +++ b/src/Client/tests/gtest_oauth_login.cpp @@ -3,15 +3,29 @@ #if USE_JWT_CPP && USE_SSL #include +#include +#include #include #include #include #include #include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + #include #include #include +#include using namespace DB; @@ -328,4 +342,236 @@ TEST(OAuthLogin, PKCEChallengeDerivation) EXPECT_NE(base64Encode(encodeSHA256(verifier2), true, true), challenge); } +// --------------------------------------------------------------------------- +// buildDeviceAuthorizationRequestBody — RFC 8628 §3.1 client authentication. +// Regression guard for the confidential-client device flow: the device +// authorization request must carry `client_secret` when one is configured +// (otherwise a confidential IdP client rejects the very first request with +// `invalid_client`), and must omit the parameter entirely for public clients. +// --------------------------------------------------------------------------- + +TEST(OAuthLogin, DeviceAuthorizationBodyIncludesClientSecret) +{ + OAuthCredentials creds; + creds.client_id = "demo-confidential"; + creds.client_secret = "t0p-s3cret"; + + const std::string body = buildDeviceAuthorizationRequestBody(creds, "openid"); + EXPECT_EQ(body, "client_id=demo-confidential&scope=openid&client_secret=t0p-s3cret"); +} + +TEST(OAuthLogin, DeviceAuthorizationBodyOmitsSecretForPublicClient) +{ + OAuthCredentials creds; + creds.client_id = "demo-public"; + + const std::string body = buildDeviceAuthorizationRequestBody(creds, "openid"); + EXPECT_EQ(body, "client_id=demo-public&scope=openid"); + // An empty value is not equivalent to omission and is rejected by several + // IdPs as invalid_client — the parameter must be absent altogether. + EXPECT_EQ(body.find("client_secret"), std::string::npos); +} + +// --------------------------------------------------------------------------- +// copyStreamWithLimit — bounded read of untrusted OAuth/OIDC responses. +// Regression guard for the unbounded OIDC discovery read (memory-exhaustion +// DoS): the browser/device login HTTP reads must cap the body size. +// --------------------------------------------------------------------------- + +TEST(OAuthLogin, CopyStreamWithLimitAcceptsWithinLimit) +{ + std::istringstream in(std::string("hello world")); + std::string out; + copyStreamWithLimit(in, out, 1024); + EXPECT_EQ(out, "hello world"); +} + +TEST(OAuthLogin, CopyStreamWithLimitAcceptsExactBoundary) +{ + // A body of exactly max_bytes is accepted; only strictly larger fails. + std::istringstream in(std::string(1024, 'x')); + std::string out; + copyStreamWithLimit(in, out, 1024); + EXPECT_EQ(out.size(), 1024u); +} + +TEST(OAuthLogin, CopyStreamWithLimitRejectsOversized) +{ + // A body larger than the limit must fail fast instead of buffering the + // whole (potentially unbounded) response into memory. + std::istringstream in(std::string(2048, 'x')); + std::string out; + EXPECT_THROW(copyStreamWithLimit(in, out, 1024), DB::Exception); +} + +// --------------------------------------------------------------------------- +// Loopback callback server — must not leak the auth URL / CSRF state. +// Regression guard for the `/start` redirect that disclosed the full +// authorization URL (including `state`) to any local process. +// --------------------------------------------------------------------------- + +namespace +{ + +struct RunningCallbackServer +{ + OAuthCallbackState state; + Poco::Net::ServerSocket server_socket; + Poco::Net::HTTPServer server; + uint16_t port; + + explicit RunningCallbackServer(const std::string & expected_state = "") + : server_socket(makeSocket()) + , server(createOAuthCallbackHandlerFactory(state), server_socket, makeParams()) + , port(server_socket.address().port()) + { + /// Set before `start`, so the handler thread reads it race-free. + state.expected_state = expected_state; + server.start(); + } + + ~RunningCallbackServer() { server.stop(); } + + static Poco::Net::ServerSocket makeSocket() + { + Poco::Net::ServerSocket socket; + socket.bind(Poco::Net::SocketAddress("127.0.0.1", 0), /*reuse_address=*/true); + socket.listen(4); + return socket; + } + + static Poco::Net::HTTPServerParams * makeParams() + { + auto * params = new Poco::Net::HTTPServerParams(); + params->setMaxQueued(4); + params->setMaxThreads(1); + return params; + } +}; + +struct HttpGetResult +{ + Poco::Net::HTTPResponse::HTTPStatus status; + bool has_location; + std::string location; + std::string body; +}; + +HttpGetResult httpGet(uint16_t port, const std::string & path_and_query) +{ + Poco::Net::HTTPClientSession session("127.0.0.1", port); + session.setTimeout(Poco::Timespan(10, 0)); + Poco::Net::HTTPRequest request(Poco::Net::HTTPRequest::HTTP_GET, path_and_query); + session.sendRequest(request); + + Poco::Net::HTTPResponse response; + std::istream & rs = session.receiveResponse(response); + + HttpGetResult result; + result.status = response.getStatus(); + result.has_location = response.has("Location"); + result.location = response.get("Location", ""); + Poco::StreamCopier::copyToString(rs, result.body); + return result; +} + +} // anonymous namespace + +TEST(OAuthLogin, LoopbackStartEndpointDoesNotLeakState) +{ + RunningCallbackServer srv; + + // `/start` (the removed leak vector) redirected to the auth URL, so a + // reintroduced leak would surface as a 3xx whose Location header carries + // `state` / `code_challenge`. Assert there is no redirect and — checking + // the Location value itself, which is where such a leak would live — that + // it discloses no secret. + const HttpGetResult r = httpGet(srv.port, "/start"); + EXPECT_EQ(r.status, Poco::Net::HTTPResponse::HTTP_NOT_FOUND); + EXPECT_FALSE(r.has_location); + EXPECT_EQ(r.location.find("state="), std::string::npos); + EXPECT_EQ(r.location.find("code_challenge="), std::string::npos); + + // Probing `/start` must never unblock the waiting flow. + std::lock_guard lock(srv.state.mtx); + EXPECT_FALSE(srv.state.done); +} + +TEST(OAuthLogin, LoopbackUnknownPathDoesNotCompleteFlow) +{ + RunningCallbackServer srv; + + const HttpGetResult r = httpGet(srv.port, "/favicon.ico"); + EXPECT_EQ(r.status, Poco::Net::HTTPResponse::HTTP_NOT_FOUND); + + std::lock_guard lock(srv.state.mtx); + EXPECT_FALSE(srv.state.done); +} + +TEST(OAuthLogin, LoopbackCallbackCompletesFlow) +{ + RunningCallbackServer srv("deadbeef"); + + // The only endpoint that drives completion is `/callback`, and only when + // the delivered `state` matches the one the flow generated. + const HttpGetResult r = httpGet(srv.port, "/callback?code=abc123&state=deadbeef"); + EXPECT_EQ(r.status, Poco::Net::HTTPResponse::HTTP_OK); + + std::lock_guard lock(srv.state.mtx); + EXPECT_TRUE(srv.state.done); + EXPECT_EQ(srv.state.code, "abc123"); + EXPECT_TRUE(srv.state.error.empty()); +} + +TEST(OAuthLogin, LoopbackErrorCallbackWithMatchingStateCompletesFlow) +{ + RunningCallbackServer srv("deadbeef"); + + // A spec-compliant IdP echoes `state` on error redirects too; such a + // denial must complete the flow so the caller can surface the real error + // (rather than time out). The state-matching gate accepts it, records the + // error, and unblocks. + const HttpGetResult r = httpGet(srv.port, "/callback?error=access_denied&state=deadbeef"); + EXPECT_EQ(r.status, Poco::Net::HTTPResponse::HTTP_OK); + + std::lock_guard lock(srv.state.mtx); + EXPECT_TRUE(srv.state.done); + EXPECT_TRUE(srv.state.code.empty()); + EXPECT_EQ(srv.state.error, "access_denied"); +} + +TEST(OAuthLogin, LoopbackCallbackWithMismatchedStateIsRejected) +{ + RunningCallbackServer srv("expected-state"); + + // A callback whose `state` does not match must be rejected and must NOT + // unblock the flow — otherwise any local process could abort a genuine + // login (DoS) by racing in a bogus callback before the real IdP redirect. + const HttpGetResult r = httpGet(srv.port, "/callback?code=attacker&state=wrong-state"); + EXPECT_EQ(r.status, Poco::Net::HTTPResponse::HTTP_BAD_REQUEST); + + std::lock_guard lock(srv.state.mtx); + EXPECT_FALSE(srv.state.done); + EXPECT_TRUE(srv.state.code.empty()); + // The mismatch is recorded so a subsequent timeout can report a likely CSRF + // failure rather than a bare timeout. + EXPECT_TRUE(srv.state.saw_state_mismatch); +} + +TEST(OAuthLogin, LoopbackFirstMatchingCallbackWins) +{ + RunningCallbackServer srv("s"); + + // Once a valid callback is recorded, a later one (e.g. an attacker racing + // the IdP after the real redirect already landed) must not overwrite it. + const HttpGetResult first = httpGet(srv.port, "/callback?code=first&state=s"); + EXPECT_EQ(first.status, Poco::Net::HTTPResponse::HTTP_OK); + const HttpGetResult second = httpGet(srv.port, "/callback?code=second&state=s"); + EXPECT_EQ(second.status, Poco::Net::HTTPResponse::HTTP_OK); + + std::lock_guard lock(srv.state.mtx); + EXPECT_TRUE(srv.state.done); + EXPECT_EQ(srv.state.code, "first"); +} + #endif // USE_JWT_CPP && USE_SSL