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

Improve Default Configuration - Add admin interface listening to localhost only by default #23

Open
benmap-brex opened this issue Feb 11, 2020 · 7 comments

Comments

@benmap-brex
Copy link

The admin panel on port 6920 is by default bound to all interfaces. Instead, bind the address to localhost only to provide for defense in depth and use a proxy or iptables to expose the port to the outside world if desired.

@cviecco
Copy link
Contributor

cviecco commented Feb 11, 2020

I would disagree with this one. The admin port is also used to collect metrics for the system and to perform unsealing operations. I would agree that:

  1. Logs /sensitive data should be not exposed by default
  2. Any write operation MUST be authenticated.

Would metrics be considered sensitive data? I would probably say no, but I am willing to hear why this kind of data can be considered sensitive.

Another option would be to keep opening the port to 0.0.0.0 by locally filter to rfc1918 addresses (and rfc rfc6598).

Can you please elaborate on this one more?

@benmap-brex benmap-brex changed the title Security Vulnerability - Exposed Administration Interface Improve Default Configuration - Add admin interface listening to localhost only by default Feb 12, 2020
@benmap-brex
Copy link
Author

I changed the title because logs arn't exposed by default and write operations are authenticated. That's great, I assumed the default config exposed logs because of our environment!

Would it be possible for this admin port to listen on localhost by default? It just helps reduce the attack surface on keymaster a bit by not exposing the port by default. Think fresh ec2-instance spun up with no firewall because it's dev and security is a bit more lax or a dev instance running on a laptop where the dev maybe forgot to enable their firewall or something.

Thank you!

@rgooch
Copy link
Member

rgooch commented Feb 13, 2020

I don't think it's a good idea to bind this to localhost by default, since the purpose of this port is to provide off-machine visibility (i.e. metrics). We would just be adding to the burden of setting up the service, since basically everyone who wants to run this will need to undo the default.

@benmap-brex
Copy link
Author

If you want to expose admin functionality, I think it's a good thing that administrators are forced to carefully consider what they are exposing and to whom. The setup is a one-time thing, but an insecure configuration an administrator is not aware of lasts forever. I think we should default to protecting our customers as much as possible by not exposing administrative interfaces to the world out of the box.

@rgooch
Copy link
Member

rgooch commented Feb 13, 2020

I don't agree, sorry. What you are proposing will force every admin who is running a system with expectations of uptime to fix what they will see as a broken configuration option (because they need to enable metrics collection).
The proposed configuration option does not present a meaningful choice, since they have to enable it in practice.

I think the right place to deal with this is by using security groups to limit access to subnets with the metrics collectors and the desktop machines used by the administrators.

@benmap-brex
Copy link
Author

One thing that's concerning to me is the metrics and admin functionality have been co-mingled onto this single port. It's one thing to have a metrics endpoint be publicly available, but another thing to also have the metrics endpoint have the ability to reconfigure the app, which is how I believe it currently is, and why I think it should be protected by default.

This is a defense in depth issue, not a bug, so it's okay to disagree here and not do this. If keymaster wants to have the strongest security posture out of the box, metrics should be split from admin, then expose metrics only, or expose admin + metrics to localhost only.

@rgooch
Copy link
Member

rgooch commented Feb 13, 2020

Splitting the functionality over different ports might be the better way to deal with this. It would certainly make it easier to reason and verify that sensitive endpoints are locked down. It opens the question of how many ports. These are the broad groupings of endpoints:

  • UI/API (currently on the "service" port 443). Authenticated
  • Metrics (currently on the "admin" port 6920). Unauthenticated
  • Read-only dashboard (currently on the "admin" port 6920). Unauthenticated
  • Logs (currently on the "admin" port 6920). Authenticated in default configuration
  • Unsealing (the only true "admin") endpoint (currently on the "admin" port 6920). Authenticated

Perhaps we can move metrics and read-only dashboard to port 6922?

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

No branches or pull requests

3 participants