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

ansible: replace test_admin key with a valid sec/pub key #2925

Merged
1 commit merged into from
Jan 26, 2018

Conversation

ghost
Copy link

@ghost ghost commented Jan 25, 2018

Status

Ready for review

Description of Changes

The test_admin_key.pub and test_admin_key.sec are both public
keys. This is fine as long as the tests do not try to decrypt
anything.

A new key is created and stored instead to allow for OSSEC tests to
decrypt mails.

Testing

  • gpg --import install_files/ansible-base/roles/ossec/files/test_admin_key.pub
  • gpg --import install_files/ansible-base/roles/ossec/files/test_admin_key.sec
  • echo foo | gpg --trust-model always --encrypt -ear 53E1113AC1F25027BA5D475B1141E2BBB5E53711 > /tmp/e
  • gpg --decrypt < /tmp/e
foo

Deployment

N/A

Checklist

If you made changes to the app code:

  • Unit and functional tests pass on the development VM

@ghost ghost added feature tests labels Jan 25, 2018
@ghost ghost requested review from conorsch and msheiny as code owners January 25, 2018 15:19
@ghost ghost force-pushed the wip-dachary-admin-key branch from b617684 to cf9815f Compare January 25, 2018 15:19
@ghost
Copy link
Author

ghost commented Jan 25, 2018

@emkll in case you're bored today :-)

@ghost
Copy link
Author

ghost commented Jan 25, 2018

@redshiftzero Looks like your work on PyCrypto upgrade is right on time !

https://circleci.com/gh/freedomofpress/securedrop/7185?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

!/bin/bash -eo pipefail
make safety

Checking file ./securedrop/requirements/test-requirements.txt
safety report
checked 22 packages, using default DB
---
No known security vulnerabilities found.


Checking file ./securedrop/requirements/admin-requirements.txt
safety report
checked 15 packages, using default DB
---
-> pycrypto, installed 2.6.1, affected <=2.6.1, id 33151
Heap-based buffer overflow in the ALGnew function in block_templace.c in Python Cryptography Toolkit (aka pycrypto) allows remote attackers to execute arbitrary code as demonstrated by a crafted iv parameter to cryptmsg.py. 
--
Makefile:101: recipe for target 'safety' failed
make: *** [safety] Error 1

@emkll emkll self-requested a review January 25, 2018 17:53
emkll
emkll previously approved these changes Jan 25, 2018
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.

Looks good, @dachary. Can confirm the private key and public key form a keypair and the fingerprint is blacklisted when validating.

To eliminate the possibility of this key being erroneously used, perhaps in the future it could be automatically generated in staging/CI with a configuration flag.

@redshiftzero
Copy link
Contributor

This will need a rebase on latest develop prior to merge due to failing staging build, due to #2931, apologies

@ghost ghost force-pushed the wip-dachary-admin-key branch from cf9815f to 1c5e513 Compare January 26, 2018 11:19
@ghost
Copy link
Author

ghost commented Jan 26, 2018

Transient CircleCI error https://circleci.com/gh/freedomofpress/securedrop/7256?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Allocating a remote Docker Engine
Got error while creating host: rpc error: code = Unknown desc = An internal error occured when provisioning resources for this job.  Provisioning Service returned status code: 503
We had an unexpected error preparing a VM for this build, potentially due to our infrastructure or cloud provider.  Please retry the build in a few minutes

@ghost ghost force-pushed the wip-dachary-admin-key branch 3 times, most recently from 6a5c4ef to d163111 Compare January 26, 2018 15:39
The test_admin_key.pub and test_admin_key.sec are both public
keys. This is fine as long as the tests do not try to decrypt
anything.

A new key is created and stored instead to allow for OSSEC tests to
decrypt mails.
@ghost ghost force-pushed the wip-dachary-admin-key branch from d163111 to 6f35b14 Compare January 26, 2018 15:39
@ghost
Copy link
Author

ghost commented Jan 26, 2018

This will need a rebase on latest develop prior to merge due to failing staging build, due to #2931, apologies

Thanks for the update :-)

@codecov-io
Copy link

codecov-io commented Jan 26, 2018

Codecov Report

Merging #2925 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2925   +/-   ##
========================================
  Coverage    85.24%   85.24%           
========================================
  Files           32       32           
  Lines         1952     1952           
  Branches       218      218           
========================================
  Hits          1664     1664           
  Misses         237      237           
  Partials        51       51

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a27c355...6f35b14. Read the comment docs.

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.

👍 Still looks good, perhaps in the future we could automate the keypair generation to avoid committing secrets to GitHub :) .

@ghost
Copy link
Author

ghost commented Jan 26, 2018

@emkll it's kind of scary to commit private keys, indeed :-)

@ghost ghost merged commit fe624fb into develop Jan 26, 2018
@ghost ghost deleted the wip-dachary-admin-key branch January 28, 2018 13:21
redshiftzero added a commit that referenced this pull request Feb 3, 2018
Conflicts:
	.circleci/config.yml

Favored develop since admin test jobs were added in develop in #2758.

	install_files/ansible-base/roles/ossec/files/test_admin_key.pub
	install_files/ansible-base/roles/ossec/files/test_admin_key.sec

Favored develop for these changes since these keys in 0.5.2 were
erroneously both public keys (fixed in #2925).

	install_files/ansible-base/securedrop-configure.yml

Deleted this file as it was removed in develop during the sdconfig
refactor (#2758) from Ansible to Python. The locale prompt additions added
in SecureDrop 0.5.2 were added in #2758 on develop.

	molecule/aws/scripts/app-tests.sh

Favored develop since the addition of RTL language testing was
added in #2930.

	molecule/aws/side_effect.yml

Favored release/0.5.2 as these changes were due to the addition
of Tor apt repo testing in CI against release branches (#2941).

	securedrop/Dockerfile

Favored develop since all these gettext commands being merged into
one RUN command was done in #2822 and is still on develop.
redshiftzero added a commit that referenced this pull request Feb 4, 2018
Conflicts:
	.circleci/config.yml

Favored develop since admin test jobs were added in develop in #2758.

	install_files/ansible-base/roles/ossec/files/test_admin_key.pub
	install_files/ansible-base/roles/ossec/files/test_admin_key.sec

Favored develop for these changes since these keys in 0.5.2 were
erroneously both public keys (fixed in #2925).

	install_files/ansible-base/securedrop-configure.yml

Deleted this file as it was removed in develop during the sdconfig
refactor (#2758) from Ansible to Python. The locale prompt additions added
in SecureDrop 0.5.2 were added in #2758 on develop.

	molecule/aws/scripts/app-tests.sh

Favored develop since the addition of RTL language testing was
added in #2930.

	molecule/aws/side_effect.yml

Favored release/0.5.2 as these changes were due to the addition
of Tor apt repo testing in CI against release branches (#2941).

	securedrop/Dockerfile

Favored develop since all these gettext commands being merged into
one RUN command was done in #2822 and is still on develop.
redshiftzero added a commit that referenced this pull request Feb 5, 2018
Conflicts:
	.circleci/config.yml

Favored develop since admin test jobs were added in develop in #2758.

	install_files/ansible-base/roles/ossec/files/test_admin_key.pub
	install_files/ansible-base/roles/ossec/files/test_admin_key.sec

Favored develop for these changes since these keys in 0.5.2 were
erroneously both public keys (fixed in #2925).

	install_files/ansible-base/securedrop-configure.yml

Deleted this file as it was removed in develop during the sdconfig
refactor (#2758) from Ansible to Python. The locale prompt additions added
in SecureDrop 0.5.2 were added in #2758 on develop.

	molecule/aws/scripts/app-tests.sh

Favored develop since the addition of RTL language testing was
added in #2930.

	molecule/aws/side_effect.yml

Favored release/0.5.2 as these changes were due to the addition
of Tor apt repo testing in CI against release branches (#2941).

	securedrop/Dockerfile

Favored develop since all these gettext commands being merged into
one RUN command was done in #2822 and is still on develop.

	docs/development/contributor_guidelines.rst

Favored develop since these contributor guidelines were added recently in #2972.
redshiftzero added a commit that referenced this pull request Feb 6, 2018
Conflicts:
	.circleci/config.yml

Favored develop since admin test jobs were added in develop in #2758.

	install_files/ansible-base/roles/ossec/files/test_admin_key.pub
	install_files/ansible-base/roles/ossec/files/test_admin_key.sec

Favored develop for these changes since these keys in 0.5.2 were
erroneously both public keys (fixed in #2925).

	install_files/ansible-base/securedrop-configure.yml

Deleted this file as it was removed in develop during the sdconfig
refactor (#2758) from Ansible to Python. The locale prompt additions added
in SecureDrop 0.5.2 were added in #2758 on develop.

	molecule/aws/scripts/app-tests.sh

Favored develop since the addition of RTL language testing was
added in #2930.

	molecule/aws/side_effect.yml

Favored release/0.5.2 as these changes were due to the addition
of Tor apt repo testing in CI against release branches (#2941).

	securedrop/Dockerfile

Favored develop since all these gettext commands being merged into
one RUN command was done in #2822 and is still on develop.

	docs/development/contributor_guidelines.rst

Favored develop since these contributor guidelines were added recently in #2972.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants