Remove embedded bouncy castle from core. Separate CloudFrontSigners and EC2 DecryptPassword to extension packages. #3435

peterrsongg · 2024-08-13T01:43:31Z

Description

This PR removes the embedded bouncy castle source copy in the third party namespace. It refactors CloudFrontSigners to separate extension packages and EC2 DecryptPassword as well. There was one other occurrence where Third Party bouncy castle was used in SNS and that has been refactored as well.

Revision 2
Added the test mentioned in #3221 and verified it worked. Made some logic change to accommodate for this test. Addressed PR feedback.

Motivation and Context

Allows us to take a dependency on Portable.Bouncy castle in the extension packages and receive updates. Removes tech debt of having a source copy in core.

Testing

E2E tests pass

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project
My change requires a change to the documentation
I have updated the documentation accordingly
I have read the README document
I have added tests to cover my changes
All new and existing tests passed

License

I confirm that this pull request can be released under the Apache 2 license

extensions/AWSSDK.Extensions.NetFramework.sln

...WSSDK.Extensions.CloudFront.Signers/AWSSDK.Extensions.CloudFront.Signers.NetFramework.csproj

extensions/src/AWSSDK.Extensions.CrtIntegration/AWSSDK.Extensions.CrtIntegration.nuspec

...AWSSDK.Extensions.CloudFront.Signers/AWSSDK.Extensions.CloudFront.Signers.NetStandard.csproj

extensions/src/AWSSDK.Extensions.EC2.DecryptPassword/AWSSDK.Extensions.EC2.DecryptPassword.sln

extensions/src/AWSSDK.Extensions.EC2.DecryptPassword/GetPasswordDataResponseExtensions.cs

extensions/test/CloudFront.SignersTests/URLSignerTest.cs

extensions/src/AWSSDK.Extensions.CloudFront.Signers/AWSSDK.Extensions.CloudFront.Signers.nuspec

dscpinheiro · 2024-08-13T12:10:39Z

If we're removing the embedded source, should we also update the notice files?

dscpinheiro · 2024-08-20T12:20:18Z

generator/.DevConfigs/aabf4e64-a7dd-4ee3-95a7-c8466c22a7e3.json

    ],
-    "type": "patch",
+    "type": "minor",


