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

Merge Admin Server into API Server and secure via TLS #1115

Merged
merged 10 commits into from
Oct 29, 2020

Conversation

jakubdyszkiewicz
Copy link
Contributor

@jakubdyszkiewicz jakubdyszkiewicz commented Oct 27, 2020

Current state (before the PR)

  • API Server available on 0.0.0.0:5681 that contains all the endpoints for managing resources in Kuma.
  • API Server did not support TLS, so you need to use API Gateway to secure it.
  • Admin Server for managing secrets and Dataplane Tokens available on 0.0.0.0:5682. Only available on TLS.
  • You can access Admin Server via localhost (127.0.0.1:5679) or when outside of your machine, generate client certs and put in the directory
  • Kumactl is juggling between API Server and Admin Server (using catalog API). It’s confusing for users what is where.
  • There is no way to configure kumactl to verify CP cert against the CA.

State after PR

  • API Server with all the same endpoints including dataplane tokens and secrets is now exposed on two ports. 1) non-TLS on 0.0.0.0:5681. 2) TLS on 0.0.0.0:5682
  • TLS is protected with the same autogenerated certs that are used for DP Server.
  • You can have a secure way to access API Server via HTTPS without any external systems
  • You don’t need to think if the endpoint is on Admin or API Server, you either use HTTP on 5681 or HTTPS on 5682 and you configure kumactl with either URL.
  • Admin endpoints (DP tokens, Secrets) still requires the same auth (request from the same machine - you can turn this off, or client certs)
  • Kumactl is now using only one endpoint (either 5681 on HTTP or 5682 on HTTPS). No more juggling with catalog.
  • Kumactl can now verify CP cert against the specified CA.
  • We can introduce one extra flag to require auth on resources modification and have a simple builtin mechanism for authentication on Universal.
  • This change also helps us to introduce later more sophisticated mechanism for auth (like OIDC)
  • Since Admin Server is gone and so its config. We were using KUMA_ADMIN_SERVER_APIS_DATAPLANE_TOKEN_ENABLED to determine if we should require a dp token on universal. This is now changed to KUMA_DP_SERVER_AUTH_TYPE where you can enable/disable auth on both K8S and Universal

Leftovers for separate PR

  • How to configure Kubernetes with client certs for TLS version
  • Adjust Kuma CP Grafana dashboard

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
@jakubdyszkiewicz jakubdyszkiewicz requested a review from a team as a code owner October 27, 2020 14:15
app/kumactl/cmd/config/config_control_planes_add.go Outdated Show resolved Hide resolved
app/kumactl/cmd/config/config_control_planes_add.go Outdated Show resolved Hide resolved
app/kumactl/pkg/cmd/root_context.go Outdated Show resolved Hide resolved
app/kumactl/pkg/resources/client.go Outdated Show resolved Hide resolved
pkg/api-server/auth/admin.go Outdated Show resolved Hide resolved
pkg/api-server/auth/admin.go Outdated Show resolved Hide resolved
pkg/api-server/auth/admin.go Outdated Show resolved Hide resolved
return
}
log.Info("attempt to access admin endpoints from the outside of the same machine without allowed certificates")
response.WriteHeader(403)
Copy link
Contributor

Choose a reason for hiding this comment

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

And if this is an authn then 401 makes more sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After putting some thoughts, I think this is authz. Authentication itself is done by mTLS. Here we are checking if you are Mesh Admin (by providing certs or having access to the machine) you can access admin endpoints

pkg/api-server/auth_test.go Outdated Show resolved Hide resolved
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Copy link
Contributor

@nickolaev nickolaev left a comment

Choose a reason for hiding this comment

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

That is a fantastic improvement! Merge it :)

@jakubdyszkiewicz jakubdyszkiewicz merged commit c3f5297 into master Oct 29, 2020
@jakubdyszkiewicz jakubdyszkiewicz deleted the feat/api-sever-one-port branch October 29, 2020 13:04
@tharun208
Copy link
Contributor

closes #184

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.

None yet

4 participants