[Hawkular-dev] Cross-Tenant endpoints in Alerting on OS

[Matt Wringe]

----- Original Message -----
> From: "Lucas Ponce" <lponce at redhat.com>
> To: "Discussions around Hawkular development" <hawkular-dev at lists.jboss.org>
> Sent: Thursday, 2 March, 2017 12:51:46 PM
> Subject: Re: [Hawkular-dev] Cross-Tenant endpoints in Alerting on OS
> 
> What is the result of this discussion ?

I think the result is that we need to have a meeting over cross tenancy and how it should be applied.

Supporting cross-tenancy is a fairly large architectural change and should be affecting multiple components.

At the very least we need to discuss how to properly handle it with regards to security integration with OpenShift.

> 
> If I understood well, even if we had a discoordination for these endpoints,
> those are not a security concern, or is there any additional test to perform
> to validate this ?
> 
> The original motivation is still valid, as for Alerting these two endpoints
> are demanded for users.

The validity of any feature request can always be up for discussion :)

> Do you want any additional enhancement around this ?
> 
> I can see two potential actions:
> - Perhaps to move the /admin endpoint prefix to /tenants to align with the
> same in metrics.

The /tenants endpoint in metrics is not for cross-tenant operations. Its probably not a great idea to to have two endpoints of the same name between components which do different things.

> - Extend the security filter on /admin (/tenants) endpoint.
> 
> Thanks,
> Lucas
> 
> 
> 
> On Fri, Feb 24, 2017 at 2:04 PM, Jay Shaughnessy < jshaughn at redhat.com >
> wrote:
> 
> 
> 
> On 2/23/2017 6:05 PM, Matt Wringe wrote:
> > Is there any reason why this being sent in private emails and not to a
> > mailing list?
> 
> Matt, Not really, sending to dev-list for anyone interested in the
> discussion...
> 
> > ----- Original Message -----
> >> There was an IRC discussion today about $SUBJECT. Here is a summary of
> >> a conversation Matt and I had to drill down into whether there was a
> >> cross-tenant security concern with the Alerting API in OS. In short,
> >> the answer seems to be no. Alerting (1.4+) offers two endpoints for
> >> fetching cross-tenant: /alerts/admin/alerts and /alerts/admin/events.
> >> Note that the 'admin' is just in the path, and was chosen just to group
> >> what we deemed were admin-level endpoints, the first two of which are
> >> these cross-tenant fetches. The 'admin' does not mean anything else in
> >> this context, it does not reflect a special user or tenant. The way
> >> these endpoints work is that that they accept a Hawkular-Tenant HTTP
> >> header that can be a comma-separated-list of tenantIds. As with any of
> >> the alerting endpoints. Alerting does not perform any security in the
> >> request handling. But, in OS the HAM deployments both have the OS
> >> security filtering in place. That filtering does two things, for a
> >> cluster-admin user it's basically a pass-thru, the CSL Hawkular-Tenant
> >> header is passed on and the endpoints work. For all other users the
> >> Hawkular-Tenant header is validated. Because each project name is a
> >> tenant name, the value must match a project name. As such, the
> >> validation fails if a CSL is supplied. This is decent behavior for now
> >> as it prevents any undesired access. Note that as a corner-case, these
> >> endpoints will work fine if the header just supplies a single tenant, in
> >> which case they are basically the same as the typical single-tenant
> >> fetch endpoints.
> > What has happened is now Alerts is not considering the Hawkular-tenant
> > header to contain just a string, but a comma separated lists of strings.
> > 
> > eg "Hawkular-tenant: projectA,projectB"
> 
> Note, not in general, comma-separated-lists handled only for the two
> cross-tenant endpoints mentioned above.
> 
> 
> > The OpenShift filter still considers this to be a string, so it will check
> > with OpenShift if the user has permission to access the project named with
> > a string value of "projectA,projectB". Since a project cannot have a ','
> > within its name, this check will always fail and return an access denied
> > error.
> > 
> > If the user is a cluster level user they are given access to everything,
> > even impossibly named projects. So a cluster level user will happen to be
> > able to use the current setup just due to how this works.
> > 
> > So there doesn't appear to be any security issue that we need to deal with
> > immediately, but we do probably want to handle this properly in the
> > future. It might not be too difficult to add support to the tenant to
> > consider a csl.
> > 
> >> I'm not totally familiar with the Metrics approach to cross-tenant
> >> handling but going forward we (Metrics and Alerting) should probably
> >> look for some consistency, if possible. Moreover, any solution should
> >> reflect what best serves OS. The idea of a CSL for the header is fairly
> >> simple and flexible. It may be something to consider, for the OS filter
> >> it would mean validating that the bearer has access to each of the
> >> individual tenants before forwarding the request.
> > I don't recall any meetings about adding multitenancy to Metrics. >From
> > what I recall, there is no plans at all to introduce multitenancy at all
> > for metrics.
> > 
> > If I was aware of this discussion when this was brought up for alerts, I
> > would have probably objected to the endpoint being called 'admin' since I
> > don't think that reflects what the true purpose of this is suppose to be.
> > Its not really an admin endpoint, but an endpoint for cross-tenancy. I
> > could have access to projectA and projectB, but not be an 'admin'
> > 
> > If we are making changes like this which affect security, I would really
> > like to be notified so that I can make sure our security filters will
> > function properly. Even if I am in the meeting when its being discussed it
> > would be good to ping me on the PR with the actual implementation.
> 
> Of course. This stuff went in in mid November and at that time we (in
> alerting) were really just getting settled with the initial integration
> into metrics for OS. Going forward I think we have a better idea of
> what is relevant to OS and can more easily flag items of import.
> 
> _______________________________________________
> hawkular-dev mailing list
> hawkular-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hawkular-dev
> 
> 
> _______________________________________________
> hawkular-dev mailing list
> hawkular-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hawkular-dev
>