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.

Sign up for GitHub

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

Jump to bottom

Add CLI option -nginx-status-allow-cidrs to allow easily restricting access to sensitive endpoints via CLI argument. #387

Merged
merged 1 commit into from
Oct 11, 2018
Merged

Add CLI option -nginx-status-allow-cidrs to allow easily restricting access to sensitive endpoints via CLI argument. #387

merged 1 commit into from
Oct 11, 2018

Conversation

r4j4h
Copy link
Contributor

@r4j4h r4j4h commented Oct 2, 2018

Add CLI option -nginx-status-allow-cidrs to allow easily restricting access to sensitive endpoints via CLI argument.

Proposed changes

This implements #355

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto master
  • I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

@r4j4h r4j4h mentioned this pull request Oct 2, 2018
Merged
6 tasks
@r4j4h
Copy link
Contributor Author

r4j4h commented Oct 2, 2018

Some notes of areas I wasn't sure about:

  • /nginx-health - I covered it initially but am not sure if we wanted to include it or just cover the status endpoints with this change
  • tests - I ran into lots of whitespace issues with trying to actually test the generated blocks so I fell back to counting "allow" directives. Please let me know if there is a better way to do this.

@pleshakov pleshakov added the enhancement Pull requests for new features/feature enhancements label Oct 3, 2018
@pleshakov
Copy link
Contributor

@r4j4h thanks for the PR! we're reviewing it and will get back to you soon

pleshakov
pleshakov suggested changes Oct 4, 2018
View reviewed changes
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

Please see my suggestions.

Additionally, could you format the go code using gofmt tool? There are formatting issues.

/nginx-health - I covered it initially but am not sure if we wanted to include it or just cover the status endpoints with this change

status health endpoint is different from NGINX status. that's why it should not be configured via nginx-status-allow-cidrs flag. I wonder what is the use case for protecting it?

cmd/nginx-ingress/main.go Outdated Show resolved Hide resolved
cmd/nginx-ingress/main.go Outdated Show resolved Hide resolved
cmd/nginx-ingress/main.go Outdated Show resolved Hide resolved
cmd/nginx-ingress/main.go Outdated Show resolved Hide resolved
cmd/nginx-ingress/main.go Outdated Show resolved Hide resolved
cmd/nginx-ingress/main.go Outdated Show resolved Hide resolved
cmd/nginx-ingress/main_test.go Outdated Show resolved Hide resolved
cmd/nginx-ingress/main_test.go Outdated Show resolved Hide resolved
internal/nginx/templates/nginx.tmpl Outdated Show resolved Hide resolved
internal/nginx/templates/templates_test.go Outdated Show resolved Hide resolved
@r4j4h
Copy link
Contributor Author

r4j4h commented Oct 5, 2018

@pleshakov Would you prefer these changes added on in subsequent commits or rebased again?

@r4j4h
Copy link
Contributor Author

r4j4h commented Oct 5, 2018

@pleshakov Great feedback, I think I made all the requested changes. The diff definitely looks cleaner! Let me know if you see more.

@r4j4h
Copy link
Contributor Author

r4j4h commented Oct 5, 2018

@pleshakov I did not include it in this MR, but internal/nginx/extensions.go is in need of gofmt pass as well

pleshakov
pleshakov suggested changes Oct 8, 2018
View reviewed changes
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

@r4j4h
thanks for making changes. Please see a few additional suggestions.

internal/nginx/templates/nginx-plus.tmpl Outdated

allow 127.0.0.1;
{{ range $value := .NginxStatusAllowCIDRs }}{{ if ne $value "" }}
allow {{$value}};{{ end }}
deny all;
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, haven't caught it during the first review. Could you move the deny all just below the range loop.

{{range ... }}
{{end}}
deny all;

internal/nginx/templates/nginx-plus.tmpl Outdated
@@ -96,10 +96,10 @@ http {

location = /dashboard.html {
}

allow 127.0.0.1;
{{ range $value := .NginxStatusAllowCIDRs }}{{ if ne $value "" }}
Copy link
Contributor

Choose a reason for hiding this comment

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

could you indent {{ range ... so that it has the indentation as the allow in the next line?

@pleshakov pleshakov requested a review from Dean-Coakley October 8, 2018 19:33
Dean-Coakley
Dean-Coakley approved these changes Oct 9, 2018
View reviewed changes
Copy link
Contributor

@Dean-Coakley Dean-Coakley left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks great! :shipit:

@pleshakov
Copy link
Contributor

@r4j4h thanks for the changes again. I have updated the suggestions left yesterday (removed a few). Sorry, if that confused you.

@r4j4h
Copy link
Contributor Author

r4j4h commented Oct 10, 2018

@pleshakov Thanks for letting me know, luckily I did not have time to start on the fixes until now so I am glad that I did not make any of the changes you removed :) Should have these changes made shortly

@r4j4h
Copy link
Contributor Author

r4j4h commented Oct 10, 2018

@pleshakov Ok, please take a look and let me know if there is anything else :)

@pleshakov
Copy link
Contributor

@r4j4h Thanks! Could you possible squash your commits into a single one? Once it is done, we will merge it.

@r4j4h
Add CLI option -nginx-status-allow-cidrs to allow easily restrictin…
833948e 
…g access to sensitive endpoints via CLI argument.
@r4j4h
Copy link
Contributor Author

r4j4h commented Oct 11, 2018

@pleshakov No problem, done!

@pleshakov pleshakov merged commit abff82b into nginx:master Oct 11, 2018
@pleshakov
Copy link
Contributor

@r4j4h thanks! the feature will be immediately available in the edge version and will go in our 1.4.0 release.

@Dean-Coakley Dean-Coakley mentioned this pull request Oct 16, 2018
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dean-Coakley Dean-Coakley Dean-Coakley approved these changes

@pleshakov pleshakov pleshakov approved these changes

Assignees
No one assigned
Labels
enhancement Pull requests for new features/feature enhancements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@r4j4h @pleshakov @Dean-Coakley