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

Resolves Warning SYSLIB021 - Derived cryptographic types are obsolete #210

Merged
merged 11 commits into from
May 8, 2023

Conversation

Eneuman
Copy link
Contributor

@Eneuman Eneuman commented Mar 19, 2023

This PR solves the SYSLIB021 - Derived cryptographic types are obsolete warning.
It also introduces tests for the MacAddressHash functionallity.

The RunCommandAndGetOutput function was changed to use the existing _platform.ExecuteAndReturnOutput to align the way the program executes externa processes. The timeout was set to 30 sec to make sure the command have enought time to finish. The process will like finish within a second under normal conditions.

@Eneuman Eneuman requested review from a team, Tatsinnit and sabbour as code owners March 19, 2023 19:34
@Eneuman
Copy link
Contributor Author

Eneuman commented Mar 19, 2023

It would be great if we could also add a tests for Mono.
I can add it if someone can get the output from the "ifconfig -a" command.

Copy link
Collaborator

@hsubramanianaks hsubramanianaks left a comment

Choose a reason for hiding this comment

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

@Eneuman Thanks for the changes, requested small changes.

@github-actions
Copy link

github-actions bot commented Apr 4, 2023

@Eneuman Thanks for your PR!

My review of this pull request is as follows:

This pull request looks good overall. It resolves the SYSLIB021 warning and introduces tests for the MacAddressHash functionality.

The changes look good. The new tests are comprehensive and cover the MacAddressHash functionality. The changes to FipsCompliantSha.cs and MACInformationProvider.cs look correct and are in line with the changes proposed.

The only suggestion I have is to add a comment to the RunCommandAndGetOutput method in MACInformationProvider.cs to explain why the timeout is set to 30 seconds. This will help other developers understand the purpose of the timeout and why it is set to 30 seconds.

@hsubramanianaks
Copy link
Collaborator

ifconfig -a

@Eneuman if you have WSL2 you can run this command ifconfig -a , if not let me know I can share some samples.

@github-actions
Copy link

github-actions bot commented Apr 5, 2023

@Eneuman Thanks for your PR!

This pull request looks good overall. It resolves the SYSLIB021 warning and introduces tests for the MacAddressHash functionality. The RunCommandAndGetOutput function was changed to use the existing _platform.ExecuteAndReturnOutput to align the way the program executes external processes. The timeout was set to 30 sec to make sure the command have enough time to finish.

The only suggestion I have is to add more comments to the code to explain the purpose of the changes. This will help other developers understand the code better and make it easier to maintain in the future.

@hsubramanianaks
Copy link
Collaborator

It would be great if we could also add a tests for Mono. I can add it if someone can get the output from the "ifconfig -a" command.

here is the sample output for ifconfig -a command (I have edited with dummy values).

        ether e6:0e:ae:14:6c:da  txqueuelen 1000  (Ethernet)
        RX packets 0  bytes 0 (0.0 B)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 0  bytes 0 (0.0 B)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0

dummy0: flags=130<BROADCAST,NOARP>  mtu 1500
        ether 6a:c7:29:b2:dc:9e  txqueuelen 1000  (Ethernet)
        RX packets 0  bytes 0 (0.0 B)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 0  bytes 0 (0.0 B)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0

eth0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1280
        inet 0.0.0.0  netmask 0.0.0.0  broadcast 0.0.0.0
        inet6 fe80::215:5dff:feb6:b81  prefixlen 64  scopeid 0x20<link>
        ether 00:15:5d:b6:0b:81  txqueuelen 1000  (Ethernet)
        RX packets 640092  bytes 605988996 (605.9 MB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 77211  bytes 5949128 (5.9 MB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0

lo: flags=73<UP,LOOPBACK,RUNNING>  mtu 65536
        inet 127.0.0.1  netmask 255.0.0.0
        inet6 ::1  prefixlen 128  scopeid 0x10<host>
        loop  txqueuelen 1000  (Local Loopback)
        RX packets 58608  bytes 361924804 (361.9 MB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 58608  bytes 361924804 (361.9 MB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0

sit0: flags=128<NOARP>  mtu 1480
        sit  txqueuelen 1000  (IPv6-in-IPv4)
        RX packets 0  bytes 0 (0.0 B)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 0  bytes 0 (0.0 B)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0

tunl0: flags=128<NOARP>  mtu 1480
        tunnel   txqueuelen 1000  (IPIP Tunnel)
        RX packets 0  bytes 0 (0.0 B)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 0  bytes 0 (0.0 B)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0 ````

@Eneuman
Copy link
Contributor Author

Eneuman commented Apr 9, 2023

@hsubramanianaks I have another suggestion, why dont we just replace this code with .Net native plattform independent code like this?

var firstInterface = NetworkInterface.GetAllNetworkInterfaces().First();
var macAddressInBytes = firstInterface.GetPhysicalAddress().GetAddressBytes();
var hashedMacAddressByteArray = MD5.HashData(macAddressInBytes);
var hashedMacAddress= BitConverter.ToString(hashedMacAddressByteArray).Replace("-", string.Empty).ToLowerInvariant();

I switched to MD5 because it's the fastest algorithm, and we are not doing encryption here ;)

@hsubramanianaks
Copy link
Collaborator

@hsubramanianaks I have another suggestion, why dont we just replace this code with .Net native plattform independent code like this?

var firstInterface = NetworkInterface.GetAllNetworkInterfaces().First();
var macAddressInBytes = firstInterface.GetPhysicalAddress().GetAddressBytes();
var hashedMacAddressByteArray = MD5.HashData(macAddressInBytes);
var hashedMacAddress= BitConverter.ToString(hashedMacAddressByteArray).Replace("-", string.Empty).ToLowerInvariant();

I switched to MD5 because it's the fastest algorithm, and we are not doing encryption here ;)

yes if it make sense to do in this PR please go ahead.

@Eneuman
Copy link
Contributor Author

Eneuman commented Apr 10, 2023

@hsubramanianaks I'll make a seperate PR for this :) This PR can just solve the warning.

Copy link
Member

@Tatsinnit Tatsinnit left a comment

Choose a reason for hiding this comment

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

☕️🙏 LGTM - Hopefully we got this all tested et. al. from internal channels. thanks heaps @hsubramanianaks and @Eneuman 🎊

@hsubramanianaks hsubramanianaks added the e2e testing e2e is in progress label Apr 18, 2023
Copy link
Collaborator

@hsubramanianaks hsubramanianaks left a comment

Choose a reason for hiding this comment

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

testing team approved these changes.

@hsubramanianaks hsubramanianaks merged commit e102da8 into Azure:main May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e testing e2e is in progress review done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants