Merged PR 8407803: PR build pipeline improvements + updated README

Disable time-consuming no-ASM tests in PR builds. Update README. Rename option to enable msbignum/RSA32 tests, and add it as an argument to the build script.
2023-02-07 03:13:42 +00:00 · 2023-02-07 03:13:42 +00:00 · 3b20ccd561
--- a/.pipelines/OneBranch.PullRequest.yml
+++ b/.pipelines/OneBranch.PullRequest.yml
@ -39,28 +39,34 @@ extends:
        parameters:
          arch: 'AMD64'
          config: 'Debug'
+          additionalArgs: '--test-legacy-impl'
      - template: .pipelines/templates/build-windows.yml@self
        parameters:
          arch: 'AMD64'
          config: 'Release'
+          additionalArgs: '--test-legacy-impl'
      - template: .pipelines/templates/build-windows.yml@self
        parameters:
          arch: 'X86'
          config: 'Debug'
+          additionalArgs: '--test-legacy-impl'
      - template: .pipelines/templates/build-windows.yml@self
        parameters:
          arch: 'X86'
          config: 'Release'
+          additionalArgs: '--test-legacy-impl'
      - template: .pipelines/templates/build-windows.yml@self
        parameters:
          arch: 'AMD64'
          config: 'Release'
+          skipTests: true
          additionalArgs: '--no-asm'
          identifier: 'NoAsm'
      - template: .pipelines/templates/build-windows.yml@self
        parameters:
          arch: 'X86'
          config: 'Release'
+          skipTests: true
          additionalArgs: '--no-asm'
          identifier: 'NoAsm'

@ -109,6 +115,7 @@ extends:
          config: 'Release'
          cc: 'gcc'
          cxx: 'g++'
+          skipTests: true
          additionalArgs: '--no-asm'
          identifier: 'NoAsm'
      - template: .pipelines/templates/build-linux.yml@self
@ -117,6 +124,7 @@ extends:
          config: 'Release'
          cc: 'clang'
          cxx: 'clang++'
+          skipTests: true
          additionalArgs: '--no-asm'
          identifier: 'NoAsm'
      - template: .pipelines/templates/build-linux.yml@self
@ -125,6 +133,7 @@ extends:
          config: 'Release'
          cc: 'gcc'
          cxx: 'g++'
+          skipTests: true
          additionalArgs: '--no-asm --no-fips'
          identifier: 'NoAsm'
      - template: .pipelines/templates/build-linux.yml@self
--- a/.pipelines/templates/build-linux.yml
+++ b/.pipelines/templates/build-linux.yml
@ -22,6 +22,9 @@ parameters:
  values:
  - g++
  - clang++
+- name: skipTests # Skip time-consuming tests for PR builds (e.g. no-ASM build)
+  type: boolean
+  default: false
 - name: additionalArgs # Additional arguments to pass to the build script
  type: string
  default: ''
@ -89,50 +92,51 @@ jobs:
        arguments: 'bin --arch ${{ parameters.arch }} --config ${{ parameters.config }} --cc ${{ parameters.cc }} --cxx ${{ parameters.cxx }} ${{ parameters.additionalArgs }}'
        workingDirectory: $(Build.SourcesDirectory)

-    - ${{ if ne(parameters.arch, 'ARM64') }}:
-      - task: PythonScript@0
-        displayName: 'Run unit tests'
-        inputs:
-          scriptSource: 'filePath'
-          scriptPath: scripts/test.py
-          arguments: 'bin noperftests'
-          workingDirectory: $(Build.SourcesDirectory)
+    - ${{ if ne(parameters.skipTests, true) }}:
+      - ${{ if ne(parameters.arch, 'ARM64') }}:
+        - task: PythonScript@0
+          displayName: 'Run unit tests'
+          inputs:
+            scriptSource: 'filePath'
+            scriptPath: scripts/test.py
+            arguments: 'bin noperftests'
+            workingDirectory: $(Build.SourcesDirectory)
+
+        - ${{ if ne(parameters.config, 'Sanitize') }}:
+          - task: PythonScript@0
+            displayName: 'Run dynamic unit tests'
+            inputs:
+              scriptSource: 'filePath'
+              scriptPath: scripts/test.py
+              arguments: 'bin dynamic:bin/module/generic/libsymcrypt.so noperftests'
+              workingDirectory: $(Build.SourcesDirectory)
+
+      - ${{ if eq(parameters.arch, 'AMD64') }}:
+        - task: PythonScript@0
+          displayName: 'Run unit tests (test YMM save/restore)'
+          inputs:
+            scriptSource: 'filePath'
+            scriptPath: scripts/test.py
+            arguments: '--glibc-disable-ymm bin testSaveYmm'
+            workingDirectory: $(Build.SourcesDirectory)
+
+      - ${{ if eq(parameters.arch, 'ARM64') }}:
+        - task: PythonScript@0
+          displayName: 'Run unit tests'
+          inputs:
+            scriptSource: 'filePath'
+            scriptPath: scripts/test.py
+            arguments: '--emulator qemu-aarch64 --emulator-lib-dir /usr/aarch64-linux-gnu/ bin noperftests +symcrypt -dh -dsa -rsa'
+            workingDirectory: $(Build.SourcesDirectory)

