Skip to content

Conversation

dongcarl
Copy link
Contributor

@dongcarl dongcarl commented Jun 7, 2021

Based on: #22075
Code reviewers: I recommend reading the new guix-{attest,verify} files instead of trying to read the diff

The following changes resolve many usability improvements which were pointed out to me:

  1. Some maintainers like to extract their "uncodesigned tarball" inside the output/ directory, resulting in the older guix-attest mistakenly attesting to the extracted contents
  2. Maintainers whose GPG keys reside on an external smartcard often need to physically interact with the smartcard as a way to approve the signing operation, having one signature per platform means a lot of fidgeting
  3. Maintainers wishing to sign on a separate machine now has the option of transferring only a subtree of output/, namely output/*/SHA256SUMS.part, in order to perform a signature (you may need to specify an $OUTDIR_BASE env var)
  4. An all.SHA256SUMS file should be usable as the base SHA256SUMS in bitcoin core torrents and on the release server.

For those who sign on an separate machine than the one you do builds on, the following steps will work:

  1. env GUIX_SIGS_REPO=/home/achow101/guix.sigs SIGNER=achow101 NO_SIGN=1 ./contrib/guix/guix-attest
  2. Copy /home/achow101/guix.sigs/<tag>/achow101 (which does not yet have signatures) to signing machine
  3. Sign the SHA256SUMS files:
    for i in "<path-to-achow101>/*.SHA256SUMS"; do
        gpg --detach-sign --local-user "<your-key-here>" --armor --output "$i"{.asc,}
    done
  4. Upload <path-to-achow101> (now with signatures) to guix.sigs

After this change, output directories will now include a SHA256SUMS.part fragment, created immediately after a successful build:

output
└── x86_64-w64-mingw32
    ├── bitcoin-4e069f7589da-win64-debug.zip
    ├── bitcoin-4e069f7589da-win64-setup-unsigned.exe
    ├── bitcoin-4e069f7589da-win64.zip
    ├── bitcoin-4e069f7589da-win-unsigned.tar.gz
    └── SHA256SUMS.part

These SHA256SUMS.part fragments look something like:

3ebd7262b1a0a5bb757fef1f70e7e14033c70f98c059bc4dbfee5d1992b25825  dist-archive/bitcoin-4e069f7589da.tar.gz
def2e7d3de5ab3e3f955344e75151df4f33713f9101f5295bd13c9375bdf633b  x86_64-w64-mingw32/bitcoin-4e069f7589da-win64-debug.zip
643049fe3ee4a4e83a1739607e67b11b7c9b1a66208a6f35a9ff634ba795500e  x86_64-w64-mingw32/bitcoin-4e069f7589da-win64-setup-unsigned.exe
a247a1ccec0ccc2e138c648284bd01f6a761f2d8d6d07d91b5b4a6670ec3f288  x86_64-w64-mingw32/bitcoin-4e069f7589da-win-unsigned.tar.gz
fab76a836dcc592e39c04fd2396696633fb6eb56e39ecbf6c909bd173ed4280c  x86_64-w64-mingw32/bitcoin-4e069f7589da-win64.zip

Meaning that they are valid SHA256SUMS files when sha256sum --check'd at the guix-build-*/output directory level

When guix-attest is invoked, these SHA256SUMS.part files are combined and sorted (by -k2, LC_ALL=C) to create:

  1. noncodesigned.SHA256SUMS for a manifest of all non-codesigned outputs, and
  2. all.SHA256SUMS for a manifest of all outputs including non-codesigned outputs

Then both files are signed, resulting in the following guix.sigs hierarchy:

4e069f7589da/
└── dongcarl
    ├── all.SHA256SUMS
    ├── all.SHA256SUMS.asc
    ├── noncodesigned.SHA256SUMS
    └── noncodesigned.SHA256SUMS.asc

@fanquake
Copy link
Member

fanquake commented Jun 9, 2021

@dongcarl can you rebase now that #22075 is merged.

@dongcarl dongcarl force-pushed the 2021-05-guix-attestation-overhaul branch from d90ca1e to 4cc35da Compare June 9, 2021 15:19
@dongcarl
Copy link
Contributor Author

dongcarl commented Jun 9, 2021

Pushed d90ca1e...4cc35da

@fanquake
Copy link
Member

Concept ACK - Haven't really looked at the changes, but I ran though this and tried signing on a separate machine, which is my usual workflow. Guix sigs for 4cc35da here: bitcoin-core/guix.sigs#23.

@laanwj laanwj added this to the 22.0 milestone Jun 10, 2021
@achow101
Copy link
Member

Is this still a draft?

If you do a build and attest with some hosts, and then another build and attest with more hosts, the second attest will not update either of the .SHA256SUMS file. I think it should because there are some cases where a builder may build some of the hosts at a different time, e.g. if they acquire the macOS SDK after building and attesting.

Otherwise I like the changes done here.

@dongcarl dongcarl marked this pull request as ready for review June 14, 2021 16:46
@laanwj
Copy link
Member

laanwj commented Jun 14, 2021

Concept ACK, changes sound great to me. Will test.

@achow101
Copy link
Member

ACK 4cc35da

Reviewed code and did a build, attest, and verify to test.

@dongcarl
Copy link
Contributor Author

Pushed 4cc35da..e2c40a4

Updated to address the possible user flow @achow101 mentioned here: #22182 (comment)

Specifically: before using an existing noncodesigned.SHA256SUMS file, we will check if it is up-to-date (if it exactly the same as merging all the noncodesigned SHA256SUMS.parts). If it is, it will be used, otherwise, we will error out with a helpful error message prompting the user to deal with the (likely faulty) noncodesigned.SHA256SUMS file.

@achow101
Copy link
Member

ACK e2c40a4

@fanquake
Copy link
Member

ping @Emzy @hebasto as you have both previously tested Guix building.

@hebasto
Copy link
Member

hebasto commented Jun 17, 2021

Concept ACK. Testing...

@hebasto
Copy link
Member

hebasto commented Jun 17, 2021

Not related to this PR, so for future follow ups:
in

check_tools cat env basename mkdir xargs find

xargs and find are not used in this script. But diff and sort are missed.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK e2c40a4, tested on Linux Mint 20.1 (x86_64) with and w/o NO_SIGN=1. Changes in contrib/guix/libexec/codesign.sh and contrib/guix/guix-verify are reviewed only.

See bitcoin-core/guix.sigs#24

@fanquake
Copy link
Member

Ran through building and using NO_SIGN again and produced new signatures for e2c40a4ed5272d72fea997bd936fba28bb753226 which match @hebasto.

@Emzy
Copy link
Contributor

Emzy commented Jun 18, 2021

tested ACK
Run through guix building. Signing device was plugged intro the build machine, so only one signature improved the usability.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 18, 2021
…d hierarchy

e2c40a4 guix-attest: Error out if SHA256SUMS is unexpected (Carl Dong)
4cc35da Rewrite guix-{attest,verify} for new hier (Carl Dong)
28a9c9b Make SHA256SUMS fragment right after build (Carl Dong)

Pull request description:

  Based on:  bitcoin#22075
  Code reviewers: I recommend reading the new `guix-{attest,verify}` files instead of trying to read the diff

  The following changes resolve many usability improvements which were pointed out to me:
  1. Some maintainers like to extract their "uncodesigned tarball" inside the `output/` directory, resulting in the older `guix-attest` mistakenly attesting to the extracted contents
  2. Maintainers whose GPG keys reside on an external smartcard often need to physically interact with the smartcard as a way to approve the signing operation, having one signature per platform means a lot of fidgeting
  3. Maintainers wishing to sign on a separate machine now has the option of transferring only a subtree of `output/`, namely `output/*/SHA256SUMS.part`, in order to perform a signature (you may need to specify an `$OUTDIR_BASE` env var)
  4. An `all.SHA256SUMS` file should be usable as the base `SHA256SUMS` in bitcoin core torrents and on the release server.

  For those who sign on an separate machine than the one you do builds on, the following steps will work:
  1. `env GUIX_SIGS_REPO=/home/achow101/guix.sigs SIGNER=achow101 NO_SIGN=1 ./contrib/guix/guix-attest`
  2. Copy `/home/achow101/guix.sigs/<tag>/achow101` (which does not yet have signatures) to signing machine
  3. Sign the `SHA256SUMS` files:
      ```bash
      for i in "<path-to-achow101>/*.SHA256SUMS"; do
          gpg --detach-sign --local-user "<your-key-here>" --armor --output "$i"{.asc,}
      done
      ```
  5. Upload `<path-to-achow101>` (now with signatures) to `guix.sigs`

  -----

  After this change, output directories will now include a `SHA256SUMS.part` fragment, created immediately after a successful build:
  ```
  output
  └── x86_64-w64-mingw32
      ├── bitcoin-4e069f7589da-win64-debug.zip
      ├── bitcoin-4e069f7589da-win64-setup-unsigned.exe
      ├── bitcoin-4e069f7589da-win64.zip
      ├── bitcoin-4e069f7589da-win-unsigned.tar.gz
      └── SHA256SUMS.part
  ```

  These `SHA256SUMS.part` fragments look something like:
  ```
  3ebd7262b1a0a5bb757fef1f70e7e14033c70f98c059bc4dbfee5d1992b25825  dist-archive/bitcoin-4e069f7589da.tar.gz
  def2e7d3de5ab3e3f955344e75151df4f33713f9101f5295bd13c9375bdf633b  x86_64-w64-mingw32/bitcoin-4e069f7589da-win64-debug.zip
  643049fe3ee4a4e83a1739607e67b11b7c9b1a66208a6f35a9ff634ba795500e  x86_64-w64-mingw32/bitcoin-4e069f7589da-win64-setup-unsigned.exe
  a247a1ccec0ccc2e138c648284bd01f6a761f2d8d6d07d91b5b4a6670ec3f288  x86_64-w64-mingw32/bitcoin-4e069f7589da-win-unsigned.tar.gz
  fab76a836dcc592e39c04fd2396696633fb6eb56e39ecbf6c909bd173ed4280c  x86_64-w64-mingw32/bitcoin-4e069f7589da-win64.zip
  ```

  Meaning that they are valid `SHA256SUMS` files when `sha256sum --check`'d at the `guix-build-*/output` directory level

  When `guix-attest` is invoked, these `SHA256SUMS.part` files are combined and sorted (by `-k2`, `LC_ALL=C`) to create:

  1. `noncodesigned.SHA256SUMS` for a manifest of all non-codesigned outputs, and
  3. `all.SHA256SUMS` for a manifest of all outputs including non-codesigned outputs

  Then both files are signed, resulting in the following `guix.sigs` hierarchy:
  ```
  4e069f7/
  └── dongcarl
      ├── all.SHA256SUMS
      ├── all.SHA256SUMS.asc
      ├── noncodesigned.SHA256SUMS
      └── noncodesigned.SHA256SUMS.asc
  ```

ACKs for top commit:
  achow101:
    ACK e2c40a4
  hebasto:
    ACK e2c40a4, tested on Linux Mint 20.1 (x86_64) with and w/o `NO_SIGN=1`. Changes in `contrib/guix/libexec/codesign.sh` and `contrib/guix/guix-verify` are reviewed only.

Tree-SHA512: 618aacefb0eb6595735a9ab6a98ea6598fce65f9ccf33fa1e7ef93bf140c0f6cfc16e34870c6aa3e4777dd3f004b92a82a994141879870141742df948ec59c1f
fanquake added a commit that referenced this pull request Jul 23, 2021
9f01fed guix/build: Remove vestigial SKIPATTEST.TAG (Carl Dong)

Pull request description:

  No longer needed or referenced by anything. A relic from prior to the great hierarchy overhaul of #22182

ACKs for top commit:
  achow101:
    ACK 9f01fed
  fanquake:
    ACK 9f01fed

Tree-SHA512: a94cf63f0c5cb8dbacf1025b6c0e81b219c2a3c93b3cbcefc239ccde29e602ecd4b717b1d93dbe53cb791a5017236fb09823c034aec42b0c31894fc9e0ab8b21
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 23, 2021
9f01fed guix/build: Remove vestigial SKIPATTEST.TAG (Carl Dong)

Pull request description:

  No longer needed or referenced by anything. A relic from prior to the great hierarchy overhaul of bitcoin#22182

ACKs for top commit:
  achow101:
    ACK 9f01fed
  fanquake:
    ACK 9f01fed

Tree-SHA512: a94cf63f0c5cb8dbacf1025b6c0e81b219c2a3c93b3cbcefc239ccde29e602ecd4b717b1d93dbe53cb791a5017236fb09823c034aec42b0c31894fc9e0ab8b21
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants