-
Notifications
You must be signed in to change notification settings - Fork 17
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
Update Halibut #700
Update Halibut #700
Conversation
[sc-65659] |
This pull request has been linked to Shortcut Story #65659: Find where we missed updating System.ServiceProcess.ServiceController in Tentacle due to CVE. |
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.
Happy for this to go out now.
I have one question out of curiosity though - in your description you say you updated the two service management libraries to 5, but they're actually 4.x. Was there a reason to move it down from 5?
@@ -28,7 +28,7 @@ | |||
</Otherwise> | |||
</Choose> | |||
<ItemGroup> | |||
<PackageReference Include="Halibut" Version="7.0.209" /> |
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.
do we also need to update the corresponding version in Server? I assume it's not required but possibly a nice tidy-up to do.
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.
Great point! I didn't think of Server but yes, I'd say we should remove the potential vulnerability there too by upgrading. I'll take care of that.
Typo 🤦🏻 One of the other upgraded dependencies from the other PR was 5 -> 8, and I got them mixed up. I've now double checked and the code is fine, so I've updated the description. |
Update Halibut to
7.0.285
in order to get fixes for CVE-2021-24112 (as introduced in OctopusDeploy/Halibut#554)As an added bonus, I pulled up a dependency from
Octopus.Tentacle
into the automated test projects where it is actually needed.Background
There is a CVE lodged against the version of System.Drawing.Common being referenced.
Results
Fixes #689
(The other part of the fix is in #694 which has already been merged)
The main result is that there is no remaining production Tentacle code that depends on
System.Drawing.Common
, implicitly or explicitly.The
_build
project still holds an implicit dependency via Nuke.Octopus.Tentacle
had a dependency onSystem.DirectoryServices.AccountManagement
even though there was no code that depended on it. The only indirect usage was in the two instances of TestUserPrincipal (one in Octopus.Tentacle.Tests, the other in Octopus.Tentacle.Tests.Integration), so the dependency has been added to those test projects instead.Keen eyes may notice that the version of
System.DirectoryServices.AccountManagement
has also changed, now downgraded from8.0.0
to4.5.0
. The upgrade from v4.5 -> v8 was done recently as part of this PR, but due to implicit dependencies and the continued need to support .NET Framework 4.8, it caused the following warning to be raised:As there was no reason other than the CVE to upgrade this assembly, I reverted this upgrade to avoid unnecessary warnings and potential conflicts.
Before
After
How to review this PR
Quality ✔️
Pre-requisites