-      - ${{ if ne(parameters.config, 'Sanitize') }}:
        - task: PythonScript@0
          displayName: 'Run dynamic unit tests'
          inputs:
            scriptSource: 'filePath'
            scriptPath: scripts/test.py
-            arguments: 'bin dynamic:bin/module/generic/libsymcrypt.so noperftests'
+            arguments: '--emulator qemu-aarch64 --emulator-lib-dir /usr/aarch64-linux-gnu/ bin dynamic:bin/module/generic/libsymcrypt.so noperftests +symcrypt -dh -dsa -rsa'
            workingDirectory: $(Build.SourcesDirectory)

-    - ${{ if eq(parameters.arch, 'AMD64') }}:
-      - task: PythonScript@0
-        displayName: 'Run unit tests (test YMM save/restore)'
-        inputs:
-          scriptSource: 'filePath'
-          scriptPath: scripts/test.py
-          arguments: '--glibc-disable-ymm bin testSaveYmm'
-          workingDirectory: $(Build.SourcesDirectory)
-
-    - ${{ if eq(parameters.arch, 'ARM64') }}:
-      - task: PythonScript@0
-        displayName: 'Run unit tests'
-        inputs:
-          scriptSource: 'filePath'
-          scriptPath: scripts/test.py
-          arguments: '--emulator qemu-aarch64 --emulator-lib-dir /usr/aarch64-linux-gnu/ bin noperftests +symcrypt -dh -dsa -rsa'
-          workingDirectory: $(Build.SourcesDirectory)
-
-      - task: PythonScript@0
-        displayName: 'Run dynamic unit tests'
-        inputs:
-          scriptSource: 'filePath'
-          scriptPath: scripts/test.py
-          arguments: '--emulator qemu-aarch64 --emulator-lib-dir /usr/aarch64-linux-gnu/ bin dynamic:bin/module/generic/libsymcrypt.so noperftests +symcrypt -dh -dsa -rsa'
-          workingDirectory: $(Build.SourcesDirectory)
-
    - task: PythonScript@0
      displayName: 'Package build output'
      inputs:
--- a/.pipelines/templates/build-windows.yml
+++ b/.pipelines/templates/build-windows.yml
@ -11,6 +11,9 @@ parameters:
  values:
  - Debug
  - Release
+- name: skipTests # Skip time-consuming tests for PR builds (e.g. no-ASM build)
+  type: boolean
+  default: false
 - name: additionalArgs # Additional arguments to pass to the build script
  type: string
  default: ''
@ -49,16 +52,15 @@ jobs:
      arguments: 'bin --arch ${{ parameters.arch }} --config ${{ parameters.config }} ${{ parameters.additionalArgs }}'
      workingDirectory: $(Build.SourcesDirectory)

-  - task: PythonScript@0
-    displayName: 'Run unit tests'
-    inputs:
-      scriptSource: 'filePath'
-      scriptPath: scripts\test.py
-      arguments: 'bin noperftests'
-      workingDirectory: $(Build.SourcesDirectory)
+  - ${{ if ne(parameters.skipTests, true) }}:
+    - task: PythonScript@0
+      displayName: 'Run unit tests'
+      inputs:
+        scriptSource: 'filePath'
+        scriptPath: scripts\test.py
+        arguments: 'bin noperftests'
+        workingDirectory: $(Build.SourcesDirectory)

-  # Cannot currently run ARM/ARM64 binaries on the Windows pipeline
-  - ${{ if or(eq(parameters.arch, 'AMD64'), eq(parameters.arch, 'X86')) }}:
    - task: PythonScript@0
      displayName: 'Run dynamic unit tests'
      inputs:
@ -67,10 +69,10 @@ jobs:
        arguments: 'bin dynamic:bin\exe\symcrypttestmodule.dll noperftests'
        workingDirectory: $(Build.SourcesDirectory)      

-    - task: PythonScript@0
-      displayName: 'Package build output'
-      inputs:
-        scriptSource: 'filePath'
-        scriptPath: scripts\package.py
-        arguments: 'bin ${{ parameters.arch }} ${{ parameters.config }} generic bin'
-        workingDirectory: $(Build.SourcesDirectory) 
+  - task: PythonScript@0
+    displayName: 'Package build output'
+    inputs:
+      scriptSource: 'filePath'
+      scriptPath: scripts\package.py
+      arguments: 'bin ${{ parameters.arch }} ${{ parameters.config }} generic bin'
+      workingDirectory: $(Build.SourcesDirectory) 
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@ -48,6 +48,14 @@ if(SYMCRYPT_FIPS_BUILD)
    add_compile_options(-DSYMCRYPT_DO_FIPS_SELFTESTS=1)
 endif()

+option(
+    SYMCRYPT_TEST_LEGACY_IMPL
+    "When enabled, the SymCrypt unit tests will be linked against and configured to run compatibility and performance tests on the legacy
+    RSA32 and msbignum cryptographic implementations. This requires access to private static libraries which are not licensed
+    for use outside of Windows, and thus can only be used by Microsoft employees building internally. It only affects the unit tests, 
+    and does not change the functionality of the SymCrypt library itself."
+    OFF)
+
 option(
    SYMCRYPT_STACK_PROTECTION
    "When enabled, SymCrypt will enable various stack protection mechanisms to protect against buffer overruns and the like. Only
--- a/README.md
+++ b/README.md
@ -22,11 +22,11 @@ Like any engineering project, SymCrypt is a compromise between conflicting requi
 In some of our Linux modules, SymCrypt uses [Jitterentropy](https://github.com/smuellerDD/jitterentropy-library)
 as a source of FIPS-certifiable entropy. To build these modules, you will need to ensure that the
 jitterentropy-library submodule is also cloned. You can do this by running
-`git submodule update --init -- jitterentropy-library` after cloning.
+`git submodule update --init -- 3rdparty/jitterentropy-library` after cloning.

-The SymCryptDependencies submodule provides the RSA32 and msbignum implementations which are used as benchmarks
-in the unit tests when compiled on Windows. Due to licensing restrictions, we cannot release these libraries
-publicly, so this submodule will only be cloneable by Microsoft employees with access to our private
+The `unittest/SymCryptDependencies` submodule provides the RSA32 and msbignum implementations which are used as
+benchmarks in the unit tests when compiled on Windows. Due to licensing restrictions, we cannot release these
+libraries publicly, so this submodule will only be cloneable by Microsoft employees with access to our private
 Azure DevOps repository. If you are external to Microsoft, you can ignore this submodule. It is only used in
 the unit tests and does not change the behavior of the SymCrypt product code.

@ -54,9 +54,32 @@ repository is provided *as is*, without warranty of any kind, express or implied
 warranties of merchantability, fitness for a particular purpose and noninfringement (see our [LICENSE](./LICENSE)).

 ## Build Instructions
-1. For Microsoft employees building the library internally, to include msbignum and RSA32 implementation benchmarks in the unit tests:
-    1. Make sure the SymCryptDependencies submodule is initialized by following the steps above (`git submodule update --init`)
-    1. In step 4 below, add the additional cmake argument `-DSYMCRYPT_INTERNAL_BUILD=1`
+For Microsoft employees building the library internally, to include msbignum and RSA32 implementation benchmarks in
+the unit tests, make sure the SymCryptDependencies submodule is initialized by following the steps above
+(`git submodule update --init`). When building, provide the additional CMake argument `-DSYMCRYPT_TEST_LEGACY_IMPL=ON`.
+This only affects the unit tests, and does not change the functionality of the SymCrypt library itself.
+
+### Using Python scripts
+Building SymCrypt can be complicated due to the number of platforms, architectures and targets supported. To improve
+ease of use and have a consistent build solution that can be leveraged by both developers and our automated CI pipelines,
+we have created a set of Python scripts to help with building, testing and packaging.
+
+1. To build SymCrypt, run `scripts/build.py build_dir` where `build_dir` is the desired build output directory.
+    * To see additional options, run `scripts/build.py --help`.
+1. To run the unit tests after a build has finished, run `scripts/test.py build_dir`.
+    * Additional positional arguments will be passed directly to the unit test executable.
+1. To package up the built binaries into an archive, run `scripts/package.py build_dir arch configuration module_name release_dir`, were:
+    * `build_dir` is the build output directory
+    * `arch` is the architecture that the build was created for
+    * `configuration` is the build configuration (Debug, Release, Sanitize)
+    * `module_name` is the name of the module you wish to package (currently only relevant for Linux builds)
+    * `release_dir` is the output directory for the release archive
+
+### Building with CMake
+If you don't want to use the Python helper scripts, or if they do not support the specific build flags you desire, you can
+build SymCrypt by directly invoking CMake. Note that Python is still required for translating SymCryptAsm and building the
+Linux modules with FIPS integrity checks.
+
 1. Run `cmake -S . -B bin` to configure your build. You can add the following optional CMake arguments to change build options:
    * `-DSYMCRYPT_TARGET_ARCH=<AMD64|X86|ARM64>` to choose a target architecture. If not specified, it will default to the host system architecture.
      * To cross-compile for Windows X86 from Windows AMD64, you must also use `-A Win32`
--- a/scripts/build.py
+++ b/scripts/build.py
@ -93,10 +93,12 @@ def configure(args : argparse.Namespace) -> None:
    if not args.fips:
        cmake_args.append("-DSYMCRYPT_FIPS_BUILD=OFF")

+    if args.test_legacy_impl:
+        cmake_args.append("-DSYMCRYPT_TEST_LEGACY_IMPL=ON")
+
    if args.toolchain:
        cmake_args.append("-DCMAKE_TOOLCHAIN_FILE=" + str(args.toolchain))

-
    if args.verbose:
        cmake_args.append("-DCMAKE_VERBOSE_MAKEFILE=ON")

@ -140,6 +142,9 @@ def main() -> None:
    parser.add_argument("--cxx", type = str, help = "Specify the C++ compiler to use. If not provided, uses platform default.")
    parser.add_argument("--no-asm", action = "store_false", dest = "asm", help = "Disable handwritten ASM optimizations.", default = True)
    parser.add_argument("--no-fips", action = "store_false", dest = "fips", help = "Disable FIPS selftests and postprocessing of binary. Currently only affects Linux targets.", default = True)
+    parser.add_argument("--test-legacy-impl", action = "store_true",
+        help = "Build unit tests with support for legacy Windows cryptographic implementations. Requires access to private static libraries.",
+        default = False)
    parser.add_argument("--toolchain", type = pathlib.Path, help = "Toolchain file to use for cross-compiling.")
    parser.add_argument("--clean", action = "store_true", help = "Clean output directory before building.")
    parser.add_argument("--configure-only", action = "store_true", help = "Run CMake configuration, but do not build.")
--- a/unittest/CMakeLists.txt
+++ b/unittest/CMakeLists.txt
@ -83,11 +83,15 @@ include_directories(inc)
 include_directories(lib)
 include_directories(resource)

-if(SYMCRYPT_INTERNAL_BUILD)
+if(SYMCRYPT_TEST_LEGACY_IMPL)
  find_library(RSA32_LIB rsa32.lib HINTS SymCryptDependencies/${SYMCRYPT_TARGET_ARCH})
  find_library(MSBIGNUM_LIB msbignum.lib HINTS SymCryptDependencies/${SYMCRYPT_TARGET_ARCH})

  include_directories(SymCryptDependencies/inc)
+else()
+  # For external builds, do not include msbignum and RSA32 which are not licensed for external use
+  add_compile_options(-DINCLUDE_IMPL_MSBIGNUM=0)
+  add_compile_options(-DINCLUDE_IMPL_RSA32=0)
 endif()

 add_subdirectory(lib)
--- a/unittest/exe_Win7nLater/CMakeLists.txt
+++ b/unittest/exe_Win7nLater/CMakeLists.txt
@ -7,16 +7,10 @@ set_source_files_properties(symcryptunittest.rc PROPERTIES LANGUAGE RC)

 add_compile_options(-DTESTDRIVER_NAME=\"SymCryptDriver_win7nlater.sys\")

-# For external builds, do not include msbignum and RSA32 which are not licensed for external use
-if(NOT SYMCRYPT_INTERNAL_BUILD)
-    add_compile_options(-DINCLUDE_IMPL_MSBIGNUM=0)
-    add_compile_options(-DINCLUDE_IMPL_RSA32=0)
-endif()
-
 add_executable(symcryptunittest_win7nlater ${SOURCES})
 target_link_libraries(symcryptunittest_win7nlater symcryptunittest_lib symcrypt_usermodewin7 bcrypt ntdll)

-if(SYMCRYPT_INTERNAL_BUILD)
+if(SYMCRYPT_TEST_LEGACY_IMPL)
    # For internal builds, append RSA32 and msbignum.
    # Calling target_link_libraries again appends to the existing list.
    target_link_libraries(symcryptunittest_win7nlater ${RSA32_LIB} ${MSBIGNUM_LIB})
--- a/unittest/exe_Win8_1nLater/CMakeLists.txt
+++ b/unittest/exe_Win8_1nLater/CMakeLists.txt
@ -7,16 +7,10 @@ set_source_files_properties(symcryptunittest.rc PROPERTIES LANGUAGE RC)

 add_compile_options(-DTESTDRIVER_NAME=\"SymCryptDriver_win7nlater.sys\")

-# For external builds, do not include msbignum and RSA32 which are not licensed for external use
-if(NOT SYMCRYPT_INTERNAL_BUILD)
-    add_compile_options(-DINCLUDE_IMPL_MSBIGNUM=0)
-    add_compile_options(-DINCLUDE_IMPL_RSA32=0)
-endif()
-
 add_executable(symcryptunittest_win8_1nlater ${SOURCES})
 target_link_libraries(symcryptunittest_win8_1nlater symcryptunittest_lib symcrypt_usermodewin8_1 bcrypt ntdll)

-if(SYMCRYPT_INTERNAL_BUILD)
+if(SYMCRYPT_TEST_LEGACY_IMPL)
    # For internal builds, append RSA32 and msbignum.
    # Calling target_link_libraries again appends to the existing list.
    target_link_libraries(symcryptunittest_win8_1nlater ${RSA32_LIB} ${MSBIGNUM_LIB})
--- a/unittest/exe_legacy/CMakeLists.txt
+++ b/unittest/exe_legacy/CMakeLists.txt
@ -7,16 +7,11 @@ set_source_files_properties(symcryptunittest.rc PROPERTIES LANGUAGE RC)

 add_compile_options(-DTESTDRIVER_NAME=\"SymCryptDriver_legacy.sys\")

-# For external builds, do not include msbignum and RSA32 which are not licensed for external use
-if(NOT SYMCRYPT_INTERNAL_BUILD)
-    add_compile_options(-DINCLUDE_IMPL_MSBIGNUM=0)
-    add_compile_options(-DINCLUDE_IMPL_RSA32=0)
-endif()

 add_executable(symcryptunittest_legacy ${SOURCES})
 target_link_libraries(symcryptunittest_legacy symcryptunittest_lib symcrypt_generic bcrypt ntdll)

-if(SYMCRYPT_INTERNAL_BUILD)
+if(SYMCRYPT_TEST_LEGACY_IMPL)
    # For internal builds, append RSA32 and msbignum.
    # Calling target_link_libraries again appends to the existing list.
    target_link_libraries(symcryptunittest_legacy ${RSA32_LIB} ${MSBIGNUM_LIB})
--- a/unittest/exe_test/CMakeLists.txt
+++ b/unittest/exe_test/CMakeLists.txt
@ -7,16 +7,10 @@ set_source_files_properties(symcryptunittest.rc PROPERTIES LANGUAGE RC)

 add_compile_options(-DTESTDRIVER_NAME=\"SymCryptDriver_test.sys\")

-# For external builds, do not include msbignum and RSA32 which are not licensed for external use
-if(NOT SYMCRYPT_INTERNAL_BUILD)
-    add_compile_options(-DINCLUDE_IMPL_MSBIGNUM=0)
-    add_compile_options(-DINCLUDE_IMPL_RSA32=0)
-endif()
-
 add_executable(symcryptunittest ${SOURCES})
 target_link_libraries(symcryptunittest symcryptunittest_lib symcrypt_common bcrypt ntdll)

-if(SYMCRYPT_INTERNAL_BUILD)
+if(SYMCRYPT_TEST_LEGACY_IMPL)
    # For internal builds, append RSA32 and msbignum.
    # Calling target_link_libraries again appends to the existing list.
    target_link_libraries(symcryptunittest ${RSA32_LIB} ${MSBIGNUM_LIB})
--- a/unittest/lib/CMakeLists.txt
+++ b/unittest/lib/CMakeLists.txt
@ -88,11 +88,6 @@ if(WIN32)
    # DNDEBUG is required to link with msbignum. This should eventually be removed.
    add_compile_options(-DNDEBUG)
    add_compile_options(-D_CRT_SECURE_NO_WARNINGS)
-
-    if(NOT SYMCRYPT_INTERNAL_BUILD)
-        add_compile_options(-DINCLUDE_IMPL_MSBIGNUM=0)
-        add_compile_options(-DINCLUDE_IMPL_RSA32=0)
-    endif()
 else()
    add_compile_options(-Wno-write-strings)
    add_compile_options(-DINCLUDE_IMPL_CAPI=0)