Remove administrative API from binding to all interfaces #3016
Conversation
|
This definitely resonates to me and seems like a much saner default 👍🏻 We were planning on making this change already, thanks for taking care of it. |
|
Ahh unit tests failing. Ill try fix this too |
kong.conf.default
Outdated
| # an entrypoint to the Admin API. | ||
| # This API lets you configure and manage Kong, | ||
| # and should be kept private and secured. | ||
|
|
||
| #admin_listen_ssl = 0.0.0.0:8444 # Address and port on which Kong will accept | ||
| #admin_listen_ssl = 127.0.0.1:8444 # Address and port on which Kong will accept |
There was a problem hiding this comment.
while waiting for CI to complete, can you fix the comment alignment? 👍
|
w00t tests pass now! |
|
I like! |
|
just wondering: is this for next minor ( first thought: @thibaultcha @p0pr0ck5 thoughts? |
|
This will certainly go into next once we have a closer look. Currently traveling so I haven’t had time to do so yet. |
There was a problem hiding this comment.
Everything looks good to me here except for that configuration file thing.Very much of a user-facing file, this one, so we do try to keep it neat and tidy - and consistent. Thanks for your understanding.
Another thing: would you mind rebasing this on top of the next branch? We will then schedule this for our 0.12 release. We will also update the base branch to next once that is done :)
Thank you!
| #admin_listen_ssl = 0.0.0.0:8444 # Address and port on which Kong will accept | ||
| # HTTPS requests to the admin API, if | ||
| # `admin_ssl` is enabled. | ||
| #admin_listen_ssl = 127.0.0.1:8444 # Address and port on which Kong will accept |
There was a problem hiding this comment.
Unfortunately, this update makes this line go over our 80 characters limit we try to respect for this file. For this particular case, it should be easy enough to simply move "accept" down a line (and ensure subsequent lines aren't over 80 char as well). Would you mind taking care of that? Thank you!
There was a problem hiding this comment.
No problems, Ill reformat that conf so its <80 wide.
Github q -- do i need to raise a new PR for a rebase against next?
Im fork I clone next branch from this repo, generate a diff from this PR, apply diff to forked branch, submit new PR?
There was a problem hiding this comment.
@pduldig-at-tw what should work is doing a git rebase next on this branch
There was a problem hiding this comment.
I added the upstream branch, then rebased it against kong/next, but it seems to have brought in all the other commits. Have I broken this PR?
There was a problem hiding this comment.
you might create a new branch and cherry pick your commits, see https://stackoverflow.com/questions/9853681/git-create-branch-from-range-of-previous-commits/9853814#9853814
There was a problem hiding this comment.
@pduldig-at-tw I fixed your branch with git rebase origin/next -i (then dropped the incorrect commits via the interactive editor-based interface) and git push --force.
There was a problem hiding this comment.
Thank you hisham! Most appreciated! I am still getting used to GH workflows here so thank you for your explanation too!
thibaultcha
added
the
pr/changes requested
Fixes insecure default of binding admin to external
To match lua configuration files
Change test to reflect 127.0.0.1 as default admin binding
make it look neater
change to admin 127.0.01
Update new default IP
|
Apart from cleaning up commit history, are we missing anything here? Would definitely love to see this merged 👍 |
Signed-off-by: Thibault Charbonnier <thibaultcha@me.com> A rationale to follow best security practises by default is here enforced by updating the default `admin_listen` property, so that it does not bind to all interfaces, but instead the local one only. As the Admin API exposes configuration data, secrets, SSL certificates and the likes, we want to prevent users from deploying Kong instances and exposing this data to the public out of negligence. Fix #3012 From #3016
|
Merged to next with some squashing and git commit message additions. Thank you @pduldig-at-tw! We'll include this in our next major release as we consider it a breaking change (but a good one!) |
Signed-off-by: Thibault Charbonnier <thibaultcha@me.com> A rationale to follow best security practises by default is here enforced by updating the default `admin_listen` property, so that it does not bind to all interfaces, but instead the local one only. As the Admin API exposes configuration data, secrets, SSL certificates and the likes, we want to prevent users from deploying Kong instances and exposing this data to the public out of negligence. Fix #3012 From #3016
Signed-off-by: Thibault Charbonnier <thibaultcha@me.com> A rationale to follow best security practises by default is here enforced by updating the default `admin_listen` property, so that it does not bind to all interfaces, but instead the local one only. As the Admin API exposes configuration data, secrets, SSL certificates and the likes, we want to prevent users from deploying Kong instances and exposing this data to the public out of negligence. Fix #3012 From #3016
As discussed in #3012 the admin API exposes some sensitive information, and it should not bind to all interfaces by default.
I feel that it is not appropriate to ship products with admin interfaces (esp with no password) open by default. This was a lesson learned by Mongo, Couchbase and ElasticSearch -- these products shipped with open defaults and data was exposed.
As issue 3012 indicated, there were over 700 exposed admin interfaces for Kong on the public internet. Lets change the defaults to prevent more from appearing.