OCPBUGS-34373: routes/custom-host permission update for externalCertificate

[chiragkyal]

https://issues.redhat.com//browse/OCPBUGS-34373

[openshift-ci-robot]

@chiragkyal: This pull request references Jira Issue OCPBUGS-34373, which is invalid:

• expected the bug to target the "4.17.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

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 openshift-eng/jira-lifecycle-plugin repository.

[chiragkyal]

/assign @alebedev87 @Miciah

[chiragkyal]

/cc @lihongan

[lihongan]

/jira refresh

[openshift-ci-robot]

@lihongan: This pull request references Jira Issue OCPBUGS-34373, which is valid.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.17.0) matches configured target version for branch (4.17.0)
• bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @lihongan

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

[openshift-bot]

Inactive enhancement proposals go stale after 28d of inactivity.

See https://github.com/openshift/enhancements#life-cycle for details.

Mark the proposal as fresh by commenting /remove-lifecycle stale.
Stale proposals rot after an additional 7d of inactivity and eventually close.
Exclude this proposal from closing by commenting /lifecycle frozen.

If this proposal is safe to close now please do so with /close.

/lifecycle stale

[chiragkyal]

/remove-lifecycle stale

[chiragkyal]

/jira refresh

[openshift-ci-robot]

@chiragkyal: This pull request references Jira Issue OCPBUGS-34373, which is valid.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.18.0) matches configured target version for branch (4.18.0)
• bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @lihongan

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

[alebedev87]

LGTM, a couple of nit picks.

[alebedev87]

/lgtm

[alebedev87]

/approve
/hold

Can I?

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alebedev87

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• enhancements/ingress/OWNERS [alebedev87]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[alebedev87]

@Miciah : feel free to unhold if you are fine with the change.

[Miciah]

This restriction really makes me nervous, for two reasons:

• This restriction potentially blocks all updates to a route that specifies spec.tls.externalCertificate if the user does not have the "custom-host" permission. Even updating an unrelated field or adding an annotation could be blocked. However, we already discussed this concern here: CFE-885:Feature route external certificate validation openshift-apiserver#407 (comment). I'll continue to defer to the API reviewers if this is all right with them.

• It isn't clear how this restriction on updates to the route improves security given that the user could update the secret without updating the route. It seems that giving a user permission to create a route with spec.tls.externalCertificate implicitly gives the user permission to update the certificate, so maybe we need to require the user to have update permission in order to specify spec.tls.externalCertificate at all.

Can you make sure the EP explicitly calls out the first point (blocking all updates) and clearly addresses the second (how this restriction improves security)?

[chiragkyal]

Can you make sure the EP explicitly calls out the first point (blocking all updates)

Sure, I'll add that information in the EP body.

---

On the second concern, I think we might need to retain certain points from the Exception in Validations between API server and the router section which explains that "custom-host" permission check is intentionally done only on the API server and not the router as well.

Post creation of the route if the permissions for `custom-host`
are revoked and the user edits the contents of the secret, the router would still be
able to successfully reload the certificate on the route.

so maybe we need to require the user to have update permission in order to specify spec.tls.externalCertificate at all.

If a user has "custom-host" "create" permission, then "update" permission is optional to update the TLS certificate. Updating TLS certificate requires either create or update permission on the routes/custom-host sub-resource. So, users having "create" permission can do both create and update route with non-empty spec.tls.externalCertificate

[Miciah]

Can you make sure the EP explicitly calls out the first point (blocking all updates)

Sure, I'll add that information in the EP body.

Thanks!

On the second concern, I think we might need to retain certain points from the Exception in Validations between API server and the router section which explains that "custom-host" permission check is intentionally done only on the API server and not the router as well.

Post creation of the route if the permissions for `custom-host`
are revoked and the user edits the contents of the secret, the router would still be
able to successfully reload the certificate on the route.

so maybe we need to require the user to have update permission in order to specify spec.tls.externalCertificate at all.

If a user has "custom-host" "create" permission, then "update" permission is optional to update the TLS certificate. Updating TLS certificate requires either create or update permission on the routes/custom-host sub-resource. So, users having "create" permission can do both create and update route with non-empty spec.tls.externalCertificate

I see—create permission is already sufficient to update spec.tls.certificate, so it would be inconsistent to require update permission to specify an external certificate. Moreover, requiring update permission to specify an external certificate doesn't prevent the user from updating the external certificate after the permission has been revoked. Do I understand correctly?

That leaves me with two unanswered questions:

• How does restricting updates on the route improve security when someone could update the secret for the external certificate?
• Can we prevent the route owner from updating the secret after custom-host create/update permissions have been revoked? Or, do we just need to accept (and document!) that giving someone permission to use an external certificate effectively gives them irrevocable permission to update the certificate later on?

[chiragkyal]

Sure, I'll add that information in the EP body.

Added

Moreover, requiring update permission to specify an external certificate doesn't prevent the user from updating the external certificate after the permission has been revoked. Do I understand correctly?

Yes, that's correct. Users can update the external certificate (secret) even after the "custom-host" permission is revoked.

How does restricting updates on the route improve security when someone could update the secret for the external certificate?

You're right, it doesn't provide additional security in this context. As mentioned in the EP, this is a known limitation.

Can we prevent the route owner from updating the secret after custom-host create/update permissions have been revoked? Or, do we just need to accept (and document!) that giving someone permission to use an external certificate effectively gives them irrevocable permission to update the certificate later on?

• One external certificate (secret) could be used (shared) by multiple routes. After the "custom-host" permissions are revoked for a certain route owner, blocking the secret update could cause issues for other route owners.

I haven't been able to identify an easier way to address this currently; it likely requires additional or potentially complex changes. This is an existing drawback, and since we haven't found a clear workaround, it should be documented.

[chiragkyal]

As discussed in the meeting, since this is an existing design choice and the bug is focused on making the custom-host permission consistent, does it make sense to discuss the resolution as part of a separate ticket if that is really a security concern?

[openshift-ci]

New changes are detected. LGTM label has been removed.

[openshift-ci]

@chiragkyal: all tests passed!

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.