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

Update safety check and cryptography version #4297

Merged
merged 4 commits into from
Apr 1, 2019
Merged

Conversation

heartsucker
Copy link
Contributor

Status

Ready for review

Description of Changes

Fixes #3682

  • Remove the rule ignoring the out of date version of cryptography
  • Run the make update-pip-requirements script to bump cryptography and other dependencies

Testing

CI should catch any incompatibilities in the libraries

Deployment

No special considerations.

Checklist

If you made changes to the server application code:

  • Linting (make ci-lint) and tests (make -C securedrop test) pass in the development container

kushaldas
kushaldas previously approved these changes Mar 26, 2019
Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

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

This looks good to me. Approved. Though I will wait for a check from @redshiftzero or @emkll

Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

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

Unit and integration tests should provide sufficient test coverage for this version bump. Proceeded with the following test/review plan:

  • Xenial app and staging tests (CI)
  • Trusty app rests (504 passed, 3 warnings in 3614.57 seconds)
  • ❗ Trusty staging tests

This PR will break package builds for Trusty. If we are to support a release for Trusty in the future based on develop, we would need to revert 035291e in order to build trusty packages:

    TASK [build-securedrop-app-code-deb-pkg : Install pip dependencies for SecureDrop.] ***
    fatal: [trusty-sd-app]: FAILED! => {"changed": false, "cmd": "/usr/bin/pip2 install -r /tmp/securedrop-app-code_0.13.0~rc1+trusty_amd64/var/www/securedrop/requirements/securedrop-app-code-requirements.txt", "msg": "stdout: Downloading/unpacking alembic==0.9.9 (from -r /tmp/securedrop-app-code_0.13.0~rc1+trusty_amd64/var/www/securedrop/requirements/securedrop-app-code-requirements.txt (line 7))\n  Running setup.py (path:/tmp/pip_build_root/alembic/setup.py) egg_info for package alembic\n    \n    warning: no files found matching '*.jpg' under directory 'docs'\n    warning: no files found matching '*.sty' under directory 'docs'\n    warning: no files found matching '*.dat' under directory 'tests'\n    no previously-included directories found matching 'docs/build/output'\nDownloading/unpacking argon2-cffi==18.1.0 (from -r /tmp/securedrop-app-code_0.13.0~rc1+trusty_amd64/var/www/securedrop/requirements/securedrop-app-code-requirements.txt (line 8))\n  Running setup.py (path:/tmp/pip_build_root/argon2-cffi/setup.py) egg_info for package argon2-cffi\n    \n    warning: no files found matching '*.txt'\n    warning: no previously-included files found matching 'src/argon2/_ffi.py'\n    warning: no previously-included files found matching '.gitmodules'\n    warning: no previously-included files found matching 'extras/libargon2/.git'\n    warning: no previously-included files found matching 'appveyor.yml'\n    warning: no previously-included files found matching '.readthedocs.yml'\n    warning: no previously-included files found matching '.travis.yml'\n    warning: no previously-included files matching '*.pyc' found under directory 'tests'\n    no previously-included directories found matching 'docs/_build'\nDownloading/unpacking asn1crypto==0.24.0 (from -r /tmp/securedrop-app-code_0.13.0~rc1+trusty_amd64/var/www/securedrop/requirements/securedrop-app-code-requirements.txt (line 9))\nDownloading/unpacking babel==2.5.1 (from -r /tmp/securedrop-app-code_0.13.0~rc1+trusty_amd64/var/www/securedrop/requirements/securedrop-app-code-requirements.txt (line 10))\nDownloading/unpacking cffi==1.11.5 (from -r /tmp/securedrop-app-code_0.13.0~rc1+trusty_amd64/var/www/securedrop/requirements/securedrop-app-code-requirements.txt (line 11))\n  Running setup.py (path:/tmp/pip_build_root/cffi/setup.py) egg_info for package cffi\n    \nDownloading/unpacking click==6.7 (from -r /tmp/securedrop-app-code_0.13.0~rc1+trusty_amd64/var/www/securedrop/requirements/securedrop-app-code-requirements.txt (line 12))\nDownloading/unpacking cryptography==2.6.1 (from -r /tmp/securedrop-app-code_0.13.0~rc1+trusty_amd64/var/www/securedrop/requirements/securedrop-app-code-requirements.txt (line 13))\n  Running setup.py (path:/tmp/pip_build_root/cryptography/setup.py) egg_info for package cryptography\n    Traceback (most recent call last):\n      File \"<string>\", line 17, in <module>\n      File \"/tmp/pip_build_root/cryptography/setup.py\", line 28, in <module>\n        \"cryptography requires setuptools 18.5 or newer, please upgrade to a \"\n    RuntimeError: cryptography requires setuptools 18.5 or newer, please upgrade to a newer version of setuptools\n    Complete output from command python setup.py egg_info:\n    Traceback (most recent call last):\n\n  File \"<string>\", line 17, in <module>\n\n  File \"/tmp/pip_build_root/cryptography/setup.py\", line 28, in <module>\n\n    \"cryptography requires setuptools 18.5 or newer, please upgrade to a \"\n\nRuntimeError: cryptography requires setuptools 18.5 or newer, please upgrade to a newer version of setuptools\n\n----------------------------------------\nCleaning up...\nCommand python setup.py egg_info failed with error code 1 in /tmp/pip_build_root/cryptography\nStoring debug log for failure in /root/.pip/pip.log\n"}

