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

Move django_ssl.conf into django.conf (single HTTP/HTTPS server) #77

Draft
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

Raruto
Copy link
Collaborator

@Raruto Raruto commented Sep 26, 2022

List of changes:

  • delete config/_nginx/django_ssl.conf
  • update config/nginx/django.conf accordingly in order to use a single HTTP/HTTPS server
  • expose WEBGIS_PUBLIC_HOSTNAME environment variable to nginx container (docker-compose.yml)
  • expose WEBGIS_PUBLIC_HOSTNAME environment variable to nginx container (docker-compose-consumer.yml)
  • update README.md section related to HTTPS additional setup
  • refactor run_certbot.sh (to improve readability and terminal output)

Closes: #67, closes: #68

- delete `config/_nginx/django_ssl.conf`
- update `config/nginx/django.conf` in order to use a singl HTTP/HTTPS server
- expose `WEBGIS_PUBLIC_HOSTNAME`
 environment variable to `nginx` container (`docker-compose.yml` and `docker-compose-consumer.yml`)
- update `README.md` section related to `HTTPS additional setup`
server_name dev.g3wsuite.it;
# if ($scheme = http) {
# return 301 https://$server_name$request_uri;
# }
Copy link
Collaborator Author

@Raruto Raruto Sep 26, 2022

Choose a reason for hiding this comment

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

Check if this is stricly necessary:

if ($scheme = http) {
  return 301 https://$server_name$request_uri;
}

or if the following rule could be enough:

listen 443 default ssl;

NB:

Copy link
Member

Choose a reason for hiding this comment

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

The if statement I think is necessary, otherwise we would have a 'loop' of constant redirections. With if we have redirection only on http protocol.

config/nginx/django.conf Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@Raruto Raruto added the refactoring Anything which could result in a API change label Sep 26, 2022
Raruto and others added 4 commits October 28, 2022 15:16
- add some variables to improve readability
- echo some minimal info to terminal (current step)
- remove `docker pull certbot/certbot` step within README.md
README.md Outdated
- launch `./run_certbot.sh`
- activate 301 redirect into `config/nginx/django.conf`
- restart compose
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ambiguous, rewrite better to indicate the exact terminal command needed to restart compose

Copy link
Member

Choose a reason for hiding this comment

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

So is it clear? 0c30c8c

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

terminal command needed to restart compose

docker compose down && docker compose up -d

by chance, does this do the same? docker compose up -d --force-recreate

Ref: Compose UP options

README.md Show resolved Hide resolved
run_certbot.sh Outdated Show resolved Hide resolved
server_name dev.g3wsuite.it;
# if ($scheme = http) {
# return 301 https://$server_name$request_uri;
# }
Copy link
Member

Choose a reason for hiding this comment

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

The if statement I think is necessary, otherwise we would have a 'loop' of constant redirections. With if we have redirection only on http protocol.

README.md Show resolved Hide resolved
README.md Outdated
- launch `./run_certbot.sh`
- activate 301 redirect into `config/nginx/django.conf`
- restart compose
Copy link
Member

Choose a reason for hiding this comment

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

So is it clear? 0c30c8c

README.md Show resolved Hide resolved
@wlorenzetti
Copy link
Member

@Raruto I checked every point and tested, if you don't want propose new changes for me it possible merge it on dev.

@Raruto
Copy link
Collaborator Author

Raruto commented Nov 9, 2022

The if statement I think is necessary, otherwise we would have a 'loop' of constant redirections. With if we have redirection only on http protocol.

@wlorenzetti given the declarative nature of nginx confing it is not immediately clear whether the ssl options are also sent to the browser when requesting the page as http version:

  ssl_certificate /etc/letsencrypt/live/$WEBGIS_PUBLIC_HOSTNAME/fullchain.pem;
  ssl_certificate_key /etc/letsencrypt/live/$WEBGIS_PUBLIC_HOSTNAME/privkey.pem;

  include /etc/letsencrypt/options-ssl-nginx.conf;
  ssl_dhparam /etc/letsencrypt/ssl-dhparams.pem;

  resolver 8.8.8.8;

  # written like this its doubtful if this condition is executed before or after the previous ssl options
  if ($scheme = http) {
      return 301 https://$server_name$request_uri;
  }

Probably an optimal solution (also in terms of future customizations) could be to keep two separate server sections and share the common portions through the include directive:

server {
    listen 80;
    server_name example.org;
    
    include mime_types;
    include default_routes;
    
    location / {
        return 301 https://$host$request_uri;
    }    
}

server {
    listen 443 ssl;
    server_name example.org;
    
    include mime_types;
    include default_routes;
    
    include /etc/letsencrypt/options-ssl-nginx.conf;

    ....
    
    location / {
        proxy_pass http://example.org; #for demo purposes
    }
}

The if statement I think is necessary, otherwise we would have a 'loop' of constant redirections. With if we have redirection only on http protocol.

In #77 (comment) i was thinking if it could be feasible to replace the if condition with the default_server directive (see also: How nginx processes a request), anyway, as you can see I still don't have a very clear idea about it..

BTW, another solution, which would not require changes to the nginx configuration files, could be to always activate ports 80 and 443 inside the container and expose them to the public only when needed (e.g. via docker-compose.override.yml, ref: #79)

So if there is no rush, I would wait a moment longer before merging this.

Raruto added a commit that referenced this pull request May 19, 2023
- rename `config/nginx/nginx.conf` into `config/nginx/nginx.conf.template`
- expose (envsubst) the following `.env` variables into `nginx.conf.template`:
  - `WEBGIS_PUBLIC_HOSTNAME`
  - `WEBGIS_ADMIN_EMAIL` (new)
  - `WEBGIS_SSL` (new)
- new folder `config/nginx/conf/`
- simplify `README.md` steps related to HTTPS additional setup
- refactor `run_certbot.sh` (ref: #77)
-
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Anything which could result in a API change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make use of environment variables within nginx config Remove /config/_nginx/django_conf.ssl
2 participants