authorization not checked in ec2 api

Bug #644092 reported by Jesse Andrews
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Compute (nova)
Fix Released
High
Devin Carlen

Bug Description

Users can see things they shouldn't be able to.

STEPS:

0) create two users a & b, NOT in the same project

1) start an instance as user a

2) get_console_output on user a's instance as user b

EXPECTED:

   the user should not get the output (user b should not be able to get console output for instances they don't own)

ACTUAL:

   console output from instance is returned successfully

DETAILS:

for instance, get_console_output in cloud.py:

    def get_console_output(self, context, instance_id, **kwargs):
        instance_ref = db.instance_get_by_str(context, instance_id[0])
        return rpc.call('%s.%s' % (FLAGS.compute_topic,
                                   instance_ref['host']),
                        {"method": "get_console_output",
                         "args": {"context": None,
                                  "instance_id": instance_ref['id']}})

sends a context (which has the user/project) into the instance_get_by_str function, but context is not used in the db layer to determine if the user has access to the instance.

Similarly for all other data store objects.

PROPOSAL:

My thought is that the datalayer should only return objects that the user is authorized to see. If we check at the API layer instead of the data access layer, we have the possibility of inconsistent rules for the same data types as the API evolves.

Revision history for this message
Jesse Andrews (anotherjesse) wrote :

This also occurs in the rackspace API.

Getting a common context and authorization would be nice.

Changed in nova:
importance: Undecided → High
milestone: none → austin-feature-freeze
Revision history for this message
Soren Hansen (soren) wrote :

I think those rbac decorators all over the cloud API got me fooled into thinking this was taken care of.

Now that I think about it, I'm not sure how they're supposed to work? They're applied before the object(s) being accessed are even known, so it only really checks if context.user has the given role on context.project, right? So any checks further down should check whether the object being accessed belongs to context.project. Is that accurate?

Revision history for this message
Michael Gundlach (gundlach) wrote : Re: [Bug 644092] Re: authorization not checked in ec2 api

On Tue, Sep 21, 2010 at 8:16 AM, Soren Hansen <email address hidden> wrote:

> Now that I think about it, I'm not sure how they're supposed to work?
> They're applied before the object(s) being accessed are even known, so
> it only really checks if context.user has the given role on
> context.project, right? So any checks further down should check whether
> the object being accessed belongs to context.project. Is that accurate?
>

Yep, that's accurate. The conversion from Tornado to eventlet (as of yet
unmerged to trunk) moves all that into an "Authorization" middleware. It
might help clarify the code if that were renamed to "MethodAuthentication"
and we do data authentication somewhere else.

Confidentiality Notice: This e-mail message (including any attached or
embedded documents) is intended for the exclusive and confidential use of the
individual or entity to which this message is addressed, and unless otherwise
expressly indicated, is confidential and privileged information of Rackspace.
Any dissemination, distribution or copying of the enclosed material is prohibited.
If you receive this transmission in error, please notify us immediately by e-mail
at <email address hidden>, and delete the original message.
Your cooperation is appreciated.

Revision history for this message
Vish Ishaya (vishvananda) wrote :
Download full text (3.8 KiB)

My thoughts on data authentication:
First we turn context into a dictionary so it is easy to pass around everywhere. The dictionary contains the following important data:
{'request_id': <random req string>,
 'user_id': ...,
 'project_id': ...,
 'superuser': <boolean, for bypassing project checking>
 'deleted' <tristate yes, no, all>}

superuser is set during authentication of the request, simply to avoid roundtrips to the auth layer.

My preference for a dictionary is simply that it is easy to pass through rabbit.

then inside the data layer, we have a simple check:
if context['superuser']:
  # don't limit the sql
elif context['project_id']:
  # add project_id = context
else:
   raise
switch on deleted to add deleted=0, deleted=1 or ignore

The user_id and request_id aren't used in the datalayer but are in context to help with logging.

On Sep 21, 2010, at 8:52 AM, Michael Gundlach wrote:

> On Tue, Sep 21, 2010 at 8:16 AM, Soren Hansen <email address hidden> wrote:
>
>> Now that I think about it, I'm not sure how they're supposed to work?
>> They're applied before the object(s) being accessed are even known, so
>> it only really checks if context.user has the given role on
>> context.project, right? So any checks further down should check whether
>> the object being accessed belongs to context.project. Is that accurate?
>>
>
> Yep, that's accurate. The conversion from Tornado to eventlet (as of yet
> unmerged to trunk) moves all that into an "Authorization" middleware. It
> might help clarify the code if that were renamed to "MethodAuthentication"
> and we do data authentication somewhere else.
>
>
> Confidentiality Notice: This e-mail message (including any attached or
> embedded documents) is intended for the exclusive and confidential use of the
> individual or entity to which this message is addressed, and unless otherwise
> expressly indicated, is confidential and privileged information of Rackspace.
> Any dissemination, distribution or copying of the enclosed material is prohibited.
> If you receive this transmission in error, please notify us immediately by e-mail
> at <email address hidden>, and delete the original message.
> Your cooperation is appreciated.
>
> --
> authorization not checked in ec2 api
> https://bugs.launchpad.net/bugs/644092
> You received this bug notification because you are a member of Nova
> Bugs, which is subscribed to OpenStack Compute (nova).
>
> Status in OpenStack Compute (Nova): New
>
> Bug description:
> Users can see things they shouldn't be able to.
>
> STEPS:
>
> 0) create two users a & b, NOT in the same project
>
> 1) start an instance as user a
>
> 2) get_console_output on user a's instance as user b
>
> EXPECTED:
>
> the user should not get the output (user b should not be able to get console output for instances they don't own)
>
> ACTUAL:
>
> console output from instance is returned successfully
>
> DETAILS:
>
> for instance, get_console_output in cloud.py:
>
> def get_console_output(self, context, instance_id, **kwargs):
> instance_ref = db.instance_get_by_str(context, instance_id[0])
> return rpc.call('%s.%s' % (FLAGS.compute_topic,
> ...

Read more...

Revision history for this message
Devin Carlen (devcamcar) wrote :
Download full text (5.9 KiB)

I like the practice of providing context all the way down the call stack. However I think first class citizens should not be dictionaries. We can accomplish the same thing with a proper object that implements todict()/fromdict().

Devin

On Sep 21, 2010, at 11:18 AM, vishvananda wrote:

> My thoughts on data authentication:
> First we turn context into a dictionary so it is easy to pass around everywhere. The dictionary contains the following important data:
> {'request_id': <random req string>,
> 'user_id': ...,
> 'project_id': ...,
> 'superuser': <boolean, for bypassing project checking>
> 'deleted' <tristate yes, no, all>}
>
> superuser is set during authentication of the request, simply to avoid
> roundtrips to the auth layer.
>
> My preference for a dictionary is simply that it is easy to pass through
> rabbit.
>
> then inside the data layer, we have a simple check:
> if context['superuser']:
> # don't limit the sql
> elif context['project_id']:
> # add project_id = context
> else:
> raise
> switch on deleted to add deleted=0, deleted=1 or ignore
>
> The user_id and request_id aren't used in the datalayer but are in
> context to help with logging.
>
> On Sep 21, 2010, at 8:52 AM, Michael Gundlach wrote:
>
>> On Tue, Sep 21, 2010 at 8:16 AM, Soren Hansen <email address hidden> wrote:
>>
>>> Now that I think about it, I'm not sure how they're supposed to work?
>>> They're applied before the object(s) being accessed are even known, so
>>> it only really checks if context.user has the given role on
>>> context.project, right? So any checks further down should check whether
>>> the object being accessed belongs to context.project. Is that accurate?
>>>
>>
>> Yep, that's accurate. The conversion from Tornado to eventlet (as of yet
>> unmerged to trunk) moves all that into an "Authorization" middleware. It
>> might help clarify the code if that were renamed to "MethodAuthentication"
>> and we do data authentication somewhere else.
>>
>>
>> Confidentiality Notice: This e-mail message (including any attached or
>> embedded documents) is intended for the exclusive and confidential use of the
>> individual or entity to which this message is addressed, and unless otherwise
>> expressly indicated, is confidential and privileged information of Rackspace.
>> Any dissemination, distribution or copying of the enclosed material is prohibited.
>> If you receive this transmission in error, please notify us immediately by e-mail
>> at <email address hidden>, and delete the original message.
>> Your cooperation is appreciated.
>>
>> --
>> authorization not checked in ec2 api
>> https://bugs.launchpad.net/bugs/644092
>> You received this bug notification because you are a member of Nova
>> Bugs, which is subscribed to OpenStack Compute (nova).
>>
>> Status in OpenStack Compute (Nova): New
>>
>> Bug description:
>> Users can see things they shouldn't be able to.
>>
>> STEPS:
>>
>> 0) create two users a & b, NOT in the same project
>>
>> 1) start an instance as user a
>>
>> 2) get_console_output on user a's instance as user b
>>
>> EXPECTED:
>>
>> the user should not get the output (user b should not be able to get console output for ins...

Read more...

Revision history for this message
Devin Carlen (devcamcar) wrote :
Download full text (5.9 KiB)

I like the practice of providing context all the way down the call stack. However I think first class citizens should not be dictionaries. We can accomplish the same thing with a proper object that implements todict()/fromdict().

Devin

On Sep 21, 2010, at 11:18 AM, vishvananda wrote:

> My thoughts on data authentication:
> First we turn context into a dictionary so it is easy to pass around everywhere. The dictionary contains the following important data:
> {'request_id': <random req string>,
> 'user_id': ...,
> 'project_id': ...,
> 'superuser': <boolean, for bypassing project checking>
> 'deleted' <tristate yes, no, all>}
>
> superuser is set during authentication of the request, simply to avoid
> roundtrips to the auth layer.
>
> My preference for a dictionary is simply that it is easy to pass through
> rabbit.
>
> then inside the data layer, we have a simple check:
> if context['superuser']:
> # don't limit the sql
> elif context['project_id']:
> # add project_id = context
> else:
> raise
> switch on deleted to add deleted=0, deleted=1 or ignore
>
> The user_id and request_id aren't used in the datalayer but are in
> context to help with logging.
>
> On Sep 21, 2010, at 8:52 AM, Michael Gundlach wrote:
>
>> On Tue, Sep 21, 2010 at 8:16 AM, Soren Hansen <email address hidden> wrote:
>>
>>> Now that I think about it, I'm not sure how they're supposed to work?
>>> They're applied before the object(s) being accessed are even known, so
>>> it only really checks if context.user has the given role on
>>> context.project, right? So any checks further down should check whether
>>> the object being accessed belongs to context.project. Is that accurate?
>>>
>>
>> Yep, that's accurate. The conversion from Tornado to eventlet (as of yet
>> unmerged to trunk) moves all that into an "Authorization" middleware. It
>> might help clarify the code if that were renamed to "MethodAuthentication"
>> and we do data authentication somewhere else.
>>
>>
>> Confidentiality Notice: This e-mail message (including any attached or
>> embedded documents) is intended for the exclusive and confidential use of the
>> individual or entity to which this message is addressed, and unless otherwise
>> expressly indicated, is confidential and privileged information of Rackspace.
>> Any dissemination, distribution or copying of the enclosed material is prohibited.
>> If you receive this transmission in error, please notify us immediately by e-mail
>> at <email address hidden>, and delete the original message.
>> Your cooperation is appreciated.
>>
>> --
>> authorization not checked in ec2 api
>> https://bugs.launchpad.net/bugs/644092
>> You received this bug notification because you are a member of Nova
>> Bugs, which is subscribed to OpenStack Compute (nova).
>>
>> Status in OpenStack Compute (Nova): New
>>
>> Bug description:
>> Users can see things they shouldn't be able to.
>>
>> STEPS:
>>
>> 0) create two users a & b, NOT in the same project
>>
>> 1) start an instance as user a
>>
>> 2) get_console_output on user a's instance as user b
>>
>> EXPECTED:
>>
>> the user should not get the output (user b should not be able to get console output for instan...

Read more...

Revision history for this message
Michael Gundlach (gundlach) wrote :
Download full text (6.6 KiB)

WSGI has an environ dictionary that is for storing context about the
request; would it make sense to replace APIRequestContext entirely, just
passing the environ dict around? Pro: reuses WSGI concept. Con: passing
around a dictionary that has more info in it than strictly needed; risks
coupling the code to WSGI more tightly.

- Michael

On Tue, Sep 21, 2010 at 2:18 PM, vishvananda <email address hidden> wrote:

> My thoughts on data authentication:
> First we turn context into a dictionary so it is easy to pass around
> everywhere. The dictionary contains the following important data:
> {'request_id': <random req string>,
> 'user_id': ...,
> 'project_id': ...,
> 'superuser': <boolean, for bypassing project checking>
> 'deleted' <tristate yes, no, all>}
>
> superuser is set during authentication of the request, simply to avoid
> roundtrips to the auth layer.
>
> My preference for a dictionary is simply that it is easy to pass through
> rabbit.
>
> then inside the data layer, we have a simple check:
> if context['superuser']:
> # don't limit the sql
> elif context['project_id']:
> # add project_id = context
> else:
> raise
> switch on deleted to add deleted=0, deleted=1 or ignore
>
> The user_id and request_id aren't used in the datalayer but are in
> context to help with logging.
>
> On Sep 21, 2010, at 8:52 AM, Michael Gundlach wrote:
>
> > On Tue, Sep 21, 2010 at 8:16 AM, Soren Hansen <email address hidden> wrote:
> >
> >> Now that I think about it, I'm not sure how they're supposed to work?
> >> They're applied before the object(s) being accessed are even known, so
> >> it only really checks if context.user has the given role on
> >> context.project, right? So any checks further down should check whether
> >> the object being accessed belongs to context.project. Is that accurate?
> >>
> >
> > Yep, that's accurate. The conversion from Tornado to eventlet (as of yet
> > unmerged to trunk) moves all that into an "Authorization" middleware. It
> > might help clarify the code if that were renamed to
> "MethodAuthentication"
> > and we do data authentication somewhere else.
> >
> >
> > Confidentiality Notice: This e-mail message (including any attached or
> > embedded documents) is intended for the exclusive and confidential use of
> the
> > individual or entity to which this message is addressed, and unless
> otherwise
> > expressly indicated, is confidential and privileged information of
> Rackspace.
> > Any dissemination, distribution or copying of the enclosed material is
> prohibited.
> > If you receive this transmission in error, please notify us immediately
> by e-mail
> > at <email address hidden>, and delete the original message.
> > Your cooperation is appreciated.
> >
> > --
> > authorization not checked in ec2 api
> > https://bugs.launchpad.net/bugs/644092
> > You received this bug notification because you are a member of Nova
> > Bugs, which is subscribed to OpenStack Compute (nova).
> >
> > Status in OpenStack Compute (Nova): New
> >
> > Bug description:
> > Users can see things they shouldn't be able to.
> >
> > STEPS:
> >
> > 0) create two users a & b, NOT in the same project
> >
> > 1) start an instance as user a
> >
> > ...

