Page MenuHome

Initial implementation of code signing routines
AcceptedPublic

Authored by Sergey Sharybin (sergey) on Fri, Nov 8, 4:23 PM.

Details

Summary

This changes integrates code signing steps into a buildbot worker
process.

The configuration requires having a separate machine running with
a shared folder access between the signing machine and worker machine.

Actual signing is happening as a "POST-INSTALL" script run by CMake,
which allows to sign any binary which ends up in the final bundle.
Additionally, such way allows to avoid signing binaries in the build
folder.
Such complexity is needed on platforms which are using CPack to
generate final bundle: CPack runs INSTALL target into its own location,
so it is useless to run signing on a folder which is considered INSTALL
by the buildbot worker.

There is a signing script which can be used as a standalone tool,
making it possible to hook up signing for macOS's bundler.

There is a dummy Linux signer implementation, which can be activated
by returning True from mock_codesign in linux_code_signer.py.
Main purpose of this signer is to give an ability to develop the
scripts on Linux environment, without going to Windows VM.

The code is based on D6036 from Nathan Letwory.

Diff Detail

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

At this state the code is so called "seems to work". Tested it in a local environment of both buildbot server and worker VMs.

The code signing VM is quite small, and is ready to be deployed to production.
For the Windows builder machine I'll need to bring the actual builder down, do all required updates and tweaks to the environment (to be able to generate .msi archives).

An extensive patch, but looks good.

This revision is now accepted and ready to land.Mon, Nov 11, 6:09 AM
Sybren A. Stüvel (sybren) requested changes to this revision.Tue, Nov 12, 12:03 PM
Sybren A. Stüvel (sybren) added inline comments.
CMakeLists.txt
593–596

Maybe add a line "We use this for the code signing" yadayada to it?

build_files/buildbot/codesign/absolute_and_relative_filename.py
39–40

You could use this syntax to have the comment as docstring for the class attribute. Could help IDEs.

base_dir: Path
"""Base directory which is where relative_filepath is relative to."""
73–77

This could be written as a list expression:

result = [AbsoluteAndRelativeFileName(base_dir, filename)
          for filename in base_dir.glob('**/*')
          if filename.is_file()]

I'll leave it up to you to determine which one is clearer.

build_files/buildbot/codesign/archive_with_indicator.py
95–97

absence → absent, both in function name and in comment.

build_files/buildbot/codesign/base_code_signer.py
25

needs to eb → need to be

'Either directory' implies there are two directories ('either this one or that one'). Probably correct would be 'from either a directory to sign all signable files in there, or by filename of a single file to sign'.

27

am → an

43–44

READ → READY

And of course a 'ready' file indicates something is 'ready'. I would write something like "which indicates that all signable files in the archive have been signed, and the archive is ready for further processing by the buildbot worker".

65

I would annotate as Iterable, not as List. You don't use any list-specific properties here, so it doesn't matter whether it's a list, tuple, generator, or other iterable.

97

is → are

103–104

Do the paths on those machines need to be identical? Or can they have different base paths (e.g. /shared on one machine and /mnt/shared on another)?

108–129

As of yet, it's undefined what is the life cycle of BaseCodeSigner instances. Some of these properties are valid for multiple signing sessions, but others are specific to a single archive. Is it like a server that handles multiple signing actions, or is it instantiated for each new signing action? Might be nice to add this to the class docstring.

150

servers → serves

152–153

By its nature, a buildbot worker only produces one build at a time and never performs concurrent builds.

155

remained → remaining

158

the environment

187

need

193

Shouldn't this actually do something other than return an empty list? If it's a function that's intentionally doing nothing and must be overridden in a subclass, mark it as @abc.abstractmethod.

197–198

See PEP 257, first line should be short enough to not wrap, and should be followed by an empty line.

207–208

Just use time.monotonic() directly instead of storing in a temporary variable; so time_slept_in_seconds = time.monotonic() - time_start. This would save a braincell from checking where else that variable is used.

219

override → overwrite

220

maybe add "but other metadata, such as timestamps, are not" for completeness.

232

Add docstring to explain what path is, or rename the parameter so that its meaning is clear.

328

I would add self.unsigned_storage_dir to the logged message so that it's easy to see where files are expected.

build_files/buildbot/codesign/config_builder.py
36

Eek, old copy-paste. Let's link to the Python 3 documentation, so https://docs.python.org/3/library/logging.config.html#configuration-dictionary-schema

build_files/buildbot/codesign/config_server_template.py
37

In the config template I would make things as close to the final configuration as possible. So, if people are assumed to set CERTIFICATE_FILEPATH to a concrete value, write CERTIFICATE_FILEPATH = Path('/some/path/to/certificate.pfx')

39

same old URL as above

build_files/buildbot/codesign/linux_code_signer.py
33

A filter separates things into two parts (one that stays in the filter, like the coffee grinds, and one that seeps through, like the coffee). Always document what a boolean means ("sign this file" or "ignore this file").

Also, the function name uses the plural "files" whereas the argument indicates a single file. This is confusing.

34

This conversion to string can be avoided by using the Path object itself:

if file.relative_filepath == Path('blender'):
...
if file.relative_filepath.parts()[-3:] == ('python', 'bin', 'python'):
...
if file.relative_filepath.suffix == '.so':
...

If you don't want to construct a Path('blender') instance every invocation, just pre-create it.

45–46

Add a docstring explaining why this is useful. Maybe it should be reading a configuration option instead of requiring hacking the code.

62

I find list comprehensions much clearer here:

files_to_be_signed = [file for file in all_files
                      if filter_files_to_be_signed(file)]
68–69

Use for file_index, file in enumerate(files) and then use {file_index+1} in the print statement.

70–71

Shouldn't this use the logging framework?

build_files/buildbot/codesign/simple_code_signer.py
35

Add type annotation for self.code_signer: Optional[BaseCodeSigner] so that the None-ness can be properly checked by mypy.

build_files/buildbot/codesign/windows_code_signer.py
37

os → is

No file is going to endswith('*.msi').

37–40

Such globals should be ALL_CAPS as they should be treated as constants.

39–40

This should be a set, not a tuple, to allow for fast suffix in blacklist_file_prefixes queries.

44

Path.name is already a string.

45–47

This can be simplified with the any() built-in:

base_name = file.relative_filepath.name
if any(base_name.startswith(prefix) for prefix in blacklist_file_prefixes):
    return False
49–53

Replace this entire block with return file.relative_filepath.suffix in extension_to_be_signed

57–68

How is this OS-specific? Couldn't this be in the base class?

93

Don't count iterations of a for-loop, just use for file_index, file in enumerate(files).

build_files/buildbot/codesign_server_windows.py
26

Maybe use shutil.which() to check whether the signing tool is available, before starting?

build_files/buildbot/slave_codesign.cmake
23

binary bundle?

25

only have → there only is

27

another → other

build_files/buildbot/slave_codesign.py
27

I thought there were "signed" and "unsigned" directories?

39

Pass type=Path here. This makes the conversion to a Path later unnecessary, and IIRC makes argparse spit out a somewhat nicer error when things can't be converted.

52–53

This is just another and-clause.

build_files/buildbot/slave_compile.py
95

asn? 'ask' maybe?

build_files/buildbot/slave_pack.py
44

Given the widespread use of pathlib in this code, path should really be annotated as str (assuming it is based on the Path(path) below.

This revision now requires changes to proceed.Tue, Nov 12, 12:03 PM
Sergey Sharybin (sergey) marked 35 inline comments as done.Tue, Nov 12, 2:00 PM
Sergey Sharybin (sergey) added inline comments.
CMakeLists.txt
593–596

If we start doing such remarks the CMake file will blow in size really rapidly, without really helping other developers. Why would they care what we use specific fine tuning knobs for? This is a know for THEM :)

build_files/buildbot/codesign/absolute_and_relative_filename.py
39–40

Looks weird. But if this is something we want to stick to lets stickto it everywhere (addons, "built-in" modules and so on).

All such possible-to-do-things-which-are-acceptable-by-developers-and-pep8 should go to the style guide https://wiki.blender.org/wiki/Style_Guide/Python

Ideally also have linter :) But oh dreams :)

73–77

Well, in this exact case the list expression is not too bad.
The bad thing begins when you start extending the logic: modifications are usually done on top of the one-liner things, making it less readable.

Is there a performance benefit over the syntax you've shown?

build_files/buildbot/codesign/base_code_signer.py
25

I like your wording.

65

Fine by me. Done.

103–104

Well, mount point might differ. But the "source" of it should be the same (so then builder and signer can share files using this directory).

Attempted to make it more clear wording.

108–129

Added note about this in the comment now.

Long story short: buildbot side is short-lived, server side is long-lived.
The properties here are "invariant", as in are not supposed to change after class construction.
Is this something you've been worrying about?

193

Purely abstract method. Marked as such now.

197–198

At the same time they don't mention length of the line? :)

Shortened it, avoiding spoon-feeding which is done at the top of the file.

220

Good point. Done.

build_files/buildbot/codesign/config_builder.py
36

Done.

P.S. Go fix it in blender-id ;)

build_files/buildbot/codesign/config_server_template.py
37

Well, it actually a platform-specific format and path.
Added more notes in the comment.

P.S. Not sure how exactly macOS signing will work, but it is not impossible that CERTIFICATE_FILEPATH will not make sense there (since from my memories the certificate there is supposed to be imported to a current keychain, and no file is used). But such specifics are easier to be addressed once the actual work is being done (as an opposite of trying to over-engineer 100% flexible design).

build_files/buildbot/codesign/linux_code_signer.py
45–46

Made naming better and described usecase.

Moving to the configuration wouldn't make it any easier: the builder configuration is in the Git repository anyway (so you can't avoid accidental commits of an unintended change).

Further, when things are lcoalized is always easier to find.

And also, this is one single platform thing, so moving this to a configuration will be misleading on other platforms. And even useless.

70–71

Indeed.

build_files/buildbot/codesign/simple_code_signer.py
35

Done.

Did you check that the code CAN be checked with mypy without a lot of burden caused by legacy nature of some other dependencies here?

build_files/buildbot/codesign/windows_code_signer.py
57–68

Yeah, after discussion here think there is a way to do it cleanly and in a way which feels that wouldn't need to be changed for macOS.

build_files/buildbot/slave_codesign.py
27

Well, this is a specific of communication between whoever-wants-file-to-be-signed and actual code signing server.

This script accepts and input and as a result of this script that input is signed. The fact that there are all the complexity of inter-virtual-machine communication is something users of this scripts shouldn't worry about.

Extended comment a bit to make it more explicit.

52–53

What is the informative message here?

build_files/buildbot/slave_pack.py
44

Can not do it here.

This script runs using system-wide python, which is not guaranteed to be new enough to support annotations.

For the rest of the patch it's not really an issue, since in there we do know that the Windows builder has been upgraded to Python 3.8.

Sergey Sharybin (sergey) marked 10 inline comments as done.

Addressed feedback.

Sybren A. Stüvel (sybren) added inline comments.
build_files/buildbot/codesign/base_code_signer.py
108–129

Is this something you've been worrying about?

Nope, just clarity, no worries about functionality.

110

o na → on a

build_files/buildbot/codesign/linux_code_signer.py
39

-> bool?

build_files/buildbot/slave_codesign.py
52–53

I mean, I don't understand why there are two if statements. This is the same:

if (sys.platform == 'win32'
    and path_to_sign.name == 'Unspecified'
    and 'WIX' in str(path_to_sign)):
This revision is now accepted and ready to land.Tue, Nov 12, 2:15 PM
build_files/buildbot/slave_codesign.py
52–53

Maybe something like this then?

if sys.platform == 'win32':
    # When using the WIX packer, we get a path like 'C:\the\real\path\WIX Unspecified'
    if path_to_sign.name == 'Unspecified' and 'WIX' in str(path_to_sign):
        path_to_sign = path_to_sign.parent
Sergey Sharybin (sergey) marked 2 inline comments as done.Tue, Nov 12, 2:46 PM

Round of mainly small fixes.

But also extended comment to WIX workaround.

Eeh, spammy fix for spelling of CPack :)