feat: add default true for azure sdk OPENSSL_DISABLE_CRL_CHECK

[YiminW]

No description provided.

[github-actions]

PR Review: azure_asr_python — default OPENSSL_DISABLE_CRL_CHECK

Thanks for the focused change. The version bump (0.2.7 → 0.2.8) is an appropriate patch bump, and the placement is right: the new set_property_by_name call sits before the advanced_params_json loop, so a user can still override it (e.g. set OPENSSL_DISABLE_CRL_CHECK to "false" in advanced params). 👍

A few things worth considering before merge:

Security — flagging the tradeoff

Disabling the CRL (Certificate Revocation List) check by default weakens TLS verification for every user of this extension: a revoked server certificate would no longer be rejected. That's a real, if small, downgrade in the security posture, applied silently to everyone rather than opt-in.

The usual motivation here is that CRL endpoints are sometimes unreachable behind restrictive firewalls/proxies, causing the Azure SDK handshake to hang or fail. That's a legitimate reliability concern, but it's worth being deliberate about defaulting off a security check for all users. A couple of options:

• Keep it default-on (as here) but document the rationale and the security implication so it's a conscious choice.
• Or expose it as a named config field (default true if you prefer) so the intent is explicit and discoverable, rather than a hardcoded property a user has to know to override via advanced_params_json.

Code clarity — add a comment

The neighboring set_property calls each have an explanatory comment (silence timeout, SDK log dump). This new line has none. Since why you'd disable CRL checking is non-obvious — and it's a security-relevant default — a one-line comment explaining the reason (e.g. avoiding handshake failures when CRL endpoints are blocked) would help future readers and reviewers.

# Disable CRL revocation check to avoid TLS handshake failures when
# the CRL distribution endpoints are unreachable (e.g. behind a proxy).
# Can be overridden via advanced_params_json.
speech_config.set_property_by_name("OPENSSL_DISABLE_CRL_CHECK", "true")

Minor

• The string literal "OPENSSL_DISABLE_CRL_CHECK" is a bare magic string. Other Azure params in this file go through speechsdk.PropertyId enums; since this one is SDK-internal and has no enum, a module-level constant in const.py would be consistent with how the extension factors out other constants — optional, but tidy.
• No tests accompany the change. A config-property set like this is hard to assert meaningfully without an Azure connection, so this is understandable; just noting it for completeness.

Overall the change is small and correct mechanically. The main ask is to make the security tradeoff explicit — via a comment and/or a named config field — rather than a silent default.

[github-actions]

PR Review

Thanks for the PR. I reviewed the full diff and the azure_asr_python extension, and I think this PR is incomplete as it stands.

Main issue: the feature code is missing

The PR title is feat: add default true for azure sdk OPENSSL_DISABLE_CRL_CHECK, but the entire diff is just a version bump:

-  "version": "0.2.7",
+  "version": "0.2.9",

There is no code that sets OPENSSL_DISABLE_CRL_CHECK. I searched the whole extension (extension.py, config.py, const.py) and the env var does not appear anywhere. So a build from this branch would not change Azure SDK CRL behavior at all, it only publishes a new version number for a behavior change that is not there.

To deliver what the title promises, the actual change is needed. OPENSSL_DISABLE_CRL_CHECK is read by the Azure Speech SDK native networking layer from the process environment, so it has to be set before the SDK opens its connection. The natural place is early in start_connection() (or on_init()), e.g.:

import os
# Default to disabling CRL checks for the Azure Speech SDK; allow opt-out via config.
os.environ.setdefault("OPENSSL_DISABLE_CRL_CHECK", "1")

A few design points worth deciding before merging:

• Make it configurable, do not hardcode. Disabling CRL checks weakens certificate revocation validation for the whole process, which is a security-relevant change. I would add a config field (e.g. disable_crl_check: bool = True in AzureASRConfig) and only set the env var based on that, so operators in stricter environments can turn it back on. Please also call out the security tradeoff in the extension docs.
• Process-global side effect. os.environ is shared by the whole process, so this affects any other extension in the same runtime that uses OpenSSL. Using setdefault (rather than unconditional assignment) at least avoids clobbering a value the operator set deliberately. Worth a comment explaining the scope.

Minor: version jump

The bump goes 0.2.7 -> 0.2.9, skipping 0.2.8. If that is intentional (a 0.2.8 published elsewhere) it is fine; otherwise 0.2.8 is the expected next patch. Also, once real behavior is added, a feature would conventionally land as a minor bump (0.3.0) rather than a patch.

Test coverage

No tests are included. Since this changes connection-time behavior, a small test asserting the env var is set (and that the config flag toggles it) would help, the extension already has a tests/ directory to extend.

Summary