Read more...

Revision history for this message
Joshua McKenty (joshua-mckenty) wrote :
Download full text (8.0 KiB)

The term "proper object" isn't very pythonic, silly Devin man. Nothin wrong
with a dict - it's a "proper object", too.

On Tue, Sep 21, 2010 at 9:06 PM, Devin Carlen <email address hidden>wrote:

> I like the practice of providing context all the way down the call
> stack. However I think first class citizens should not be dictionaries.
> We can accomplish the same thing with a proper object that implements
> todict()/fromdict().
>
> Devin
>
>
> On Sep 21, 2010, at 11:18 AM, vishvananda wrote:
>
> > My thoughts on data authentication:
> > First we turn context into a dictionary so it is easy to pass around
> everywhere. The dictionary contains the following important data:
> > {'request_id': <random req string>,
> > 'user_id': ...,
> > 'project_id': ...,
> > 'superuser': <boolean, for bypassing project checking>
> > 'deleted' <tristate yes, no, all>}
> >
> > superuser is set during authentication of the request, simply to avoid
> > roundtrips to the auth layer.
> >
> > My preference for a dictionary is simply that it is easy to pass through
> > rabbit.
> >
> > then inside the data layer, we have a simple check:
> > if context['superuser']:
> > # don't limit the sql
> > elif context['project_id']:
> > # add project_id = context
> > else:
> > raise
> > switch on deleted to add deleted=0, deleted=1 or ignore
> >
> > The user_id and request_id aren't used in the datalayer but are in
> > context to help with logging.
> >
> > On Sep 21, 2010, at 8:52 AM, Michael Gundlach wrote:
> >
> >> On Tue, Sep 21, 2010 at 8:16 AM, Soren Hansen <email address hidden> wrote:
> >>
> >>> Now that I think about it, I'm not sure how they're supposed to work?
> >>> They're applied before the object(s) being accessed are even known, so
> >>> it only really checks if context.user has the given role on
> >>> context.project, right? So any checks further down should check whether
> >>> the object being accessed belongs to context.project. Is that accurate?
> >>>
> >>
> >> Yep, that's accurate. The conversion from Tornado to eventlet (as of
> yet
> >> unmerged to trunk) moves all that into an "Authorization" middleware.
> It
> >> might help clarify the code if that were renamed to
> "MethodAuthentication"
> >> and we do data authentication somewhere else.
> >>
> >>
> >> Confidentiality Notice: This e-mail message (including any attached or
> >> embedded documents) is intended for the exclusive and confidential use
> of the
> >> individual or entity to which this message is addressed, and unless
> otherwise
> >> expressly indicated, is confidential and privileged information of
> Rackspace.
> >> Any dissemination, distribution or copying of the enclosed material is
> prohibited.
> >> If you receive this transmission in error, please notify us immediately
> by e-mail
> >> at <email address hidden>, and delete the original message.
> >> Your cooperation is appreciated.
> >>
> >> --
> >> authorization not checked in ec2 api
> >> https://bugs.launchpad.net/bugs/644092
> >> You received this bug notification because you are a member of Nova
> >> Bugs, which is subscribed to OpenStack Compute (nova).
> >>
> >> Status in OpenStack Compute (Nova): New
> >>
> >> Bug description...

