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

sources/azure: validate IMDS network configuration metadata #1257

Merged
merged 1 commit into from
Feb 14, 2022

Conversation

cjp256
Copy link
Contributor

@cjp256 cjp256 commented Feb 10, 2022

Due to race conditions and caching, IMDS may return stale or incomplete
metadata. Add some validation to detect these scenarios and report
appropriate telemetry.

Signed-off-by: Chris Patterson cpatterson@microsoft.com

@cjp256 cjp256 force-pushed the azure-log-missing-metadata branch 3 times, most recently from 378e256 to c34f236 Compare February 10, 2022 18:49
@@ -178,6 +178,26 @@ def find_dev_from_busdev(camcontrol_out, busdev):
return None


def normalize_mac_address(mac: str):
"""Normalize mac address with colons and lower-case."""
Copy link
Member

Choose a reason for hiding this comment

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

What "non-colon" formats is this expected to normalize? Just curious since I don't see any new tests in this PR that appear to make use of this. This would obviously fail the format 'HHHH.HHHH.HHHH' that some network gear vendors use, but I assume that's out of scope for the inputs that this is supposed to normalize.

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 haven't added tests yet, this was just a draft for early review :D

I expect get_interfaces() will always be in this lower-case, colon format - but I figured I'd pass it through regardless.

IMDS metadata uses upper-case w/o colons, so it needs the normalization for comparison.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use this method here too:

mac = ":".join(re.findall(r"..", intf["macAddress"]))

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, thanks!

@cjp256 cjp256 force-pushed the azure-log-missing-metadata branch 3 times, most recently from bd6a3d8 to 0ebae8c Compare February 11, 2022 19:56
@cjp256 cjp256 marked this pull request as ready for review February 11, 2022 19:57
@cjp256 cjp256 force-pushed the azure-log-missing-metadata branch from 0ebae8c to 984d5b4 Compare February 11, 2022 22:40
Copy link
Contributor

@anhvoms anhvoms left a comment

Choose a reason for hiding this comment

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

LGTM

@cjp256 cjp256 force-pushed the azure-log-missing-metadata branch 3 times, most recently from 443cb30 to eee1ee9 Compare February 13, 2022 22:54
Due to race conditions and caching, IMDS may return stale or incomplete
metadata.  Add some validation to detect these scenarios and report
appropriate telemetry.

Introduce normalize_mac_address() to allow for comparison of mac
addresses, replacing that found inline in:
_generate_network_config_from_imds_metadata()

Add validation of final fetch of IMDS metadata.

Signed-off-by: Chris Patterson <cpatterson@microsoft.com>
@cjp256 cjp256 force-pushed the azure-log-missing-metadata branch from eee1ee9 to f893801 Compare February 14, 2022 13:52
Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

LGTM!

@TheRealFalcon TheRealFalcon merged commit 32fcbb5 into canonical:main Feb 14, 2022
@cjp256 cjp256 deleted the azure-log-missing-metadata branch April 5, 2022 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants