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

Check gitian signatures in a verification stage #6

Merged
merged 1 commit into from
Dec 3, 2021

Conversation

patricklodder
Copy link
Member

@patricklodder patricklodder commented Nov 13, 2021

Makes the Dockerfile multi-stage, verifies the binary against dogecoin/gitian.sigs, then builds the final image in a second stage.

Alternative to my proposal in #5 that does everything inside the Dockerfile, making the verification independent from GitHub, improving decentralization but also making the Dockerfile itself much more complex.

@AbcSxyZ
Copy link
Contributor

AbcSxyZ commented Nov 13, 2021

Nice. I'm doing some research & test related to image portability to understand it more, I will test it after that.

@xanimo
Copy link
Member

xanimo commented Nov 13, 2021

Looks good! Threw together something similar last night but yours is cleaner :P Will do a test run when I get back home.

xanimo
xanimo previously approved these changes Nov 14, 2021
Copy link
Member

@xanimo xanimo left a comment

Choose a reason for hiding this comment

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

tested works great. ACK (for whenever you're ready to publish)

Copy link
Contributor

@AbcSxyZ AbcSxyZ left a comment

Choose a reason for hiding this comment

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

Small details as suggestion, but looks good to me. Tested to generate amd & arm executables, working properly.

Depending on the repository organization we will choose, it may need to deal with architecture for the base image using FROM ${ARCH}/ubuntu:bionic.

On pgp verification, did you think about the scenario where a release has only new signers ? With the current release process, their key will be only available for the next tag and not for the current one, which may be a scenario where the verification fails.

1.14.4/x86_64-bionic/Dockerfile Outdated Show resolved Hide resolved
1.14.4/x86_64-bionic/Dockerfile Outdated Show resolved Hide resolved
@patricklodder
Copy link
Member Author

Depending on the repository organization we will choose, it may need to deal with architecture for the base image using FROM ${ARCH}/ubuntu:bionic.

Yes. For the final stage. I think that for the intermediate verify stage, we should standardize on a stable, recent, actively maintained x86_64 (or old-school amd64 as OCI seems to call it) source image - debian:bullseye(-slim)?

On pgp verification, did you think about the scenario where a release has only new signers ? With the current release process, their key will be only available for the next tag and not for the current one, which may be a scenario where the verification fails.

That would be an epic fail, yes. I think that the public keys should be maintained with the signatures on dogecoin/gitian.sigs rather than with the source code. Let's file an issue on dogecoin/dogecoin and see what people think?

@AbcSxyZ
Copy link
Contributor

AbcSxyZ commented Nov 15, 2021

I think that the public keys should be maintained with the signatures on dogecoin/gitian.sigs rather than with the source code.

Can make sense if keys are only used to verify gitian build. Looking at keys documentation, it says pgp can be used for git commits, depend on your usage.

@patricklodder
Copy link
Member Author

With the latest release tag not being signed and no one raising that, I doubt people care. But yes, I agree that we could do different things with this. I'll raise it.

@xanimo
Copy link
Member

xanimo commented Nov 15, 2021

I think that the public keys should be maintained with the signatures on dogecoin/gitian.sigs rather than with the source code.

Can make sense if keys are only used to verify gitian build. Looking at keys documentation, it says pgp can be used for git commits, depend on your usage.

Yeah so if you have gpg set up on your local dev env you can run:

gpg --list-keys --fingerprint #to find your key
git config --global user.signingKey #key...
git config --global commit.gpgSign true
# and then finally either enter manually every time you open a shell or put on last line of your ~/.bashrc:
export GPG_TTY=$(tty)

this will result in the commit showing that green 'verified' as shown here with pats commits (oops realized i didn't have any commits lol):
Screenshot from 2021-11-15 15-46-11

:)

@xanimo
Copy link
Member

xanimo commented Nov 16, 2021

forgot to mention you need to add your gpg key and tick 'verified commits' checkbox in your settings > ssh and gpg keys section in github:
Screenshot from 2021-11-15 15-56-45

@AbcSxyZ
Copy link
Contributor

AbcSxyZ commented Nov 16, 2021

@xanimo This process do not use public keys from contrib/gitian-keys and it's managed by GitHub. I suppose you can check the signature of a commit locally by importing public keys from a repo.

@patricklodder
Copy link
Member Author

I'll rebase this after #14

@AbcSxyZ
Copy link
Contributor

AbcSxyZ commented Nov 29, 2021

It was something that question me, but is the process of downloading PGP keys to check shasum secure ? If not embedded in the Dockerfile, someone who will get access to GitHub repositories for Dogecoin may be able to add his own key, his own signature and his own corrupted binary no ?

I found the following information in docker images documentation about security:

The Dockerfile should be written to help mitigate interception attacks during build. Our requirements focus on three main objectives: verifying the source, verifying author, and verifying the content; these are respectively accomplished by the following: using https where possible; importing PGP keys with the full fingerprint in the Dockerfile to check signatures; embedding checksums directly in the Dockerfile.

@patricklodder
Copy link
Member Author

patricklodder commented Nov 29, 2021

someone who will get access to GitHub repositories for Dogecoin may be able to add his own key, his own signature and his own corrupted binary no ?

Someone that maliciously gains access to dogecoin/dogecoin releases will likely also gain access to the Dockerfile here so it cannot be fully mitigated as long as we use GitHub as a source. We can do some pinning in the Dockerfile but that reduces decentralization. Docker standard procedures are built for authoritative software, not so much the kind of decentralized, liberal software releases that we do with Dogecoin.

There are numerous improvements to be made on top of this PR, I suggest we handle those independent from this.

@AbcSxyZ
Copy link
Contributor

AbcSxyZ commented Nov 29, 2021

Yeah, the Dockerfile would be compromised too, still some potential issues. But hard coding the pgp keys may be something to consider, everything rely on potentially compromised download.

@patricklodder
Copy link
Member Author

But hard coding the pgp keys may be something to consider, everything rely on potentially compromised download.

Pinning the PGP keys of individual developers will give a subset of developers power. This is extremely dangerous for a software that controls billions worth of assets, because no developer should be trusted like that, including you or me. It's more likely that a single person gets compromised than a group. Like I said, this PR should either be rejected or not be blocked by further improvements.

@patricklodder patricklodder marked this pull request as ready for review November 29, 2021 16:52
@patricklodder
Copy link
Member Author

Rebased on top of #14, ready for review.

xanimo
xanimo previously approved these changes Nov 29, 2021
Copy link
Member

@xanimo xanimo left a comment

Choose a reason for hiding this comment

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

tested. works great! ACK

1.14.4/x86_64-bionic/Dockerfile Outdated Show resolved Hide resolved
Copy link
Contributor

@AbcSxyZ AbcSxyZ left a comment

Choose a reason for hiding this comment

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

Few suggestions to improve code quality, working well otherwise.

What about the previous issue when there are only new signers to a release ?

In overall, from a security point of view, I'm not sure about the risks of downloading PGP keys to verify files integrity. If the download is compromised, everything seems compromised. I somehow see it as defeating the purpose of the verification (at least for some scenario), possibly breaking this Dockerfile idea of "verifying the source, verifying the author, and verifying the content".

I raise a warning, but I'm not sure about security implication and the good approach we should use for now. Surprisingly, there is no documentation about good practice for a decentralized software using a gitian build process !

If you think it's sufficient, looks good to me to download keys & do the verification in this way.

P.S.: I will suggest it in a different PR, but I realized we probably can replace a lot of && by ;, which may be cleaner.

1.14.4/x86_64-bionic/Dockerfile Outdated Show resolved Hide resolved
Comment on lines 44 to 50
RUN RLS_FILE_NAME=`cat .filename` && \
wget ${RLS_LOCATION}/${RLS_FILE_NAME} && \
gitian/bin/gverify --no-markup -d sigs -r ${SIG_PATH} \
${DESCRIPTOR_PATH} | grep OK | \
shuf -n 1 | sed s/:.*// > random_signer.txt && \
grep ${RLS_FILE_NAME} sigs/${SIG_PATH}/$(cat random_signer.txt)/*assert | shasum -c && \
mv ${RLS_FILE_NAME} dogecoin.tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

Could find the architecture, download binaries & signature check all at once. It avoids the use of an intermediary file.

RUN set -ex && ARCHITECTURE=$(dpkg --print-architecture) \
    && if [ "${ARCHITECTURE}" = "amd64" ]; then RLS_ARCH=x86_64 ; fi \
    && if [ "${ARCHITECTURE}" = "arm64" ]; then RLS_ARCH=aarch64; fi \
    && if [ "${ARCHITECTURE}" = "armhf" ]; then RLS_ARCH=arm && RLS_LIB=gnuabihf; fi \
    && if [ "${ARCHITECTURE}" = "i386" ]; then RLS_ARCH=i686-pc; fi \
    && if [ "${RLS_ARCH}" = "" ]; then echo "Could not determine architecture"; exit 1; fi \
    && RLS_FILE_NAME=dogecoin-${RLS_VERSION}-${RLS_ARCH}-${RLS_OS}-${RLS_LIB}.tar.gz; \
    wget ${RLS_LOCATION}/${RLS_FILE_NAME} && \
    gitian/bin/gverify --no-markup -d sigs -r ${SIG_PATH} \
      ${DESCRIPTOR_PATH} | grep OK | \
      shuf -n 1 | sed s/:.*// > random_signer.txt && \
    grep ${RLS_FILE_NAME} sigs/${SIG_PATH}/$(cat random_signer.txt)/*assert | shasum -c && \
    mv ${RLS_FILE_NAME} dogecoin.tar.gz

Copy link
Member Author

Choose a reason for hiding this comment

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

At the cost of having a clear separation of declaration and execution, yes. Does the file bother you?

Copy link
Contributor

@AbcSxyZ AbcSxyZ Nov 30, 2021

Choose a reason for hiding this comment

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

I see. Are you maybe following a coding practice which do not apply to Dockerfiles ? Separation of declaration and execution is not the rule in Dockerfile.

For exemple, php & golang are mixing declaration and execution. And probably all other Docker official images.

It's even a recommendation to use the cache with more efficiency, from cacheability recommendation:

Ensure that lines which are less likely to change come before lines that are more likely to change (with the caveat that each line should generate an image that still runs successfully without assumptions of later lines).

For example, the line that contains the software version number (ENV MYSOFTWARE_VERSION 4.2) should come after a line that sets up the APT repository .list file (RUN echo 'deb http://example.com/mysoftware/debian some-suite main' > /etc/apt/sources.list.d/mysoftware.list).

Doing all declarations at the top of a Dockerfile is defeating the utility of caching mechanism.

Copy link
Contributor

@AbcSxyZ AbcSxyZ Dec 3, 2021

Choose a reason for hiding this comment

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

What about this previous comment ? I was more in favor of doing all at once, I thought at first it was a matter of taste and wouldn't complain about your arguments of separation of declaration & execution.

But this separation isn't appropriate for Dockerfiles, it's not done anywhere else (at least in official images), it's against best practices, and it's making cache unusable.

Let's drop now this idea of ​​separating the declaration and the execution.

With my understanding, both alternate. Step are organized from the less frequently edited to the most edited, and each step is having needed variables declared right above it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I of course agree with the recommendation that getting more cache hits gets enhanced by doing things in the order of the least changed execution. However, I don't think we have done the analysis properly of what changes more often from a resulting image perspective, and in which scenarios we absolutely need a cache wipe. Note that the entire verification stage doesn't contain a COPY command. So let's do it separately. It's not just this statement.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've recorded this in #22

1.14.4/x86_64-bionic/Dockerfile Outdated Show resolved Hide resolved
@patricklodder
Copy link
Member Author

What about the previous issue when there are only new signers to a release ?

Then the CI must fail on PR and we solve it if that happens.

In overall, from a security point of view, I'm not sure about the risks of downloading PGP keys to verify files integrity.

I think this meets the most authenticity criteria, given that there is no way (by design) to have an official Dogecoin Core - there is no authority. The only thing you can have is an auditable Dogecoin Core binary, and we store a sample of audit logs in the gitian.sigs repository, so the iteration over multiple published build audits are imho the best we can do. Your or my signature is meaningless on anything else than the code we wrote or a process we ourselves executed.

Besides having this build-time check, we can create a script that shasums the binaries inside the container image at runtime and have a way that verifies those independently. So you would docker run dogecoin:latest getshasums and then compare that output against an independent script or website that verifies it, citing matching gitian.sigs sources... or of course do the gitian build yourself and then validate it against that.

If the download is compromised, everything seems compromised.

If the uploaded binaries and the gitian.sigs repository are compromised, yes. Which means it all depends on GitHub security. Since however binaries published on dogecoin.com have even less security than the Dockerfile we create with this PR, I think we're improving it. We could use IPFS instead of GitHub for the build audits, but that needs peers that pin things or it'll just be another single point of failure.

breaking this Dockerfile idea of "verifying the source, verifying the author, and verifying the content".

We can pin a shasum. But the reason we are doing a lot of work and a lot of discussion is because you didn't want to pin the shasum... so this is becoming a bit circular.

@AbcSxyZ
Copy link
Contributor

AbcSxyZ commented Nov 30, 2021

Then the CI must fail on PR and we solve it if that happens.

Fine. I think those public key only available for the next release is still an improvable design (for here but not only).

The idea was to follow Docker security recommendation, and they are saying checking only checksum is the "Less Secure Alternate". Also, I thought it could be a way to facilitate the management of different versions/Dockerfile, it seems I was wrong for this and it may be a bad idea.

I went back to the documentation, and they are suggesting to use PGP verification + hard coded checksum :)
This way, everyone could be happy !

@patricklodder
Copy link
Member Author

Alright, so let's do it step by step then from the guide:

[..] verifying the source, verifying author, and verifying the content; these are respectively accomplished by the following: using https where possible; importing PGP keys with the full fingerprint in the Dockerfile to check signatures; embedding checksums directly in the Dockerfile

  1. Verifying the source (authenticity). Suggested method: https downloads. This is to prevent mitm attacks on the outgoing connections. We do this.
  2. Verifying the author (authenticity). Suggested method: PGP signing. This is to prevent someone hijacking the server and injecting a false binary. We do this with the gitian process introduced here, but with the note that there is no real authoritative version.
  3. Verifying the content (integrity). Suggested method: sha256sum of the download. We do this intrinsically with the gitian process per the first example they give as python does it, but I agree that pinning will help further due to the decentralized nature of our authenticity check.

Adding the pinning today, then we should be good, I think!

@patricklodder
Copy link
Member Author

Added pinning, rebased and squashed.

@xanimo
Copy link
Member

xanimo commented Dec 3, 2021

linux/arm64, linux/arm, linux/amd64, linux/686 buildx confirmed on:

Architecture:                    aarch64
CPU op-mode(s):                  32-bit, 64-bit
Byte Order:                      Little Endian
CPU(s):                          4
On-line CPU(s) list:             0-3
Thread(s) per core:              1
Core(s) per socket:              4
Socket(s):                       1
Vendor ID:                       ARM
Model:                           3
Model name:                      Cortex-A72
Stepping:                        r0p3
CPU max MHz:                     1500.0000
CPU min MHz:                     600.0000
BogoMIPS:                        108.00
Flags:                           fp asimd evtstrm crc32 cpuid

https://hub.docker.com/repository/docker/xanimo/1.14.4-dogecoin/tags?page=1&ordering=last_updated

1.14.4/x86_64-bionic/Dockerfile Outdated Show resolved Hide resolved
Comment on lines +30 to +33
RUN echo 72ee42424835cdfb4111b284c98f78919b7a9ede6f8d509b2abe31f7b3eb1f09 dogecoin-1.14.4-aarch64-linux-gnu.tar.gz > SHASUMS \
&& echo d023b7a6dfc5d92b1635f0fa03e14c9fc787a3eae94fba0cc3aca53b62a8e9ac dogecoin-1.14.4-arm-linux-gnueabihf.tar.gz >> SHASUMS \
&& echo 6e93f5edccf528b44112f2088be3ac8f4f44151a757754da09c8c53cdd725815 dogecoin-1.14.4-i686-pc-linux-gnu.tar.gz >> SHASUMS \
&& echo 6266235abe4bcbd41ea57bdf42f11ef89aa69f0386e8c8846d5228af69e7fa13 dogecoin-1.14.4-x86_64-linux-gnu.tar.gz >> SHASUMS
Copy link
Contributor

@AbcSxyZ AbcSxyZ Dec 3, 2021

Choose a reason for hiding this comment

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

The here doc syntax could have been great:

COPY <<-EOF SHASUMS
    72ee42424835cdfb4111b284c98f78919b7a9ede6f8d509b2abe31f7b3eb1f09 dogecoin-1.14.4-aarch64-linux-gnu.tar.gz
EOF

But it's too early, still experimental 😢

Copy link
Member Author

Choose a reason for hiding this comment

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

We'll just do it once it's fully available.

Makes the Dockerfile multi-stage, verifies the binary against
dogecoin/gitian.sigs and pinned shasums, and then proceed to
build the final image in a second stage

Co-authored-by: AbcSxyZ <34010605+AbcSxyZ@users.noreply.github.com>
Copy link
Contributor

@AbcSxyZ AbcSxyZ left a comment

Choose a reason for hiding this comment

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

Ack with #22 raised

Copy link
Member

@xanimo xanimo left a comment

Choose a reason for hiding this comment

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

@xanimo xanimo merged commit 32a6724 into dogecoin:main Dec 3, 2021
@patricklodder patricklodder mentioned this pull request Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic version build using dynamic hash verification
3 participants