add update verb to routes/custom-host for admin role

[rajatchopra]

Fix, as requested in https://bugzilla.redhat.com/show_bug.cgi?id=1524707
Also, to address issue #15772

@openshift/sig-networking

[rajatchopra]

[test]

[enj]

/hold

@openshift/sig-security

Need to determine what this change actually means from a security and networking prescriptive.

[smarterclayton]

/lgtm

[enj]

Clayton can you explain how adding this permission is OK?

[smarterclayton]

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:

1. online wasn't allowed to create or edit custom hosts or TLS certs
2. default origin/OCP allowed that for project admins, so as to indicate that this was a higher level of permission

In the future we may relax this to the edit role.

[enj]

@smarterclayton so does that mean we no longer need #18195 ?

[smarterclayton]

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> .

[enj]

/hold cancel

[liggitt]

/hold

I did not think we wanted namespaced users to be able to change their routes' hostnames.

[smarterclayton]

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> .

[openshift-ci-robot]

[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:

• pkg/cmd/OWNERS [smarterclayton]
• test/testdata/OWNERS [smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[openshift-merge-robot]

/lgtm cancel //PR changed after LGTM, removing LGTM. @rajatchopra @smarterclayton

[rajatchopra]

Okay so the final proposed fix is in PR #18312
Closing this PR in favour of that.

[openshift-ci-robot]

@rajatchopra: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/unit	f3a3cee	link	/test unit

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.