Allow PublicSign=true even if full keys are available #12749
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
To validate that this is roughly the right fix, I patched the Any ideas on further testing and/or unit tests that might be useful here? |
ae7b7c3 to
69bc4e6
Compare
|
cc @mmitche |
|
LGTM. Usually we'd put tests for this in arcade-validation (testing msbuild isn't always the easiest). IMO though, this particular issue is niche enough that if we will catch it in source build validation, it's fine to simply make a fix here. |
69bc4e6 to
3963289
Compare
mmitche
previously approved these changes
Mar 3, 2023
mmitche
reviewed
Mar 4, 2023
| @@ -5,6 +5,7 @@ | |||
| <!-- | |||
| Reads variables: | |||
| SignAssembly "true" to sign the output assembly of the current project | |||
| FullAssemblySigningSupported "false" to use public signing even when full signing is possible | |||
There was a problem hiding this comment.
Can you put a comment here with some context on when FullAssemblySigningSupported might be set to false?
There was a problem hiding this comment.
Does the new comment read okay? Any suggestions on the details or phrasing?
In certain environments - such as RHEL 9 - full signing does not work. That's because full signing requires SHA1 which is considered weak and was disabled in OpenSSL. Trying to use full signing leads to a Interop+Crypto+OpenSslCryptographicException. For more details, see dotnet/runtime#65874. In contrast, public signing doesn't use SHA1 and works fine in these environments. To make sure we can still build projects in those environments using arcade, allow arcade consumers to select public signing even when we have all the keys for full signing. Fixes: dotnet#12515
3963289 to
2d4af4a
Compare
mmitche
reviewed
Mar 6, 2023
mmitche
previously approved these changes
Mar 6, 2023
Co-authored-by: Matt Mitchell <mmitche@microsoft.com>
mmitche
approved these changes
Mar 6, 2023
omajid
added a commit
to omajid/dotnet-installer
that referenced
this pull request
Mar 20, 2023
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: - dotnet/runtime#65874 - dotnet/source-build#3202 - dotnet/arcade#12515
omajid
added a commit
to omajid/dotnet-installer
that referenced
this pull request
Mar 21, 2023
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
omajid
added a commit
to omajid/dotnet-arcade
that referenced
this pull request
Mar 22, 2023
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
1 task
omajid
added a commit
to omajid/dotnet-arcade
that referenced
this pull request
Mar 24, 2023
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
added a commit
to omajid/dotnet-aspnetcore
that referenced
this pull request
Mar 27, 2023
This property is being added to arcade via dotnet/arcade#12749 and dotnet/arcade#12940 Once this property is added to arcade, it flows correctly to the main aspnetcore build, but not to the build for repo tasks. The repo tasks still need this, otherwise they end up using full signing. Fix that by manually passing the property along (using env var) when building the repo tasks.
wtgodbe
pushed a commit
to dotnet/aspnetcore
that referenced
this pull request
Mar 30, 2023
) This property is being added to arcade via dotnet/arcade#12749 and dotnet/arcade#12940 Once this property is added to arcade, it flows correctly to the main aspnetcore build, but not to the build for repo tasks. The repo tasks still need this, otherwise they end up using full signing. Fix that by manually passing the property along (using env var) when building the repo tasks.
omajid
added a commit
to omajid/dotnet-arcade
that referenced
this pull request
Mar 30, 2023
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
mmitche
pushed a commit
that referenced
this pull request
Mar 31, 2023
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In certain environments - such as RHEL 9 - full signing does not work. That's because full signing requires SHA1 which is considered weak and was disabled in OpenSSL. Trying to use full signing leads to a Interop+Crypto+OpenSslCryptographicException. For more details, see dotnet/runtime#65874.
In contrast, public signing doesn't use SHA1 and works fine in these environments.
To make sure we can still build projects in those environments using arcade, allow arcade consumers to select public signing even when we have all the keys for full signing.
Fixes: #12515
To double check: