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

Download certificates when goal state source is Fast Track #2761

Merged
merged 22 commits into from
Mar 7, 2023
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
da72c37
Update version to dummy 1.0.0.0'
maddieford Nov 8, 2022
59dbd22
Revert version change
maddieford Nov 8, 2022
633a826
Merge remote-tracking branch 'upstream/develop' into develop
maddieford Nov 21, 2022
14a743f
Merge remote-tracking branch 'upstream/develop' into develop
maddieford Dec 8, 2022
54ea0f3
Merge remote-tracking branch 'upstream/develop' into develop
maddieford Jan 10, 2023
e79c4c5
Merge remote-tracking branch 'upstream/develop' into develop
maddieford Feb 8, 2023
498b612
Merge remote-tracking branch 'upstream/develop' into develop
maddieford Feb 14, 2023
4365b87
Download certificate in case of ft gs source
maddieford Feb 14, 2023
4783956
Update unit test after separating extensionsconfig and certificates
maddieford Feb 14, 2023
7ddfa95
Update unit tests so that certificates are downloaded in all cases
maddieford Feb 14, 2023
a8534b9
Update unit test message
maddieford Feb 14, 2023
f5c20e6
Add unit tests for downloading certs
maddieford Feb 27, 2023
81683eb
Update goal state before checking for cert
maddieford Feb 27, 2023
f8b6af0
Update unit test names
maddieford Feb 28, 2023
cefc941
Update mock to check for etag before updating
maddieford Feb 28, 2023
0252c3f
Update protected settings test
maddieford Mar 1, 2023
a93d09b
Update failing unit tests after mock updatE
maddieford Mar 1, 2023
f74b629
Update failing unit test
maddieford Mar 1, 2023
be36724
Make cert re-download more readable
maddieford Mar 7, 2023
7c13ded
Make certs_uri a data member
maddieford Mar 7, 2023
93ab957
Update bitwise check for consistency
maddieford Mar 7, 2023
3f39db4
Merge branch 'develop' into download_cert
maddieford Mar 7, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 26 additions & 15 deletions azurelinuxagent/common/protocol/goal_state.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,9 @@ class GoalStateProperties(object):
HostingEnv = 0x2
SharedConfig = 0x4
ExtensionsGoalState = 0x8
RemoteAccessInfo = 0x10
All = RoleConfig | HostingEnv | SharedConfig | ExtensionsGoalState | RemoteAccessInfo
Certificates = 0x10
RemoteAccessInfo = 0x20
All = RoleConfig | HostingEnv | SharedConfig | ExtensionsGoalState | Certificates | RemoteAccessInfo


