Router metrics should be protected by delegated auth #16975
Conversation
Allow additional authorization checking from the master by performing
delegating auth when the router metrics endpoint is accessed. Use the
cmux library to put HTTP and HTTPS on the same port so that we can avoid
complex setup requirements for routers which must be HTTP available for
healthchecks, but should be HTTPS protected for accepting client
authentication.
Uses the delegating authorizer to protect the endpoint with the SAR for
group: route.openshift.io
resource: routers
name: <router-name>
subresource: metrics for /metrics and debug for /debug
openshift-ci-robot
added
the
size/L
openshift-merge-robot
added
the
approved
| }, | ||
| Checks: []healthz.HealthzChecker{check}, | ||
| } | ||
| if certFile := util.Env("ROUTER_METRICS_TLS_CERT_FILE", ""); len(certFile) > 0 { |
There was a problem hiding this comment.
Why is this an env var and not a mounted file?
There was a problem hiding this comment.
Why is this an env var and not a mounted file?
Oh, nm. Misread. It's the name. Why isn't that a flag?
There was a problem hiding this comment.
Following convention mostly for things we aren't sure we want a formal API in flags for. I could set it experimental as a flag I guess.
|
It's not particularly pretty, but I think it works. How many releases are we going to continue allowing http access to healthz? |
| Password: o.StatsPassword, | ||
| Authenticator: authn, | ||
| Authorizer: authz, | ||
| Record: authorizer.AttributesRecord{ |
There was a problem hiding this comment.
This structure looks ok to me. It allows subdivision for particular routers and the subresource pattern lets you say things like: you can see metrics for all routers.
|
Right now it has to be http because it's a host port that cloud load
balancers use to heartbeat / health check, and a few of them only support
HTTP. Hoping though we can transition later. Technically nodes need to
start having a unified health check for kubelet plus other key services,
which if we have then this may be less important. But haven't thought that
far
On Oct 20, 2017, at 6:36 PM, David Eads <[email protected]> wrote:
It's not particularly pretty, but I think it works. How many releases are
we going to continue allowing http access to healthz?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#16975 (comment)>,
or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABG_pyKIIOxPnehaAG8ux4sWsl2jtXT6ks5suMvqgaJpZM4QA4I7>
.
|
|
/test extended_conformance_gce |
|
/test extended_conformance_install_update |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, smarterclayton The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
smarterclayton
added
kind/delivery-blocker
kind/bug
|
Automatic merge from submit-queue. |
|
@smarterclayton Do you have a PR for the doc changes? |
|
no, we won't enable this until 3.8
…On Wed, Oct 25, 2017 at 2:50 PM, Phil Cameron ***@***.***> wrote:
@smarterclayton <https://github.com/smarterclayton> Do you have a PR for
the doc changes?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16975 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p5_Ekgx-i-QAOqZ0ibA2q-U8ZAtfks5svy6ZgaJpZM4QA4I7>
.
|
|
/lgtm Thanks @smarterclayton. |
Allow additional authorization checking from the master by performing
delegating auth when the router metrics endpoint is accessed. Use the
cmux library to put HTTP and HTTPS on the same port so that we can avoid
complex setup requirements for routers which must be HTTP available for
healthchecks, but should be HTTPS protected for accepting client
authentication.
Uses the delegating authorizer to protect the endpoint with the SAR for
@deads2k this is a compromise between three factors:
Opinions?