Repository navigation
Conversation
|
@microsoft-github-policy-service agree |
HarshShroff
left a comment
There was a problem hiding this comment.
Checked this out at 4f10937. With the api extras installed, tests/models/test_api_tls.py gives 8 passed on the branch. If I swap in main's api_models.py, it's 2 failed / 6 passed, and the failures are exactly the two custom-CA cases (sync and async), which lines up with the description.
One gap I hit: ca_cert_path pointing at a CA directory (a hashed dir, the kind openssl rehash / c_rehash produces) works in sync mode, but async crashes. requests accepts a directory for verify=, so num_concurrent=1 goes through fine. ssl.create_default_context(cafile=...) only takes a file, so num_concurrent>1 raises IsADirectoryError: [Errno 21] Is a directory before it sends anything. The remote tokenizer in utils.py passes the same value to requests as well, so a directory works there too. That means the same model args can work or crash depending on num_concurrent.
I ran this against a local HTTPS server with a self-signed CA, comparing the PEM file with a rehashed directory holding the same cert, for both LocalCompletionsAPI and LocalChatCompletion:
| ca_cert_path | sync | async |
|---|---|---|
| PEM file | ok | ok |
| CA directory | ok | IsADirectoryError |
Picking capath for a directory fixed it, and all 8 cells pass:
if self.verify_certificate and self.ca_cert_path:
if os.path.isdir(self.ca_cert_path):
ssl_config = ssl.create_default_context(capath=self.ca_cert_path)
else:
ssl_config = ssl.create_default_context(cafile=self.ca_cert_path)
else:
ssl_config = self.verify_certificateWith that change the 8 TLS tests still pass. If directories aren't meant to be supported (the new docs line does say PEM bundle), a clear error for a directory up front would also work, so sync and async at least behave the same way.
TemplateAPIacceptsca_cert_pathand passes it to a remote tokenizer, but its inference transports ignore that path. A tokenizer can therefore connect successfully to a service using a private CA while the actual completions request fails with a certificate verification error. Both the synchronous and concurrent inference paths are affected.Honor the configured CA path in
requests.postand create an SSL context for the async connector. Support both PEM bundles and OpenSSL-hashed CA directories (cafileversuscapath), matching the synchronous transport. Keep the existing default verification and explicitly disabled verification behavior. Document the option in the API guide.The regression runs the public
generate_untilpath against a temporary local HTTPS server, with a freshly generated self-signed certificate. It covers bothLocalCompletionsAPIandLocalChatCompletion, sync and async requests, PEM-file and hashed-directory trust, default rejection of the untrusted certificate, hostname-mismatch rejection with either custom trust source, and explicitly disabled verification with an unused nonexistent path. It generates certificates usingopensslat test time (skips if unavailable), commits no private keys, and needs no model API or external server.The CA-directory case was reported and independently reproduced in HarshShroff's review. Before the follow-up, the expanded matrix had 4 failed, 20 passed, with only async directory cases raising
IsADirectoryErrorinstead of succeeding or checking the hostname. The follow-up usescapathfor directories and extends the real HTTPS regressions accordingly.Validation:
test_model_tokenized_call_usage,test_get_batched_requests_with_no_ssl).git diff --checkpassed.AI assistance: Codex was used to inspect the transports, reproduce the issue and review feedback, implement the fix and run the local checks during a user-authorized contribution search.