The version bump is harmless on its own, but as submitted the PR does not implement the titled feature. I would hold merge until the env-var-setting code (ideally behind a config flag) is added, then re-confirm the version bump matches. Happy to look again once the implementation is in.

[github-actions]

PR Review: add default true for azure sdk OPENSSL_DISABLE_CRL_CHECK

Small, focused change. A couple of points worth considering before merge.

Security — the main one to weigh

OPENSSL_DISABLE_CRL_CHECK=true disables Certificate Revocation List checking for the Azure SDK's TLS connections. CRL checks are what let a client reject a certificate that has been revoked (e.g. after a key compromise). Turning this off by default for every deployment is a real, if modest, reduction in TLS security posture.

This is a common workaround — CRL endpoints are sometimes unreachable and cause connection hangs/timeouts — so the intent is understandable. But the tradeoff is being applied to all users unconditionally. Two suggestions:

• Consider making it opt-in via config rather than a hardcoded default, or at least document why it's on by default so operators understand the implication.
• A brief code comment explaining why CRL is disabled (the connectivity issue it works around) would help future maintainers, similar to the comment on the silence-timeout line just above it.

Since it's set before the advanced_params_json loop (lines 156-169), a user can override it by passing OPENSSL_DISABLE_CRL_CHECK in advanced params — that ordering is good. Worth confirming that's the intended escape hatch.

Version bump

manifest.json jumps from 0.2.7 to 0.2.10, skipping 0.2.8 and 0.2.9. For a single-line feature addition I'd have expected 0.2.8. If the intermediate versions were used elsewhere that's fine, otherwise this looks like it may be unintentional.

Test coverage

No test accompanies the change. A small assertion that the property is set on speech_config during start_connection would lock in the behavior and document the intent, if the extension's test setup makes that practical.

Overall the change is reasonable and the placement is correct. The security default is the one thing I'd want an explicit decision on rather than an implicit default.

[github-actions]

PR Review — azure_asr_python

Summary

The PR title advertises a feature (add default true for azure sdk OPENSSL_DISABLE_CRL_CHECK), but the net diff against main is only a version bump in manifest.json (0.2.7 → 0.2.11). The feature code was added in commit 82416a1 ("add disable ssl for azure asr adk") and then reverted in ff5cc81 ("restore azure asr change"). The result is a PR whose contents no longer match its title or stated intent.

Main concern: title/intent vs. actual diff

The shipped change does not implement what the title and commits describe. Two ways to read this:

1. The revert was intentional and only the version bump is meant to ship. In that case the PR title and description should be updated to reflect that (e.g. chore(azure_asr_python): bump version to 0.2.11), and ideally the body should explain why the version is bumped without an accompanying code change.
2. The revert was a mistake and the SSL/CRL feature was supposed to remain. In that case the feature code needs to be restored before merge.

Could you clarify which case applies? As it stands, the diff and the title disagree, which will be confusing in the history and changelog.

Version bump

• The version jumps 0.2.7 → 0.2.11, skipping .8, .9, .10. If those intermediate versions weren't released elsewhere, a single-step bump (0.2.8) is more conventional for a no-op/maintenance change. If they correspond to the intermediate commits, that's fine — worth a one-line note in the PR description.
• A bare version bump with no functional change is unusual. If the intent is to trigger a republish of the package, a short rationale in the PR body would help reviewers and future archaeologists.

If the OPENSSL_DISABLE_CRL_CHECK feature is meant to ship

Since the feature touches TLS certificate validation, a few things to keep in mind when it's reintroduced:

• Security: OPENSSL_DISABLE_CRL_CHECK disables certificate revocation list checking, which weakens TLS verification. Defaulting it to true removes a security control for all users of this extension by default. Please confirm this is genuinely required (some networks/proxies do break CRL fetching) and consider defaulting to false with an opt-in config flag instead, plus a comment explaining the tradeoff. At minimum this should be configurable via AzureASRConfig rather than hardcoded.
• Tests: the new behavior should be covered. The extension already has a solid test suite under tests/ (reconnect, finalize, invalid params, vendor errors), so a test asserting the SDK property is set as expected would fit the existing pattern.
• Config surface: a new toggle should be added to config.py (AzureASRConfig) and documented, consistent with how finalize_mode, dump, etc. are exposed.

Code quality / other notes

• The two feature commit messages use feat: but one mixes "adk" (likely a typo for "sdk"). Per the repo's conventional-commit guidance in AGENTS.md, the final squashed commit should have a message that matches what actually ships.
• The PR description is empty. A short summary of what changed and what was tested would help, per the repo conventions.

Verdict

No bugs in the shipped diff (it's a one-line version string change), but the PR should not merge as-is until the title/description are reconciled with the actual contents — either correct the metadata for a maintenance bump, or restore the intended feature with config + tests + a security rationale for the default.