Page MenuHome

Sign executable and thumbnail DLL.
Needs RevisionPublic

Authored by Nathan Letwory (jesterking) on Thu, Oct 10, 12:32 PM.

Details

Summary

This process is injected in the pack procedure
for Windows.

Waits until signed files are available.

  • pack blender.exe and BlendThumb.dll in tosign.zip and write to_sign on shared drive
  • wait for signed file to appear in designated directory.
  • unzip to original location

On the signing machine winsigner.py needs to be
running and have access to the signing certificate.

Current proposed winsigner.py can be found at P1137

Diff Detail

Repository
rB Blender
Branch
jesterKing/codesign (branched from master)
Build Status
Buildable 5337
Build 5337: arc lint + arc unit

Event Timeline

possible improvements:

  • install timeout, if hit fail the process.
  • handle file io exceptions, fail the process if any
  • add the msi signing part - winsigner.py already should be able to handle this

You can put winsigner.py in build_files/buildbot/ as well, easier if the client and server code is in the same repository. We can still check that no bad changes were done to the script when updating it manually.

You could put most of the client and server code in a buildbot_codesign.py that can be imported as a module by both slave_pack.py and a codesign_server.py. That way you can share a bit of code.

build_files/buildbot/slave_pack.py
127–128

Do we need to sign more files, like the Python executables and libraries? Or is it enough to sign the executable, and that is then allowed to load any unsigned dll?

139

We may want to put a timeout here so it fails rather than hanging forever. The buildbot may have a timeout for steps already, but I'm not sure or how long it is.

Sign executable and thumbnail DLL.

This process is injected in the pack procedure
for Windows.

Waits until signed files are available.

  • pack blender.exe and BlendThumb.dll in tosign.zip and write to_sign on shared drive
  • wait for signed file to appear in designated directory.
  • unzip to original location

On the signing machine winsigner.py needs to be
running and have access to the signing certificate.

Can you make it so the code signing can be enabled/disabled from the buildbot master? Basically add an argument in create_builder_from_arguments in buildbot_utils.py, disabled by default.

Then when this patch is approved it can be committed, and we actually enable it on the buildbot once all the setup is done there.

Brecht Van Lommel (brecht) requested changes to this revision.Thu, Oct 10, 2:23 PM
This revision now requires changes to proceed.Thu, Oct 10, 2:23 PM
Sergey Sharybin (sergey) requested changes to this revision.Thu, Oct 10, 2:35 PM
Sergey Sharybin (sergey) added inline comments.
build_files/buildbot/slave_pack.py
108–109

Can the configuration be moved to a single file, which will be easier to find?

111–114

In 2019, shall we aim to use pathlib in the new code?

114

signed_file is a bad name choice. It is a signature that the signed file exists, and not a file itself.

116

Residue of a debug?

124

Comment is a sentence, starting with a capital letter and ending with a full-stop.

127–128

Either we should sign all binaries and libraries here, or have them signed in the SVN repository.

Just blender and thumbnailer is not not enough here.

139

There is a timeout argument to build steps. For the compilation step we are using timeout of 3600 seconds, for other steps default timeout of 1200 seconds is used.

But even then it's better to have explicit check here, to help troubleshooting what exactly is going wrong.
For the same reason it is REALLY a good idea to log every step.

build_files/buildbot/winsigner.py
8

Missing \ around D:\Dev ?

8–12

Move to a configuration file.

19

Same comment as buildbot's side.

21
incantation = b"signtool sign /v /f {PFX} /t {TIMESTAMP_URL}"
23

Overdocumentation.

36

all_files ?

41
list(...) + list(...) + list(...)
43

Don't do it. Store command as list to begin with.

45

Name allfn doesn't make sense here. It points to a single file, not all of them.

47

No newline at end of file

47–48

Follow pep8.

57

allfn is bad name.

63

Same as above.

67–68

Use touch logic, which is likely yo be more atomic.

build_files/buildbot/slave_pack.py
127–128

Depends on how we want to go about it.

Bare minimum: just the blender.exe , and installer .msi this will stop the ZOMG SCARY CODE! dialogs that windows tends to pop up.
Lets be nice: Add the thumbnailer, it'll be loaded into a different process (explorer.exe) lets be nice and leave a calling card.
Allow users to verify nobody messed with the code: Sign all dll's, exe's and .pyd's that do not have a signature of their own (ie not the ms crt dlls)

Sign executable and thumbnail DLL.

This process is injected in the pack procedure
for Windows.

Waits until signed files are available.

  • pack blender.exe and BlendThumb.dll in tosign.zip and write to_sign on shared drive
  • wait for signed file to appear in designated directory.
  • unzip to original location

On the signing machine winsigner.py needs to be
running and have access to the signing certificate.

Differential Revision: https://developer.blender.org/D6036

Clean up code

  • address variable naming
  • make functions out of all possible parts
  • use pathlib throughout
  • split off configuration
Nathan Letwory (jesterking) marked 21 inline comments as done.Fri, Oct 11, 12:44 PM

I think I addressed most concerns now.

Still need to ensure all necessary files are included in the signing process, but wanted to update the current patch with the latest state for further review.

Apparently updating the diff by amending makes for rather confusing inline commenting, so I marked most as done

Fix copy/paste error in variable name.

Sergey Sharybin (sergey) requested changes to this revision.Fri, Oct 11, 2:29 PM

Please pay more attention to naming and anti-patterns which are being pointed out but still spreading over the code.

build_files/buildbot/buildbot_codesign.py
52

Why is this hardcoded?

55

You can find more clear and meaningful name.

71

You can also use type annotations.

83–84

pathfiles_absolute and pathfiles_relative ?

Even the latter one is not really true, since it's a list of pairs. But clearer name does exist (assuming this is really needed to store pairs, the purpose of this is unclear at this moment).

86–87
allfiles.extend(pathfiles)
117–119

Never re-purpose variables. Especially if they do change type.

filetuple.endswith

Makes no sense, tuple does not have endswith().

141

Double space.

build_files/buildbot/buildbot_config.py
1

buildbot_codesign_config.py ?

Is also a good practice to have buildbot_codesign_config_template.py in the repo which then expected to be copied locally and adjusted to a specific setup.

4

Don't think this is enough for macOS signing process. That could easily take like 30min.

Brecht will know better though.

Also, PEP8 again, spaces around operator.

This revision now requires changes to proceed.Fri, Oct 11, 2:29 PM

Please also add an option to disable code signing with an argument (see my comment above).