Fix recommendations from hadolint #44
Conversation
There was a problem hiding this comment.
nice you beat me to with regards to pipefail and everything with exception to separate wildcard args for rm and xargs vuln.
i'm totally fine switching from wget to curl if it saves an extraneous dep but will wait for your sec research and proposal to eval again.
just needs a rebase if merging #42 first, otherwise good to go! ACK!
There was a problem hiding this comment.
Note that gverify can fail so I explicitly negate that, but already introduced a check with the filename quote fix, so this is checked procedurally instead.
You would like to enable the Dockerfile with no signer to avoid the issue we were speaking about, where only new signers would release the binary ? In this case it changes the use of -u variable in my comments, I suppose you didn't add it for this reason.
If this is the case, we may try to use a more explicit approach.
| && grep ${RLS_FILE_NAME} SHASUMS | sha256sum -c \ | ||
| && mv ${RLS_FILE_NAME} dogecoin.tar.gz | ||
| && RLS_FILE_NAME="dogecoin-${RLS_VERSION}-${RLS_ARCH}-${RLS_OS}-${RLS_LIB}.tar.gz" \ | ||
| && wget -q --show-progress --progress=bar:force:noscroll "${RLS_LOCATION}/${RLS_FILE_NAME}" \ |
There was a problem hiding this comment.
I agree if a curl -o <file> <url> is good for the linter it's nicer.
We can also directly rename it to dogecoin.tar.gz during this line instead of the last mv.
There was a problem hiding this comment.
directly rename it to dogecoin.tar.gz
That would require a change in the logic for shasum checking that at this point I think will make things less readable. Let's consider curl separate from this PR, which is to comply with linters.
| && mv ${RLS_FILE_NAME} dogecoin.tar.gz | ||
| && RLS_FILE_NAME="dogecoin-${RLS_VERSION}-${RLS_ARCH}-${RLS_OS}-${RLS_LIB}.tar.gz" \ | ||
| && wget -q --show-progress --progress=bar:force:noscroll "${RLS_LOCATION}/${RLS_FILE_NAME}" \ | ||
| && GITIAN_OUTPUT=$(gitian/bin/gverify --no-markup -d sigs -r "${SIG_PATH}" "${DESCRIPTOR_PATH}") || true \ |
There was a problem hiding this comment.
I see why you used || true, gverify exit with 1 whenever a signature in the list is invalid, so probably always.
The script must have a RANDOM_SIGNER, it will stop without it. Related to my previous comment, we could use set -u and remove variable check/error message.
A bit annoying this gverify behavior.
There was a problem hiding this comment.
It's just that sigs got merged without checking, so there is a set of signatures that causes mismatch. This can happen also when there is contention, so it's good that it exits with an error. Just not entirely cool for our use case, but I don't think it should change for generic gverify.
As an alternative, we can script a gitian verification process that suits our needs better, but with it nearing end-of-life, we may as well settle on negating the error exit for now.
There was a problem hiding this comment.
It's just that sigs got merged without checking, so there is a set of signatures that causes mismatch. This can happen also when there is contention, so it's good that it exits with an error. Just not entirely cool for our use case, but I don't think it should change for generic gverify.
As an alternative, we can script a gitian verification process that suits our needs better, but with it nearing end-of-life, we may as well settle on negating the error exit for now.
i think this is fine but a note that fdov, @AbcSxyZ and my keys got merged after v1.14.5 tag so it's only pulling langerhans, michi, rnicoll, stapler117, whit and your keys:
that's why fdov, @AbcSxyZ and mine get:
gpg: Can'\''t check signature: No public key
xanimo: BAD SIGNATURE
because our key's aren't imported in find dogecoin/contrib/gitian-keys -name '*.pgp' -exec gpg --import '{}' + line. if we pulled from -b 1.14.6-dev we're included/can verify. regardless since nearing eol and having 4/6 gitian signers for v1.14.5 and only needing 1 to gverify against is fine and works to verify authenticity.
patricklodder
force-pushed
the
fix/docker-lint-fixes
branch
from
74472d8
to
ff0e0e6
Compare
fixes https://github.com/hadolint/hadolint/wiki/DL4006 note: this wouldn't work with gverify as it exits with 1 if any supplied signature set mismatches. Since however we allow for this by checking a random OK as we already pin the hash too, this we can ignore gverify returning 1. If no signers are valid, we catch this in the script before attempting a shasum
patricklodder
force-pushed
the
fix/docker-lint-fixes
branch
from
ff0e0e6
to
cc93b80
Compare
|
@AbcSxyZ if you have a moment to review would be greatly appreciated ❤️ |
There was a problem hiding this comment.
utACK for me. Would be great to add some improvement afterward:
- Using curl & rename during the download
GITIAN_OUTPUT=$(gitian/bin/gverify --no-markup -d sigs -r "${SIG_PATH}" "${DESCRIPTOR_PATH}") || trueuse true inside the command substitution for clarity imo- I still think using
set -ucould simplify the Dockerfile, it could be great to discuss it.
Fixes:
curlbecause it's cleaner but that's just a personal preference so I ignored myself for now. May need to look at this again from a security perspectivegverifycan fail so I explicitly negate that, but already introduced a check with the filename quote fix, so this is checked procedurally instead.I left the
aptrelated issues open because those rely on #42.