-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
add update verb to routes/custom-host for admin role #18177
Conversation
[test] |
/hold @openshift/sig-security Need to determine what this change actually means from a security and networking prescriptive. |
/lgtm |
Clayton can you explain how adding this permission is OK? |
Users who can create something can edit it, because they can always delete it and recreate. There is no security benefit to preventing this. The original separation for online was:
In the future we may relax this to the edit role. |
@smarterclayton so does that mean we no longer need #18195 ? |
Yes, as described in the comments of the bug.
…On Thu, Jan 25, 2018 at 5:50 PM, Mo Khan ***@***.***> wrote:
@smarterclayton <https://github.com/smarterclayton> so does that mean we
no longer need #18195 <#18195> ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18177 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_pyeNIOQLZ4NuzVyFCch8r7lslROZks5tOQUlgaJpZM4RjvBy>
.
|
/hold cancel |
/hold I did not think we wanted namespaced users to be able to change their routes' hostnames. |
Bah, you're right. Reading through the commit log reminded me of the
original subtleties.
original intent was to control three things
1. Can you request a custom host ("create" "routes/custom-host")
2. Can you change custom host ("update" "routes/custom-host")
3. Are you allowed to set TLS certificates on a route
Online only wanted 1, because 2 allows you to steal routes, and 3 allows
you to break some routers. In general, out of the box we don't want to
give "update" "routes/custom-host" to anyone - I was wrong above. We also
don't want to give everyone access to update tls certificates.
When we implemented this, we went through a long chain of argument on 3 -
should we even allow you to set TLS certs when you don't have permission to
create custom hosts. Setting a publicly facing TLS cert doesn't have a ton
of value if your admin isn't letting you have a custom name, and the admin
is encouraged to use the default cert. But even if the admin didn't have a
default cert, if they could still (via the API with a higher privileged
operation) set certs, they would be safe. So we made it so that create
custom-host controlled setting TLS certs on the route, which solved the
online problem and preserved the less strictly tenanted use case as well -
an admin bot that wanted to set certs could be given create-host.
The set of people who should be able to update a host on a route is only
"update" "routes/custom-host". The set of people who should be able to
update tls certificates on the route are:
1. people who can create custom-hosts (they could delete and recreate the
route, and we shouldn't be abusive)
2. people who can update custom-hosts (they can steal other people's
routes, and so we're not worried about them)
The check for updating TLS certificates should involve checking for either
of those permissions. Strictly speaking:
1. "update" "routes/custom-host" should allow unrestricted changes to routes
2. "create" "routes/custom-host" should allow setting host on creation and
changing tls certificate
3. without those, you should be unable to set either host or tls certificate
We should update the strategy check for "update" to
1. check update if the host has changed, and if false, disallow any changes
2. if host hasn't changed but certs have, check "create", and if false,
disallow changes
3. allow changes
…On Thu, Jan 25, 2018 at 8:25 PM, Jordan Liggitt ***@***.***> wrote:
/hold
I did not think we wanted namespaced users to be able to change their
routes' hostnames.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18177 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_px4R6a6qcussQb0cRnuK6Fdu7Vwiks5tOSmbgaJpZM4RjvBy>
.
|
…t role): bz1524707
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rajatchopra, 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 |
/lgtm cancel //PR changed after LGTM, removing LGTM. @rajatchopra @smarterclayton |
Okay so the final proposed fix is in PR #18312 |
@rajatchopra: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Automatic merge from submit-queue (batch tested with PRs 18422, 18312). tls update will be possible with 'create' permissions on custom-host Supercedes #18177 Fix for bz: https://bugzilla.redhat.com/show_bug.cgi?id=1524707
Fix, as requested in https://bugzilla.redhat.com/show_bug.cgi?id=1524707
Also, to address issue #15772
@openshift/sig-networking