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

[Lucas Ponce]

What is the result of this discussion ?

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.

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.
- 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/hawkular-dev/attachments/20170302/cc3d2ebf/attachment.html