Change md5withRSA to SHA1withRSA which is the default sig alg. #927

fekberg · 2017-10-09T19:38:56Z

SHA1withRSA is now the default signature algorithm according to this JDK issue: https://bugs.openjdk.java.net/browse/JDK-6560733

dnfclas · 2017-10-09T19:39:00Z

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

fekberg · 2017-10-09T19:49:42Z

I'd much rather see apksigner being used instead as per this https://bugzilla.xamarin.com/show_bug.cgi?id=57914

Meanwhile, I think moving away from MD5 is a good idea.

dellis1972 · 2017-10-10T08:39:55Z

We have to be careful with the signing algorithm. In previous attempts to upgrade we found that apps published on the google play store no longer installed on older devices. Especially those which are stuck on older versions of android because the vendor stopped issuing updates. I would love to know if this change has the same problem.

In addition to apksigner we should probably parameterise the -sigalg value with an MSBuild property which defaults to md5withRSA but will allow the developer to upgrade if they want too.

fekberg · 2017-10-10T08:47:50Z

Do you have any records on what those devices are? I've got a hard time finding any good information about it.

Parameterising -sigalg is probably a better idea than changing it to another default value.

dellis1972 · 2017-10-10T08:51:23Z

@fekberg I have an old Galaxy S1 which is on Android 2.3.6 which won't installer apps signed with a new algorithm. You get a weird error from google play when you try to. Its been a while though so I can't remember the exact problem.

fekberg · 2017-10-10T08:56:40Z

@dellis1972 That's a very old phone.. :) I haven't seen any apps that still targets anything below 4.x

dellis1972 · 2017-10-10T09:01:45Z

I know games that target 2.3.6, its a very small part of the market though. Still, doing this with a parameter that the developer can override is probably the best way forward in this case.

I'm gonna work on apksigner this week so hopefully that will help too.

fekberg · 2017-10-10T09:04:12Z

@dellis1972 We had a discussion yesterday on Slack in regards of adding apksigner instead. Not sure you saw that?

Anyways, in the meantime, I could update the PR to parameterize this instead. Haven't done that before, is it enough to just add it as a property in this class and it magically works?

dellis1972 · 2017-10-10T09:09:18Z

you can add a new string property SigningAlgorithm or something to the class. It will not need the [Required] attribute since we will make it optional.
The you can do a null check and replace the value with the old default md5withRSA.

You will also need to edit Xamarin.Android.Common.targets. Look for the _Sign target and add

SigningAlgorithm="$(AndroidSigningAlgorithm)"

at around [1].

[1] https://github.com/xamarin/xamarin-android/blob/master/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets#L2441.

dellis1972 · 2017-10-10T09:10:34Z

if you are feeling adventurous you could also document it at some point around https://github.com/xamarin/xamarin-android/blob/master/Documentation/build_process.md#AndroidVersionCodeProperties :P

fekberg · 2017-10-10T09:22:18Z

Awesome, thanks for pointing these out!

…rsign

fekberg · 2017-10-10T10:27:10Z

@dellis1972 I've updated the PR with changes to the target.

I set it to this: SigningAlgorithm="$(_ApkSigningAlgorithm)" to use the same convention as the other parameters in that section.

dellis1972

Apart from the IDE messing up the formatting (we use tabs rather than spaces I'm afraid) this looks ok apart from the naming of _ApkSigningAlgorithm. Once that is fixed I'll merge this

dellis1972 · 2017-10-10T10:45:11Z

src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets

 		ToolExe="$(JarsignerToolExe)"
 		TimestampAuthorityUrl="$(JarsignerTimestampAuthorityUrl)"
 		TimestampAuthorityCertificateAlias="$(JarsignerTimestampAuthorityCertificateAlias)"
+		SigningAlgorithm="$(_ApkSigningAlgorithm)"


we should use ApkSigningAlgorithm rather than _ApkSigningAlgorithm. In msbuild the convention is _ prefixed properties are private and should not be used by the end user. Ones without are public. So we should remove the _ since we expect the user to override this.

fekberg · 2017-10-10T11:04:18Z

Should be corrected now!

dellis1972 · 2017-10-10T12:06:20Z

build

dellis1972 · 2017-10-10T12:11:32Z

@fekberg cool. perfect. Just waiting for the PR builder to give it the thumbs up and we are good :) thanks for the PR!

dellis1972 · 2017-10-10T13:20:32Z

src/Xamarin.Android.Build.Tasks/Tasks/AndroidSignPackage.cs

 			cmd.AppendSwitchIfNotNull ("-keypass ", KeyPass);
 			cmd.AppendSwitchIfNotNull ("-digestalg ", "SHA1");
-			cmd.AppendSwitchIfNotNull ("-sigalg ", "md5withRSA");
+			cmd.AppendSwitchIfNotNull ("-sigalg ", string.IsNullorWitespace(SigningAlgorithm) ? "md5withRSA" :SigningAlgorithm);


@fekberg we have a typo here

IsNullorWitespace vs IsNullOrWitespace

Oh my, sorry about that!

dellis1972 · 2017-10-10T14:13:19Z

@fekberg really sorry, looks like you committed changes to the .sln.

fekberg · 2017-10-10T14:19:51Z

I was sure I reverted that.. Let me try again :)

dellis1972 · 2017-10-10T14:28:14Z

cool :) thanks

dellis1972 · 2017-10-10T14:28:20Z

build

dellis1972 · 2017-10-10T14:43:53Z

build

fekberg · 2017-10-10T19:45:49Z

@dellis1972 Thanks for all the help, happy I can be of service!

Allow the default `.apk` signing algorithm used with `jarsigner -sigalg` to be overridden via the new `$(AndroidApkSigningAlgorithm)` MSBuild property. If not overridden, the existing value of `jarsigner -sigalg md5withRSA` will be used to sign the `.apk`.

jonpryor · 2017-10-11T00:48:01Z

Manually merged in commit 1b4af39, and documentation added.

…et#927) Allow the default `.apk` signing algorithm used with `jarsigner -sigalg` to be overridden via the new `$(AndroidApkSigningAlgorithm)` MSBuild property. If not overridden, the existing value of `jarsigner -sigalg md5withRSA` will be used to sign the `.apk`.

Changes: dotnet/java-interop@7f1a5ab...bc5bcf4 * dotnet/java-interop@bc5bcf4f: [Java.Base] Begin binding JDK-11 java.base module (dotnet#909) * dotnet/java-interop@af91b9c2: [ci] Use descriptive test run titles. (dotnet#926) * dotnet/java-interop@aae8f251: [Java.Interop.Tools.Generator] Add some enumification helper methods (dotnet#866) * dotnet/java-interop@111ebca8: [Java.Interop] Treat warnings as errors. (dotnet#925) * dotnet/java-interop@7a32bb97: [java-source-utils] Transform XML using XSLT (dotnet#924) * dotnet/java-interop@7f55b2dc: [logcat-parse] Use C# verbatim strings for paths (dotnet#927) * dotnet/java-interop@2601146b: [java-source-utils] Fix regresion from 77c9c5fa (dotnet#923)

Changes: dotnet/java-interop@7f1a5ab...aac3e9a * dotnet/java-interop@aac3e9ac: [Java.Interop, Java.Base] Fix xamarin-android integration issues (#929) * dotnet/java-interop@bc5bcf4f: [Java.Base] Begin binding JDK-11 java.base module (#909) * dotnet/java-interop@af91b9c2: [ci] Use descriptive test run titles. (#926) * dotnet/java-interop@aae8f251: [Java.Interop.Tools.Generator] Add some enumification helper methods (#866) * dotnet/java-interop@111ebca8: [Java.Interop] Treat warnings as errors. (#925) * dotnet/java-interop@7a32bb97: [java-source-utils] Transform XML using XSLT (#924) * dotnet/java-interop@7f55b2dc: [logcat-parse] Use C# verbatim strings for paths (#927) * dotnet/java-interop@2601146b: [java-source-utils] Fix regresion from 77c9c5fa (#923) (So many inadvertent issues as part of this bump…) dotnet/java-interop@bc5bcf4f added an *altered* `Java.Interop.JavaTypeParametersAttribute` custom attribute, altered to "appease" various Code Analysis warnings such as [CA1813][0] "Avoid unsealed attributes". This introduction caused some unit tests to fail to build: error CS0433: The type 'JavaTypeParametersAttribute' exists in both 'Java.Interop, Version=0.1.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065' and 'Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065' [C:\…\temp\LibraryProjectZipWithLint\BindingsProject\BindingsProject.csproj] The original idea had been to use [type forwarders][1], so that `Java.Interop.JavaTypeParametersAttribute` from `Mono.Android.dll` would be forwarded to the same type in `Java.Interop.dll`. This idea was rejected after further consideration, as it would likely break [forward compatibility][2] (dotnet/java-interop@e56a8c8e). dotnet/java-interop@aac3e9ac fixes this by making `JavaTypeParametersAttribute` conditional on `NET`, causing the type to only be present when targeting .NET 6+. `Mono.Android.dll` for .NET 6 -- and *not* Classic Xamarin.Android -- can then use a type forwarder. This can be done because there's no forward compatibility constraint on .NET 6. However, by making `JavaTypeParametersAttribute` conditional on `NET`, the `_CheckApiCompatibility` target started failing -- rightfully! -- because the `JavaTypeParametersAttribute` type had been removed. "Worse", there was no way to accept this change in e.g. `tests/api-compatibility/acceptable-breakages-vReference.txt`, because the `<CheckApiCompatibility/>` task requires that there be no extra entries. Square this circle by adding a `CheckApiCompatibility.TargetFramework` property, and looking for `acceptable-breakages-*-{TargetFramework}.txt` *before* looking for `acceptable-breakages-*.txt`. This allows us to special-case the acceptable breakages based on `$(TargetFramework)`. Finally, update `azure-pipelines.yaml` so that the `java-source-utils` unit tests are *not* run on Windows. This is because they currently fail on Windows, which is something we'll worry about later. For now, disable the `java-source-utils` tests when building on Windows. [0]: https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1813 [1]: https://docs.microsoft.com/en-us/dotnet/standard/assembly/type-forwarding [2]: https://en.wikipedia.org/wiki/Forward_compatibility

Change md5withRSA to SHA1withRSA which is the default sig alg.

7e1f931

SHA1withRSA is now the default signature algorithm according to this JDK issue: https://bugs.openjdk.java.net/browse/JDK-6560733

dnfclas added the cla-not-required label Oct 9, 2017

Add support for overriding the default signing algorithm used with ja…

03027bd

…rsign

dellis1972 requested changes Oct 10, 2017

View reviewed changes

ApkSigningAlgorithm is not private and should not be prefixed with _

24d0a75

dellis1972 approved these changes Oct 10, 2017

View reviewed changes

dellis1972 reviewed Oct 10, 2017

View reviewed changes

fekberg force-pushed the patch-1 branch 2 times, most recently from 5f53bb3 to 24d0a75 Compare October 10, 2017 14:22

Fix typo in AndroidSignPackage

1c3b505

fekberg force-pushed the patch-1 branch from d95a1d3 to 1c3b505 Compare October 10, 2017 14:28

jonpryor closed this Oct 11, 2017

jonpryor mentioned this pull request Dec 7, 2021

Bump to xamarin/Java.Interop/main@bc5bcf4f #6549

Merged

github-actions bot locked and limited conversation to collaborators Feb 4, 2024

Change md5withRSA to SHA1withRSA which is the default sig alg. #927

Change md5withRSA to SHA1withRSA which is the default sig alg. #927

Uh oh!

Conversation

fekberg commented Oct 9, 2017

Uh oh!

dnfclas commented Oct 9, 2017

Uh oh!

fekberg commented Oct 9, 2017

Uh oh!

dellis1972 commented Oct 10, 2017

Uh oh!

fekberg commented Oct 10, 2017

Uh oh!

dellis1972 commented Oct 10, 2017

Uh oh!

fekberg commented Oct 10, 2017

Uh oh!

dellis1972 commented Oct 10, 2017

Uh oh!

fekberg commented Oct 10, 2017

Uh oh!

dellis1972 commented Oct 10, 2017

Uh oh!

dellis1972 commented Oct 10, 2017

Uh oh!

fekberg commented Oct 10, 2017

Uh oh!

fekberg commented Oct 10, 2017

Uh oh!

dellis1972 left a comment

Choose a reason for hiding this comment

Uh oh!

dellis1972 Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

fekberg commented Oct 10, 2017

Uh oh!

dellis1972 commented Oct 10, 2017

Uh oh!

dellis1972 commented Oct 10, 2017

Uh oh!

dellis1972 Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

fekberg Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

dellis1972 commented Oct 10, 2017

Uh oh!

fekberg commented Oct 10, 2017

Uh oh!

dellis1972 commented Oct 10, 2017

Uh oh!

dellis1972 commented Oct 10, 2017

Uh oh!

dellis1972 commented Oct 10, 2017

Uh oh!

fekberg commented Oct 10, 2017

Uh oh!

jonpryor commented Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jonpryor commented Oct 11, 2017 •

edited

Loading