Resolving build issues by modifying build logic locally results in app code package install failiure due to old setuptools version as noted in #3441 (comment)
This means that Trusty staging CI jobs ( https://github.com/freedomofpress/securedrop/blob/develop/.circleci/config.yml#L347 ) will now always consistently fail at package build time (and if we go down this path, we should consider removing them to reduce infra costs).

@@ -26,7 +26,8 @@ RUN curl -LO https://launchpad.net/~ubuntu-mozilla-security/+archive/ubuntu/ppa/
RUN gem install sass -v 3.4.23

COPY requirements requirements
RUN pip install -r requirements/securedrop-app-code-requirements.txt && \
RUN pip install -U setuptools==40.8.0 && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason it is not tracked in requirements files? It might be worth documenting why we need to install this specific version

Copy link
Contributor

Choose a reason for hiding this comment

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

Per #3441 (comment) , the setuptools version bundled in Xenial should be sufficient to use the latest cryptography library and may clash with system-provided setuptools package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so we could remove it from the Xenial build. But no, we shoudn't put it in requirements. This is considered a bad practice. I'll wait for others to comment on the main review comment before pushing a change.

@emkll
Copy link
Contributor

emkll commented Mar 27, 2019

So to recap the discussions above and discussions during standup, as part of this PR, we should ensure that the nightly cron job staging-with-rebase-trusty is removed, since it will reliably fail. Since release/0.12.1 branch uses the Trusty staging logic by default, so any PR to 0.12.1 will have CI against Trusty VMs (and that new installs should always be using Xenial at this point), I propose we forgo a nightly Trusty CI staging job.

Based on my previous comments, I think we should also, if possible, not install setuptools in Xenial dev image. I think it's fine to keep it in the Trusty environment to ensure compatibility with the latest cryptography package.

@heartsucker
Copy link
Contributor Author

Someone can append/rebase those changes if they need it in by Monday.

heartsucker and others added 4 commits March 28, 2019 17:08
Update to cryptography now means that packaging and application will no longer work under Trusty. Remove CI targets for trusty-staging and remove setuptools for Xenial container, as it shouldn't be necessary.
@emkll emkll force-pushed the update-safety-check branch from ee0034d to b79fc1d Compare March 28, 2019 21:09
@emkll
Copy link
Contributor

emkll commented Mar 29, 2019

This is now ready for re-review, I've removed the Trusty cron target and did minimal cleanup (scoped to circle config). To avoid scope creep in this PR I propose we open a follow-up to delete the now dead platform conditional CI logic (primarily bash scripts) used for Trusty/Xenial support.

@emkll emkll requested a review from conorsch March 29, 2019 15:36
Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Changes look good! Thanks, @heartsucker !

@conorsch conorsch merged commit 77e0226 into develop Apr 1, 2019
@conorsch conorsch deleted the update-safety-check branch April 1, 2019 17:35
@conorsch conorsch mentioned this pull request May 2, 2019
4 tasks
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.

4 participants