Glance allows to sort images by private fields

[Mike Fedosin]

Glance api supports sorting only by 'name', 'status', 'container_format', 'disk_format', 'size', 'id', 'created_at', 'updated_at'
But now it's possible to make sorting by private fields like checksum or min_ram (/images?sort_key=checksum), that violates api.

It's possible because there is no key validation on the api layer in v2.
There is a check on the db in pagination, but it covers all the fields (not only api), which causes a problem.

[Feilong Wang (flwang)]

hi Mike, what did you mean 'violates api'? I'm trying to understand the impact of this. Thanks.

[Mike Fedosin (mfedosin)]

Oh, sure :) I mean the following: in api there is a set of keys that we can sort the index output by ('name', 'status', 'container_format', 'disk_format', 'size', 'id', 'created_at', 'updated_at'), other keys are not supported.
In v1 there is a check on both client and server sides (https://github.com/openstack/python-glanceclient/blob/master/glanceclient/v1/images.py#L224 and https://github.com/openstack/glance/blob/master/glance/registry/api/v1/images.py#L287), but in v2 there are no checks at all. In glance client it's okay, because currently it doesn't support sort_key and sort_dir option (but there are my commits in gerrit which implement it). But glance server now can accept any sort key like 'blah' or 'foo', etc.
Then it calls 'list' in domain model with sort_key='foo', it successfully gone through this, reaches the db layer in sqlalchemy/api.py and in pagination method it performs the check that sort key is one of the db model fields (https://github.com/openstack/glance/blob/master/glance/db/sqlalchemy/api.py#L345). If sort key is 'foo' then it raises an exception, but if sort key is 'checksum' it goes further and returns full image list sorted by checksum.

So I see two problems here:
1. I think sort key validation should be performed on the api layer near the sort dir, not on the db - current implementation looks ugly.
2. Now it supports sorting by all model fields and it is violates api.

Just look at my commit https://review.openstack.org/#/c/139996/

[Erno Kuvaja (jokke)]

I do not think we should artificially limit something that does work just based on the fact that we do not explicitly state supporting it. There is clear difference between the two statements "we do not support this" and "we guarantee that this will not work".

I will close this for now. Please feel free to reopen if you have some other concerns around this than "violates api".

[Mike Fedosin (mfedosin)]

Damn :) I'm really trying to explain it right: check in pagination is very weak - it looks over all fields in model, just private ones!

Example: you can request /v2/images/sort_key=_sa_class_manager and it will pass the check and then there will be an exception in sql after executing the query: ArgumentError: SQL expression object or string expected and glance will return 500 code. It shouldn't be.

Also you can request /v2/images/sort_key=properties and it also will pass the check. There will not be an exception and you will get all images sorted by properties list id.

These requests make no sense, they are illogical and we have to process them right.

I don't mind about other keys like min_ram or something but we should deprecate '_sa_class_manager' as a key!

[Mike Fedosin (mfedosin)]

It's a list of currently appropriate sort keys that successfully pass trough the pagination check:
_decl_class_registry
_extra_keys
_sa_class_manager
checksum
container_format
created_at
deleted
deleted_at
disk_format
id
is_public
locations
members
metadata
min_disk
min_ram
name
owner
properties
protected
size
status
tags
updated_at
virtual_size

The first three and metadata belong to model class only and they not presented in db. So sorting by these keys will raise ArgumentError with 500 status code from glance.

[Nikhil Komawar (nikhil-komawar)]

The ongoing review is at : https://review.openstack.org/#/c/139996/ (if it gets missed from that big comment)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/139996
Committed: https://git.openstack.org/cgit/openstack/glance/commit/?id=f9820a25d38584fdcc9ffde44de6ec146b6de4fb
Submitter: Jenkins
Branch: master

commit f9820a25d38584fdcc9ffde44de6ec146b6de4fb
Author: Mike Fedosin <email address hidden>
Date: Mon Dec 8 16:22:05 2014 +0300

    Add sort key validation in v2 api

    Since v2 api has no sort key validation and the default
    pagination check is used it's possible to request something
    like /images?sort_key=_sa_class_manager, which causes an
    inner SQL exception with 500 response code from the server.

    This code validates input sort key and raises an exception
    if the parameter is out of the supported keys list.

    Closes-bug: 1400366

    Change-Id: I0cf58ad198375a2f6f58bd7820cbb9d86003247a