Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to provide a custom http agent and disable the auth header #284

Merged
merged 3 commits into from
Jun 19, 2024

Conversation

slvrtrn
Copy link
Contributor

@slvrtrn slvrtrn commented Jun 18, 2024

New features

  • Added an option to provide a custom HTTP Agent in the client configuration via the http_agent option (#283, related: #278). The following conditions apply if a custom HTTP Agent is provided:

    • The max_open_connections and tls options will have no effect and will be ignored by the client, as it is a part of the underlying (default) HTTP Agent configuration.
    • keep_alive.enabled will only regulate the default value of the Connection header (true -> Connection: keep-alive, false -> Connection: close).
    • While the idle socket management will still work, it is now possible to disable it completely by setting the keep_alive.idle_socket_ttl value to 0.
  • Added a new client configuration option: set_basic_auth_header which disables the Authorization header that is set by the client by default for every outgoing HTTP request. One of the possible scenarios when it is necessary to disable this header is when a custom HTTPS agent is used, and the server requires TLS authorization with certificates. For example:

    const agent = new https.Agent({
      ca: fs.readFileSync('./ca.crt'),
    })
    const client = createClient({
      url: 'https://server.clickhouseconnect.test:8443',
      http_agent: agent,
      // With a custom HTTPS agent, the client won't use the default HTTPS connection implementation; the basic TLS headers should be provided manually
      http_headers: {
        'X-ClickHouse-User': 'default',
        'X-ClickHouse-Key': '',
      },
      // Authorization header conflicts with the TLS headers; disable it.
      set_basic_auth_header: false,
    })

NB: It is currently not possible to set the set_basic_auth_header option via the URL params.

Checklist

@slvrtrn slvrtrn requested a review from mshustov June 18, 2024 13:07
Copy link

sonarcloud bot commented Jun 18, 2024

@slvrtrn
Copy link
Contributor Author

slvrtrn commented Jun 18, 2024

Docs: ClickHouse/clickhouse-docs#2412

ca: ca_cert,
})
const client = createClient({
url: 'https://server.clickhouseconnect.test:8443',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we run the test against Cloud?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can, but this is slightly more annoying - it will have to choose between HTTP and HTTPS agent, plus I wanted to test the auth with a CA cert (so I'll need to add that to the repo as well).

/** Enable or disable the `Authorization` header with basic auth for the outgoing HTTP(s) requests.
* @experimental - unstable API, might be a subject to change in the future; please provide your feedback in the repository.
* @default true (enabled) */
set_basic_auth_header?: boolean
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if we change the client signature

  /** The name of the user on whose behalf requests are made.
   *  @default default */
  username?: string | null
  /** The user password.
   *  @default empty string */
  password?: string | null

if null is provided, no defaults are applied, and no auth header is set?

@slvrtrn slvrtrn merged commit e88d3eb into main Jun 19, 2024
29 checks passed
@slvrtrn slvrtrn deleted the custom-http-agent branch June 19, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants