[go: nahoru, domu]

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 TLS for Nginx Helm Chart #2115

Merged
merged 3 commits into from
Apr 13, 2020
Merged

Add TLS for Nginx Helm Chart #2115

merged 3 commits into from
Apr 13, 2020

Conversation

alles-klar
Copy link
Contributor
  • fixing nginx tls config (missing "/")
  • add tls config for nginx in helm chart
  • standardize boolean variables for all bash and helm variables

On behalf of DB Systel GmbH

@alles-klar alles-klar changed the base branch from master to dev March 27, 2020 14:30
@alles-klar alles-klar closed this Mar 27, 2020
@alles-klar alles-klar reopened this Mar 27, 2020
@ptrovatelli
Copy link
Contributor
ptrovatelli commented Apr 4, 2020

Nice feature. Could you drop a few words in the kubernetes.md to explain the difference between this (end to end encryption to the nginx pod) and the the pre-existing way to configure TLS at ingress level (which was fine too I guess provided the cluster itself is sufficiently hardened and has things like mutual tls between nodes)?
Also could you document the command you would use to get your own certificate to the required location during helm install ?
thanks!

Copy link
Contributor
@ptrovatelli ptrovatelli left a comment

Choose a reason for hiding this comment

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

as commented, could you add a bit of documentation?
thanks!

@alles-klar
Copy link
Contributor Author

@ptrovatelli I added some documentation for the new part. But there is still room for improvements. Maybe in some later PRs :)

@valentijnscholten valentijnscholten added this to the 1.6.0 milestone Apr 12, 2020
Copy link
Contributor
@madchap madchap left a comment

Choose a reason for hiding this comment

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

LGTM. Will merge now.

@alles-klar
Copy link
Contributor Author

as commented, could you add a bit of documentation?
thanks!

@ptrovatelli it semms I cant merge the PR in github until you approve that your requested change is fulfilled. Please take a look and merge if possible.

@ptrovatelli
Copy link
Contributor

OK OK go... please let all remember that self-signed certificate by itself is only a first step because it's only good against network sniffing. Protection against man in the middle requires proper CA validation.

@madchap
Copy link
Contributor
madchap commented Apr 13, 2020

Alright.. merging...

@madchap madchap merged commit 9c9fc79 into DefectDojo:dev Apr 13, 2020
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.

4 participants