-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[App Service] Fix #30357: az webapp config ssl bind: Use full Site object to avoid Azure Policy denial
#33061
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |||||||||||||
| view_in_browser, | ||||||||||||||
| sync_site_repo, | ||||||||||||||
| _match_host_names_from_cert, | ||||||||||||||
| _update_host_name_ssl_state, | ||||||||||||||
| bind_ssl_cert, | ||||||||||||||
| list_publish_profiles, | ||||||||||||||
| show_app, | ||||||||||||||
|
|
@@ -639,6 +640,59 @@ def test_update_webapp_platform_release_channel_latest(self): | |||||||||||||
| self.assertEqual(result.additional_properties["properties"]["platformReleaseChannel"], "Latest") | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| class TestUpdateHostNameSslState(unittest.TestCase): | ||||||||||||||
| @mock.patch('azure.cli.command_modules.appservice.custom._generic_site_operation', autospec=True) | ||||||||||||||
| def test_update_host_name_ssl_state_passes_full_site(self, generic_site_op_mock): | ||||||||||||||
| """Test that _update_host_name_ssl_state passes the full Site object (not a partial one) | ||||||||||||||
| to begin_create_or_update, preserving policy-sensitive fields like https_only.""" | ||||||||||||||
| cmd_mock = _get_test_cmd() | ||||||||||||||
| Site, HostNameSslState, SslState = cmd_mock.get_models('Site', 'HostNameSslState', 'SslState') | ||||||||||||||
|
|
||||||||||||||
| webapp = Site(name='mySite', location='eastus', tags={'env': 'prod'}) | ||||||||||||||
| webapp.https_only = True | ||||||||||||||
| webapp.host_name_ssl_states = [ | ||||||||||||||
| HostNameSslState(name='existing.contoso.com', | ||||||||||||||
| ssl_state=SslState.sni_enabled, | ||||||||||||||
| thumbprint='EXISTINGTHUMB') | ||||||||||||||
| ] | ||||||||||||||
|
|
||||||||||||||
| _update_host_name_ssl_state(cmd_mock, 'myRg', 'mySite', webapp, | ||||||||||||||
| 'www.contoso.com', SslState.sni_enabled, 'NEWTHUMB') | ||||||||||||||
|
|
||||||||||||||
| generic_site_op_mock.assert_called_once() | ||||||||||||||
| call_args = generic_site_op_mock.call_args | ||||||||||||||
| passed_site = call_args[0][5] # (cli_ctx, rg, name, op, slot, site) | ||||||||||||||
|
|
||||||||||||||
| # The passed object should be the original webapp, preserving all fields | ||||||||||||||
| self.assertTrue(passed_site.https_only, | ||||||||||||||
| "https_only must be preserved to avoid Azure Policy denial") | ||||||||||||||
| self.assertEqual(passed_site.location, 'eastus') | ||||||||||||||
| self.assertEqual(passed_site.tags, {'env': 'prod'}) | ||||||||||||||
|
|
||||||||||||||
| # host_name_ssl_states should contain only the binding being updated | ||||||||||||||
| self.assertEqual(len(passed_site.host_name_ssl_states), 1) | ||||||||||||||
| ssl_state = passed_site.host_name_ssl_states[0] | ||||||||||||||
| self.assertEqual(ssl_state.name, 'www.contoso.com') | ||||||||||||||
| self.assertEqual(ssl_state.thumbprint, 'NEWTHUMB') | ||||||||||||||
| self.assertTrue(ssl_state.to_update) | ||||||||||||||
|
|
||||||||||||||
| @mock.patch('azure.cli.command_modules.appservice.custom._generic_site_operation', autospec=True) | ||||||||||||||
| def test_update_host_name_ssl_state_with_slot(self, generic_site_op_mock): | ||||||||||||||
| """Test that slot parameter is correctly forwarded.""" | ||||||||||||||
| cmd_mock = _get_test_cmd() | ||||||||||||||
| Site, SslState = cmd_mock.get_models('Site', 'SslState') | ||||||||||||||
| webapp = Site(name='mySite', location='westus') | ||||||||||||||
|
|
||||||||||||||
| _update_host_name_ssl_state(cmd_mock, 'myRg', 'mySite', webapp, | ||||||||||||||
| 'www.contoso.com', SslState.sni_enabled, 'THUMB', slot='staging') | ||||||||||||||
|
|
||||||||||||||
| call_args = generic_site_op_mock.call_args | ||||||||||||||
| # slot is the 5th positional arg (index 4) after cli_ctx, rg, name, operation_name | ||||||||||||||
| self.assertEqual(call_args[0][4], 'staging') | ||||||||||||||
| # site is the 6th positional arg (index 5) | ||||||||||||||
| self.assertIs(call_args[0][5], webapp) | ||||||||||||||
|
||||||||||||||
| # site is the 6th positional arg (index 5) | |
| self.assertIs(call_args[0][5], webapp) | |
| # site is the 6th positional arg (index 5); verify it has the expected properties | |
| site_arg = call_args[0][5] | |
| self.assertEqual(site_arg.name, webapp.name) | |
| self.assertEqual(site_arg.location, webapp.location) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
slotis provided, this helper assumes the passedwebappobject represents that slot. In the current call path (_update_ssl_bindingfetchesclient.web_apps.get(...)regardless ofslot), this can cause a slot PUT (begin_create_or_update_slot) to reuse the production Site payload (including policy-sensitive fields likehttps_only), potentially triggering policy denial or unintentionally overwriting slot-specific settings. Consider fetching the Site for the slot (client.web_apps.get_slot(...)or_generic_site_operation(..., 'get', slot)) and using that object for the update whenslotis not None.