I think type here is ignored by our build system (since there's an overrideVersion as well).

Also, is the -preview suffix required? For example, see the config we use for the first preview: https://github.com/aws/aws-sdk-net/blob/15a866102abf3763fa4c180c936a248d3b40c45b/generator/.DevConfigs/525da348-b9e8-4f4d-9dc7-47b2ee7f4716.json

The preview actually failed the E2E test I ran last night, so I will remove it

extensions/test/EC2.DecryptPasswordTests/EC2.DecryptPassword.NetStandard.csproj

normj · 2024-08-20T23:12:05Z

...AWSSDK.Extensions.CloudFront.Signers/AWSSDK.Extensions.CloudFront.Signers.NetStandard.csproj

+	<GenerateAssemblyFileVersionAttribute>false</GenerateAssemblyFileVersionAttribute>
+	<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
+  </PropertyGroup>
+  <ItemGroup>


Project file should be marked as trimmable for the .NET 8 target.

<PropertyGroup Condition="'$(TargetFramework)' == 'net8.0'"> <IsTrimmable>true</IsTrimmable> </PropertyGroup>

good call, added.

normj · 2024-08-20T23:12:19Z

...SSDK.Extensions.EC2.DecryptPassword/AWSSDK.Extensions.EC2.DecryptPassword.NetStandard.csproj

+		<GenerateAssemblyFileVersionAttribute>false</GenerateAssemblyFileVersionAttribute>
+		<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
+	</PropertyGroup>
+	<ItemGroup>


Project file should be marked as trimmable for the .NET 8 target.

<PropertyGroup Condition="'$(TargetFramework)' == 'net8.0'"> <IsTrimmable>true</IsTrimmable> </PropertyGroup>

normj · 2024-08-21T18:08:49Z

extensions/src/AWSSDK.Extensions.CloudFront.Signers/AWSSDK.Extensions.CloudFront.Signers.nuspec

+        <dependency id="AWSSDK.CloudFront" version="4.0.1.0-preview" />
+        <dependency id="BouncyCastle.Cryptography" version="2.4.0" />
+      </group>
+    </dependencies>


You are missing the dependency groups for .NET Core 3.1 and .NET 8.

Hmm I tested the nuget package locally and it worked (for net 8), and given that the CrtIntegration nuspec file doesn't have a target for netcore31 and net 8, I don't think I need one here, unless the CrtIntegration nuspec is doing something different.

I wasn't sure, and looks like the service nuspec's do this as well so I added them in the latest revision

I assumed it worked by resolving to use the .NET Standard 2.0 version of the assembly.

normj · 2024-08-21T18:09:10Z

extensions/src/AWSSDK.Extensions.CloudFront.Signers/AWSSDK.Extensions.CloudFront.Signers.nuspec

+    <file src=".\bin\Release\net472\AWSSDK.Extensions.CloudFront.Signers.pdb" target="lib\net472" />
+
+    <file src=".\bin\Release\netstandard2.0\AWSSDK.Extensions.CloudFront.Signers.dll" target="lib\netstandard2.0"/>
+    <file src=".\bin\Release\netstandard2.0\AWSSDK.Extensions.CloudFront.Signers.pdb" target="lib\netstandard2.0"/>


You need to add the .NET Core 3.1 and .NET 8 files.

will add these

normj · 2024-08-21T18:10:22Z

...sions/src/AWSSDK.Extensions.EC2.DecryptPassword/AWSSDK.Extensions.EC2.DecryptPassword.nuspec

+    <tags>AWS Amazon aws-sdk-v4</tags>
+    <icon>images\AWSLogo.png</icon>
+    <dependencies>
+      <group targetFramework="net472">


Missing .NET Core 3.1 and .NET 8 group dependencies.

normj · 2024-08-21T18:10:34Z

...sions/src/AWSSDK.Extensions.EC2.DecryptPassword/AWSSDK.Extensions.EC2.DecryptPassword.nuspec

+    <file src=".\bin\Release\net472\AWSSDK.Extensions.EC2.DecryptPassword.dll" target="lib\net472" />
+    <file src=".\bin\Release\net472\AWSSDK.Extensions.EC2.DecryptPassword.pdb" target="lib\net472" />
+
+    <file src=".\bin\Release\netstandard2.0\AWSSDK.Extensions.EC2.DecryptPassword.dll" target="lib\netstandard2.0"/>


Add the .NET Core 3.1 and .NET 8 files.

normj · 2024-08-21T21:10:05Z

Notice.txt

@@ -10,29 +10,6 @@ purpose, commercial or non-commercial, and by any means.

 ----------------

-** Parsing PEM files from Bouncy Castle


You need add a new entry for BouncyCastle NuGet package. Like we recently did for the new System.* NuGet packages we added for V4. https://github.com/aws/aws-sdk-net/blob/v4-development/Notice.txt#L127

normj · 2024-08-21T21:10:47Z

buildtools/NoticeForZips.txt

@@ -10,34 +10,6 @@ purpose, commercial or non-commercial, and by any means.

 ----------------

-** Parsing PEM files from Bouncy Castle


You need add a new entry for BouncyCastle NuGet package. Like we recently did for the new System.* NuGet packages we added for V4. https://github.com/aws/aws-sdk-net/blob/v4-development/Notice.txt#L127

normj · 2024-08-21T21:12:58Z

extensions/src/AWSSDK.Extensions.CloudFront.Signers/AWSSDK.Extensions.CloudFront.Signers.nuspec

+    </dependencies>
+
+  </metadata> 
+  <files>


Add the documentation xml files so VS intellisense can use them.

normj · 2024-08-21T21:13:18Z

...sions/src/AWSSDK.Extensions.EC2.DecryptPassword/AWSSDK.Extensions.EC2.DecryptPassword.nuspec

+    </dependencies>
+
+  </metadata> 
+  <files>


Add the documentation xml files so VS intellisense can use them.

…ont signers and ec2 decrypt password to extension packages to take a dependency on BouncyCastle.Cryptography

peterrsongg force-pushed the petesong/remove-embedded-bouncy-castle branch from 75141c6 to 60d5ee5 Compare August 13, 2024 01:44

dscpinheiro reviewed Aug 13, 2024

View reviewed changes

peterrsongg requested review from dscpinheiro and boblodgett August 13, 2024 22:21

peterrsongg force-pushed the petesong/remove-embedded-bouncy-castle branch from 57b42f5 to 237e7a6 Compare August 17, 2024 22:03

dscpinheiro reviewed Aug 20, 2024

View reviewed changes

normj reviewed Aug 20, 2024

View reviewed changes

dscpinheiro approved these changes Aug 21, 2024

View reviewed changes

normj requested changes Aug 21, 2024

View reviewed changes

peterrsongg requested a review from normj August 21, 2024 21:03

normj requested changes Aug 21, 2024

View reviewed changes

normj approved these changes Aug 21, 2024

View reviewed changes

refactor: remove third party bouncy castle from core and move cloudfr…

27d18ee

…ont signers and ec2 decrypt password to extension packages to take a dependency on BouncyCastle.Cryptography

peterrsongg force-pushed the petesong/remove-embedded-bouncy-castle branch 2 times, most recently from 906f8c6 to 65d9228 Compare August 23, 2024 03:56

generate documentation file for extensions

362858a

peterrsongg force-pushed the petesong/remove-embedded-bouncy-castle branch from 65d9228 to 362858a Compare August 23, 2024 06:30

peterrsongg merged commit 7056953 into v4-development Aug 23, 2024
1 check passed

dscpinheiro deleted the petesong/remove-embedded-bouncy-castle branch August 24, 2024 00:02

peterrsongg added the v4 label Sep 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove embedded bouncy castle from core. Separate CloudFrontSigners and EC2 DecryptPassword to extension packages. #3435

Remove embedded bouncy castle from core. Separate CloudFrontSigners and EC2 DecryptPassword to extension packages. #3435

peterrsongg commented Aug 13, 2024 •

edited

Loading

dscpinheiro commented Aug 13, 2024

dscpinheiro Aug 20, 2024

peterrsongg Aug 20, 2024

normj Aug 20, 2024

peterrsongg Aug 21, 2024

normj Aug 20, 2024

normj Aug 21, 2024

peterrsongg Aug 21, 2024

peterrsongg Aug 21, 2024

normj Aug 21, 2024

normj Aug 21, 2024

peterrsongg Aug 21, 2024

normj Aug 21, 2024

normj Aug 21, 2024

normj Aug 21, 2024

normj Aug 21, 2024

normj Aug 21, 2024

normj Aug 21, 2024

		@@ -10,29 +10,6 @@ purpose, commercial or non-commercial, and by any means.

		----------------

		** Parsing PEM files from Bouncy Castle

		@@ -10,34 +10,6 @@ purpose, commercial or non-commercial, and by any means.

		----------------

		** Parsing PEM files from Bouncy Castle

Remove embedded bouncy castle from core. Separate CloudFrontSigners and EC2 DecryptPassword to extension packages. #3435

Remove embedded bouncy castle from core. Separate CloudFrontSigners and EC2 DecryptPassword to extension packages. #3435

Conversation

peterrsongg commented Aug 13, 2024 • edited Loading

Description

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

dscpinheiro commented Aug 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterrsongg commented Aug 13, 2024 •

edited

Loading