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

Add HTTPS-related variables to securedrop-admin sdconfig prompt #3366

Merged
merged 7 commits into from
May 4, 2018

Conversation

redshiftzero
Copy link
Contributor

Status

Ready for review

Description of Changes

Fixes #3299 and adds a Makefile target for generating self-signed certs that @conorsch made the other day. Note that I decided to fix the bug in a minimal way - given that the release is on Tuesday, I decided the most conservative method to resolve the bug was the best approach.

Testing

Currently testing in staging VMs (this is also #3297)

Deployment

This is implemented in such a way as to not clobber existing HTTPS variables - in case users have manually edited site-specific due to the underlying bug here.

Checklist

If you made changes to securedrop-admin:

  • Linting and tests (make -C admin test) pass in the admin development container

If you made changes to documentation:

  • Doc linting (make docs-lint) passed locally

Conor Schaefer and others added 6 commits May 3, 2018 13:42
Useful for debugging. Haven't wired up a true CA yet, just trying to
debug the vars propagation right now. These vars seem "good enough" to
me.

(cherry picked from commit 017e28bd9f5771061e2727393affaff7a33ad574)
These files contain only the string "TEST FILE ONLY"
Unfortunately the defaults cannot be used due to producing an
annoying user experience - users not using HTTPS would need to
backspace out the default values.
@redshiftzero redshiftzero requested review from emkll and a user May 4, 2018 18:17
@codecov-io
Copy link

codecov-io commented May 4, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #3366   +/-   ##
========================================
  Coverage    85.81%   85.81%           
========================================
  Files           34       34           
  Lines         2157     2157           
  Branches       238      238           
========================================
  Hits          1851     1851           
  Misses         250      250           
  Partials        56       56

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 46ed4fb...547b0e1. Read the comment docs.

emkll
emkll previously approved these changes May 4, 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.

👍 from me, see minor comments inline.

Confirmed that key/certificate generation works, and combined with the improved sdconfig logic, allows to enable HTTPS on the source interface of my prod vms.
screenshot from 2018-05-04 15-08-11

@@ -138,6 +138,11 @@ libvirt-share: ## Configure ACLs to allow RWX for libvirt VM (e.g. Admin Worksta
@find "$(PWD)" -type d -and -user $$USER -exec setfacl -m u:libvirt-qemu:rwx {} +
@find "$(PWD)" -type f -and -user $$USER -exec setfacl -m u:libvirt-qemu:rw {} +

.PHONY: self-signed-https-certs
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like make is not bought in as a dependency when doing ./securedrop-admin setup
screenshot from 2018-05-04 14-49-48

Since this functionality isn't documented for end users and will likely only be used for dev/ci, I think this is acceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, just for dev/ci, and this is good as it will discourage admins from going off script and using these Makefile targets

keyfile_dest_dir="install_files/ansible-base"

function generate-test-https-certs {
openssl genrsa -out "${keyfile_dest_dir}/${keyfile_basename}.key" 2048
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we using 2048 for performance reasons? Would it make sense to state that this functionality should not be used in a production context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call I'll make this 4096 and also state not to use this in production

@emkll emkll self-requested a review May 4, 2018 20:13
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.

Thanks for the changes, LGTM. Good catch on the .gitignore!

@redshiftzero redshiftzero mentioned this pull request May 4, 2018
4 tasks
@redshiftzero redshiftzero merged commit 4a2cde6 into develop May 4, 2018
@redshiftzero redshiftzero deleted the fix-sdconfig-https branch May 4, 2018 20:59
@emkll emkll mentioned this pull request May 4, 2018
21 tasks
redshiftzero added a commit that referenced this pull request May 4, 2018
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.

securedrop-admin sdconfig does not set https-related config variables
3 participants