Allow disabling full signing in source-build #15873

omajid · 2023-03-20T21:18:50Z

Full Signing requires RSA+SHA1 which is disabled in some environments (eg, RHEL 9, CentOS Stream 9). Expose a top-level flag to allow that to be disabled easily by users.

The actual implementation of that flag is not here; it's in arcade (see dotnet/arcade#12749). Some repos have some nice-to-have fixes (eg, dotnet/source-build-reference-packages#566), but it's mostly optional. The bootstrap arcade SDK being used to build this needs needs to contain the changes for us to be able to build SBRP and then arcade and then all the other repos that will use arcade.

For more context around RSA+SHA1 and the alternative (using public signing), see:

omajid · 2023-03-20T21:19:42Z

src/SourceBuild/content/build.sh

@@ -6,6 +6,7 @@ usage() {
    echo "usage: $0 [options]"
    echo "options:"
    echo "  --clean-while-building             cleans each repo after building (reduces disk space usage)"
+    echo "  --disable-full-signing             do not use full signing, useful if platform doesn't support RSA+SHA1"


Any thoughts about addition of this top-level flag? Should we prefer ./build.sh -- -p:FullAssemblySigningSupported=false instead?

The MSBuild properties can be pretty hard to discover so if this is likely to be a common scenario for distro maintainers I would favor it being an argument in the build script. I think you're the best one to tell us how much it'll probably get though :)

This option will be useful immediately on RHEL 9 and CentOS Stream 9. We could replace the "unsupported" OPENSSL_ENABLE_SHA1_SIGNATURES=1 in #15289 with this too.

I expect it will be useful for Fedora some time soon too (see https://lwn.net/Articles/887832/) but the timeline is less clear.

Debian also has a plan to remove SHA1, but it's unclear what their implementation is (do they only audit/fix runtime/application code or do they remove RSA+SHA1 from OpenSSL itself?). They could take advantage of this too.

Looking at this question another way: if we add this flag now, can we remove it some time later? Would that be considered a breaking change?

Looking at this question another way: if we add this flag now, can we remove it some time later? Would that be considered a breaking change?

I think we should feel free to remove it between major releases and anytime between preview releases.

Can we make FullAssemblySigningSupported=false the default behavior?

Is there a need to add a flag? Who wants to opt-in to the full signing?

Who wants to opt-in to the full signing?

Would Microsoft's build of the SDK using the VMR want to use full signing?

I would avoid adding it as a flag.

If I recall correctly, full signing had no use for source-build.
So disabling it by default seems the proper thing.

If Microsoft still wants it for their builds, may we can add a Condition so it gets enabled automatically for their builds.

Okay, I flipped everything around and removed the flag in ./build.sh. Also updated CI.

Full Signing requires RSA+SHA1 which is disabled in some environments (eg, RHEL 9, CentOS Stream 9). Default to turning it off. Expose a top-level property to allow Full Signing to be re-enabled by users. The actual implementation of that flag is not here; it's in arcade (see dotnet/arcade#12749). Some repos have some nice-to-have fixes (eg, dotnet/source-build-reference-packages#566), but it's mostly optional. The bootstrap arcade SDK being used to build this needs needs to contain the changes for us to be able to build SBRP and arcade; then all the other repos that will use the just-built arcade. For more context around RSA+SHA1 and the alternative (using public signing), see: - dotnet/runtime#65874 - dotnet/source-build#3202 - dotnet/arcade#12515

MichaelSimons · 2023-03-22T13:00:59Z

src/SourceBuild/content/repo-projects/Directory.Build.props

@@ -114,6 +115,8 @@

    <!-- https://github.com/dotnet/source-build/issues/3081 -->
    <EnvironmentVariables Include="CheckEolTargetFramework=false" />
+
+    <EnvironmentVariables Include="FullAssemblySigningSupported=$(FullAssemblySigningSupported)" />


One disadvantage of this approach is that it introduces a difference in the way the full product source-build behaves versus the individual repos when built in the source-build configuration. Have you considered moving this into the ArPow infra in arcade?

That's a good point! I hadn't thought of it like that.

With a change like this in arcade, would it be much harder for someone to turn on Full Signing? IIRC, we were talking about how modifying inner args is much harder from the VMR level?

diff --git a/src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/SourceBuildArcadeBuild.targets b/src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/SourceBuildArcadeBuild.targets index 3933e436..da02d040 100644 --- a/src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/SourceBuildArcadeBuild.targets +++ b/src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/SourceBuildArcadeBuild.targets @@ -13,6 +13,8 @@ <InnerSourceBuildRepoRoot Condition="'$(InnerSourceBuildRepoRoot)' == ''">$(CurrentRepoSourceBuildSourceDir)</InnerSourceBuildRepoRoot> <CleanInnerSourceBuildRepoRoot Condition="'$(CleanInnerSourceBuildRepoRoot)' == ''">true</CleanInnerSourceBuildRepoRoot> + + <FullAssemblySigningSupported Condition="'$(FullAssemblySigningSupported)' != ''">false</FullAssemblySigningSupported> </PropertyGroup> <Target Name="ExecuteWithSourceBuiltTooling" @@ -87,6 +89,8 @@ <InnerBuildArgs>$(InnerBuildArgs) /p:DotNetBuildOffline=true</InnerBuildArgs> <InnerBuildArgs Condition=" '$(DotNetPackageVersionPropsPath)' != '' ">$(InnerBuildArgs) /p:DotNetPackageVersionPropsPath=$(DotNetPackageVersionPropsPath)</InnerBuildArgs> + + <InnerBuildArgs>$(InnerBuildArgs) /p:FullAssemblySigningSupported=$(FullAssemblySigningSupported)</InnerBuildArgs> </PropertyGroup> <ItemGroup>

would it be much harder for someone to turn on Full Signing?

I don't think so, you could still override via an EnvironmentVariable.

IIRC, we were talking about how modifying inner args is much harder from the VMR level?

The pieces that make it hard don't apply here IMO. Flowing inner args gets complicated because because there are multiple use cases to consider - e.g. does the arg apply to only the outer build, inner build, both outer and inner.

Full Signing requires RSA+SHA1 which is disabled in some environments (eg, RHEL 9, CentOS Stream 9). Default to turning it off. Expose a top-level property to allow Full Signing to be re-enabled by users. The actual implementation of that flag was in dotnet#12749 (commit 3840d43). Once a version of arcade including this fix is used to build the individual repos (in source-build mode) or the VMR, everything should default to public signing. For more context around RSA+SHA1 and the alternative (using public signing), see: - dotnet/runtime#65874 - dotnet/source-build#3202 - dotnet#12515 - dotnet/installer#15873

omajid · 2023-03-22T20:35:29Z

Closing in favour of dotnet/arcade#12940

Full Signing requires RSA+SHA1 which is disabled in some environments (eg, RHEL 9, CentOS Stream 9). Default to turning it off. Expose a top-level property to allow Full Signing to be re-enabled by users. The actual implementation of that flag was in dotnet#12749 (commit 3840d43). Once a version of arcade including this fix is used to build the individual repos (in source-build mode) or the VMR, everything should default to public signing. For more context around RSA+SHA1 and the alternative (using public signing), see: - dotnet/runtime#65874 - dotnet/source-build#3202 - dotnet#12515 - dotnet/installer#15873

Full Signing requires RSA+SHA1 which is disabled in some environments (eg, RHEL 9, CentOS Stream 9). Default to turning it off. Expose a top-level property to allow Full Signing to be re-enabled by users. The actual implementation of that flag was in #12749 (commit 3840d43). Once a version of arcade including this fix is used to build the individual repos (in source-build mode) or the VMR, everything should default to public signing. For more context around RSA+SHA1 and the alternative (using public signing), see: - dotnet/runtime#65874 - dotnet/source-build#3202 - #12515 - dotnet/installer#15873

omajid requested a review from a team as a code owner March 20, 2023 21:18

omajid commented Mar 20, 2023

View reviewed changes

omajid force-pushed the source-build-disable-full-signing branch from c64591e to bd09f95 Compare March 21, 2023 21:21

MichaelSimons reviewed Mar 22, 2023

View reviewed changes

MichaelSimons added the Area-SourceBuild label Mar 22, 2023

omajid mentioned this pull request Mar 22, 2023

Disable Full Signing in source-build dotnet/arcade#12940

Merged

1 task

omajid closed this Mar 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow disabling full signing in source-build #15873

Allow disabling full signing in source-build #15873

omajid commented Mar 20, 2023 •

edited

Loading

omajid Mar 20, 2023 •

edited

Loading

crummel Mar 20, 2023

omajid Mar 21, 2023

MichaelSimons Mar 21, 2023

tmds Mar 21, 2023 •

edited

Loading

omajid Mar 21, 2023

tmds Mar 21, 2023 •

edited

Loading

omajid Mar 21, 2023

MichaelSimons Mar 22, 2023

omajid Mar 22, 2023

omajid Mar 22, 2023

MichaelSimons Mar 22, 2023

omajid commented Mar 22, 2023

Allow disabling full signing in source-build #15873

Allow disabling full signing in source-build #15873

Conversation

omajid commented Mar 20, 2023 • edited Loading

omajid Mar 20, 2023 • edited Loading

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

tmds Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmds Mar 21, 2023 • edited Loading

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

omajid commented Mar 22, 2023

omajid commented Mar 20, 2023 •

edited

Loading

omajid Mar 20, 2023 •

edited

Loading

tmds Mar 21, 2023 •

edited

Loading

tmds Mar 21, 2023 •

edited

Loading