Read more...

Revision history for this message
Eric Day (eday) wrote :
Download full text (9.0 KiB)

I would vote sticking with an object or dict, as the context may
be passed over the message bus to workers. Most of the WSGI environ
members won't make much sense in the worker process.

-Eric

On Tue, Sep 21, 2010 at 07:06:56PM -0000, Michael Gundlach wrote:
> WSGI has an environ dictionary that is for storing context about the
> request; would it make sense to replace APIRequestContext entirely, just
> passing the environ dict around? Pro: reuses WSGI concept. Con: passing
> around a dictionary that has more info in it than strictly needed; risks
> coupling the code to WSGI more tightly.
>
> - Michael
>
> On Tue, Sep 21, 2010 at 2:18 PM, vishvananda <email address hidden>
> wrote:
>
> > My thoughts on data authentication:
> > First we turn context into a dictionary so it is easy to pass around
> > everywhere. The dictionary contains the following important data:
> > {'request_id': <random req string>,
> > 'user_id': ...,
> > 'project_id': ...,
> > 'superuser': <boolean, for bypassing project checking>
> > 'deleted' <tristate yes, no, all>}
> >
> > superuser is set during authentication of the request, simply to avoid
> > roundtrips to the auth layer.
> >
> > My preference for a dictionary is simply that it is easy to pass through
> > rabbit.
> >
> > then inside the data layer, we have a simple check:
> > if context['superuser']:
> > # don't limit the sql
> > elif context['project_id']:
> > # add project_id = context
> > else:
> > raise
> > switch on deleted to add deleted=0, deleted=1 or ignore
> >
> > The user_id and request_id aren't used in the datalayer but are in
> > context to help with logging.
> >
> > On Sep 21, 2010, at 8:52 AM, Michael Gundlach wrote:
> >
> > > On Tue, Sep 21, 2010 at 8:16 AM, Soren Hansen <email address hidden> wrote:
> > >
> > >> Now that I think about it, I'm not sure how they're supposed to work?
> > >> They're applied before the object(s) being accessed are even known, so
> > >> it only really checks if context.user has the given role on
> > >> context.project, right? So any checks further down should check whether
> > >> the object being accessed belongs to context.project. Is that accurate?
> > >>
> > >
> > > Yep, that's accurate. The conversion from Tornado to eventlet (as of yet
> > > unmerged to trunk) moves all that into an "Authorization" middleware. It
> > > might help clarify the code if that were renamed to
> > "MethodAuthentication"
> > > and we do data authentication somewhere else.
> > >
> > >
> > > Confidentiality Notice: This e-mail message (including any attached or
> > > embedded documents) is intended for the exclusive and confidential use of
> > the
> > > individual or entity to which this message is addressed, and unless
> > otherwise
> > > expressly indicated, is confidential and privileged information of
> > Rackspace.
> > > Any dissemination, distribution or copying of the enclosed material is
> > prohibited.
> > > If you receive this transmission in error, please notify us immediately
> > by e-mail
> > > at <email address hidden>, and delete the original message.
> > > Your cooperation is appreciated.
> > >
> > > --
> > > authorization not checked in ec2 api
> > > https://bu...

Read more...

Revision history for this message
Devin Carlen (devcamcar) wrote :
Download full text (10.9 KiB)

I don't think the WSGI context will be of any use and we'd have to modify it along the way. The context object Vish proposed contains only information specific to Nova.

Devin

On Sep 21, 2010, at 12:52 PM, Eric Day wrote:

> I would vote sticking with an object or dict, as the context may
> be passed over the message bus to workers. Most of the WSGI environ
> members won't make much sense in the worker process.
>
> -Eric
>
> On Tue, Sep 21, 2010 at 07:06:56PM -0000, Michael Gundlach wrote:
>> WSGI has an environ dictionary that is for storing context about the
>> request; would it make sense to replace APIRequestContext entirely, just
>> passing the environ dict around? Pro: reuses WSGI concept. Con: passing
>> around a dictionary that has more info in it than strictly needed; risks
>> coupling the code to WSGI more tightly.
>>
>> - Michael
>>
>> On Tue, Sep 21, 2010 at 2:18 PM, vishvananda <email address hidden>
>> wrote:
>>
>>> My thoughts on data authentication:
>>> First we turn context into a dictionary so it is easy to pass around
>>> everywhere. The dictionary contains the following important data:
>>> {'request_id': <random req string>,
>>> 'user_id': ...,
>>> 'project_id': ...,
>>> 'superuser': <boolean, for bypassing project checking>
>>> 'deleted' <tristate yes, no, all>}
>>>
>>> superuser is set during authentication of the request, simply to avoid
>>> roundtrips to the auth layer.
>>>
>>> My preference for a dictionary is simply that it is easy to pass through
>>> rabbit.
>>>
>>> then inside the data layer, we have a simple check:
>>> if context['superuser']:
>>> # don't limit the sql
>>> elif context['project_id']:
>>> # add project_id = context
>>> else:
>>> raise
>>> switch on deleted to add deleted=0, deleted=1 or ignore
>>>
>>> The user_id and request_id aren't used in the datalayer but are in
>>> context to help with logging.
>>>
>>> On Sep 21, 2010, at 8:52 AM, Michael Gundlach wrote:
>>>
>>>> On Tue, Sep 21, 2010 at 8:16 AM, Soren Hansen <email address hidden> wrote:
>>>>
>>>>> Now that I think about it, I'm not sure how they're supposed to work?
>>>>> They're applied before the object(s) being accessed are even known, so
>>>>> it only really checks if context.user has the given role on
>>>>> context.project, right? So any checks further down should check whether
>>>>> the object being accessed belongs to context.project. Is that accurate?
>>>>>
>>>>
>>>> Yep, that's accurate. The conversion from Tornado to eventlet (as of yet
>>>> unmerged to trunk) moves all that into an "Authorization" middleware. It
>>>> might help clarify the code if that were renamed to
>>> "MethodAuthentication"
>>>> and we do data authentication somewhere else.
>>>>
>>>>
>>>> Confidentiality Notice: This e-mail message (including any attached or
>>>> embedded documents) is intended for the exclusive and confidential use of
>>> the
>>>> individual or entity to which this message is addressed, and unless
>>> otherwise
>>>> expressly indicated, is confidential and privileged information of
>>> Rackspace.
>>>> Any dissemination, distribution or copying of the enclosed material is
>>> prohibited.
>>>> If you receive this transmis...

Revision history for this message
Jesse Andrews (anotherjesse) wrote :

regardless of whether the context object is a dict or a class or a modified WSGI environ:

Does the idea of doing authorization at the data layer by checking against this variable (and hence always passing it in) seem like a good idea?

Right now many of the RS API and workers use either {} or None...

We would have to change all of these... (so regardless of what the context variable is, we need to send it to the workers when we do work)

Revision history for this message
Jesse Andrews (anotherjesse) wrote :

Soren wrote:

"Now that I think about it, I'm not sure how they're supposed to work? They're applied before the object(s) being accessed are even known, so it only really checks if context.user has the given role on context.project, right? So any checks further down should check whether the object being accessed belongs to context.project. Is that accurate?"

This is what I am proposing. That the datalayer should check to see if the project is allowed to access the object.

Revision history for this message
Devin Carlen (devcamcar) wrote :

++ data layer is last line of defense to prevent coding errors from presenting data that doesn't belong to the user in context.

On Sep 21, 2010, at 3:49 PM, anotherjesse wrote:

> Soren wrote:
>
> "Now that I think about it, I'm not sure how they're supposed to work?
> They're applied before the object(s) being accessed are even known, so
> it only really checks if context.user has the given role on
> context.project, right? So any checks further down should check whether
> the object being accessed belongs to context.project. Is that accurate?"
>
> This is what I am proposing. That the datalayer should check to see if
> the project is allowed to access the object.
>
> --
> authorization not checked in ec2 api
> https://bugs.launchpad.net/bugs/644092
> You received this bug notification because you are a member of Nova
> Bugs, which is subscribed to OpenStack Compute (nova).
>
> Status in OpenStack Compute (Nova): New
>
> Bug description:
> Users can see things they shouldn't be able to.
>
> STEPS:
>
> 0) create two users a & b, NOT in the same project
>
> 1) start an instance as user a
>
> 2) get_console_output on user a's instance as user b
>
> EXPECTED:
>
> the user should not get the output (user b should not be able to get console output for instances they don't own)
>
> ACTUAL:
>
> console output from instance is returned successfully
>
> DETAILS:
>
> for instance, get_console_output in cloud.py:
>
> def get_console_output(self, context, instance_id, **kwargs):
> instance_ref = db.instance_get_by_str(context, instance_id[0])
> return rpc.call('%s.%s' % (FLAGS.compute_topic,
> instance_ref['host']),
> {"method": "get_console_output",
> "args": {"context": None,
> "instance_id": instance_ref['id']}})
>
> sends a context (which has the user/project) into the instance_get_by_str function, but context is not used in the db layer to determine if the user has access to the instance.
>
> Similarly for all other data store objects.
>
> PROPOSAL:
>
> My thought is that the datalayer should only return objects that the user is authorized to see. If we check at the API layer instead of the data access layer, we have the possibility of inconsistent rules for the same data types as the API evolves.
>
>

Revision history for this message
Jay Pipes (jaypipes) wrote :

Marking this Fix Committed as Devin's auth branch is now in trunk.

Changed in nova:
assignee: nobody → Devin Carlen (devcamcar)
status: New → Fix Committed
Eric Day (eday)
Changed in nova:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.