Repository navigation
Implement SendX5C flag for certificate authentication - #576
Rodrigo Brandão (rodrigobr-msft) merged 7 commits into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The implementation and validation are consistent with the stated requirements and include focused automated tests for the newly supported auth type.
Pull request overview
Adds full support for the existing AuthTypes.certificate_subject_name mode by validating configuration and wiring the correct MSAL client_credential options so Certificate Subject Name authentication behaves as expected in MsalAuth.
Changes:
- Extend
AgentAuthConfigurationvalidation to requireCERT_PFX_FILEforcertificate_subject_nameauth. - Update
MsalAuth._create_client_application()to passpublic_certificate=Trueforcertificate_subject_namecredentials. - Add targeted tests covering both configuration validation and MSAL credential creation for the new auth type.
File summaries
| File | Description |
|---|---|
tests/hosting_core/test_auth_configuration.py |
Adds validation test cases ensuring CERT_PFX_FILE is required/accepted for certificate_subject_name. |
tests/authentication_msal/test_msal_auth.py |
Adds an MSAL client creation test asserting public_certificate=True is set for certificate_subject_name. |
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/authorization/agent_auth_configuration.py |
Updates config validation logic to include certificate_subject_name in the certificate PFX requirement. |
libraries/microsoft-agents-authentication-msal/microsoft_agents/authentication/msal/msal_auth.py |
Implements MSAL credential construction for certificate_subject_name using PFX path + public_certificate=True. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@microsoft-github-policy-service agree |
|
This is good, but I believe this code implements the behavior for a SendX5C flag rather than the CertificateSubjectName auth type. |
|
Thanks Rodrigo Brandão (@rodrigobr-msft), For |
Yes, and like in the .NET implementation, that would introduce a new configuration setting |
There was a problem hiding this comment.
🟡 Changes recommended
CertificateSubjectName remains unsupported in MSAL creation, with required validation and test coverage incomplete.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/authorization/agent_auth_configuration.py:160
- Validation still ignores
AuthTypes.certificate_subject_name:_validate()only checksAuthTypes.certificate(agent_auth_configuration.py:240-243). A subject-name configuration with noCERT_PFX_FILEtherefore succeeds instead of raising the required configuration error. Include both certificate enum values in the PFX check.
self.SEND_X5C = coerce_bool(
send_x5c if send_x5c is not None else kwargs.get("SENDX5C", False),
default=False,
name="SENDX5C",
)
tests/authentication_msal/test_msal_auth.py:293
- These tests exercise
AuthTypes.certificate, notAuthTypes.certificate_subject_name, so they pass while the auth type named in the PR remains unimplemented. Add a subject-name case covering missingCERT_PFX_FILEand the MSAL credential'spublic_certificate=Truesetting.
def test_create_client_application_send_x5c(self, mocker):
config = AgentAuthConfiguration(
auth_type=AuthTypes.certificate,
tenant_id="12345678-1234-1234-1234-123456789abc",
client_id="test-client-id",
cert_pfx_file="test-cert.pfx",
send_x5c=True,
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
|
Thanks Rodrigo Brandão (@rodrigobr-msft), I’ve updated this PR to add the new |
There was a problem hiding this comment.
🟡 Changes recommended
A critical backward-compatibility issue changes positional constructor argument binding and must be fixed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
|
Thanks for the contribution! |
There was a problem hiding this comment.
🔵 Needs a closer look
Align the PR description and acceptance criteria with the implemented authentication behavior.
Review details
Suppressed comments (1)
libraries/microsoft-agents-authentication-msal/microsoft_agents/authentication/msal/msal_auth.py:310
- The PR description still says this change resolves #348 by implementing
CertificateSubjectName, but this branch only handlesAuthTypes.certificate;AuthTypes.certificate_subject_namestill reaches the unsupported-auth path. Since the stated scope is now the SendX5C flag, please update the description/remove that issue resolution, or add the subject-name handling so the acceptance criteria match the implementation.
if self._msal_configuration.SEND_X5C:
client_credential["public_certificate"] = True
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
c4c297c
into
microsoft:main
Resolves #348 by implementing
CertificateSubjectNameauthentication support.Issue:
CertificateSubjectNameis already defined inAuthTypes, but was not handled byMsalAuth.Solution:
CERT_PFX_FILEis provided when usingCertificateSubjectNamepublic_certificate=TrueCorresponding tests were implemented in:
tests/hosting_core/test_auth_configuration.pytests/authentication_msal/test_msal_auth.pyFull test suite passed:
3573 passed, 417 skipped, 0 failed in 57.26s