Statistics default period_start leaks information

[justinsb]

When issuing a request to the "GET /v2/meters/(meter_name)/statistics" endpoint with a period, if period_start is not specified, it defaults to the lowest timestamp of all the data in the system - not just the current meter, but even data belonging to other users (I think!).

That is surprising to me; I would have expected it to default to e.g. the epoch, or some other well-defined year (2000?)

It seems that this is both unfriendly to users (who really must specify period_start) and unfriendly to the system (which must find the minimum start_time).

If I'm right about it being for all users, it's even a theoretical security issue, because it discloses another user's data. Though this is of no practical concern IMHO.

Is this a bug? Or is there a reason for this behaviour that I'm missing?

[Eoghan Glynn (eglynn)]

What storage backend are you using?

Looking at the mongodb implementation, the map-reduce is applied to pre-selected samples only for the user's project (assuming the role is non-admin) so only matching samples are included in the determination of the overall duration and period_start for each period, not all samples.

Similarly for the sqlalchemy backend, the selection of samples for the callers project happens in advance of the minimum timestamp being calculated.

Is it the admin or non-admin case that you're more concerned with?

Is the main issue that a timestamp range should be defaulted if none is explicitly specified for the stats query?

[justinsb (justin-fathomdb)]

I have 3 concerns:

1) Inefficient: My primary concern is for efficiency; I don't want to scan the entire system (or every resource) to find the minimum timestamp across all samples.

2) Unexpected: I also think it's odd behaviour from a user point of view. I think if a user specifies hourly increments and doesn't specify a starting point, they would expect to see times aligned with the hours (00:00, 01:00 etc).

3) Insecure: Finally, I think there's also a security issue with information disclosure, because users can see the timestamp of someone else's sample if they specify a period without specifying period_start.

Of these three concerns, I'll freely admit the third is the least of my concerns, but also likely much more effective at getting the change I want for the first two reasons :-) Presuming I'm right of course!

Looking at the code, I think hbase and sqlalchemy avoid the security issue, but db2 and mongo do not:

impl_db2.py in get_meter_statistics:

        if period:
            if sample_filter.start:
                ...
            else:
                period_start = self.db.meter.find(
                    limit=1, sort=[('timestamp',
                                    pymongo.ASCENDING)])[0]['timestamp']

impl_mongodb.py in get_meter_statistics:

        if period:
            if sample_filter.start:
                ...
            else:
                period_start = self.db.meter.find(
                    limit=1, sort=[('timestamp',
                                    pymongo.ASCENDING)])[0]['timestamp']

period_start is then returned to the user:

db2:
 stat.period_start = (period_start +
                                     datetime.
                                     timedelta(**(_to_offset(periods))))

mongo's map reduce is a lot more complicated, but seems to do the same thing. It must do, or else user-specified period_start would be broken.

I think impl_hbase and impl_sqlalchemy at least avoid the security issue; they do filter the rows to find the minimum matching the sample. It's still inefficient and unexpected behaviour though, IMHO.

To be clear: the information disclosure is in the default period_start. It exposes a different user's sample timestamp. I propose we default this to the Unix Epoch or Y2K instead. This won't break any documented API. It will be more efficient, will better match user's expectations, and will be more secure.

If filtering by user is permitted behaviour, I guess I can just do that. It is odd that some of the backends do filter, and some don't. I'm also wondering why the behaviour is here in the first place. As I said originally, it's harder to implement, and I can't imagine any user actually expects it. If there's a good reason for it, then please let me know what it is, but it's not documented and it "feels wrong".

[justinsb (justin-fathomdb)]

So this somehow got set to "Opinion", which doesn't really make sense ... I believe is either Invalid (there is no information disclosure), or Confirmed (there is information disclosure). Is it OK to re-set back to New so it gets a second look?

[justinsb (justin-fathomdb)]

It's been 3 weeks without a reply, so I'm going to reopen. Hope this is OK!

[Igor Degtiarov (idegtiarov)]

Will verify and fix if I find any issues with that bug.

[Igor Degtiarov (idegtiarov)]

I've found that now we have a really odd output of the start/end period of statistics for the sample if period doesn't set. The same strange output we have with the duration start/end fields when set the period param. I have tried it with mongo backend. So will start with the patch for mongodb.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/143820

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/143820
Committed: https://git.openstack.org/cgit/openstack/ceilometer/commit/?id=262e2cf2f31630f3ab41788dc82153f6a85ac8c0
Submitter: Jenkins
Branch: master

commit 262e2cf2f31630f3ab41788dc82153f6a85ac8c0
Author: Igor Degtiarov <email address hidden>
Date: Thu Jan 22 16:25:36 2015 +0200

    [MongoDB] Improves get_meter_statistics method

    Now period_end that returns in get_meter_statistic method
    doesn't have correct value on MongoDB backend if period
    doesn't set in query.

    This patch improves that.

    Change-Id: I980f630e0a113638d4b06608ae999ae56d92338f
    Closes-Bug: #1372442