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

[INFRA] fix circleci: adding apt keys #910

Closed
wants to merge 1 commit into from

Conversation

sappelhoff
Copy link
Member

@sappelhoff sappelhoff commented Oct 27, 2021

This PR is intended to fix the CircleCI continuous integration service step. I debugged it when logged into the session via ssh, and this was the fix. I don't know why adding the keys is now needed. It might be a temporary thing. In any case, I don't think it hurts.

@effigies
Copy link
Collaborator

Can we Google these particular keygrips and verify that Ubuntu vouches for them? I'm hesitant to simply fetch keys that are missing, as a tampered package signed with an unofficial key would look the same.

@sappelhoff
Copy link
Member Author

I tried to google a bit, but searching for example for DCC9EFBF77E11517 on https://keyserver.ubuntu.com results in "Not found".

I also don't fully understand in how far this is a security issue: I thought all we are doing here is adding some "public keys", so that packages, signed with "private keys" can be validated. So for this to become a security issue, someone would have to "capture" the git-all namespace for a package, and ship their malware with that package, signing it with their private key, which we then verify by naively adding these public keys as done in this PR. Is that correct, or do you mean something else?

It sounds possible, ... would then maybe the alternative solution from the stackexchange thread be preferable?

sudo apt-get install debian-archive-keyring (haven't tried it, but looks like it might work just as well)

BTW: I got the public keys I add in this PR by browsing the CircleCI log.

@effigies
Copy link
Collaborator

I also don't fully understand in how far this is a security issue: I thought all we are doing here is adding some "public keys", so that packages, signed with "private keys" can be validated. So for this to become a security issue, someone would have to "capture" the git-all namespace for a package, and ship their malware with that package, signing it with their private key, which we then verify by naively adding these public keys as done in this PR. Is that correct, or do you mean something else?

The security issue is that we are installing software. The public key verifies that the package we are installing has the same hash as that seen by the holder of the private key, so the security model is "By verifying packages with these public keys, I am trusting the person/machine that holds a corresponding public key would not intentionally package malware." If we simply add a public key because a package was signed with it, then there is no verification.

Here's the attack:

  1. I fetch a package and add some code that mines bitcoins.
  2. I create a new PGP key called 'Debian Signing Key (pinky swear)' and upload the public key to key servers.
  3. Sign the malicious package with my key. This is a legit signature, just not by a legitimate signer.
  4. I trick some Debian mirrors into hosting my package. Apt responds by refusing to install.
  5. Annoyed users just look at the missing key, update their keyring, and reinstall.
  6. Literal profit.

Not saying that's what this is, but we should definitely get verified keys, not just do step 5.

sudo apt-get install debian-archive-keyring (haven't tried it, but looks like it might work just as well)

If this works, then it is what we should do. It would mean that Debian has intentionally rotated their keys and is attesting to new keys using the old keys.

@effigies
Copy link
Collaborator

effigies commented Oct 27, 2021

Tried it out in ci/keyring: https://app.circleci.com/pipelines/github/bids-standard/bids-specification/3366/workflows/845aae52-fdb1-40e5-9d38-af807144ee86/jobs/9096

Seems apt-get update won't work without already having the right keys, so we can't reliably bootstrap our way to confidence from within the container. Perhaps @yarikoptic can update it?

@sappelhoff
Copy link
Member Author

Thanks Chris, I agree that the best solution would be to update the linkchecker image. Help with that would be much appreciated.

@sappelhoff
Copy link
Member Author

@yarikoptic would "updating the linkchecker image" involve:

  1. updating https://github.com/yarikoptic/linkchecker (pulling the 728 new commits from upstream)
  2. tag a new version
  3. running (with the new version added)
    #!/bin/bash
    # A script to build docker image with desired patched linkchecker
    # using neurodocker
    # TODOs:
    # - minimize image (currently >500MB) using reprozip
    # see https://github.com/kaczmarj/neurodocker/#minimize-existing-docker-image
    # for instructions
    # tag for patched linkchecker -- will also serve our image version
    version="9.4.0.anchorfix1"
    # docker image build
    build=1
    neurodocker generate docker \
    --base neurodebian:nd100 \
    --pkg-manager apt \
    --install python-pip python-requests python-dnspython python-setuptools python-wheel \
    --run "sed -e 's,^deb ,deb-src ,g' /etc/apt/sources.list > /etc/apt/sources.list.d/deb-sources.list \
    && apt-get update \
    && apt-get build-dep -y linkchecker \
    && pip install pyxdg https://github.com/yarikoptic/linkchecker/archive/$version.zip \
    && mkdir ~/.linkchecker && echo '[AnchorCheck]' > ~/.linkchecker/linkcheckerrc \
    " \
    --entrypoint /usr/local/bin/linkchecker \
    | docker build -t yarikoptic/linkchecker:$version-$build -
  4. uploading to dockerhub

?

If yes, maybe we should:

  1. fork linkchecker with the bids-standard GitHub org or the bids-maintenance GitHub "bot-user"
  2. do the updating / tagging from that fork, instead of the yarikoptic fork
  3. build a new image and push to https://hub.docker.com/u/bids

What do y'all think?

@sappelhoff
Copy link
Member Author

closing, this will be better addressed in #932

@sappelhoff sappelhoff closed this Nov 17, 2021
@sappelhoff sappelhoff deleted the fix/ci/keys branch January 30, 2022 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants