tls update will be possible with 'create' permissions on custom-host #18312

rajatchopra · 2018-01-26T22:14:55Z

Supercedes #18177

Fix for bz: https://bugzilla.redhat.com/show_bug.cgi?id=1524707

… Checks on changing host stay the same.

rajatchopra · 2018-01-26T22:16:35Z

liggitt · 2018-01-27T19:15:29Z

pkg/route/registry/route/strategy.go

@@ -214,14 +214,37 @@ func (s routeStrategy) validateHostUpdate(ctx apirequest.Context, route, older *
 		if hostChanged {


this isn't quite what was described in #18177 (comment):

check update if the host has changed, and if false, disallow any changes

if host hasn't changed but certs have, check "create", and if false, disallow changes

allow changes

I think we should only run the update custom-host SAR (the one above) if hostChanged

@liggitt It implements the logic as described. Although this code does an extra check for 'update' always, it is to prevent a case where someone has an 'update' permission but does not have 'create'. We should allow that case to be able to update TLS certs.
Review again please.

pravisankar

LGTM

pravisankar · 2018-01-31T01:05:41Z

@smarterclayton can you please review this pr? (@liggitt is on pto this week)
I think the current implementation matches with what you suggested in #18177 (comment)

knobunc · 2018-01-31T19:24:26Z

@rajatchopra We'll need to amend openshift/openshift-docs#7398 when we get this merged.

enj · 2018-02-01T18:35:34Z

Please copy the entire requirement from @smarterclayton at #18177 (comment) as a comment into this part of the code and reference it throughout the checks. No one will remember in a few months how we came to this conclusion.

@openshift/sig-security

tnozicka · 2018-02-01T18:47:16Z

this is exactly what I need to run http://github.com/tnozicka/openshift-acme as regular user (hopefully the last part)

lgtm but agree with @enj to have that extended comment in code

tnozicka · 2018-02-01T18:48:03Z

/retest

tnozicka · 2018-02-01T19:02:53Z

/cherrypick release-3.7

openshift-cherrypick-robot · 2018-02-01T19:02:54Z

@tnozicka: once the present PR merges, I will cherry-pick it on top of release-3.7 in a new PR and assign it to you.

In response to this:

/cherrypick release-3.7

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.

tnozicka · 2018-02-01T19:03:06Z

/cherrypick release-3.8

openshift-cherrypick-robot · 2018-02-01T19:03:07Z

@tnozicka: once the present PR merges, I will cherry-pick it on top of release-3.8 in a new PR and assign it to you.

In response to this:

/cherrypick release-3.8

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.

knobunc · 2018-02-02T14:52:21Z

For posterity, the comment at #18177 (comment) by @smarterclayton that led to this approach was:

original intent was to control three things

Can you request a custom host ("create" "routes/custom-host")

Can you change custom host ("update" "routes/custom-host")

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:

people who can create custom-hosts (they could delete and recreate the
route, and we shouldn't be abusive)

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:

"update" "routes/custom-host" should allow unrestricted changes to routes

"create" "routes/custom-host" should allow setting host on creation and
changing tls certificate

without those, you should be unable to set either host or tls certificate

We should update the strategy check for "update" to

check update if the host has changed, and if false, disallow any changes

if host hasn't changed but certs have, check "create", and if false,
disallow changes

allow changes

knobunc · 2018-02-05T18:48:15Z

/lgtm

openshift-ci-robot · 2018-02-05T18:48:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knobunc, rajatchopra

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/route/OWNERS~~ [knobunc,rajatchopra]

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 · 2018-02-05T21:54:21Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-merge-robot · 2018-02-05T22:02:45Z

Automatic merge from submit-queue (batch tested with PRs 18422, 18312).

openshift-cherrypick-robot · 2018-02-05T22:03:17Z

@tnozicka: new pull request created: #18459

In response to this:

/cherrypick release-3.7

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.

openshift-cherrypick-robot · 2018-02-05T22:03:53Z

@tnozicka: new pull request created: #18460

In response to this:

/cherrypick release-3.8

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.

@jwforres

Automatic merge from submit-queue. Allow users to edit route TLS if they can create custom hosts Console change for openshift/origin#18312 Closes #2699 /assign @jwforres /hold because the upstream change hasn't merged

enj · 2018-02-14T22:49:46Z

@knobunc it would be helpful to actually address the comments before merging the code.

tls update will be possible with 'create' permissions on custom-host.…

6785f92

… Checks on changing host stay the same.

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 26, 2018

openshift-ci-robot requested review from liggitt and smarterclayton January 26, 2018 22:15

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 26, 2018

rajatchopra mentioned this pull request Jan 26, 2018

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

Closed

This was referenced Jan 27, 2018

Update route editor TLS permission checks openshift/origin-web-console#2699

Closed

Allow users to edit route TLS if they can create custom hosts openshift/origin-web-console#2730

Merged

liggitt reviewed Jan 27, 2018

View reviewed changes

pravisankar approved these changes Jan 31, 2018

View reviewed changes

pravisankar added the component/routing label Jan 31, 2018

enj mentioned this pull request Feb 1, 2018

Regular user can't grant update privileges to update route.openshift.io/routes/custom-host #18397

Closed

openshift-ci-robot added the sig/security label Feb 1, 2018

openshift-ci-robot assigned knobunc Feb 5, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 5, 2018

openshift-merge-robot merged commit bb140ad into openshift:master Feb 5, 2018

This was referenced Feb 5, 2018

[release-3.7] tls update will be possible with 'create' permissions on custom-host #18459

Closed

[release-3.8] tls update will be possible with 'create' permissions on custom-host #18460

Closed

tnozicka mentioned this pull request Feb 6, 2018

refactor assistance tnozicka/openshift-acme#47

Closed

rajatchopra deleted the tls_admit_bug branch February 6, 2018 21:05

This was referenced Mar 17, 2018

Route not updating with TLS information tnozicka/openshift-acme#56

Closed

Run on OpenShift Online tnozicka/openshift-acme#28

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls update will be possible with 'create' permissions on custom-host #18312

tls update will be possible with 'create' permissions on custom-host #18312

rajatchopra commented Jan 26, 2018

rajatchopra commented Jan 26, 2018

liggitt Jan 27, 2018

rajatchopra Jan 29, 2018

pravisankar left a comment

pravisankar commented Jan 31, 2018

knobunc commented Jan 31, 2018

enj commented Feb 1, 2018

tnozicka commented Feb 1, 2018 •

edited

Loading

tnozicka commented Feb 1, 2018

tnozicka commented Feb 1, 2018

openshift-cherrypick-robot commented Feb 1, 2018

tnozicka commented Feb 1, 2018

openshift-cherrypick-robot commented Feb 1, 2018

knobunc commented Feb 2, 2018

knobunc commented Feb 5, 2018

openshift-ci-robot commented Feb 5, 2018

openshift-merge-robot commented Feb 5, 2018

openshift-merge-robot commented Feb 5, 2018

openshift-cherrypick-robot commented Feb 5, 2018

openshift-cherrypick-robot commented Feb 5, 2018

enj commented Feb 14, 2018

		@@ -214,14 +214,37 @@ func (s routeStrategy) validateHostUpdate(ctx apirequest.Context, route, older *
		if hostChanged {

tls update will be possible with 'create' permissions on custom-host #18312

tls update will be possible with 'create' permissions on custom-host #18312

Conversation

rajatchopra commented Jan 26, 2018

rajatchopra commented Jan 26, 2018

liggitt Jan 27, 2018

Choose a reason for hiding this comment

rajatchopra Jan 29, 2018

Choose a reason for hiding this comment

pravisankar left a comment

Choose a reason for hiding this comment

pravisankar commented Jan 31, 2018

knobunc commented Jan 31, 2018

enj commented Feb 1, 2018

tnozicka commented Feb 1, 2018 • edited Loading

tnozicka commented Feb 1, 2018

tnozicka commented Feb 1, 2018

openshift-cherrypick-robot commented Feb 1, 2018

tnozicka commented Feb 1, 2018

openshift-cherrypick-robot commented Feb 1, 2018

knobunc commented Feb 2, 2018

knobunc commented Feb 5, 2018

openshift-ci-robot commented Feb 5, 2018

openshift-merge-robot commented Feb 5, 2018

openshift-merge-robot commented Feb 5, 2018

openshift-cherrypick-robot commented Feb 5, 2018

openshift-cherrypick-robot commented Feb 5, 2018

enj commented Feb 14, 2018

tnozicka commented Feb 1, 2018 •

edited

Loading