class GoalStateInconsistentError(ProtocolError):
Expand Down Expand Up @@ -96,6 +97,7 @@ def __init__(self, wire_client, goal_state_properties=GoalStateProperties.All, s
self._hosting_env = None
self._shared_conf = None
self._certs = EmptyCertificates()
self._certs_uri = None
self._remote_access = None

self.update(silent=silent)
Expand Down Expand Up @@ -140,7 +142,7 @@ def extensions_goal_state(self):

@property
def certs(self):
if not self._goal_state_properties & GoalStateProperties.ExtensionsGoalState:
if not self._goal_state_properties & GoalStateProperties.Certificates:
raise ProtocolError("Certificates is not in goal state properties")
else:
return self._certs
Expand Down Expand Up @@ -292,6 +294,10 @@ def _update(self, force_update):
self._check_certificates()

def _check_certificates(self):
# Re-download certificates in case they have been removed from disk since last download
if self._goal_state_properties & GoalStateProperties.Certificates and self._certs_uri is not None:
self._download_certificates(self._certs_uri)
# Check that certificates needed by extensions are in goal state certs.summary
for extension in self.extensions_goal_state.extensions:
for settings in extension.settings:
if settings.protectedSettings is None:
Expand All @@ -301,6 +307,20 @@ def _check_certificates(self):
message = "Certificate {0} needed by {1} is missing from the goal state".format(settings.certificateThumbprint, extension.name)
raise GoalStateInconsistentError(message)

def _download_certificates(self, certs_uri):
xml_text = self._wire_client.fetch_config(certs_uri, self._wire_client.get_header_for_cert())
certs = Certificates(xml_text, self.logger)
# Log and save the certificates summary (i.e. the thumbprint but not the certificate itself) to the goal state history
for c in certs.summary:
message = "Downloaded certificate {0}".format(c)
self.logger.info(message)
add_event(op=WALAEventOperation.GoalState, message=message)
if len(certs.warnings) > 0:
self.logger.warn(certs.warnings)
add_event(op=WALAEventOperation.GoalState, message=certs.warnings)
self._history.save_certificates(json.dumps(certs.summary))
return certs

def _restore_wire_server_goal_state(self, incarnation, xml_text, xml_doc, vm_settings_support_stopped_error):
msg = 'The HGAP stopped supporting vmSettings; will fetched the goal state from the WireServer.'
self.logger.info(msg)
Expand Down Expand Up @@ -435,18 +455,8 @@ def _fetch_full_wire_server_goal_state(self, incarnation, xml_doc):

certs = EmptyCertificates()
certs_uri = findtext(xml_doc, "Certificates")
if (GoalStateProperties.ExtensionsGoalState & self._goal_state_properties) and certs_uri is not None:
xml_text = self._wire_client.fetch_config(certs_uri, self._wire_client.get_header_for_cert())
certs = Certificates(xml_text, self.logger)
# Log and save the certificates summary (i.e. the thumbprint but not the certificate itself) to the goal state history
for c in certs.summary:
message = "Downloaded certificate {0}".format(c)
self.logger.info(message)
add_event(op=WALAEventOperation.GoalState, message=message)
if len(certs.warnings) > 0:
self.logger.warn(certs.warnings)
add_event(op=WALAEventOperation.GoalState, message=certs.warnings)
self._history.save_certificates(json.dumps(certs.summary))
if (GoalStateProperties.Certificates & self._goal_state_properties) and certs_uri is not None:
narrieta marked this conversation as resolved.
Show resolved Hide resolved
certs = self._download_certificates(certs_uri)

remote_access = None
if GoalStateProperties.RemoteAccessInfo & self._goal_state_properties:
Expand All @@ -463,6 +473,7 @@ def _fetch_full_wire_server_goal_state(self, incarnation, xml_doc):
self._hosting_env = hosting_env
self._shared_conf = shared_config
self._certs = certs
self._certs_uri = certs_uri
self._remote_access = remote_access

return extensions_config
Expand Down
4 changes: 4 additions & 0 deletions tests/protocol/mockwiredata.py
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ def __init__(self, data_files=None):
self.in_vm_artifacts_profile = None
self.vm_settings = None
self.etag = None
self.prev_etag = None
self.imds_info = None

self.reload()
Expand Down Expand Up @@ -242,9 +243,12 @@ def mock_http_get(self, url, *_, **kwargs):
elif "/vmSettings" in url:
if self.vm_settings is None:
resp.status = httpclient.NOT_FOUND
elif self.call_counts["vm_settings"] > 0 and self.prev_etag == self.etag:
resp.status = httpclient.NOT_MODIFIED
else:
content = self.vm_settings
response_headers = [('ETag', self.etag)]
self.prev_etag = self.etag
self.call_counts["vm_settings"] += 1
elif '{0}/metadata/compute'.format(IMDS_ENDPOINT) in url:
content = json.dumps(self.imds_info.get("compute", "{}"))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ def test_it_should_parse_requested_version_properly(self):
data_file = mockwiredata.DATA_FILE_VM_SETTINGS.copy()
data_file["vm_settings"] = "hostgaplugin/vm_settings-requested_version.json"
with mock_wire_protocol(data_file) as protocol:
protocol.mock_wire_data.set_etag(888)
goal_state = GoalState(protocol.client)
families = goal_state.extensions_goal_state.agent_families
for family in families:
Expand Down
60 changes: 56 additions & 4 deletions tests/protocol/test_goal_state.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
class GoalStateTestCase(AgentTestCase, HttpRequestPredicates):
def test_it_should_use_vm_settings_by_default(self):
with mock_wire_protocol(mockwiredata.DATA_FILE_VM_SETTINGS) as protocol:
protocol.mock_wire_data.set_etag(888)
extensions_goal_state = GoalState(protocol.client).extensions_goal_state
self.assertTrue(
isinstance(extensions_goal_state, ExtensionsGoalStateFromVmSettings),
Expand Down Expand Up @@ -156,11 +157,12 @@ def http_get_handler(url, *_, **__):
protocol.set_http_handlers(http_get_handler=None)
goal_state.update()
self._assert_directory_contents(
self._find_history_subdirectory("234-987"), ["VmSettings.json"])
self._find_history_subdirectory("234-987"), ["VmSettings.json", "Certificates.json"])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test shows that we create history subdirectory with Certificates.json when we have a fast track goal state. Should I add an additional test to verify the certificates are downloaded?


def test_it_should_redact_the_protected_settings_when_saving_to_the_history_directory(self):
with mock_wire_protocol(mockwiredata.DATA_FILE_VM_SETTINGS) as protocol:
protocol.mock_wire_data.set_incarnation(888)
protocol.mock_wire_data.set_etag(888)

goal_state = GoalState(protocol.client)

Expand All @@ -173,7 +175,7 @@ def test_it_should_redact_the_protected_settings_when_saving_to_the_history_dire
if len(protected_settings) == 0:
raise Exception("The test goal state does not include any protected settings")

history_directory = self._find_history_subdirectory("888-1")
history_directory = self._find_history_subdirectory("888-888")
extensions_config_file = os.path.join(history_directory, "ExtensionsConfig.xml")
vm_settings_file = os.path.join(history_directory, "VmSettings.json")
for file_name in extensions_config_file, vm_settings_file:
Expand All @@ -198,14 +200,14 @@ def test_it_should_save_vm_settings_on_parse_errors(self):
data_file = mockwiredata.DATA_FILE_VM_SETTINGS.copy()
data_file["vm_settings"] = invalid_vm_settings_file
protocol.mock_wire_data = mockwiredata.WireProtocolData(data_file)
protocol.mock_wire_data.set_etag(888)

with self.assertRaises(ProtocolError): # the parsing error will cause an exception
_ = GoalState(protocol.client)

# Do an extra call to update the goal state; this should save the vmsettings to the history directory
# only once (self._find_history_subdirectory asserts 1 single match)
time.sleep(0.1) # add a short delay to ensure that a new timestamp would be saved in the history folder
protocol.mock_wire_data.set_etag(888)
with self.assertRaises(ProtocolError):
_ = GoalState(protocol.client)

Expand Down Expand Up @@ -375,13 +377,63 @@ def test_it_should_raise_when_the_tenant_certificate_is_missing(self):
with mock_wire_protocol(data_file) as protocol:
data_file["vm_settings"] = "hostgaplugin/vm_settings-missing_cert.json"
protocol.mock_wire_data.reload()
protocol.mock_wire_data.set_etag(888)

with self.assertRaises(GoalStateInconsistentError) as context:
_ = GoalState(protocol.client)

expected_message = "Certificate 59A10F50FFE2A0408D3F03FE336C8FD5716CF25C needed by Microsoft.OSTCExtensions.VMAccessForLinux is missing from the goal state"
self.assertIn(expected_message, str(context.exception))

def test_it_should_download_certs_on_a_new_fast_track_goal_state(self):
data_file = mockwiredata.DATA_FILE_VM_SETTINGS.copy()

with mock_wire_protocol(data_file) as protocol:
goal_state = GoalState(protocol.client)

cert = "BD447EF71C3ADDF7C837E84D630F3FAC22CCD22F"
crt_path = os.path.join(self.tmp_dir, cert + ".crt")
prv_path = os.path.join(self.tmp_dir, cert + ".prv")

# Check that crt and prv files are downloaded after processing goal state
self.assertTrue(os.path.isfile(crt_path))
self.assertTrue(os.path.isfile(prv_path))

# Remove .crt file
os.remove(crt_path)
if os.path.isfile(crt_path):
raise Exception("{0}.crt was not removed.".format(cert))

# Update goal state and check that .crt was downloaded
protocol.mock_wire_data.set_etag(888)
goal_state.update()
self.assertTrue(os.path.isfile(crt_path))

def test_it_should_download_certs_on_a_new_fabric_goal_state(self):
data_file = mockwiredata.DATA_FILE_VM_SETTINGS.copy()

with mock_wire_protocol(data_file) as protocol:
protocol.mock_wire_data.set_vm_settings_source(GoalStateSource.Fabric)
goal_state = GoalState(protocol.client)

cert = "BD447EF71C3ADDF7C837E84D630F3FAC22CCD22F"
crt_path = os.path.join(self.tmp_dir, cert + ".crt")
prv_path = os.path.join(self.tmp_dir, cert + ".prv")

# Check that crt and prv files are downloaded after processing goal state
self.assertTrue(os.path.isfile(crt_path))
self.assertTrue(os.path.isfile(prv_path))

# Remove .crt file
os.remove(crt_path)
if os.path.isfile(crt_path):
raise Exception("{0}.crt was not removed.".format(cert))

# Update goal state and check that .crt was downloaded
protocol.mock_wire_data.set_incarnation(999)
goal_state.update()
self.assertTrue(os.path.isfile(crt_path))

def test_it_should_refresh_the_goal_state_when_it_is_inconsistent(self):
#
# Some scenarios can produce inconsistent goal states. For example, during hibernation/resume, the Fabric goal state changes (the
Expand Down Expand Up @@ -412,7 +464,7 @@ def http_get_handler(url, *_, **__):
goal_state = GoalState(protocol.client)

self.assertEqual(2, protocol.mock_wire_data.call_counts['goalstate'], "There should have been exactly 2 requests for the goal state (original + refresh)")
self.assertEqual(2, http_get_handler.certificate_requests, "There should have been exactly 2 requests for the goal state certificates (original + refresh)")
self.assertEqual(4, http_get_handler.certificate_requests, "There should have been exactly 4 requests for the goal state certificates (2x original + 2x refresh)")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous 2 requests became 4 requests for this case because we're downloading certificates in _fetch_full_wire_server_goal_state and _update directly.

Should I add logic to only download certificates once per goals state?


thumbprints = [c.thumbprint for c in goal_state.certs.cert_list.certificates]

Expand Down
2 changes: 1 addition & 1 deletion tests/protocol/test_hostplugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -998,7 +998,7 @@ def test_it_should_save_the_timestamp_of_the_most_recent_fast_track_goal_state(s

# A fabric goal state should remove the state file
protocol.mock_wire_data.set_vm_settings_source(GoalStateSource.Fabric)

protocol.mock_wire_data.set_etag(888)
_ = host_ga_plugin.fetch_vm_settings()

self.assertFalse(os.path.exists(state_file), "{0} was not removed by a Fabric goal state".format(state_file))
Expand Down
2 changes: 1 addition & 1 deletion tests/protocol/test_wire.py
Original file line number Diff line number Diff line change
Expand Up @@ -1101,7 +1101,7 @@ def test_forced_update_should_update_the_goal_state_and_the_host_plugin_when_the

def test_reset_should_init_provided_goal_state_properties(self):
with mock_wire_protocol(mockwiredata.DATA_FILE) as protocol:
protocol.client.reset_goal_state(goal_state_properties=GoalStateProperties.All & ~GoalStateProperties.ExtensionsGoalState)
protocol.client.reset_goal_state(goal_state_properties=GoalStateProperties.All & ~GoalStateProperties.Certificates)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this test after separating ExtensionsGoalState and Certificates in GoalStateProperties.


with self.assertRaises(ProtocolError) as context:
_ = protocol.client.get_certs()
Expand Down