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

Use SSL for nginx APT repository #939

Merged
merged 1 commit into from
Nov 14, 2016
Merged

Use SSL for nginx APT repository #939

merged 1 commit into from
Nov 14, 2016

Conversation

saz
Copy link
Sponsor Contributor

@saz saz commented Oct 20, 2016

The nginx APT repository is currently not using https.
This PR will fix this.

@wyardley
Copy link
Collaborator

@saz: at a minimum, the spec tests will probably need to be updated for us to merge this (to reflect the changed URLs).
https://github.com/voxpupuli/puppet-nginx/blob/master/.github/CONTRIBUTING.md
has some info on how to run tests, etc. (any additional commits should be squashed down to 1).

@wyardley
Copy link
Collaborator

Technically, it just needs the tests to be updated, but I'm adding the 'needs tests' flag until the tests are passing.

@zachfi
Copy link
Contributor

zachfi commented Oct 24, 2016

I believe this will require the apt-transport-tls package to function correctly, which is not installed by default.

@wyardley
Copy link
Collaborator

@xaque208 Ah, didn't realize that, but that may be another reason not to do this.

@wyardley
Copy link
Collaborator

Assuming what @xaque208 said is correct, I think it makes sense to either a) not implement this, or b) make it a configurable option, defaulting to off. If you want to make a pass at implementing b), I think this can be discussed, but there will need to be test cases for both.

Maybe a boolean option in nginx (not nginx::config), called something like apt_use_ssl, defaulting to false?

@wyardley
Copy link
Collaborator

@xaque208: looking again, we're already using https for passenger repo, and there's already a:
ensure_packages([ 'apt-transport-https', 'ca-certificates' ]) in package/debian.pp, so this is already taken care of, I think, if the OP just changes the spec tests.

@saz
Copy link
Sponsor Contributor Author

saz commented Oct 28, 2016

I'll change the pr asap. Sorry for missing this!

@zachfi
Copy link
Contributor

zachfi commented Oct 28, 2016

Does ensure_packages() just add the resources to the catalog if they are not already present? @wyardley

@zachfi
Copy link
Contributor

zachfi commented Oct 28, 2016

@wyardley Never mind, read the code and answered my own question.

@wyardley
Copy link
Collaborator

@xaque208 Yeah, just noticed this offhand when I was testing some unrelated acceptance test stuff related to Ubuntu 16; the Passenger repo is already using https (and works in the acceptance tests), so I think this will work as-is. Still need to work out whether it's worth embedding the GPG key in the package and whether it's feasible / practical to deal with GPG key expiration directly in the module.

@saz: thanks! I mentioned it above, but make sure to squash the commits.

There are a couple PRs that are pending that have a bit of overlap, but I think (hope) they won't conflict.

@wyardley
Copy link
Collaborator

wyardley commented Nov 9, 2016

@saz: have you had a chance to update the tests?

@saz
Copy link
Sponsor Contributor Author

saz commented Nov 14, 2016

@wyardley should be working now.

@wyardley wyardley merged commit 1806298 into voxpupuli:master Nov 14, 2016
@wyardley
Copy link
Collaborator

👍

@saz saz deleted the patch-3 branch November 15, 2016 13:45
Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
Use SSL for nginx APT repository
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.

None yet

3 participants