fix: return OpaqueAccessDenied for NotFound remote cluster errors

[nklaassen]

This PR modifies OpaqueAccessDenied to return an identical generic NotFound error whether the input error is NotFound or AccessDenied. The commit also updates call sites of OpaqueAccessDenied related to remote clusters to use it in the paths where there is any error fetching the cluster as well as when access is denied.

It doesn't do much good to return AccessDenied errors as NotFound if they don't match the NotFound error you would get if the resource really didn't exist. It's trivial to tell the errors apart and discover the existence of a resource you shouldn't be allowed to access. This PR attempts to mitigate that issue and properly hide the existence of remote clusters the user should not be allowed to list.

Changelog: generic "not found" errors are returned whether a remote cluster can't be found or access is denied.

[espadolini]

I think we have plenty of code around that does "if update() is notfound then create()" or "if create() is alreadyexists then update()" - are we ok with potentially breaking that logic? If a Foo doesn't exist and a user that doesn't have blanket read or list permissions for Foos attempts to interact with that Foo, shouldn't the error be AccessDenied rather than NotFound?

[nklaassen]

I'll try to address each of your comments here @espadolini

I think we have plenty of code around that does "if update() is notfound then create()"

The only case that will change is where update() used to return AccessDenied and now returns NotFound. In this case we will try the create and get an AlreadyExists error, unless the user has the update verb but not create, and they'll get AccessDenied. I don't see what this would break, the operation ultimately fails either way, and from an RBAC perspective I think it's fine. If you have the create verb we kind of have to leak the existence of resources that already exists if you can name them. But the update verb alone doesn't need to leak anything, we can just treat it as a NotFound. But I would be open to reverting my changes to any Update endpoints if you think it's better or safer to return the AccessDenied

or "if create() is alreadyexists then update()"

No changes here, I'm not touching any Create endpoints, there's no need to.

If a Foo doesn't exist and a user that doesn't have blanket read or list permissions for Foos attempts to interact with that Foo, shouldn't the error be AccessDenied rather than NotFound?

Yes, I'm not changing this, if they don't have read or list permissions they will still get AccessDenied. It only changes if they try to Get a resource they're not allowed to access by label, now they get NotFound instead of AccessDenied, and I think that's correct

[espadolini]

now they get NotFound instead of AccessDenied, and I think that's correct

I'm arguing that that's just a bad lie, whereas returning AccessDenied even if the item doesn't exist is the correct thing to do, since an item that's missing can't match any non-blanket access permissions that the user might have.

[nklaassen]

now they get NotFound instead of AccessDenied, and I think that's correct

I'm arguing that that's just a bad lie, whereas returning AccessDenied even if the item doesn't exist is the correct thing to do, since an item that's missing can't match any non-blanket access permissions that the user might have.

First I'll push back a bit: I have never heard of returning AccessDenied in place of a NotFound error, whereas returning NotFound when access denied is quite common and we are already doing it in some places. A NotFound means that the resource is not found among the set of resources that the user is allowed to list/read, that is not a lie. I don't really want to go this far, but there's even an argument to be made that a missing resource would match a non-blanket access permission that is a negative label match.

Second: I don't really want to waste our time debating this. A customer filed a support ticket because they were able to discover trusted clusters that they weren't supposed to know about, I thought I could push a quick fix, the relevant change is in lib/proxy/router.go. It is already returning an OpaqueAccessDenied error but only in the path where access is denied, not in the NotFound path, so it's completely ineffective. Would you feel more comfortable approving this if I only change that paths where we're already using OpaqueAccessDenied ineffectively?

[rosstimothy]

I would suggest scoping this down to just the problem that we are trying to solve for now. It reduces the risk of tsh or tctl doing the wrong thing because the error returned from an API is now different.

[espadolini]

A NotFound means that the resource is not found among the set of resources that the user is allowed to list/read, that is not a lie.

I'm convinced, NotFound is the better of the two errors; the situation I was worried about with the failure to create after a not found error would require permissions to (over)write but not read, which is basically never a thing.

[nklaassen]

I went ahead and removed all changes not related to remote clusters to reduce the risk on this PR

[espadolini]

Up to you if you want to scope it down and remove the new uses of OpaqueAccessDenied like @rosstimothy suggested or not.

edit: it's only extending the existing uses to make them effective now

[public-teleport-github-review-bot]

@nklaassen See the table below for backport results.

Branch	Result
branch/v13	Failed
branch/v14	Failed
branch/v15	Failed