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

NoVerifyServerName disables sending SNI #313

Open
etan-status opened this issue Sep 14, 2022 · 1 comment
Open

NoVerifyServerName disables sending SNI #313

etan-status opened this issue Sep 14, 2022 · 1 comment

Comments

@etan-status
Copy link
Contributor

When TLSFlags.NoVerifyServerName is specified, in tlsstream.nim, BearSSL is configured with sslClientReset(res.ccontext, "", 0). While this disables server name verification, it also disables sending SNI, leading to connection failure when connecting to servers that require SNI extension to be present (e.g., Alchemy).

SNI extension should still be sent, even when NoVerifyServerName is specified.

(empty string seems to have same behaviour as NULL)

/**
 * \brief Prepare or reset a client context for a new connection.
 *
 * The `server_name` parameter is used to fill the SNI extension; the
 * X.509 "minimal" engine will also match that name against the server
 * names included in the server's certificate. If the parameter is
 * `NULL` then no SNI extension will be sent, and the X.509 "minimal"
 * engine (if used for server certificate validation) will not check
 * presence of any specific name in the received certificate.
 *
 * Therefore, setting the `server_name` to `NULL` shall be reserved
 * to cases where alternate or additional methods are used to ascertain
 * that the right server public key is used (e.g. a "known key" model).
 *
 * If `resume_session` is non-zero and the context was previously used
 * then the session parameters may be reused (depending on whether the
 * server previously sent a non-empty session ID, and accepts the session
 * resumption). The session parameters for session resumption can also
 * be set explicitly with `br_ssl_engine_set_session_parameters()`.
 *
 * On failure, the context is marked as failed, and this function
 * returns 0. A possible failure condition is when no initial entropy
 * was injected, and none could be obtained from the OS (either OS
 * randomness gathering is not supported, or it failed).
 *
 * \param cc               client context.
 * \param server_name      target server name, or `NULL`.
 * \param resume_session   non-zero to try session resumption.
 * \return  0 on failure, 1 on success.
 */
int br_ssl_client_reset(br_ssl_client_context *cc,
	const char *server_name, int resume_session);
etan-status added a commit to status-im/nim-json-rpc that referenced this issue Sep 14, 2022
Setting `NoVerifyHost`, `NoVerifyServerName` by default leads to
hard-to-debug bugs, it should always be explicit if wanted.
Note: This is also a workaround for status-im/nim-chronos#313
@Menduist
Copy link
Contributor

Seems to be a bearssl limitation:
https://github.com/status-im/BearSSL/blob/acc70b1be60a6f321e2da618cd35d901b1a598a4/src/x509/x509_minimal.t0#L750-L756

The presence of "server_name" is used as a flag of "verify server name", so we can't send the server_name without verifying it

etan-status added a commit to status-im/nim-json-rpc that referenced this issue Sep 16, 2022
Setting `NoVerifyHost`, `NoVerifyServerName` by default leads to
hard-to-debug bugs, it should always be explicit if wanted.
Note: This is also a workaround for status-im/nim-chronos#313
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

No branches or pull requests

2 participants