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

New docker image, dockerfile and cie integration #55

Merged
merged 101 commits into from
Jan 31, 2023

Conversation

MdreW
Copy link
Collaborator

@MdreW MdreW commented Sep 15, 2022

New docker image configurable with environments and NGINX as frontend

MdreW and others added 30 commits March 30, 2022 14:40
…metrizzato. Dockerfile che parte da image alpine aggiornata e con timezone Europe/Rome. Directory example duplicata.
…n example e del compose. Da completare le modifiche. Problemi nella sostituzione con jq di alcune variabili. Da cambiare il proprietario di proxy_conf.yaml ed i valori di default da dare ad alcune variabili.
Dockerfile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@@ -0,0 +1,89 @@
user nginx;
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to not enable http by default, to avoid ssl split attacks


# max upload size
client_max_body_size 8M;

Copy link
Member

Choose a reason for hiding this comment

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

@@ -0,0 +1,105 @@
user nginx;
Copy link
Member

Choose a reason for hiding this comment

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

this is quite duplicated considering nginx.conf_proxy_http

please clean up or explain the rationale for this

Copy link
Contributor

Choose a reason for hiding this comment

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

Non vengono utilizzate entrambe le configurazioni, ma solo una.
La mia scelta è stata quella di usare nginx.conf_uwsgi_pass come default. Ma di dare anche a chi preferisce il proxy http una configurazione già fatta da poter utilizzare. Credo di averlo spiegato in un qualche punto della documentazione (forse è accennato solo in un commento nel run.sh), devo ricontrollare ed in caso aggiungerlo.
https://github.com/IDEM-GARR-AAI/Satosa-Saml2Spid/blob/b70b650bfeb3e51ff6b794e22bbbf172339c3715/example/run.sh#L179
Il fatto è che secondo me ci sono vantaggi e svantaggi in entrambe le soluzioni, ma quella più adatta a "produzione" mi sembra quella che utilizza uwsgi_pass.
L'immagine creata con il Dockerfile ha come default, nel run.sh , l'avvio di uwsgi con --socket (che comunque dovrebbe essere il default) e questo impone di utilizzare in nginx uwsgi_pass. In questo modo uwsgi dovrebbe essere più veloce, perché non deve fare il lavoro extra di tradurre in http, tanto poi a parlare in http e https ci pensa nginx.
Chi invece vuole usare la configurazione proxy_http deve cambiare la configurazione in example/run.sh , oppure sostituire nel container running il file run.sh , con la configurazione modificata, andando ad aggiungerlo nel docker-compose.yml.
Possiamo pure pensare di scegliere solo una delle due possibilità e fornire quindi una sola configurazione.

Copy link
Member

Choose a reason for hiding this comment

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

ok, la tua scelta mi torna

magari sarà più difficile da mantenere ma non importa dato che queste configurazioni hanno lo scopo di esemplificare il deployment

@@ -109,7 +110,7 @@ LOGGING:
backends.spidsaml2:
level: INFO
formatter: simple
handlers: [spid_daily]
Copy link
Member

Choose a reason for hiding this comment

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

with spid daily we apply the SPID retention policy, we can enable multiple handlers and I suggest to keep the spid_daily up and running as previously configured

Copy link
Member

Choose a reason for hiding this comment

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

is there any update for this log handler?
may we leave it as commented, to let the user decide to collect the spid related logs in a separate log file?

Copy link
Member

@peppelinux peppelinux left a comment

Choose a reason for hiding this comment

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

few thing still need to be changed but the work is awesome, great!

@peppelinux
Copy link
Member

peppelinux commented Jan 31, 2023

Hey guys, I'm writing in english because we have some collegues of the R&S world that's interested in this project, so let's speak english :-)

I have enabled the GH container deployment for each new release.

COPY example/ $BASEDIR/
COPY requirements.txt $BASEDIR/
COPY oids.conf $BASEDIR/pki/
COPY build_spid_certs.sh $BASEDIR/pki/
Copy link
Member

Choose a reason for hiding this comment

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

deprecated, we should use spid-compliant-certificates for this

build_spid_certs.sh should be removed from this repository

RUN wget https://registry.spid.gov.it/metadata/idp/spid-entities-idps.xml -O metadata/idp/spid-entities-idps.xml
RUN apk add --update xmlsec libffi-dev libressl-dev python3 py3-pip python3-dev procps git openssl build-base gcc wget bash jq \
&& cd $BASEDIR/pki/ \
&& chmod 755 $BASEDIR/pki/build_spid_certs.sh \
Copy link
Member

Choose a reason for hiding this comment

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

here spid-compliant-certificates insteadl of build_spid_certs

<p>Se sei l'amministratore di questa risorsa magari puoi
trovare dettagli interessanti nei log.</p>
<br/>
<p><em>Faithfully yours, nginx.</em></p>
Copy link
Member

Choose a reason for hiding this comment

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

please remove "nginx"

Please try again later.</p>
<p>If you are the system administrator of this resource then you should check
the error log for details.</p>
<p><em>Faithfully yours, nginx.</em></p>
Copy link
Member

Choose a reason for hiding this comment

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

pelase remove "nginx"

@@ -109,7 +110,7 @@ LOGGING:
backends.spidsaml2:
level: INFO
formatter: simple
handlers: [spid_daily]
Copy link
Member

Choose a reason for hiding this comment

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

is there any update for this log handler?
may we leave it as commented, to let the user decide to collect the spid related logs in a separate log file?

@peppelinux peppelinux marked this pull request as ready for review January 31, 2023 23:13
@peppelinux peppelinux merged commit 17c9f6d into italia:master Jan 31, 2023
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.

3 participants