Skip to content
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

Sign almost all the things #870

Merged
merged 15 commits into from
Aug 14, 2024
Merged

Sign almost all the things #870

merged 15 commits into from
Aug 14, 2024

Conversation

lewing
Copy link
Member

@lewing lewing commented Aug 14, 2024

The goal here is to sign everything we can. After much testing the main insight is that 3PartyScriptsSHA2 works for the script files and some of the previously complicated files no longer exist. There are still a couple of problems:

  1. It is actively wrong to sign the emscripten library js since emscripten uses text transforms to combine it all together so we should test what ends up in the output before merging this. Because of this it's likely we'll need to not sign js still. I don't see a workable way to sign some tooling js and not the library js (perhaps we could sign some files before they are packaged?)
  2. The arcade tooling shouldn't attempt to sign zero length files, it will fail and retry until timeout. There are valid reasons for thing like zero length __init__.py files to exist and there is no way to deal with them at the moment.
  3. Because of something unknown that is causing the windows embuilder steps to take unreasonably long (unrelated to this pr) those legs appear to be even more likely to timeout with the additional signing.

@lewing lewing changed the title Lewing/update signing Sign almost all the things Aug 14, 2024
@lewing
Copy link
Member Author

lewing commented Aug 14, 2024

Copy link
Member

@joeloff joeloff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. For the 0-byte files, should we add something in Arcade to always ignore 0 byte files or allow repos to set a flag that says don't sign 0 byte files because otherwise we'll need to continue adding exclusions like the RemoveDir logic in the emsdk.proj

@@ -362,6 +362,6 @@ extends:
- template: /eng/common/templates-official/post-build/post-build.yml@self
parameters:
enableSourceLinkValidation: false
enableSigningValidation: false
enableSigningValidation: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of time does this add to the build?

@mmitche
Copy link
Member

mmitche commented Aug 14, 2024

  • The arcade tooling shouldn't attempt to sign zero length files, it will fail and retry until timeout. There are valid reasons for thing like zero length __init__.py files to exist and there is no way to deal with them at the moment.

dotnet/arcade#15001

@mmitche
Copy link
Member

mmitche commented Aug 14, 2024

  • It is actively wrong to sign the emscripten library js since emscripten uses text transforms to combine it all together so we should test what ends up in the output before merging this. It's likely well need to not sign js still. I don't see a workable way to sign some tooling js and not the library js (perhaps we could sign some files before they are packaged?)

Can you describe a pattern that you would write in your Sign.props that we could implement to handle these cases? Generally the sign infra has always focused on file name and file extensions to calculate target sig. In some cases it takes into account public key tokens.

<FileExtensionSignInfo Update=".ps1" CertificateName="None" />
<FileExtensionSignInfo Update=".js" CertificateName="None" />
<FileExtensionSignInfo Include=".vbs" CertificateName="None" />
<FileExtensionSignInfo Update=".py" CertificateName="3PartyScriptsSHA2" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to the .vbs entry?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no vbs files anymore

eng/Signing.props Outdated Show resolved Hide resolved
eng/Signing.props Outdated Show resolved Hide resolved
lewing and others added 2 commits August 14, 2024 14:22
Co-authored-by: Alexander Köplinger <[email protected]>
Co-authored-by: Alexander Köplinger <[email protected]>
@lewing lewing marked this pull request as ready for review August 14, 2024 19:49
@lewing
Copy link
Member Author

lewing commented Aug 14, 2024

3. Because of something unknown that is causing the windows embuilder steps to take unreasonably long (unrelated to this pr) those legs appear to be even more likely to timeout with the additional signing.

Interestingly this slowdown appears to only happen on the official build.

@lewing lewing merged commit f5ca0b7 into dotnet:main Aug 14, 2024
13 checks passed
@lewing lewing deleted the lewing/update-signing branch August 14, 2024 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants