Send the client secret in a header where the provider takes one (#1171)

Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>

Author: jviotti

DEPENDENCIES

@@ -1,6 +1,6 @@
 vendorpull https://github.com/sourcemeta/vendorpull 1dcbac42809cf87cb5b045106b863e17ad84ba02
 uwebsockets https://github.com/uNetworking/uWebSockets v20.79.0
-core https://github.com/sourcemeta/core 1ffe47bdda541e13733d61116739f5a8b6092eea
+core https://github.com/sourcemeta/core 8aff35edd5285002de0ac87a2c06fc4bca4b4d5d
 blaze https://github.com/sourcemeta/blaze ca1949507ea5f4215f9a55ca796cd074602ff705
 jsonbinpack https://github.com/sourcemeta/jsonbinpack f775b2df5fa89d5a70acb940a5c938173811fea7
 jsonschema https://github.com/sourcemeta/jsonschema v16.3.0

enterprise/authentication/authentication.cc

@@ -957,6 +957,15 @@ struct Authentication::Impl {
         resolved.end_session = document.value().end_session_endpoint().value();
       }
 
+      // RFC 6749 Section 2.3.1 requires every server to accept the client
+      // secret in an authorization header and discourages carrying it in the
+      // request body, so the body is used only where the header is refused.
+      // A provider that lists nothing is taken to accept the header, which is
+      // what the specification assigns to saying nothing
+      resolved.token_endpoint_basic_auth =
+          document.value().supports_token_endpoint_auth_method(
+              "client_secret_basic");
+
       cached.source = server;
       cached.resolved = std::move(resolved);
     }

enterprise/server/include/sourcemeta/one/enterprise_server_action_auth_callback_v1.h

@@ -199,7 +199,8 @@ class ActionAuthCallback_v1 : public sourcemeta::one::RouterAction {
 
     const auto id_token{this->exchange(
         endpoints.value().token, policy->client_id, client_secret.value(),
-        redirect_uri, code, verifier->to_string())};
+        redirect_uri, code, verifier->to_string(),
+        endpoints.value().token_endpoint_basic_auth)};
     if (!id_token.has_value()) {
       this->fail(request, response, sourcemeta::core::HTTP_STATUS_BAD_GATEWAY,
                  "urn:sourcemeta:one:auth-exchange-failed",
@@ -449,13 +450,17 @@ class ActionAuthCallback_v1 : public sourcemeta::one::RouterAction {
     }
   }
 
-  [[nodiscard]] auto exchange(const std::string_view token_endpoint,
-                              const std::string_view client_id,
-                              const std::string_view client_secret,
-                              const std::string_view redirect_uri,
-                              const std::string_view code,
-                              const std::string_view code_verifier) const
-      -> std::optional<std::string> {
+  // RFC 6749 Section 2.3.1 has every server accept the client secret in an
+  // authorization header, and asks that carrying it in the request body be
+  // limited to clients that cannot send one. A body is the part of a request
+  // that logging and proxies keep, while an authorization header is the part
+  // they already know to redact, so the header is used wherever the provider
+  // takes it
+  [[nodiscard]] auto exchange(
+      const std::string_view token_endpoint, const std::string_view client_id,
+      const std::string_view client_secret, const std::string_view redirect_uri,
+      const std::string_view code, const std::string_view code_verifier,
+      const bool basic_auth) const -> std::optional<std::string> {
     try {
       sourcemeta::core::HTTPSystemRequest fetch{
           std::string{token_endpoint}, sourcemeta::core::HTTPMethod::POST};
@@ -466,8 +471,16 @@ class ActionAuthCallback_v1 : public sourcemeta::one::RouterAction {
       sourcemeta::core::SecureString body;
       sourcemeta::core::oauth_build_token_request_code(code, redirect_uri,
                                                        code_verifier, {}, body);
-      sourcemeta::core::oauth_client_secret_post(client_id, client_secret,
-                                                 body);
+      if (basic_auth) {
+        sourcemeta::core::SecureString authorization;
+        sourcemeta::core::oauth_client_secret_basic(client_id, client_secret,
+                                                    authorization);
+        fetch.header("authorization", std::move(authorization));
+      } else {
+        sourcemeta::core::oauth_client_secret_post(client_id, client_secret,
+                                                   body);
+      }
+
       fetch.body(std::move(body), "application/x-www-form-urlencoded");
       const auto result{fetch.send()};
       if (result.status.code < 200 || result.status.code >= 300) {

enterprise/unit/authentication/authentication_test.cc

@@ -2174,6 +2174,110 @@ TEST(provider_endpoints_are_retrieved_once_and_reused) {
   EXPECT_EQ(*calls, 1);
 }
 
+TEST(a_provider_naming_no_authentication_method_takes_the_header) {
+  const std::array<std::string_view, 1> paths{{"/portal"}};
+  const std::array<sourcemeta::one::Authentication::Policy, 1> policies{
+      {{.paths = paths,
+        .type = sourcemeta::one::Authentication::Type::OIDC,
+        .issuer = "https://login.test",
+        .client_id = "client",
+        .client_secret_variable = "ONE_TEST_OIDC_AUTH_ABSENT",
+        .name = "okta",
+        .session_secret_variable = SESSION_SECRET_VARIABLE}}};
+  const auto path{test_path("oidc_auth_absent.bin")};
+  sourcemeta::one::Authentication::save(policies, path, path, anywhere);
+
+  const std::map<std::string, std::string> responses{
+      {"https://login.test/.well-known/openid-configuration",
+       R"JSON({
+         "issuer": "https://login.test",
+         "authorization_endpoint": "https://login.test/authorize",
+         "token_endpoint": "https://login.test/token",
+         "jwks_uri": "https://login.test/jwks",
+         
+         "response_types_supported": [ "code" ],
+         "subject_types_supported": [ "public" ],
+         "id_token_signing_alg_values_supported": [ "RS256" ]
+       })JSON"}};
+  const sourcemeta::one::Authentication authentication{
+      path, stub_fetcher(responses, nullptr)};
+
+  const auto endpoints{authentication.endpoints("okta")};
+  EXPECT_TRUE(endpoints.has_value());
+  // RFC 8414 Section 2 makes an absent list mean `client_secret_basic`, so
+  // saying nothing is an answer rather than the absence of one
+  EXPECT_TRUE(endpoints.value().token_endpoint_basic_auth);
+}
+
+TEST(a_provider_naming_the_header_takes_the_header) {
+  const std::array<std::string_view, 1> paths{{"/portal"}};
+  const std::array<sourcemeta::one::Authentication::Policy, 1> policies{
+      {{.paths = paths,
+        .type = sourcemeta::one::Authentication::Type::OIDC,
+        .issuer = "https://login.test",
+        .client_id = "client",
+        .client_secret_variable = "ONE_TEST_OIDC_AUTH_BASIC",
+        .name = "okta",
+        .session_secret_variable = SESSION_SECRET_VARIABLE}}};
+  const auto path{test_path("oidc_auth_basic.bin")};
+  sourcemeta::one::Authentication::save(policies, path, path, anywhere);
+
+  const std::map<std::string, std::string> responses{
+      {"https://login.test/.well-known/openid-configuration",
+       R"JSON({
+         "issuer": "https://login.test",
+         "authorization_endpoint": "https://login.test/authorize",
+         "token_endpoint": "https://login.test/token",
+         "jwks_uri": "https://login.test/jwks",
+         "token_endpoint_auth_methods_supported": [ "client_secret_basic", "client_secret_post" ],
+         "response_types_supported": [ "code" ],
+         "subject_types_supported": [ "public" ],
+         "id_token_signing_alg_values_supported": [ "RS256" ]
+       })JSON"}};
+  const sourcemeta::one::Authentication authentication{
+      path, stub_fetcher(responses, nullptr)};
+
+  const auto endpoints{authentication.endpoints("okta")};
+  EXPECT_TRUE(endpoints.has_value());
+  // Offering both, the header is the one RFC 6749 Section 2.3.1 asks for
+  EXPECT_TRUE(endpoints.value().token_endpoint_basic_auth);
+}
+
+TEST(a_provider_refusing_the_header_gets_the_body_instead) {
+  const std::array<std::string_view, 1> paths{{"/portal"}};
+  const std::array<sourcemeta::one::Authentication::Policy, 1> policies{
+      {{.paths = paths,
+        .type = sourcemeta::one::Authentication::Type::OIDC,
+        .issuer = "https://login.test",
+        .client_id = "client",
+        .client_secret_variable = "ONE_TEST_OIDC_AUTH_POST",
+        .name = "okta",
+        .session_secret_variable = SESSION_SECRET_VARIABLE}}};
+  const auto path{test_path("oidc_auth_post.bin")};
+  sourcemeta::one::Authentication::save(policies, path, path, anywhere);
+
+  const std::map<std::string, std::string> responses{
+      {"https://login.test/.well-known/openid-configuration",
+       R"JSON({
+         "issuer": "https://login.test",
+         "authorization_endpoint": "https://login.test/authorize",
+         "token_endpoint": "https://login.test/token",
+         "jwks_uri": "https://login.test/jwks",
+         "token_endpoint_auth_methods_supported": [ "client_secret_post" ],
+         "response_types_supported": [ "code" ],
+         "subject_types_supported": [ "public" ],
+         "id_token_signing_alg_values_supported": [ "RS256" ]
+       })JSON"}};
+  const sourcemeta::one::Authentication authentication{
+      path, stub_fetcher(responses, nullptr)};
+
+  const auto endpoints{authentication.endpoints("okta")};
+  EXPECT_TRUE(endpoints.has_value());
+  // A provider that does not take the header leaves the body as the only way
+  // to authenticate, so the preference gives way rather than the login failing
+  EXPECT_FALSE(endpoints.value().token_endpoint_basic_auth);
+}
+
 TEST(provider_endpoints_of_an_unreachable_provider_are_absent) {
   const auto calls{std::make_shared<int>(0)};
   const std::array<std::string_view, 1> paths{{"/portal"}};

src/authentication/include/sourcemeta/one/authentication.h

@@ -212,6 +212,9 @@ class SOURCEMETA_ONE_AUTHENTICATION_EXPORT Authentication {
     std::string jwks_uri{};
     // Absent from a provider that does not offer to end its own session
     std::string end_session{};
+    // Whether the provider takes the client secret in an authorization header
+    // rather than in the request body
+    bool token_endpoint_basic_auth{true};
   };
 
   // What the named interactive policy's provider says about itself, retrieved